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

feat: build windows containers #1467

Merged
merged 22 commits into from
Mar 1, 2024
Merged

feat: build windows containers #1467

merged 22 commits into from
Mar 1, 2024

Conversation

sumo-drosiek
Copy link
Contributor

@sumo-drosiek sumo-drosiek commented Feb 26, 2024

Adds building windows containers. This PR uses docker command in order to build manifest, as for some reason docker buildx was not always working, also buildx is not supported on windows

➜  ~ docker manifest inspect public.ecr.aws/sumologic/sumologic-otel-collector-dev:latest
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1093,
         "digest": "sha256:1fde89d7412f56cac8e295ec067895780d36755c1e0c442e2832024c54b09d4f",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1093,
         "digest": "sha256:5815d396401f4f24d69d90c7df4b918c030212719178d368d7d25ccda1b4fbb8",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1788,
         "digest": "sha256:79230acc35c7a076a9eab9e851839971a6f43c74d4ef184811962f1904ac5f2c",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "10.0.17763.5458"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1788,
         "digest": "sha256:711ab12580c5d2c1bd18785be0e9e675b62d6df01bf13d5c22793d567f0c71a3",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "10.0.20348.2322"
         }
      }
   ]
}

@sumo-drosiek sumo-drosiek force-pushed the drosiek-windows-container branch 18 times, most recently from b6f1df8 to 829ac66 Compare February 28, 2024 10:11
@sumo-drosiek sumo-drosiek changed the title Windows container playground feat: build windows containers Feb 28, 2024
@sumo-drosiek sumo-drosiek marked this pull request as ready for review February 28, 2024 18:15
@sumo-drosiek sumo-drosiek requested a review from a team as a code owner February 28, 2024 18:15
@sumo-drosiek sumo-drosiek force-pushed the drosiek-windows-container branch from d29aed2 to b0615d4 Compare February 28, 2024 18:15
@@ -0,0 +1,7 @@
FROM mcr.microsoft.com/windows/servercore:ltsc2022

Choose a reason for hiding this comment

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

Do we have to rely on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, this is complicated. We cannot use nanoserver as it doesn't provide Netapi32.dll link

Also it means for windows what are the base image and what system we try to run it on. Manifest should have addition os.version field, and ideally we should provide multiple windows images. Not sure if newer windows can run older images, as github actions had issues with running image based on older tag.

Due to above, I'm not sure if this should be run for release branches, as those are still experimental images

Copy link
Contributor Author

Choose a reason for hiding this comment

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

➜  ~ docker manifest inspect public.ecr.aws/sumologic/sumologic-otel-collector-dev:0fa281c6c04691e937f403218b09de6e91f04d29
{
   "schemaVersion": 2,
   "mediaType": "application/vnd.docker.distribution.manifest.list.v2+json",
   "manifests": [
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1093,
         "digest": "sha256:719c9ffed95ff6eda12c6564e70c1c025352dacb376f5dfc01a37cdc87f6953d",
         "platform": {
            "architecture": "amd64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1093,
         "digest": "sha256:8a04067bc357fee33f1e11b468e408ed26950f0ea7382d47ccc6b3614f2150d9",
         "platform": {
            "architecture": "arm64",
            "os": "linux"
         }
      },
      {
         "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
         "size": 1788,
         "digest": "sha256:914c7d0ee0927e2d065f6b74f03ab036e9758afa76cb09d54cf99340dbb9609b",
         "platform": {
            "architecture": "amd64",
            "os": "windows",
            "os.version": "10.0.20348.2322"
         }
      }
   ]
}

Working on building multitple images, as this one is not working on my eks windows cluster 😅

Choose a reason for hiding this comment

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

🙉 Yeah, let's start with just dev builds then. This PR overall looks a bit messy and I think we should try to consolidate our image building logic a bit more. So it'd be better to just do it for dev images, then clean up a bit, figure out the edge cases and then start building release images.

ci/build-push-multiplatform.sh Show resolved Hide resolved
@sumo-drosiek sumo-drosiek force-pushed the drosiek-windows-container branch from 59e08f6 to cfe0250 Compare February 29, 2024 08:01
Dominik Rosiek and others added 21 commits February 29, 2024 15:41
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
Signed-off-by: Dominik Rosiek <[email protected]>
@sumo-drosiek sumo-drosiek force-pushed the drosiek-windows-container branch from acecdc1 to c886a1b Compare February 29, 2024 14:41
Comment on lines 5 to 16
if echo "${PLATFORM}" | grep windows; then
while ! docker images; do
echo "Cannot connect to docker daemon"
sleep 1
done
else
while ! docker buildx ls; do
echo "Cannot connect to docker daemon"
sleep 1
done

DOCKER_BUILDX_LS_OUT=$(docker buildx ls <<-END

Choose a reason for hiding this comment

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

Why do we need to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To verify if we have connection with docker server

Copy link

Choose a reason for hiding this comment

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

Is this actually necessary in practice? Do we get into situations where we try to build and the docker daemon hasn't started yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not really as it will fail further in the pipeline eventually

.

if [[ "${BUILD_PLATFORM}" == "windows" ]]; then
docker build \
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo Feb 29, 2024

Choose a reason for hiding this comment

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

Have we tried downloading the unattended installation of the buildx component from here?, looks like there are windows executables

https://github.com/docker/buildx?tab=readme-ov-file#manual-download

Binaries from the github release page
https://github.com/docker/buildx/releases/tag/v0.12.1

got the above from this comment

Copy link
Contributor

Choose a reason for hiding this comment

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

oh nvm, looks like this is the root cause - docker/buildx#176, buildx on windows containers not supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, maybe it is suppored, but I would prefer to have more control over what happening with windows image builds and also avoid experimenting support. I would feel more comfortable to switch to it when it will be fully supported, or maybe if we go for FIPS/arm64

@sumo-drosiek sumo-drosiek merged commit ccc3cdf into main Mar 1, 2024
30 checks passed
@sumo-drosiek sumo-drosiek deleted the drosiek-windows-container branch March 1, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants