Refactor cluster/gce/trusty/node.yaml #16960
Conversation
|
cc/ @dchen1107 |
|
I have manually run e2e tests against this change. No failure happened. |
|
@yifan-gu, FYI this change has a minor conflict with your pr 16760 in a couple of files. So, the one merged later will need to sync and resolve the conflict. |
|
Labelling this PR as size/L |
k8s-github-robot
added
the
size/L
|
@andyzheng0831 Ack, thanks :) |
|
@andyzheng0831 this isn't intended to get cherry-picked, right? |
|
@roberthbailey no, we don't plan to cherry pick this to 1.1 release. This refactoring is mainly for the next step, i.e., master on trusty. |
|
Thanks. In that case, if you don't mind I'll take a look at this next week once 1.1 is cut. |
|
Absolutely no problem. Please take your time |
|
GCE e2e test build/test passed for commit 6842d52e2eccd342193d816ab91889634aca6038. |
|
@andyzheng0831 We can probably merge this PR before #16760 |
dchen1107
added
sig/node
| @@ -0,0 +1,221 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Shouldn't with your refactory, the methods defined here can be reused for both coreos and trusty, so that we can remove those duplicated code when introducing trusty at the first place?
There was a problem hiding this comment.
Good idea. These functions are also useful for coreos, and others are not suitable.
- config_hostname()
- create_dirs()
- create_kubelet_kubeconfig()
- create_kubeproxy_kubeconfig()
- install_kube_binary_config()
Maybe we can refactor the code step by step: (1) move the code from trusty/node.yaml to a script (as shown in this PR); (2) refactor coreos config to make use of the script.
There was a problem hiding this comment.
FWIW, I think mount-master-pd(), create-salt-master-auth() in configure-vm.sh are reusable as well. I'd like to have those in a seperate shell so all the distros can import.
There was a problem hiding this comment.
Yes, they may be reusable across different distros. This PR only addresses the stuff we have already checked in. When I step on the master side work, I will be more clear about what master configuration should be reused. In addition, this refactoring is supposed to handle the trusty node config. Step-by-step work is easier to track and merge.
There was a problem hiding this comment.
Ok, I can take this as is for now and expect next PRs for handle those. Please add a TODO.
There was a problem hiding this comment.
Thanks. I notice Yifan is refactoring and adding coreos supports. I would like to coordinate with Yifan for the refactoring effort, so as to avoid conflict and to make stuff under cluster/gce/ cleaner and conciser.
@yifan-gu , does it make sense?
|
LGTM |
dchen1107
added
the
lgtm
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
GCE e2e build/test failed for commit 6842d52e2eccd342193d816ab91889634aca6038. |
|
PR changed after LGTM, removing LGTM. |
k8s-github-robot
removed
the
lgtm
|
GCE e2e build/test failed for commit 6c774dde364354909772c9519860d243c81122c8. |
|
@andyzheng0831 what is changed with your new push? |
|
No real change. I just resynced it, as it was made one week ago.
|
|
@dchen1107 I find that renaming node.yaml to node.mime seems like an unwise action. In #13101, I staged node.yaml for GKE use. If I change the name here, I have to change GKE code, which will unnecessarily complicate the logic. I would like to revert the name change part. Do you have any concern? |
The node.yaml has some logic that will be also used by the kubernetes master on trusty work (issue #16702). This change moves the code shared by the master and node configuration to a separate script, and the master and node configuration can source it to use the code. Moreover, this change stages the script for GKE use.
|
I revert the renaming code, and add staging the newly added cluster/gce/trusty/configure.sh |
|
GCE e2e build/test failed for commit 5ca0704. |
|
The test keeps failing. I check the Jenkins log and find the following problem. This happens even before creating the master and nodes. Maybe Jenkins administrator should clean up something. ERROR: (gcloud.compute.addresses.create) Some requests did not succeed: |
|
GCE e2e build/test failed for commit 5ca0704. |
|
Jenkins still fails with the following message 22:01:02 ERROR: Step ‘Publish JUnit test result report’ failed: No test report files were found. Configuration error? |
|
@k8s-bot test this please. |
|
GCE e2e test build/test passed for commit 5ca0704. |
|
e2e passed! |
|
LGTM. |
dchen1107
added
the
lgtm
|
Continuous integration appears to have missed, closing and re-opening to trigger it |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 5ca0704. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
Node.yaml has some logic that will be also used by the kubernetes master on trusty work (issue #16702). This change moves the code shared by the master and nodes configuration to a separate script, and the master and node configuration jobs can source it to use the code. Moreover, this change renames node.yaml to node.mime. Actually, the file is in MIME format instead of yaml. The original name confuses people.