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

CI: only push tar instead of also container image #925

Merged
merged 1 commit into from
Jul 8, 2024
Merged

CI: only push tar instead of also container image #925

merged 1 commit into from
Jul 8, 2024

Conversation

BenTheElder
Copy link
Member

related to #920

AFAICT we don't need to push the image, and it will be tricker to enable pushing to a container registry with GC enabled, instead we can only push the tarball for e2e builds and leave container image pushing to release

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 8, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 8, 2024
@AnishShah
Copy link

looks like all NPD CI jobs is running NPD in standalone mode.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2024
This was referenced Jul 8, 2024
Merged
Closed
Closed
@wangzhen127
Copy link
Member

/lgtm
/hold

@DigitalVeer Can you revert kubernetes/test-infra#32893 now since we have accumulated enough failure signals?

@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 Jul 8, 2024
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2024
BenTheElder
BenTheElder commented Jul 8, 2024
View reviewed changes
test/build.sh Outdated
@@ -138,7 +138,7 @@ function build-ci() {
export REGISTRY="${NPD_STAGING_REGISTRY}/ci"
export VERSION="$(get-version)-$(date +%Y%m%d.%H%M)"
export TAG="${VERSION}"
make push
make push-tar
Copy link
Member Author

Choose a reason for hiding this comment

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

/hold

instead of reverting the test-infra change, let's switch to only push-tar here but add building but not pushing

Copy link
Member

Choose a reason for hiding this comment

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

SGTM

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2024
BenTheElder
BenTheElder commented Jul 8, 2024
View reviewed changes
test/build.sh
@@ -128,7 +128,7 @@ function build-pr() {
export REGISTRY="${NPD_STAGING_REGISTRY}/pr/${PR}"
export VERSION=$(get-version)
export TAG="${VERSION}"
make push
make push-tar
Copy link
Member Author

@BenTheElder BenTheElder Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

NOTE: pull-npd-build already runs make build which includes build-container, so there's no reason to build it in PR e2e runs.

Copy link
Member Author

Choose a reason for hiding this comment

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

(specifically it runs ./test/build.sh install-lib && make build)

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here to explain that this is only used by presubmit e2e? The build job is using make build which covers container build already?

@BenTheElder
Copy link
Member Author

/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 Jul 8, 2024
@BenTheElder
Copy link
Member Author

/hold

Actually, we don't need it here either do we?

What we really want is:

  1. presubmit build: ensures we can build, doesn't push. we have that already
  2. presubmit e2es: upload and test tarball, don't need to push container (this PR)
  3. postsubmit/periodic e2es: either upload and test tarball, or use a pre-uploaded tarball
  4. postsubmit/periodic build: ensure we can build container image
  5. release process: publishes builds for releases to permanent storage. we have that already.

@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 Jul 8, 2024
@wangzhen127
Copy link
Member

presubmit build: ensures we can build, doesn't push. we have that already

Yes

presubmit e2es: upload and test tarball, don't need to push container (this PR)

Yes. And no need to build container either.

postsubmit/periodic e2es: either upload and test tarball, or use a pre-uploaded tarball

This uses pre-uploaded tarball from postsubmit/periodic build.

postsubmit/periodic build: ensure we can build container image
release process: publishes builds for releases to permanent storage. we have that already.

We use github file upload for release. We need to build both container and tar file. We only need to push tar file.

@BenTheElder
Copy link
Member Author

So right nowci-npd-build runs test/build.sh ci while presubmit runs test/build.sh install-lib && make build

Periodic e2e seem to do:
./test/build.sh get-ci-env ...
(like this one https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-npd-e2e-kubernetes-gce-ubuntu/1810419258287460352)

Presubmit e2e do: ./test/build.sh -p $(BUILD_ID) pr

So "CI" mode is only used for ci-npd-build and push-tar + build-container makes sense there I think, and that should work on the community infra unless we run into bucket issues.

wangzhen127
wangzhen127 reviewed Jul 8, 2024
View reviewed changes
test/build.sh
@@ -128,7 +128,7 @@ function build-pr() {
export REGISTRY="${NPD_STAGING_REGISTRY}/pr/${PR}"
export VERSION=$(get-version)
export TAG="${VERSION}"
make push
make push-tar
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment here to explain that this is only used by presubmit e2e? The build job is using make build which covers container build already?

test/build.sh
@@ -138,7 +138,7 @@ function build-ci() {
export REGISTRY="${NPD_STAGING_REGISTRY}/ci"
export VERSION="$(get-version)-$(date +%Y%m%d.%H%M)"
export TAG="${VERSION}"
make push
make push-tar build-container
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment to explain that we do not need to push container here for CI tests.

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

@BenTheElder
Copy link
Member Author

So let's try this as-is, right now the ci-npd-build failure logs just point to the GCR push which we're dropping
/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 Jul 8, 2024
@BenTheElder
CI: build container and push tar, PR: push tar
4e0b915 
We only need the tar to run CI tests, but we should also test building the container.

We release the container and binaries independently of this, this script is for e2e tests.
@wangzhen127
Copy link
Member

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AnishShah, BenTheElder, wangzhen127

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 merged commit f0308d2 into kubernetes:master Jul 8, 2024
11 checks passed
@BenTheElder BenTheElder deleted the push-tar branch July 8, 2024 23:45
@BenTheElder BenTheElder mentioned this pull request Jul 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wangzhen127 wangzhen127 wangzhen127 left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@wangzhen127 wangzhen127

@AnishShah AnishShah

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BenTheElder @AnishShah @wangzhen127 @k8s-ci-robot