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

normalize the image building targets #8400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mainred
Copy link
Contributor

@mainred mainred commented Feb 21, 2025

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR fixes the post-provider-azure-push-images job failure which is caused by building windows images and out to Linux docker Engine. Fix this by normalizing the image building targets.

  • removed makefile targets build-all-node-images which are not achievable on one OS platform
  • push image artifacts will be triggered by docker buildx --output=type=registry which can reuse the build target but specify OUTPUT_TYPE=registry. pushing images does not relies on building image targets in the future.
    Besides fixing the image building job issue. we also
  • simplified the makefile target name like replacing push-multi-arch-controller-manager-image with manifest-controller-manager
  • removed redundant variable name like $NODE_MANAGER_FULL_IMAGE_NAME and $ALL_NODE_MANAGER_IMAGES, and use $(NODE_MANAGER_IMAGE):$(IMAGE_TAG) specificly to represent the image+tag, and use foreach to iterate the OS and ARCH to form the image tags for different OS and ARCH.
  • expose only necessary makefile targets with comment, you may try this by make help
  • we can build node manager with windows host process container image when WINDOWS_USE_HOST_PROCESS_CONTAINERS is set to true
  • leave only windows 2022 and 2019

Finally, in order to not break the PR check-in and post-provider-azure-push-images, we introduced IS_RELEASE variable. PR check-in in capz ci-build-azure-ccm.sh build and publish amd64 ccm images, but expect a arch-agnostic image inappropriately, we keep this ability and hope to replace it with make ALL_ARCH.linux=amd64 manifest-controller-manager after this PR is merged.

Tests have been done:

I have tested this change against ado pipeline, pr check-in, and post-provider-azure-push-images job.

ordinary image push
IMAGE_REGISTRY=mainred make manifest-controller-manager

docker manifest inspect mainred/azure-cloud-controller-manager:b4aebbe | jq '.manifests[].platform' | tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"arm","os":"linux","variant":"v7"}
{"architecture":"arm64","os":"linux"}
IMAGE_REGISTRY=mainred make manifest-node-manager
 docker manifest inspect mainred/azure-cloud-node-manager:b4aebbe| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"arm","os":"linux","variant":"v7"}
{"architecture":"arm64","os":"linux"}
{"architecture":"amd64","os":"windows","os.version":"10.0.17763.6893"}
{"architecture":"amd64","os":"windows","os.version":"10.0.20348.3207"}
node manager manifest with windows host process contianer image
WINDOWS_USE_HOST_PROCESS_CONTAINERS=true IMAGE_REGISTRY=mainred  make manifest-node-manager-image

docker manifest inspect mainred/azure-cloud-node-manager:b4aebbe | jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"arm","os":"linux","variant":"v7"}
{"architecture":"arm64","os":"linux"}
{"architecture":"amd64","os":"windows"}
current capz support
IMAGE_REGISTRY=mainred make build-node-image-linux-amd64 push-node-image-linux-amd64 push-node-image-windows-hpc-amd64 manifest-node-manager-image-windows-hpc-amd64
 docker manifest inspect mainred/azure-cloud-node-manager:e5a7448| jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"amd64","os":"windows"}
IMAGE_REGISTRY=mainred make build-ccm-image-amd64 push-ccm-image-amd64 make ARCH=amd64 build-ccm-image
docker manifest inspect mainred/azure-cloud-controller-manager:e5a7448
{
        "schemaVersion": 2,
        "mediaType": "application/vnd.docker.distribution.manifest.v2+json",
        "config": {
                "mediaType": "application/vnd.docker.container.image.v1+json",
                "size": 2244,
                "digest": "sha256:6a2fc4890486748fc9d5b64af3a644b3a60baff87dd7a210dd7435688d9121a0"
        },
        "layers": [
                {
                        "mediaType": "application/vnd.docker.image.rootfs.diff.tar.gzip",
                        "size": 104238,
                        "digest": "sha256:688513194d7a0f0d77e4a3692748d21c4ccd1af6a5ff9012f18f053ed9573c13"
                },
             ... ...
        ]
}
future capz change

capz can still build only minimal amd64-based image for test as it has been doing so far.

make IMAGE_REGISTRY=mainred ALL_ARCH.linux=amd64 manifest-controller-manager

 docker manifest inspect mainred/azure-cloud-controller-manager:b4aebbe| jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture": "amd64","os": "linux"}

make IMAGE_REGISTRY=mainred WINDOWS_USE_HOST_PROCESS_CONTAINERS=true ALL_ARCH.linux=amd64 manifest-node-manager

docker manifest inspect mainred/azure-cloud-node-manager:b4aebbe| jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"amd64","os":"windows"}
release
make IMAGE_REGISTRY=mainred release-staging 

docker manifest inspect mainred/azure-cloud-controller-manager:b4aebbe| jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"arm","os":"linux","variant":"v7"}
{"architecture":"arm64","os":"linux"}

make CLOUD_BUILD_IMAGE=cnm IMAGE_REGISTRY=mainred release-staging
docker manifest inspect mainred/azure-cloud-node-manager:b4aebbe| jq '.manifests[].platform'| tr -d "[:space:]"|sed 's/}/&\n/g'
{"architecture":"amd64","os":"linux"}
{"architecture":"arm","os":"linux","variant":"v7"}
{"architecture":"arm64","os":"linux"}
{"architecture":"amd64","os":"windows","os.version":"10.0.17763.6893"}
{"architecture":"amd64","os":"windows","os.version":"10.0.20348.3207"}

for ado pipelines,

OUTPUT_TYPE="docker,dest=azure-cloud-controller-manager.tar" ARCH=amd64 make build-ccm-image
ls -l azure-cloud-controller-manager.tar
-rw-rw-r-- 1 azureuser azureuser 31205376 Feb 20 15:22 azure-cloud-controller-manager.tar


OUTPUT_TYPE="docker,dest=azure-cloud-node-manager.tar" ARCH=amd64 make build-node-image-Linux
ls -l azure-cloud-node-manager.tar
-rw-rw-r-- 1 azureuser azureuser 30639616 Feb 20 15:29 azure-cloud-node-manager.tar

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 21, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mainred
Once this PR has been reviewed and has the lgtm label, please assign feiskyer for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 21, 2025
@mainred mainred force-pushed the use-docker-build-registry-push branch from e5a7448 to 8ad6b7b Compare February 22, 2025 01:51
@mainred
Copy link
Contributor Author

mainred commented Feb 22, 2025

/test pull-cloud-provider-azure-e2e-ccm-vmss-capz

@mainred
Copy link
Contributor Author

mainred commented Feb 24, 2025

/hold

let's hold the PR until a version is cut from release-1.32

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 24, 2025
@MartinForReal
Copy link
Contributor

it's time to revisit #1145

@mainred
Copy link
Contributor Author

mainred commented Feb 26, 2025

/test pull-cloud-provider-azure-e2e-capz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants