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.

Sign up for GitHub

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

Jump to bottom

Release images bundle instead of separate images #11522

Merged
merged 1 commit into from
May 20, 2021
Merged

Release images bundle instead of separate images #11522

merged 1 commit into from
May 20, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented May 19, 2021

kOps releases contain a considerable number of files, out of which only the binaries are currently used for mirrors.
As agreed during Office Hours, images should be bundled into a single archive file.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 19, 2021
johngmyers
johngmyers reviewed May 19, 2021
View reviewed changes
Makefile Outdated
@@ -769,6 +771,8 @@ bazel-version-dist: bazel-version-dist-linux-amd64 bazel-version-dist-linux-arm6
tools/sha256 ${BAZELUPLOAD}/kops/${VERSION}/darwin/amd64/kops ${BAZELUPLOAD}/kops/${VERSION}/darwin/amd64/kops.sha256
cp bazel-bin/cmd/kops/windows-amd64/kops ${BAZELUPLOAD}/kops/${VERSION}/windows/amd64/kops.exe
tools/sha256 ${BAZELUPLOAD}/kops/${VERSION}/windows/amd64/kops.exe ${BAZELUPLOAD}/kops/${VERSION}/windows/amd64/kops.exe.sha256
tar cfvz ${BAZELUPLOAD}/kops/${VERSION}/images/images.tar.gz -C ${BAZELIMAGES} ${BAZELIMAGES}/*.tar.gz ${BAZELIMAGES}/*.tar.gz.sha256
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite see the point of including the .sha256 of the individual images into the tar file.

Copy link
Member Author

Choose a reason for hiding this comment

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

People may want to validate those individually, or compare to what we have in the GCS bucket.

Copy link
Member

Choose a reason for hiding this comment

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

I'm struggling to figure out a valid use case for comparing a .sha256 in a tar file against another file in the same tar file. What would be the value in validating those individually or comparing to what is in the GCS bucket?

With separate files you can retrieve the small file through a more secure path or use the redundancy to detect cosmic-ray-type errors in the big file. But given the unlikelihood of doing a partial-file download or wanting to salvage part of a slightly corrupted tar file, these use cases don't apply well to checksums embedded in the big file.

Copy link
Member Author

Choose a reason for hiding this comment

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

They don't. There is no real use case, but doesn't break anything either.
if you think it would be best to exclude them, I don't mind doing it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it would be best to exclude them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@hakman hakman force-pushed the release-image-bundle branch 4 times, most recently from 43c04a4 to bebde39 Compare May 19, 2021 04:50
@hakman
Copy link
Member Author

hakman commented May 19, 2021

/retest

@hakman
Copy link
Member Author

hakman commented May 20, 2021

@johngmyers Is this ok as is now?

@johngmyers
Copy link
Member

I need time to run it in order to see what it produces. It's not obvious by reading the code.

@hakman
Copy link
Member Author

hakman commented May 20, 2021

If it helps, I tried to to run this as part of the build to see the output also:
https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kops/11522/pull-kops-e2e-kubernetes-aws/1394876368268824576/build-log.txt

tar cfvz /home/prow/go/src/k8s.io/kops/.bazelbuild/upload/kops/1.22.0-alpha.2+030ced6efb/images/images.tar.gz -C /home/prow/go/src/k8s.io/kops/.bazelbuild/dist/images .
./
./kops-controller-amd64.tar.gz.sha256
./kube-apiserver-healthcheck-amd64.tar.gz
./kops-controller-amd64.tar.gz
./kops-controller-arm64.tar.gz.sha256
./kube-apiserver-healthcheck-arm64.tar.gz.sha256
./dns-controller-arm64.tar.gz
./kops-controller-arm64.tar.gz
./dns-controller-amd64.tar.gz
./dns-controller-arm64.tar.gz.sha256
./kube-apiserver-healthcheck-arm64.tar.gz
./kube-apiserver-healthcheck-amd64.tar.gz.sha256
./dns-controller-amd64.tar.gz.sha256
tar tfv /home/prow/go/src/k8s.io/kops/.bazelbuild/upload/kops/1.22.0-alpha.2+030ced6efb/images/images.tar.gz
drwxr-xr-x root/root         0 2021-05-19 04:47 ./
-r-xr-xr-x root/root        65 2021-05-19 04:46 ./kops-controller-amd64.tar.gz.sha256
-r-xr-xr-x root/root  11600764 2021-05-19 04:46 ./kube-apiserver-healthcheck-amd64.tar.gz
-r-xr-xr-x root/root  40432271 2021-05-19 04:46 ./kops-controller-amd64.tar.gz
-r-xr-xr-x root/root        65 2021-05-19 04:47 ./kops-controller-arm64.tar.gz.sha256
-r-xr-xr-x root/root        65 2021-05-19 04:47 ./kube-apiserver-healthcheck-arm64.tar.gz.sha256
-r-xr-xr-x root/root  37781342 2021-05-19 04:47 ./dns-controller-arm64.tar.gz
-r-xr-xr-x root/root  37504590 2021-05-19 04:47 ./kops-controller-arm64.tar.gz
-r-xr-xr-x root/root  40790638 2021-05-19 04:46 ./dns-controller-amd64.tar.gz
-r-xr-xr-x root/root        65 2021-05-19 04:47 ./dns-controller-arm64.tar.gz.sha256
-r-xr-xr-x root/root  10460840 2021-05-19 04:47 ./kube-apiserver-healthcheck-arm64.tar.gz
-r-xr-xr-x root/root        65 2021-05-19 04:46 ./kube-apiserver-healthcheck-amd64.tar.gz.sha256
-r-xr-xr-x root/root        65 2021-05-19 04:46 ./dns-controller-amd64.tar.gz.sha256
tools/sha256 /home/prow/go/src/k8s.io/kops/.bazelbuild/upload/kops/1.22.0-alpha.2+030ced6efb/images/images.tar.gz /home/prow/go/src/k8s.io/kops/.bazelbuild/upload/kops/1.22.0-alpha.2+030ced6efb/images/images.tar.gz.sha256

@hakman
Copy link
Member Author

hakman commented May 20, 2021

I also was thinking that maybe we start doing make bazel-version-dist instead of make bazel-build in pull-kops-bazel-build. This would make it easier to review changes like this one and have more confidence that release builds are not broken.
/cc @rifelpet

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 May 20, 2021
@hakman hakman mentioned this pull request May 20, 2021
Merged
@k8s-ci-robot k8s-ci-robot merged commit b5e5e85 into kubernetes:master May 20, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone May 20, 2021
olemarkus pushed a commit to olemarkus/kops that referenced this pull request May 20, 2021
@k8s-ci-robot
Merge pull request kubernetes#11522 from hakman/release-image-bundle
558bc5d 
Release images bundle instead of separate images
@hakman hakman deleted the release-image-bundle branch May 20, 2021 08:53
k8s-ci-robot added a commit that referenced this pull request May 20, 2021
@k8s-ci-robot
Merge pull request #11543 from hakman/automated-cherry-pick-of-#11522…
b0c17fa 
…-upstream-release-1.21

Automated cherry pick of #11522: Release images bundle instead of separate images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johngmyers johngmyers johngmyers approved these changes

@mikesplain mikesplain Awaiting requested review from mikesplain

@rifelpet rifelpet Awaiting requested review from rifelpet

Assignees

@johngmyers johngmyers

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.22
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@hakman @johngmyers @k8s-ci-robot