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

images/git: Build for arm64 #22450

Merged
merged 2 commits into from
Jun 18, 2021
Merged

Conversation

LorbusChris
Copy link
Contributor

@LorbusChris LorbusChris commented Jun 7, 2021

This commit adds a Google Cloud Build definition for building git
arm64 images and pushing them as a manifest list together with their
amd64 counterparts.

This is a prereq for #16588 and #22362

Blocked on: #22599 #22444

/cc @stevekuznetsov

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 7, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @LorbusChris. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/images sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 7, 2021
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

/ok-to-test

(but I can't speak for the correctness of the change itself)

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 7, 2021
@LorbusChris
Copy link
Contributor Author

If someone with knowledge of Google Cloud Build could take a look that'd be great :)

@LorbusChris
Copy link
Contributor Author

/cc @BenTheElder

@k8s-ci-robot k8s-ci-robot requested a review from BenTheElder June 8, 2021 14:32
@LorbusChris
Copy link
Contributor Author

looking at the file history, @fejta might be best suited for a review of these changes.
/cc @fejta
/uncc @BenTheElder

@k8s-ci-robot k8s-ci-robot requested review from fejta and removed request for BenTheElder June 9, 2021 12:07
LorbusChris added a commit to LorbusChris/test-infra that referenced this pull request Jun 10, 2021
LorbusChris added a commit to LorbusChris/test-infra that referenced this pull request Jun 10, 2021
LorbusChris added a commit to LorbusChris/test-infra that referenced this pull request Jun 10, 2021
@spiffxp
Copy link
Member

spiffxp commented Jun 10, 2021

/cc

@k8s-ci-robot k8s-ci-robot requested a review from spiffxp June 10, 2021 15:57
@spiffxp
Copy link
Member

spiffxp commented Jun 10, 2021

Testing without bazel or pushing to k8s-testimages

$ go install ./images/builder
$ builder --project=k8s-staging-test-infra --scratch-bucket=gs://k8s-staging-test-infra-gcb images/git

/hold
looks like your change doesn't quite work

Step #0: Successfully built 142bd5110560
Step #0: Successfully tagged gcr.io/k8s-staging-test-infra/git:v20210610-b2adebc132
Finished Step #0
Starting Step #1
Step #1: Already have image (with digest): gcr.io/cloud-builders/docker
Step #1: Sending build context to Docker daemon  31.23kB
Step #1: Step 1/4 : FROM gcr.io/k8s-prow/alpine:v20200605-44f6c96
Step #1:  ---> 828d3cee2013
Step #1: Step 2/4 : ARG IMAGE_ARG
Step #1:  ---> Using cache
Step #1:  ---> a3e8ba8fdf41
Step #1: Step 3/4 : ENV IMAGE=${IMAGE_ARG}
Step #1:  ---> Running in b3733184b215
Step #1: Removing intermediate container b3733184b215
Step #1:  ---> 488fce3d02e7
Step #1: Step 4/4 : RUN apk add --no-cache git openssh
Step #1:  ---> Running in a29162f7efd7
Step #1: fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/main/x86_64/APKINDEX.tar.gz
Step #1: fetch http://dl-cdn.alpinelinux.org/alpine/v3.12/community/x86_64/APKINDEX.tar.gz
Step #1: (1/14) Installing nghttp2-libs (1.41.0-r0)
Step #1: (2/14) Installing libcurl (7.77.0-r0)
Step #1: (3/14) Installing expat (2.2.9-r1)
Step #1: (4/14) Installing pcre2 (10.35-r0)
Step #1: (5/14) Installing git (2.26.3-r0)
Step #1: (6/14) Installing openssh-keygen (8.3_p1-r2)
Step #1: (7/14) Installing ncurses-terminfo-base (6.2_p20200523-r0)
Step #1: (8/14) Installing ncurses-libs (6.2_p20200523-r0)
Step #1: (9/14) Installing libedit (20191231.3.1-r0)
Step #1: (10/14) Installing openssh-client (8.3_p1-r2)
Step #1: (11/14) Installing openssh-sftp-server (8.3_p1-r2)
Step #1: (12/14) Installing openssh-server-common (8.3_p1-r2)
Step #1: (13/14) Installing openssh-server (8.3_p1-r2)
Step #1: (14/14) Installing openssh (8.3_p1-r2)
Step #1: Executing busybox-1.31.1-r16.trigger
Step #1: OK: 28 MiB in 29 packages
Step #1: Removing intermediate container a29162f7efd7
Step #1:  ---> 934f2cab9251
Step #1: [Warning] One or more build-args [ARCH] were not consumed
Step #1: Successfully built 934f2cab9251
Step #1: Successfully tagged gcr.io/k8s-staging-test-infra/git:v20210610-b2adebc132-arm64
Finished Step #1
Starting Step #2
Step #2: Already have image (with digest): gcr.io/cloud-builders/docker
Step #2: no such manifest: gcr.io/k8s-staging-test-infra/git:v20210610-b2adebc132
Finished Step #2
ERROR
ERROR: build step 2 "gcr.io/cloud-builders/docker" failed: step exited with non-zero status: 1

@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 Jun 10, 2021
@LorbusChris
Copy link
Contributor Author

@spiffxp thank you for taking a look 🙏
I've pushed an update, could you try again?

LorbusChris added a commit to LorbusChris/test-infra that referenced this pull request Jun 10, 2021
Uses a custom arm64 rebuild of the git image
TODO(lorbus): Use image build from kubernetes#22450

Requires: kubernetes#22450
LorbusChris added a commit to LorbusChris/test-infra that referenced this pull request Jun 10, 2021
Uses a custom arm64 rebuild of the git image
TODO(lorbus): Use image build from kubernetes#22450

Requires: kubernetes#22450
@LorbusChris
Copy link
Contributor Author

LorbusChris commented Jun 11, 2021

I'm not sure cloudbuild can actually do what I'd like it to do here. In any case this would require #22444 to go in first as that's the base image for this one.

@fejta
Copy link
Contributor

fejta commented Jun 12, 2021

Is docker buildx useful here?

@LorbusChris
Copy link
Contributor Author

@fejta I think it would be. Is there a builder base that has it? (It doesn't seem to be included in gcr.io/cloud-builders/docker)

@LorbusChris LorbusChris changed the title images/git: Build for arm64 WIP: images/git: Build for arm64 Jun 16, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2021
@LorbusChris LorbusChris changed the title WIP: images/git: Build for arm64 images/git: Build for arm64 Jun 18, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 18, 2021
@LorbusChris
Copy link
Contributor Author

#22444 has merged, this should be ready to go now
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2021
@@ -12,7 +12,7 @@
# See the License for the specific language governing permissions and
# limitations under the License.

FROM gcr.io/k8s-prow/alpine:v20200605-44f6c96
FROM gcr.io/k8s-prow/alpine:latest
Copy link
Contributor

@fejta fejta Jun 18, 2021

Choose a reason for hiding this comment

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

Please use a tagged version. We automatically send PRs to upgrade to the latest tagged version when this changes. (this is specific behavior for gcr.io/k8s-prow 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.

That probably means I have to wait until alpine is rebuilt with the changes from #22444, and the auto-PR changing the tag has merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm looks like #22444 broke alpine postsubmit job :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That issue seemed to be a transient one. Thanks for re-renunning it @fejta!
Thus, this PR here should be good to go now :)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, LorbusChris

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

The pull request process is described 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 approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2021
@LorbusChris
Copy link
Contributor Author

/hold
I forgot to update the FROM directive 😬, one sec

@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 Jun 18, 2021
This commit switches the builder base image used for building the git
image to `gcr.io/k8s-testimages/gcb-docker-gcloud` in order to do
multi-arch (amd64 and arm64) builds via `docker buildx build`, yielding
a multi-arch manifest.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2021
@LorbusChris
Copy link
Contributor Author

phew close call 😄

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2021
@fejta
Copy link
Contributor

fejta commented Jun 18, 2021 via email

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2021
@k8s-ci-robot k8s-ci-robot merged commit 1017135 into kubernetes:master Jun 18, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/images cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants