-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Take node labels from cloud tags on AWS #9575
Conversation
/cc @rifelpet since reverting his PR. |
/retest |
/assign @justinsb |
/approve cancel |
cced7d0
to
b92da3e
Compare
I think @rifelpet was saying that AWS will fix soon the inability to handle tags with empty values. Maybe best to exclude from this PR. |
As can be seen from the expected test results, this PR will try to push |
For Cluster Autoscaler it does not matter what value the tags have. Could set the value to "1" or "true" for now and remove it when the AWS fix is fully deployed. It is already available in |
b92da3e
to
9e3e916
Compare
eca4809
to
cf3b6e2
Compare
I believe you can remove the revert commit now, the AWS fix has been rolled out. |
cf3b6e2
to
9d7c8b5
Compare
@olemarkus that Test_ServerGroupModelBuilder looks difficult to update. A good project would be to refactor it to use golden output files like the AWS tests use. |
It's #9211 that's causing I'm inclined to disable that test until it can be recoded to use |
I'm trying to understand the underlying problem. We update the instancegroup to add/remove labels, and then they get applied immediately? I'd like to get the nodes able to get their configuration directly from kops-controller rather than requiring it to be on S3 (and giving the nodes S3 permission); that does require all the information in the instancegroup.yaml. The PoC is in #8198 . So the issue is that if we head in that direction, we likely won't be able to put all the information into the instance tags (?). I feel that the problem likely still exists when booting the nodes as well, as they read the latest cluster & instancegroup from S3. So I'm trying to understand the problem & the direction here :-) The changes in here to fix the labels are good though - we should have been using BuildNodeLabels - that looks like a bug. |
82cf4cb
to
4d20ab1
Compare
I've listed the dependency graph in #9229. The underlying problem is indeed that changes to labels get applied too soon. There's also the issue that the instancegroup spec is written as the versioned API but read as the unversioned API. The direction I'm trying to take is refactoring away from reading things that are updated at edit time towards things that are updated only on a "kops update cluster --yes" or "terraform apply". Not that I understand why people want kops to generate Terraform output. |
/retest |
1 similar comment
/retest |
Yeah it is a beast, but it has also been very helpful. I'll about converting to golden output after I am done with the PR/branches I have in queue now. |
@justinsb I'm having some difficulty teasing out the remaining intent of #8198, especially since my recent kops-controller worker-node bootstrap work addresses the problem of getting the keypairs. We are currently advertising a model where edits to the cluster and instancegroup specs don't have an immediate effect on the cluster. In this model the admin can get a preview of the cloud-provider-level changes and the changes are applied as a set only when the admin takes a confirming action. After that point new instances get the new specification, whereas existing nodes tend to keep their existing configuration. Reality is a lot more fuzzy, with some changes taking effect at edit and/or preview time. I expect admins would find such discrepancies undesirably surprising. |
4d20ab1
to
8e98e9e
Compare
8e98e9e
to
f4cecc5
Compare
Thanks @johngmyers ... I think my objections were around whether there were going to be more things to solve down the road, which is a silly thing for me to block on ... let's try it and see! /approve We can discuss today whether we want this in 1.19 |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb 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 |
…575-upstream-release-1.19 Automated cherry pick of #9575: Rename NodeReconciler to LegacyNodeReconciler
On AWS, refactors the kops-controller to source the node labels from the instance's cloud tags. This is so kops-controller doesn't read the (possibly unapplied) instancegroup spec from the state store.
The other cloud providers are strongly encouraged to refactor to this design.
Reverts #9519 due to its inability to handle tags with empty values (as mentioned in #9536).Fixes #9856