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

Return missing ClusterID error instead of ignoring it #60125

Merged
merged 2 commits into from Feb 27, 2018

Conversation

@vainu-arto
Contributor

vainu-arto commented Feb 21, 2018

This fixes issue #57382. In the cases I'm aware of kubelet cannot function if it can't detect the cluster it is running in, so the error should be passed up to the caller preventing initialization when kubelet would fail. This way the error can be detected and kubelet startup attempted again later (giving AWS time to apply the tags).

On AWS kubelet returns an error when started under conditions that do not allow it to work (AWS has not yet tagged the instance).
@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 21, 2018

/ok-to-test

@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 21, 2018

/assign @justinsb
Will it break something?

@vainu-arto

This comment has been minimized.

Contributor

vainu-arto commented Feb 21, 2018

If there is a case where kubelet is running on AWS and not having instance tags is expected and ok, this change will break that use case.

@jsafrane

This comment has been minimized.

Member

jsafrane commented Feb 21, 2018

/retest

@jberkus

This comment has been minimized.

jberkus commented Feb 23, 2018

reminder: we are now in Code Slush. This PR needs priority/ kind/ and status/approved-for-milestone labels in order to merge into 1.10. Thanks!

@chrislovecnm

This comment has been minimized.

Member

chrislovecnm commented Feb 23, 2018

/test pull-kubernetes-bazel-test

@chrislovecnm

This comment has been minimized.

Member

chrislovecnm commented Feb 23, 2018

We need e2e with spot instances ....

@chrislovecnm

This comment has been minimized.

Member

chrislovecnm commented Feb 23, 2018

If there is a case where kubelet is running on AWS and not having instance tags is expected and ok, this change will break that use case.

There is a use case for this?

@justinsb added the comment around the time of adding support for shared tags, so I am not certain what this change would break.

@justinsb

This comment has been minimized.

Member

justinsb commented Feb 26, 2018

/lgtm

I think the problems we're seeing with spot instances outweigh the risks to people with poorly configured clusters - better to force tagging than to limp along.

vainu-arto added some commits Feb 21, 2018

Add clusterid tags to the instances in AWS tests
In practice these were in most cases required to exist, but kubelet did not
previously enforce this. It now does, so these tests need to change a bit.
@justinsb

This comment has been minimized.

Member

justinsb commented Feb 26, 2018

/test pull-kubernetes-bazel-test

@justinsb

This comment has been minimized.

Member

justinsb commented Feb 26, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 26, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 26, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, vainu-arto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 27, 2018

Automatic merge from submit-queue (batch tested with PRs 60435, 60334, 60458, 59301, 60125). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 3ca89a3 into kubernetes:master Feb 27, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation vainu-arto authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@chen-anders

This comment has been minimized.

chen-anders commented Feb 27, 2018

Would we be able to cherry-pick this bug fix into 1.8/1.9? We had 2 clusters that were severely affected by this issue (#57382) when we had upgraded to 1.8. (e.g. any delay in AWS tagging the spot instance would cause somewhat silently failing kubelet init)

@vainu-arto vainu-arto deleted the vainu-arto:aws-missing-tags-error branch Mar 5, 2018

k8s-merge-robot added a commit that referenced this pull request Mar 30, 2018

Merge pull request #61136 from chen-anders/automated-cherry-pick-of-#…
…60125-release-1.9

Automatic merge from submit-queue.

Automated cherry pick of #60125: Add clusterid tags to the instances in AWS tests

Cherry pick of #60125 on release-1.9.

#60125: Add clusterid tags to the instances in AWS tests

k8s-merge-robot added a commit that referenced this pull request Apr 3, 2018

Merge pull request #61138 from chen-anders/automated-cherry-pick-of-#…
…60125-release-1.8

Automatic merge from submit-queue.

Automated cherry pick of #60125: Add clusterid tags to the instances in AWS tests

Cherry pick of #60125 on release-1.8.

#60125: Add clusterid tags to the instances in AWS tests
@rphillips

This comment has been minimized.

Member

rphillips commented Apr 5, 2018

Is there an upgrade path for Kubernetes clusters running on AWS that do not set the KubernetesCluster or the modern kubernetes.io/cluster/[clusterID] tags?

@justinsb

This comment has been minimized.

Member

justinsb commented Apr 6, 2018

@rphillips not sure I understand - you have to set one of the tags, I'm afraid. Is that a problem for your use case?

@rphillips

This comment has been minimized.

Member

rphillips commented Apr 6, 2018

Tags were never set on older versions of bootkube. I have fixed that going forward, but I suspect if other users of kubernetes had never set any tags, then upgrades are going to fail. I didn't see a warning in the release notes regarding this.

@jsravn

This comment has been minimized.

Contributor

jsravn commented Apr 24, 2018

Tags were never set on older versions of bootkube. I have fixed that going forward, but I suspect if other users of kubernetes had never set any tags, then upgrades are going to fail. I didn't see a warning in the release notes regarding this.

The 1.5->1.6 upgrade deletes your nodes when they reboot or kubelet is restarted. No mention in the CHANGELOG about requiring a tag - prior to 1.6 it wasn't necessary, at least unofficially. I made #50915 for it a while ago.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment