Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support to run Windows OVS in container #5052

Merged
merged 1 commit into from
Jul 26, 2023
Merged

Add support to run Windows OVS in container #5052

merged 1 commit into from
Jul 26, 2023

Conversation

Atish-iaf
Copy link
Contributor

For #4952

@rajnkamr rajnkamr added the area/OS/windows Issues or PRs related to the Windows operating system. label May 29, 2023
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 4263190 to e23ef56 Compare May 30, 2023 03:17
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from e23ef56 to 38d7dbf Compare May 30, 2023 09:38
@Atish-iaf Atish-iaf requested a review from XinShuYang May 30, 2023 09:40
@Atish-iaf Atish-iaf marked this pull request as ready for review May 30, 2023 10:15
@rajnkamr rajnkamr added this to the Antrea v1.13 release milestone May 30, 2023
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 38d7dbf to e0cba14 Compare June 2, 2023 08:26
@Atish-iaf Atish-iaf requested a review from XinShuYang June 2, 2023 09:48
@XinShuYang
Copy link
Contributor

Please update the README. https://github.com/antrea-io/antrea/blob/main/docs/windows.md#add-windows-antrea-agent-daemonset

@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from e0cba14 to 3a47a98 Compare June 5, 2023 03:08
@XinShuYang XinShuYang requested a review from wenyingd June 6, 2023 03:18
@XinShuYang XinShuYang mentioned this pull request Jun 6, 2023
Closed
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 3a47a98 to 0c892b7 Compare June 6, 2023 18:51
@Atish-iaf Atish-iaf requested a review from wenyingd June 6, 2023 19:01
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch 2 times, most recently from 942c13b to 3061a98 Compare June 7, 2023 18:31
wenyingd
wenyingd reviewed Jun 8, 2023
View reviewed changes
build/yamls/windows/containerd/kustomization.yml Outdated Show resolved Hide resolved
build/yamls/antrea-windows-ovs-containerd.yml Outdated Show resolved Hide resolved
build/yamls/windows/ovs-containerd/conf/Run-AntreaOVS-Containerd.ps1 Outdated Show resolved Hide resolved
docs/windows.md Outdated Show resolved Hide resolved
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 3061a98 to 1706c48 Compare June 8, 2023 08:12
@Atish-iaf Atish-iaf requested a review from wenyingd June 8, 2023 16:23
@rajnkamr rajnkamr mentioned this pull request Jun 15, 2023
Merged
@Atish-iaf
Copy link
Contributor Author

/test-windows-containerd-e2e

@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 44b2118 to 11518c0 Compare July 24, 2023 09:59
@XinShuYang
Copy link
Contributor

/test-windows-containerd-e2e

@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 11518c0 to 9b4427d Compare July 24, 2023 16:33
@Atish-iaf
Copy link
Contributor Author

/test-windows-containerd-e2e

@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 9b4427d to db2eb46 Compare July 24, 2023 17:52
@Atish-iaf
Copy link
Contributor Author

/test-windows-containerd-e2e

XinShuYang
XinShuYang previously approved these changes Jul 25, 2023
View reviewed changes
Copy link
Contributor

@XinShuYang XinShuYang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

wenyingd
wenyingd reviewed Jul 25, 2023
View reviewed changes
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. Besides, remember to revert the changes in test.sh before ready for merge.

build/yamls/windows/containerd-with-ovs/containerd-with-ovs.yml Outdated Show resolved Hide resolved
build/yamls/windows/containerd/conf/Run-AntreaAgent-Containerd.ps1 Show resolved Hide resolved
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from db2eb46 to 9f12dc7 Compare July 25, 2023 08:53
@Atish-iaf
Copy link
Contributor Author

/test-windows-containerd-e2e

wenyingd
wenyingd reviewed Jul 25, 2023
View reviewed changes
build/yamls/antrea-windows-containerd-with-ovs.yml
- $env:CONTAINER_SANDBOX_MOUNT_POINT/var/lib/antrea-windows/Run-AntreaOVS-Containerd.ps1
command:
- powershell
image: antrea/windows-ovs:1809-3.0.5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we use image antrea/antrea-windows:latest to run antrea-ovs ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, we can do that. please see my comment here #5052 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Atish-iaf , as confirmed, ovs server side bins are working fine with antrea-windows:latest image, we should go ahead with single image antrea-windows:latest for ovs container as well

@Atish-iaf
Copy link
Contributor Author

/test-windows-containerd-e2e

1 similar comment
@XinShuYang
Copy link
Contributor

/test-windows-containerd-e2e

tnqn
tnqn reviewed Jul 25, 2023
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall

build/images/Dockerfile.build.windows
@@ -44,3 +47,10 @@ COPY --from=antrea-build-windows /antrea/bin/antrea-cni.exe /k/antrea/cni/antre

RUN mkdir C:\k\antrea\utils
COPY --from=antrea-build-windows /wins/wins.exe /k/antrea/utils/wins.exe

COPY --from=windows-ovs /Windows/System32/vcruntime140.dll /Windows/System32/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can continue the current approach first, then have a separate PR to unify the image and validate everything works fine. I feel we'd better get it done in this release to avoid the inconsistency that user needs to pull an extra image in v1.13 only.

build/yamls/windows/containerd-with-ovs/containerd-with-ovs.yml Outdated Show resolved Hide resolved
hack/generate-manifest-windows.sh Outdated Show resolved Hide resolved
hack/generate-manifest-windows.sh Outdated Show resolved Hide resolved
hack/generate-manifest-windows.sh Outdated Show resolved Hide resolved
hack/windows/Clean-AntreaNetwork.ps1 Outdated
Comment on lines 132 to 135
if (Test-Path -Path $OVS_DB_PATH) {
Write-Host "Failed to delete file: $OVS_DB_PATH"
exit 1
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't Remove-Item return error and exit the script if it fails? Is it necessary to check the file again?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set $ErrorActionPreference='Stop' and removed checking file again.

hack/windows/Install-OVS.ps1 Show resolved Hide resolved
@Atish-iaf
Add support to run Windows OVS in container
1eb9e12 
For antrea-io#4952

Signed-off-by: Shuyang Xin <gavinx@vmware.com>
Signed-off-by: Naman Agarwal <naman.agarwal75@gmail.com>
Signed-off-by: Kumar Atish <atish.iaf@gmail.com>
@Atish-iaf Atish-iaf force-pushed the ovs-containerd-yaml branch from 9f12dc7 to 1eb9e12 Compare July 25, 2023 18:30
@Atish-iaf Atish-iaf requested review from tnqn and wenyingd July 25, 2023 18:42
@XinShuYang
Copy link
Contributor

/test-windows-containerd-e2e

tnqn
tnqn approved these changes Jul 26, 2023
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@XinShuYang
Copy link
Contributor

/test-windows-containerd-conformance
/test-windows-containerd-networkpolicy

wenyingd
wenyingd approved these changes Jul 26, 2023
View reviewed changes
Copy link
Contributor

@wenyingd wenyingd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tnqn
Copy link
Member

tnqn commented Jul 26, 2023

/skip-all

@tnqn tnqn merged commit fea8ddd into antrea-io:main Jul 26, 2023
@Atish-iaf Atish-iaf mentioned this pull request Oct 18, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luolanzone luolanzone luolanzone left review comments

@NamanAg30 NamanAg30 NamanAg30 left review comments

@XinShuYang XinShuYang XinShuYang left review comments

@rajnkamr rajnkamr rajnkamr left review comments

@tnqn tnqn tnqn approved these changes

@wenyingd wenyingd wenyingd approved these changes

@jianjuns jianjuns Awaiting requested review from jianjuns

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes. area/OS/windows Issues or PRs related to the Windows operating system.
Projects
None yet
Milestone
Antrea v1.13 release
Development

Successfully merging this pull request may close these issues.
8 participants
@Atish-iaf @XinShuYang @NamanAg30 @tnqn @wenyingd @rajnkamr @jianjuns @luolanzone