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.

Sign up for GitHub

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

Jump to bottom

Return missing ClusterID error instead of ignoring it #60125

Merged
merged 2 commits into from Feb 27, 2018
Merged

Return missing ClusterID error instead of ignoring it #60125

merged 2 commits into from Feb 27, 2018

Conversation

ghost
Copy link

@ghost ghost 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).

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 21, 2018
@jsafrane
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 21, 2018
@jsafrane
Copy link
Member

/assign @justinsb
Will it break something?

@ghost
Copy link
Author

ghost 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
Copy link
Member

/retest

@jberkus
Copy link

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
Copy link
Contributor

/test pull-kubernetes-bazel-test

@chrislovecnm
Copy link
Contributor

We need e2e with spot instances ....

@chrislovecnm
Copy link
Contributor

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
Copy link
Member

/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.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 26, 2018
@justinsb justinsb added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/bug Categorizes issue or PR as related to a bug. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 26, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 26, 2018
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@vainu-arto
Add clusterid tags to the instances in AWS tests
a58f16b 
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.
@vainu-arto
Return missing ClusterID error instead of ignoring it
cba110a 
This fixes issue #57382.
@justinsb
Copy link
Member

/test pull-kubernetes-bazel-test

@justinsb
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018
@k8s-ci-robot
Copy link
Contributor

[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

@tpepper tpepper mentioned this pull request Feb 26, 2018
Closed
@k8s-github-robot
Copy link

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-github-robot k8s-github-robot merged commit 3ca89a3 into kubernetes:master Feb 27, 2018
@chen-anders
Copy link

chen-anders commented Feb 27, 2018 •
edited
Loading

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)

This was referenced Feb 27, 2018
Closed
Closed
@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2018
@mbohlool mbohlool removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2018
@ghost ghost deleted the aws-missing-tags-error branch March 5, 2018 08:27
This was referenced Mar 13, 2018
Merged
Merged
@jlaswell jlaswell mentioned this pull request Mar 20, 2018
Closed
This was referenced Mar 27, 2018
Closed
Closed
k8s-github-robot pushed a commit that referenced this pull request Mar 30, 2018
Merge pull request #61136 from chen-anders/automated-cherry-pick-of-#…
4d7e9ac 
…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-github-robot pushed a commit that referenced this pull request Apr 3, 2018
Merge pull request #61138 from chen-anders/automated-cherry-pick-of-#…
bd1385a 
…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
Copy link
Member

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
Copy link
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
Copy link
Member

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.

@superseb superseb mentioned this pull request Apr 14, 2018
Closed
@jsravn
Copy link
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.

@azman0101 azman0101 mentioned this pull request Jun 15, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm Awaiting requested review from chrislovecnm

@jsafrane jsafrane Awaiting requested review from jsafrane

Assignees

@justinsb justinsb

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.