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

Use release/push-build.sh in ci-kubernetes-bazel-build #2732

Closed

Conversation

ixdy
Copy link
Member

@ixdy ixdy commented May 11, 2017

This will upload the Bazel-built release artifacts to gs://kubernetes-release-dev/ci-bazel/$version, and will also generate the gs://kubernetes-release-dev/ci-bazel/latest*.txt files we need for CI.

Dependent on #2731.
x-ref #2704

@pipejakob this is changing the GCS paths for debs yet again (but hopefully for the last time)... is this going to break kubeadm CI?

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 11, 2017
@pipejakob
Copy link
Contributor

/assign

@pipejakob
Copy link
Contributor

If you're changing the GCS path again, then yes, it'll break our kubeadm e2e jobs.

@@ -34,7 +34,14 @@ if [[ "${rc}" == 0 ]]; then
echo "Kubernetes version missing; not uploading ci artifacts."
rc=1
else
bazel run //:ci-artifacts -- "gs://kubernetes-release-dev/bazel/${version}" && rc=$? || rc=$?
push_build="../release/push-build.sh"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused. Does this actually work at CI time? Even given your changes in #2731, I'm not sure what mechanism pulls the release repository. Is that done automatically for all jobs? I thought that this job only pulled k8s.io/kubernetes, so this looks like something that would only work locally.

args:
- "--branch=$(PULL_REFS)"
- "--repo=k8s.io/release=master"
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, my other comment obviously missed this subtle line.

@pipejakob
Copy link
Contributor

One of the problems of the kubeadm e2e prow pipeline is that steps can't communicate with each other, so the bazel build step that generates the .debs can't tell the e2e step where to find them, which is why we rely on well-known locations for them instead. We've just hit the problem where Kubernetes 1.5.x .debs are in a different location than 1.6.x and onward, so we have switching code to determine the correct path.

With this change moving the location again, and no obvious version differences to key off of, I'm not sure the best way to safely migrate the kubeadm pipelines one at a time. We could have another big-bang where we update them all at once, but if we want to do it incrementally, we'll probably either need to add a flag to the scenario to specify the storage location or bake the scenario into the e2e-kubeadm image as a way to version the scheme.

@ixdy
Copy link
Member Author

ixdy commented May 19, 2017

This should be the last time this changes. It should also let the kubeadm jobs use the same method as the e2e jobs in finding artifacts - just read the gs://kubernetes-release-dev/ci-bazel/latest.txt (or latest-1.6.txt) file to determine the most-recently build version, then read debs from gs://kubernetes-release-dev/ci-bazel/$VERSION. It should allow us to eliminate this logic, too.

@pipejakob
Copy link
Contributor

I'm having trouble following that last comment, so apologies if we're just not on the same page.

Is it an improvement to read a remote singleton latest.txt file instead of just looking up the $VERSION from the Kubernetes repo itself? I could see it being useful for periodic jobs, but for CI wouldn't that just introduce a small race condition where we might accidentally use a different version than the commit we think we're testing, and misreport the results? If we know the exact commit we're trying to test in CI, and already have the repository checked out at that commit, why would we want to make a network call to look up the version (which could potentially be mismatched, if another build just finished after ours) instead of using the version information we already have locally in the repo?

As for this logic, I'm not sure exactly what this will eliminate. The code uses a separate identifier than the normal $VERSION for presubmit jobs because the building of .debs occurs in a different job than the one consuming them (they're built in the bazel job, and then kubeadm e2e jobs are chained afterward as run_on_success targets*), and performing the same merge twice at different times produces a different $VERSION (see #2509). I don't think we want to use something like latest.txt for presubmit jobs because then that race window of potentially testing the wrong artifacts becomes even more crucial if it means letting bad code in the repo or reporting failures to the wrong PR. The 1.6.x special-case code will only go away if we cherry-pick these changes into the release-1.6 branch, but that case would also go away if we just cherry-pick your previous commits to push artifacts to bazel/$VERSION/bin/$OS/$ARCH to release-1.6.

* This is temporarily untrue, but is meant to be true. The presubmit kubeadm e2e was moved off of the bazel prow pipeline in #2568 until it proved more stable, and should be put back now that it's been fixed.

@ixdy
Copy link
Member Author

ixdy commented Jun 2, 2017

Yeah, you raise a good point. For periodic jobs, we just want to use whatever is latest, but for presubmit and postsubmit chained jobs, we really want to test whatever was built in the previous stage. I don't think there's any way of passing along information between chained jobs, short of checking out the repository (which is a little gross).

@spxtr any thoughts on how chained prow jobs could pass metadata along? Is there some existing functionality I'm missing?

@fejta
Copy link
Contributor

fejta commented Aug 7, 2017

Is this still alive? Please update and/or close

@ixdy
Copy link
Member Author

ixdy commented Aug 7, 2017

I'd probably still like to do this, though it likely requires some additional work to pass metadata between jobs in prow, which I think @BenTheElder might be looking into. I will close this for now.

@ixdy ixdy closed this Aug 7, 2017
@BenTheElder
Copy link
Member

Yep, see #3966 for the start of that.

@ixdy ixdy deleted the prow-bazel-build-release-push-build branch May 15, 2018 23:51
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants