Make the kubelet on a GCE master check instance metadata for manifests #11963
Conversation
|
GCE e2e build/test failed for commit 18fa01fc0062e2f362438f0452618fa48474aa7b. |
|
Jenkins failure is due to a test environment issue: |
|
@k8s-bot test this please |
|
GCE e2e build/test passed for commit 18fa01fc0062e2f362438f0452618fa48474aa7b. |
| @@ -41,6 +41,12 @@ | |||
| {% endif -%} | |||
|
|
|||
| {% set config = "--config=/etc/kubernetes/manifests" -%} | |||
|
|
|||
| {% set manifest_url = "" -%} | |||
| {% if grains['roles'][0] == 'kubernetes-master' and grains.cloud in ['gce'] -%} | |||
There was a problem hiding this comment.
Is there a reason to only do this on the master? We will (hopefully soon) treat the master node on GCE just like any other node, so we should look at reducing the number of special cases we apply to the master.
There was a problem hiding this comment.
Well, the daemon controller will handle the node case in the very near future, and adding it to the nodes adds another edge-feature that isn't worth properly supporting given that there's not much use for it once the daemon controller exists.
That said, reducing special cases is a reasonable goal, so I'm happy to go that way if you'd prefer.
There was a problem hiding this comment.
I don't think there's much reason to do it on the nodes, just wanted to think about what it would entail.
|
LGTM. Please cherry pick this into the 1.0 release branch as well. |
|
|
||
| {% set manifest_url = "" -%} | ||
| {% if grains['roles'][0] == 'kubernetes-master' and grains.cloud in ['gce'] -%} | ||
| {% set manifest_url = "--manifest-url=http://metadata.google.internal/computeMetadata/v1beta1/instance/attributes/google-container-manifest" -%} |
There was a problem hiding this comment.
This should use the v1 api rather than the v1beta1 api.
There was a problem hiding this comment.
I'm testing it, but I don't think the kubelet knows to set the "Metadata-Flavor: Google" header on its requests
There was a problem hiding this comment.
Yeah, I've confirmed the kubelet isn't smart enough to hit the v1 metadata server...
There was a problem hiding this comment.
Ugh, that means that an internal change I have won't work (I haven't tested it yet).
There was a problem hiding this comment.
Working on it in #11982
roberthbailey
added
the
lgtm
|
I noticed this morning that the downside of this is that it causes the kubelet to spit out an error log message every 20 seconds if there is no pod manifest in the metadata. Maybe this setting should be controlled by a piece of kube-env? |
a-robinson
removed
the
lgtm
Primary motivation: enable GKE and other cluster-as-a-service folks to easily run additional logic on the master without having to modify salt or SSH to the master after it's been created.
|
Switched to use the v1 metadata API after rebasing onto #12024. Running one final kube-up with a pod specified in the metadata before re-attaching the LGTM label I previously removed. |
|
Seems to be working swimmingly, and I verified as part of the other PR that it doesn't complain more than 3 times if there isn't a manifest in the metadata. re-applying @roberthbailey's LGTM |
a-robinson
added
the
lgtm
|
GCE e2e build/test passed for commit 94ae0a9. |
|
Still lgtm. |
|
Shippable flaked on the integration test. Should be good to merge. I'd kick shippable, but the retry button is missing. |
|
pinging @mikedanese to get this in sometime today |
|
It's on my radar. |
Make the kubelet on a GCE master check instance metadata for manifests
|
Please cc to me or node team next time on any node / kubelet related configuration changes. I were surprised with this pr when I saw #15122 yesterday. We spent sometime on making node configuration consistently (of course not true in today's situation), but this change is totally opposite on what we are doing. cc/ @yujuhong |
|
cc/ @kubernetes/goog-node |
Primary motivation: enable GKE and other cluster-as-a-service folks to easily run additional logic on the master without having to modify salt or SSH to the master after it's been created.
Verified to work on GCE.
@brendandburns @roberthbailey @zmerlynn