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

Make kubeadm job friendlier for local execution. #2183

Merged
merged 1 commit into from
Mar 13, 2017

Conversation

pipejakob
Copy link
Contributor

This lets users gracefully invoke the job's Docker image with mounted volumes for the necessary repositories (instead of cloning them).

Previously, they would have to comment out these git clone lines and rebuild the image or encounter an error because the directories already existed.

This lets users gracefully invoke the job's Docker image with mounted
volumes for the necessary repositories (instead of cloning them).
Previously, they would have to comment out these "git clone" lines and
rebuild the image or encounter an error because the directories already
existed.
@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
ln -s "${GOOGLE_APPLICATION_CREDENTIALS}" kubernetes-anywhere/phase1/gce/account.json
if [ ! -e kubernetes-anywhere/phase1/gce/account.json ]; then
ln -s "${GOOGLE_APPLICATION_CREDENTIALS}" kubernetes-anywhere/phase1/gce/account.json
fi

./test-infra/jenkins/bootstrap.py \
Copy link
Contributor

Choose a reason for hiding this comment

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

The fact that this image clones test infra seems to be the core problem.

@fejta fejta closed this Mar 9, 2017
@pipejakob
Copy link
Contributor Author

Instead of just closing the PR without merging, can we discuss the best way forward for this?

Cloning the test-infra in order to call bootstrap.py is what almost all of our other images do:

https://github.com/kubernetes/test-infra/blob/master/images/e2e-prow/runner
https://github.com/kubernetes/test-infra/blob/master/images/gcloud/runner
https://github.com/kubernetes/test-infra/blob/master/images/pull-test-infra-gubernator/runner
https://github.com/kubernetes/test-infra/blob/master/images/pull_kubernetes_bazel/runner

Are you suggesting that I bake the test-infra repo into the image instead? Is this PR not an incremental improvement over what's there?

@pipejakob pipejakob reopened this Mar 9, 2017
@fejta
Copy link
Contributor

fejta commented Mar 10, 2017

I now understand better why this image exists, but I still do not like its new dynamic behavior. Wouldn't it be easier and more efficient to just run the bootstrap command directly?
/assign

@pipejakob
Copy link
Contributor Author

You're right that I could run bootstrap.py directly for local execution, but what I'm trying to do is have local execution match production execution much more closely, which means running the entire job through the real Docker image. You might remember that the first iteration of this job in Jenkins required a lot of post-merge debugging precisely because my local testing environment didn't match the way Jenkins ran the job, which led to a lot of small fixes and long feedback loops to see if the job would get any further in its execution before hitting another error. In order to speed up development time, I want fewer surprises between local development and post-merge behavior, which means fewer differences between the two. This change makes it much easier to run the full e2e test locally from within the actual Docker container that will be used in production, but I am open to any other solutions that achieve the same result. This basically codifies what I had been doing locally (commenting out these git clone lines so I could mount local volumes instead), but if there's a better strategy to have more accurate test runs locally, I'm definitely open minded.

@fejta
Copy link
Contributor

fejta commented Mar 10, 2017

Ideally I would like to identify why your local run of bootstrap did not accurate represent what the container would do and we fix that.

However this is good for now.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 10, 2017
@pipejakob pipejakob merged commit 9987c74 into kubernetes:master Mar 13, 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.

3 participants