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 CI .deb builds in kubeadm e2e job. #2180

Merged

Conversation

pipejakob
Copy link
Contributor

Since this job is chained after ci-kubernetes-bazel-build (which publishes .debs), use those .debs while testing.

Since this job is chained after ci-kubernetes-bazel-build (which
publishes .debs), use those .debs while testing.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 8, 2017
# current working directory at the repository root. Grab the SCM_REVISION so we
# can use the .debs built during the bazel-build job that should have already
# succeeded.
export SCM_VERSION=$(./hack/print-workspace-status.sh | grep ^STABLE_BUILD_SCM_REVISION | cut -d' ' -f2)
Copy link
Member

Choose a reason for hiding this comment

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

this looks very magical, given here we are going to only support env vars, could this logic be in the kubeadm image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hrm, I don't think it can be in the image runner. The call stack looks like:

ci-kubernetes-e2e-kubeadm/runner # image entrypoint
\-> bootstrap.py
   \-> jobs/ci-kubernetes-e2e-kubeadm-gce.sh (the script in question)
      \-> e2e-runner.sh
         \-> kubetest

The kubernetes repo gets pulled by bootstrap.py, so the logic needs to be executed after that step, and I need to adjust the E2E_OPTS before e2e-runner.sh is invoked. I thought the job script was the perfect place for this, but if we only want to support static environment variables there, then does that mean I'll need to create a custom scenario to support this kind of logic (like kops has)?

Copy link
Member

Choose a reason for hiding this comment

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

You can get this in to make the test green first, and we can figure out where to move the logic to. It's easy to add a flag to the e2e scenario.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what? bootstrap.py should be launching an image, the runner should not be calling bootstrap.py

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fejta That may be the case when the job runs under Jenkins and uses dockerized-e2e-runner.sh, but this is a prow job, so it has its own custom Docker image. Prow launches the image defined in config.yaml, and the rest of the chain is the above, with the job calling e2e-runner.sh instead of dockerized-e2e-runner.sh to avoid Docker-in-Docker.

@krzyzacy Sounds good to me. Did you have any other feedback about the diff, or was that a very informal LGTM? :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Apologies! I clearly don't have the right context here. I'll get out of your way

Copy link
Member

Choose a reason for hiding this comment

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

@fejta this is within prow, so it's more like local mode, but it's not using e2e scenario yet.

I'll let @fejta have the final stamp, see if he agrees with me.

Copy link
Member

Choose a reason for hiding this comment

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

Seems he decided to throw this to me 👿

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fejta No worries, thanks for trying to help out. And yeah, this is definitely to support local testing of the job, but shouldn't actually affect how it runs in prow.

@krzyzacy Thanks. :-)

@fejta
Copy link
Contributor

fejta commented Mar 9, 2017

/assign

@fejta
Copy link
Contributor

fejta commented Mar 9, 2017

/assign @krzyzacy
/unassign

@k8s-ci-robot k8s-ci-robot assigned krzyzacy and unassigned fejta Mar 9, 2017
@krzyzacy
Copy link
Member

krzyzacy commented Mar 9, 2017

/lgtm

go for it

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 9, 2017
@pipejakob pipejakob merged commit 05ed222 into kubernetes:master Mar 9, 2017
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. lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants