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

Add support for running GCI on the GCE cloud provider #25425

Merged
merged 1 commit into from
May 19, 2016
Merged

Add support for running GCI on the GCE cloud provider #25425

merged 1 commit into from
May 19, 2016

Conversation

andyzheng0831
Copy link

The supports and configs for GCI added here will be used in 1.3 release and upwards. For 1.2.X releases, we will still use cluster/gce/trusty code. Therefore, we will keep maintaining the trusty code as long as 1.2-release branch is not deprecated. Unlike the trusty code, the GCI cluster initialization is through systemd.

This PR is fully tested using e2e tests. I wrote a script to repeatedly spin up a cluster, run CI and slow tests, and then tear down for 15 times for both pure GCI cluster and hybrid cluster (master on GCI, nodes on containervm). The entire tests lasted for more than 3 days, and no failure was shown. I also verified creating trusty cluster, as this PR revises some places affecting it.

@roberthbailey @dchen1107 please review it.

cc/ @fabioy @wonderfly @kubernetes/goog-image FYI.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 10, 2016
@roberthbailey roberthbailey assigned roberthbailey and unassigned ikehz May 10, 2016
@roberthbailey roberthbailey changed the title Add supports and configs dedicated for GCI Add support for running GCI on the GCE cloud provider May 10, 2016
@roberthbailey roberthbailey added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 10, 2016
@andyzheng0831
Copy link
Author

Update the PR by running "hack/verify-flags-underscore.py -e > hack/verify-flags/exceptions.txt". But I have no idea why there are some flags not related with this PR updated by the script.

@@ -1116,11 +1118,10 @@ function kube::release::gcs::copy_release_artifacts() {

# Having the configure-vm.sh script and trusty code from the GCE cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

should this comment now say "configure-vm.sh script and gci code from ..."

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for catching it. Will correct this.

@roberthbailey
Copy link
Contributor

@zmerlynn can you take a look as well since these scripts are closely related to configure-vm.sh?

@andyzheng0831
Copy link
Author

@roberthbailey An updated version is uploaded, which addressed your comments. But we still have a couple of comments waiting for more information from @zmerlynn and @mikedanese . I am reruning e2e for at least 5 times against this new version

@andyzheng0831
Copy link
Author

@zmerlynn @roberthbailey I just updated the PR by adding the /etc/motd support (see functions set-broken-motd and reset-motd) and licensing work. Since /usr/local/ is readonly in GCI, I push LICENSE file in /home/kubernetes, and display the info by /etc/motd. I tested the new version using e2e tests.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016
# Finds the master PD device; returns it in MASTER_PD_DEVICE
function find-master-pd {
MASTER_PD_DEVICE=""
# TODO(zmerlynn): GKE is still lagging in master-pd creation.
Copy link
Member

Choose a reason for hiding this comment

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

Since you forced into rebase anyways, can you remove this TODO from both sides? I've been meaning to remove it from one, but I'd rather not see it copied. :) (It's been done for a while.)

Copy link
Author

Choose a reason for hiding this comment

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

Sure, will remove it

@zmerlynn
Copy link
Member

LGTM. I left a nit but you can fix it or not.

@andyzheng0831
Copy link
Author

@zmerlynn @roberthbailey The PR is rebased. I am running e2e on GCI again to make sure no new problem introduced.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016
cp "${KUBE_HOME}/kube-manifests/kubernetes/kube-registry-proxy.yaml" /etc/kubernetes/manifests
fi
fi
reset-motd
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be the last thing that we do. Why do you configure fluentd after putting up the message?

Copy link
Author

Choose a reason for hiding this comment

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

Probably a paste error. Will update it.

@andyzheng0831
Copy link
Author

Let me also backport #25504 into this PR. In anyway I will need to merge it from trusty code to gci.

@andyzheng0831
Copy link
Author

Please check the latest version. I already ran e2e on GCI to verify it.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016
@andyzheng0831
Copy link
Author

Rebase done

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 18, 2016
@roberthbailey roberthbailey added lgtm "Looks good to me", indicates that a PR is ready to be merged. e2e-not-required labels May 18, 2016
@roberthbailey
Copy link
Contributor

@lavalamp -- Adding the e2e-not-required label since the only part of this PR that would be tested by the submit queue in the 1 line change to kubelet-checker.sh which is low risk.

@k8s-bot
Copy link

k8s-bot commented May 18, 2016

GCE e2e build/test passed for commit a737e1e.

@lavalamp lavalamp merged commit aabc0a3 into kubernetes:master May 19, 2016
@andyzheng0831 andyzheng0831 deleted the real-gci branch May 19, 2016 00:32
@lavalamp
Copy link
Member

GKE cluster start up broke... this is the only thing that changed the start up path or that looks remotely suspicious. Reverting, sorry.

@wonderfly
Copy link
Contributor

Looks like there is no plan to cherry pick this to release-1.2? I am asking because the support of OS_DISTRIBUTION=gci in release-1.2 is broken in the sense that it doesn't let users choose MASTER_IMAGE_PROJECT so all GCI release tests (that grab GCI images from a staging project) will fail if they set OS_DISTRIBUTION=gci (as what's happening right now).

I'd like to send a PR directly to the release-1.2 branch to fix that, but that doesn't seem to be a recommended practice...

@roberthbailey
Copy link
Contributor

Looks like there is no plan to cherry pick this to release-1.2?

Not that I know of. I think @andyzheng0831's plan was to keep using trusty, since it exists on that release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants