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 back push-manifests in Makefile #8254

Merged
merged 1 commit into from
Nov 22, 2023
Merged

Add back push-manifests in Makefile #8254

merged 1 commit into from
Nov 22, 2023

Conversation

hjiawei
Copy link
Contributor

@hjiawei hjiawei commented Nov 22, 2023

Description

Reimplement push-manifests in lib.Makefile using docker manifest command [1]. It was accidentally removed in [2].

[1] https://docs.docker.com/engine/reference/commandline/manifest/
[2] https://github.com/projectcalico/calico/pull/8103/files#diff-abfd44a7d28b43da680f905985a06fe893b7e4aa8920d87fee8070b261645b55L915

Related issues/PRs

Todos

  • Tests
  • Documentation
  • Release note

Release Note

TBD

Reminder for the reviewer

Make sure that this PR has the correct labels and milestone set.

Every PR needs one docs-* label.

  • docs-pr-required: This change requires a change to the documentation that has not been completed yet.
  • docs-completed: This change has all necessary documentation completed.
  • docs-not-required: This change has no user-facing impact and requires no docs.

Every PR needs one release-note-* label.

  • release-note-required: This PR has user-facing changes. Most PRs should have this label.
  • release-note-not-required: This PR has no user-facing changes.

Other optional labels:

  • cherry-pick-candidate: This PR should be cherry-picked to an earlier release. For bug fixes only.
  • needs-operator-pr: This PR is related to install and requires a corresponding change to the operator.

@hjiawei hjiawei requested a review from a team as a code owner November 22, 2023 04:23
@marvin-tigera marvin-tigera added this to the Calico v3.28.0 milestone Nov 22, 2023
@marvin-tigera marvin-tigera added release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels Nov 22, 2023
@hjiawei hjiawei requested a review from coutinhop November 22, 2023 04:23
@hjiawei
Copy link
Contributor Author

hjiawei commented Nov 22, 2023

I don't have enough permission to do a real manifest push but I think the following dryrun output is what we need.

For example, from calicoctl folder:

$ BRANCH_NAME=master DRYRUN=true make cd
...
make var-require REQUIRED_VARS=REGISTRY-BUILD_IMAGE-IMAGETAG FAIL_NOT_SET=true
...
docker tag ctl:latest-amd64 calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-amd64
docker tag ctl:latest-amd64 calico/ctl:v3.28.0-0.dev-190-g60af20fa8098
docker tag ctl:latest-arm64 calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-arm64
docker tag ctl:latest-ppc64le calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-ppc64le
docker tag ctl:latest-s390x calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-s390x
...
make push-image-arch-to-registry-amd64 push-image-arch-to-registry-arm64 push-image-arch-to-registry-ppc64le push-image-arch-to-registry-s390x BUILD_IMAGE=ctl
...
echo [DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-amd64
[DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-amd64
echo [DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098
[DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098
echo [DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-arm64
[DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-arm64
echo [DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-ppc64le
[DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-ppc64le
echo [DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-s390x
[DRY RUN] docker push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-s390x
...
echo [DRY RUN] docker manifest create calico/ctl:v3.28.0-0.dev-190-g60af20fa8098 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-amd64 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-arm64 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-ppc64le --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-s390x
[DRY RUN] docker manifest create calico/ctl:v3.28.0-0.dev-190-g60af20fa8098 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-amd64 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-arm64 --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-ppc64le --amend calico/ctl:v3.28.0-0.dev-190-g60af20fa8098-s390x
echo [DRY RUN] docker manifest push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098
[DRY RUN] docker manifest push calico/ctl:v3.28.0-0.dev-190-g60af20fa8098

# push multi-arch manifest where supported.
push-manifests: var-require-all-IMAGETAG $(addprefix sub-manifest-,$(call escapefs,$(PUSH_MANIFEST_IMAGES)))
sub-manifest-%:
$(DOCKER) manifest create $(call unescapefs,$*):$(IMAGETAG) $(addprefix --amend ,$(addprefix $(call unescapefs,$*):$(IMAGETAG)-,$(VALIDARCHES)))
Copy link
Member

Choose a reason for hiding this comment

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

any specific reason not to restore MANIFEST_TOOL and MANIFEST_TOOL_CMD exactly as it is in https://github.com/projectcalico/calico/pull/8103/files#diff-abfd44a7d28b43da680f905985a06fe893b7e4aa8920d87fee8070b261645b55L915?

In particular, I believe the -v $(DOCKER_CONFIG):/root/.docker/config.json arg will be necessary for credentials when running on semaphore... I had to do something similar for windows manifests:

calico/lib.Makefile

Lines 1319 to 1345 in 004a6e3

# When running on semaphore, just copy the docker config, otherwise run
# 'docker-credential-gcr configure-docker' as well.
ifdef SEMAPHORE
DOCKER_CREDENTIAL_CMD = cp /root/.docker/config.json_host /root/.docker/config.json
else
DOCKER_CREDENTIAL_CMD = cp /root/.docker/config.json_host /root/.docker/config.json && \
docker-credential-gcr configure-docker
endif
# This needs the $(WINDOWS_DIST)/bin/docker-credential-gcr binary in $PATH and
# also the local ~/.config/gcloud dir to be able to push to gcr.io. It mounts
# $(DOCKER_CONFIG) and copies it so that it can be written to on the container,
# but not have any effect on the host config.
CRANE_BINDMOUNT_CMD := \
docker run --rm \
--net=host \
--init \
--entrypoint /bin/sh \
-e LOCAL_USER_ID=$(LOCAL_USER_ID) \
-v $(CURDIR):/go/src/$(PACKAGE_NAME):rw \
-v $(DOCKER_CONFIG):/root/.docker/config.json_host:ro \
-e PATH=$${PATH}:/go/src/$(PACKAGE_NAME)/$(WINDOWS_DIST)/bin \
-v $(HOME)/.config/gcloud:/root/.config/gcloud \
-w /go/src/$(PACKAGE_NAME) \
$(CALICO_BUILD) -c $(double_quote)$(DOCKER_CREDENTIAL_CMD) && crane
DOCKER_MANIFEST_CMD := docker manifest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The manifest tool is removed from go-build v0.90. docker manifest command (even though still experimental) was merged into docker cli back in 2017. After that, projects from kubernetes, aws, ibm switched to that. I think it would make more sense for us to use the official utility as well.

Copy link
Contributor Author

@hjiawei hjiawei Nov 22, 2023

Choose a reason for hiding this comment

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

For semaphore build, we do docker login first. docker manifest command will use the same credential once we login to a registry.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see! Yes, just noticed that the manifest cmd for the windows images is just docker manifest

@hjiawei hjiawei merged commit 9a2e768 into master Nov 22, 2023
2 checks passed
@hjiawei hjiawei deleted the fix-push-manifests branch November 22, 2023 18:12
hjiawei added a commit to hjiawei/go-build that referenced this pull request Nov 22, 2023
hjiawei added a commit to hjiawei/go-build that referenced this pull request Nov 22, 2023
@caseydavenport caseydavenport added docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact labels May 3, 2024
@marvin-tigera marvin-tigera removed release-note-required Change has user-facing impact (no matter how small) docs-pr-required Change is not yet documented labels May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required Docs not required for this change release-note-not-required Change has no user-facing impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants