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.

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

Add Windows OVS image for windows ovs containerisation #4936

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

rajnkamr
Copy link
Contributor

@rajnkamr rajnkamr commented May 5, 2023

Description-
On windows node, a new windows ovs image will be used for running ovs container, antrea agent container will use antrea-windows image. On windows node two images will need to be available i.e antrea windows ~745MB and antrea windows ovs ~ 3.6 GB.
ovs image will tagged with ovs release version

@rajnkamr rajnkamr requested a review from XinShuYang May 5, 2023 03:00
@rajnkamr rajnkamr added this to the Antrea v1.12 release milestone May 5, 2023
@XinShuYang
Copy link
Contributor

/test-windows-containerd-e2e

@XinShuYang
Copy link
Contributor

/test-windows-e2e

@rajnkamr rajnkamr marked this pull request as ready for review May 7, 2023 17:00
@rajnkamr rajnkamr requested a review from wenyingd May 7, 2023 17:00
@rajnkamr rajnkamr requested a review from XinShuYang May 8, 2023 05:28
@luolanzone
Copy link
Contributor

@rajnkamr could you squash 3 commits into one commit? and I suppose this change depends on #4705? @XinShuYang

@rajnkamr
Copy link
Contributor Author

rajnkamr commented May 8, 2023

@rajnkamr could you squash 3 commits into one commit? and I suppose this change depends on #4705? @XinShuYang

Squashed 3 commits in one, ovs version no has to be updated in dependency file, otherwise #4705 remain independent @XinShuYang can confirm !

@XinShuYang
Copy link
Contributor

@rajnkamr could you squash 3 commits into one commit? and I suppose this change depends on #4705? @XinShuYang

Squashed 3 commits in one, ovs version no has to be updated in dependency file, otherwise #4705 remain independent @XinShuYang can confirm !

Thanks for the reminder, 4705 has been merged.

@tnqn tnqn added the area/OS/windows Issues or PRs related to the Windows operating system. label May 9, 2023
@luolanzone
Copy link
Contributor

@rajnkamr could you check and fix Windows image build issue? https://github.com/antrea-io/antrea/actions/runs/4913939105/jobs/8774697360?pr=4936
and please also update the PR summary to indicate this is for #4952

@luolanzone
Copy link
Contributor

Btw, you also need to sign-off in your commit, the DCO check is failed https://github.com/antrea-io/antrea/pull/4936/checks?check_run_id=13308968729

@rajnkamr
Copy link
Contributor Author

rajnkamr commented May 9, 2023

@rajnkamr could you check and fix Windows image build issue? https://github.com/antrea-io/antrea/actions/runs/4913939105/jobs/8774697360?pr=4936
and please also update the PR summary to indicate this is for #4952

@luolanzone , this is due to build image tagged with old tag which was uploaded to registry, we need to upload new image with ovs tag, we will upload new image to registry

@rajnkamr

This comment was marked as duplicate.

@rajnkamr rajnkamr changed the title Adds ovs env to windows base image Adding support for Windows OVS image and updating the related building script May 9, 2023
@rajnkamr rajnkamr changed the title Adding support for Windows OVS image and updating the related building script Adding Windows OVS image for windows ovs containerisation May 9, 2023
@rajnkamr rajnkamr changed the title Adding Windows OVS image for windows ovs containerisation Add Windows OVS image for windows ovs containerisation May 9, 2023
@rajnkamr rajnkamr requested a review from tnqn May 11, 2023 04:04
@rajnkamr
Copy link
Contributor Author

/test-networkpolicy

@rajnkamr
Copy link
Contributor Author

/test-conformance

@rajnkamr
Copy link
Contributor Author

/test-networkpolicy

@rajnkamr
Copy link
Contributor Author

/test-e2e

@XinShuYang
Copy link
Contributor

@rajnkamr Could you upload the new base image to your personal dockerhub and share the image name?
@tnqn I assume we can't upload windows image from an unmerged PR right? Could you help upload this image to enable CI test in next step?

@rajnkamr
Copy link
Contributor Author

rajnkamr commented Jun 13, 2023

@XinShuYang @tnqn
New base windows image(with ovs tag) is available at my personal dockerhub - rajnishk1608/base-windows:0af6780141,
Once this image is loaded to antrea dockerhub, we can re run the test

build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
hack/build-antrea-windows-all.sh Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
hack/build-antrea-windows-all.sh Show resolved Hide resolved
powershell -Command "Start-Process -FilePath C:\vc_redist.x64.exe -ArgumentList ‘/install’,’/quiet’,’/norestart’ -Verb RunAs -Wait"

# Download and extract OVS
RUN curl.exe -fLo C:\ovs-${env:WIN_OVS_VERSION}-antrea.0-win64.zip https://downloads.antrea.io/ovs/ovs-${env:WIN_OVS_VERSION}-antrea.0-win64.zip; \
Copy link
Contributor

Choose a reason for hiding this comment

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

Better to verify sha256 checksum here?

$FileHash = Get-FileHash $OVSZip

Copy link
Contributor Author

@rajnkamr rajnkamr Jun 29, 2023

Choose a reason for hiding this comment

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

Image creation based on download is not be a regular activity and is done only as & when new container ovs image is needed with new ovs versions and the ovs url used is also antrea specific only.
Also many places opensource urls are used in dockerfile like build/images/base-windows/Dockerfile, it might be useful to have checksum for all download cases, we can have a single github issue and handle this task separately.

Makefile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Show resolved Hide resolved
hack/build-antrea-windows-all.sh Outdated Show resolved Hide resolved
hack/build-antrea-windows-all.sh Outdated Show resolved Hide resolved
hack/build-antrea-windows-all.sh Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
@rajnkamr
Copy link
Contributor Author

#4936 (comment),

Windows system path "C:/windows/system32" seems to be working fine

build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
@rajnkamr rajnkamr force-pushed the ovs branch 2 times, most recently from a304481 to 153f9d2 Compare July 3, 2023 14:29
build/images/base-windows/README.md Outdated Show resolved Hide resolved
build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
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
@wenyingd @XinShuYang do you have other comments?

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

build/images/ovs/Dockerfile.windows Show resolved Hide resolved
server core image for running windows antrea ovs container.
The container image has bundled ovs windows executables along
with the dependencies like ssl libraries and vc pkg.
bin and sbin paths are also added to windows path in the container image to
facilitate various ovs executables required for debugging ovs flows

Signed-off-by: Rajnish Kumar <rajnishk1@vmware.com>
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

@tnqn
Copy link
Member

tnqn commented Jul 6, 2023

/skip-all

@tnqn tnqn merged commit 7be2f83 into antrea-io:main Jul 6, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants