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

[Windows] Use hpc base image and buildx to build Agent Windows image #6325

Merged
merged 1 commit into from
May 29, 2024

Conversation

wenyingd
Copy link
Contributor

@wenyingd wenyingd commented May 13, 2024

The changes in this patch include,

  1. Use Windows hpc as the base image to store Antrea binary and OVS files
  2. Use doocker buildx to build Windows image on Linux base environment
  3. Modify OVS init container to install redist files
  4. Place openssl dll files into OVS binary directory instead of Windows system
    path
  5. Remove original windows-base image, and download CNI file in agent image

Using the hpc base image and buildx on a fresh env, the time to build Windows
related images and push to remote registry is as follows,

  1. windows-ovs: 31.6s
  2. antrea-windows: 232s

Fix #6311

Makefile Outdated Show resolved Hide resolved
build/images/base-windows/Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-windows-e2e

@wenyingd wenyingd force-pushed the issue_6311 branch 2 times, most recently from 6a539a9 to 928082a Compare May 16, 2024 04:00
@wenyingd
Copy link
Contributor Author

/test-windows-e2e

@wenyingd
Copy link
Contributor Author

/test-windows-all

@luolanzone luolanzone added the area/OS/windows Issues or PRs related to the Windows operating system. label May 17, 2024
@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-windows-all

.github/workflows/build.yml Outdated Show resolved Hide resolved
Comment on lines 23 to 38
RUN --mount=type=cache,target=/go/pkg/mod/ \
--mount=type=bind,source=go.sum,target=go.sum \
--mount=type=bind,source=go.mod,target=go.mod \
go mod download

COPY . /antrea

RUN sh -c 'make windows-bin'

FROM antrea/windows-ovs:${WIN_BUILD_OVS_TAG} as windows-ovs

FROM mcr.microsoft.com/powershell:lts-nanoserver-${NANOSERVER_VERSION}
SHELL ["pwsh", "-NoLogo", "-Command", "$ErrorActionPreference = 'Stop'; $ProgressPreference = 'SilentlyContinue';"]

LABEL maintainer="Antrea <[email protected]>"
LABEL description="A Docker image to deploy the Antrea CNI."
RUN --mount=type=cache,target=/go/pkg/mod/ \
--mount=type=cache,target=/root/.cache/go-build/ \
make windows-bin
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO, it is better to do the steps that are more likely to have changes last in the Dockerfile, for better caching.
Shouldn't we do this after installing the CNI plugin binaries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I will update accordingly.

build/images/Dockerfile.build.windows Outdated Show resolved Hide resolved
build/images/build-utils.sh Outdated Show resolved Hide resolved
build/images/build-windows.sh Outdated Show resolved Hide resolved
build/images/ovs/build.sh Outdated Show resolved Hide resolved
build/yamls/antrea-windows-with-ovs.yml Outdated Show resolved Hide resolved
@luolanzone luolanzone added this to the Antrea v2.1 release milestone May 21, 2024
@wenyingd wenyingd force-pushed the issue_6311 branch 3 times, most recently from 169bd82 to 8697723 Compare May 21, 2024 03:57
@wenyingd
Copy link
Contributor Author

/test-windows-containerd-conformance

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

Windows CI tests have passed.

XinShuYang
XinShuYang previously approved these changes May 23, 2024
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, since this PR involves more code changes, after discussing with Wenying, #6325 will be merged after #6312 is merged.

@wenyingd
Copy link
Contributor Author

/test-windows-all

@wenyingd
Copy link
Contributor Author

/test-windows-all

build/images/ovs/Dockerfile.windows Outdated Show resolved Hide resolved
build/images/ovs/build.sh Outdated Show resolved Hide resolved
build/yamls/antrea-windows-with-ovs.yml Show resolved Hide resolved

# Check if the VC redistributable is already installed.
$OVSRedistDir="$mountPath\openvswitch\redist"
if (Test-Path $OVSRedistDir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need that check (it feels like the directory is guaranteed to exist), and what should be the behavior if it doesn't exist (should we fail the initContainer instead)?.

Copy link
Contributor Author

@wenyingd wenyingd May 27, 2024

Choose a reason for hiding this comment

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

This is because when we are using Windows nanoserver as the base image, we didn't deliver the vc_runtime.exe in the images. Instead, we install it in the middle stage, and copy the extracted vcruntime.dll files into nanoserver path "C:\windows\system32", finally we add the container image's system path into environment Path. So when running containers based on the image, the dll (vcruntime.dll) are accessible. If we directly access the directory "openvswitch\redist" in the init-container, it may fail if somebody manually update image tag with the new manifest.

But for hpc based image, there is no system path like "c:\system\windows" exist in the container image, and we can't extract the dll files during the building stages because we can't run .exe file based on a Ubuntu iamge. So we have to copy the exe file into the container image, and install it in init-container stage to extract the dll files on the target Windows Node (host).

If the dll files doesn't exist on the target Windows Node, Windows OVS binary may fail to run.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a comment for the if (Test-Path $OVSRedistDir) check would be good, but you can do it in your next PR since you want to improve this logic anyway (as discussed in the other comment).

docs/windows.md Outdated Show resolved Hide resolved
@wenyingd
Copy link
Contributor Author

/test-windows-all

1. Use Windows hpc as the base image to store Antrea binary and OVS files
2. Use doocker buildx to build Windows image on Linux base environment
3. Modify OVS init container to install redist files
4. Place openssl dll files into OVS binary directory instead of Windows system
   path
5. Remove original windows-base image, and download CNI file in agent image

Using the hpc base image and buildx on a fresh env, the time to build Windows
related images and push to remote registry is as follows,
1. windows-ovs: 31.6s
2. antrea-windows: 232s

Signed-off-by: Wenying Dong <[email protected]>
@wenyingd
Copy link
Contributor Author

/skip-all
/test-windows-all

@wenyingd
Copy link
Contributor Author

@antoninbas Can we move this change forward or we shall wait for another round of review?

@wenyingd
Copy link
Contributor Author

/test-windows-e2e


# Check if the VC redistributable is already installed.
$OVSRedistDir="$mountPath\openvswitch\redist"
if (Test-Path $OVSRedistDir) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a comment for the if (Test-Path $OVSRedistDir) check would be good, but you can do it in your next PR since you want to improve this logic anyway (as discussed in the other comment).

@antoninbas antoninbas merged commit b109bf8 into antrea-io:main May 29, 2024
54 of 57 checks passed
@antoninbas antoninbas added the action/release-note Indicates a PR that should be included in release notes. label May 29, 2024
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.

Use buildx and hpc image to build Antrea Windows images
5 participants