Delete nodes from k8s api during rolling-update #5794
Conversation
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
/lgt... hrm.
Why have we waited to hit this before implementing deleteNode on the k8s side?
This code works, but it feels a bit weird to me.
I'm a little bit concerned that we don't seem to have a way to differentiate between node $A launched on Monday and node $A launched on Tuesday (whether a k8s/k8s issue or a kops issue).
It's probably not an incredibly common problem, but it doesn't seem unreasonable to imagine this could be a fairly common edge case.
Is the only time we'd conceivably have two nodes in the case of cordoning/rolling-update or run into this?
Could we just assign the node a uuid?
This may be just a kops problem and if so, that's fine with me- let's fix it here,
pkg/instancegroups/instancegroups.go
Outdated
| @@ -172,12 +175,26 @@ func (r *RollingUpdateInstanceGroup) RollingUpdate(rollingUpdateData *RollingUpd | |||
| } | |||
| } | |||
|
|
|||
| // We unregister the node before deleting it; if the replacement comes up with the same name it would otherwise still be cordoned | |||
| // (GCE seems to recycle names more rapidly) | |||
There was a problem hiding this comment.
It often seems like GCE tries to re-use names.
pkg/instancegroups/instancegroups.go
Outdated
| // (GCE seems to recycle names more rapidly) | ||
| if !isBastion && !rollingUpdateData.CloudOnly { | ||
| if u.Node == nil { | ||
| glog.Warningf("skipping node unregister from kubernetes; node was not set") |
There was a problem hiding this comment.
what do you mean by "not set"?
can you use a more descriptive/useful word or phrase here pls? :)
There was a problem hiding this comment.
Switched to glog.Warningf("no kubernetes Node associated with %s, skipping node deletion", instanceId)
This prevents a race where if the new node comes back with the same name, it will still be cordoned. This seems to be more likely on GCE.
justinsb
force-pushed
the
delete_nodes_during_rolling_update
branch
from
637208e
to
e982087
Compare
|
So I think it's a kube problem, but the role of kops is just to make sure that we don't have two instances with the same name. (Edit: that we don't bring up a new node with the same name as an existing Node) It's much less likely on AWS, where we name instances after their internal IP, but it could happen if the new instance reuses the same IP. It seems to always happen on GCE with MIGs, at least right now. Not sure whether that has always been the case. I guess the right place to fix it is for the kubelet to know that it isn't the same node as the existing node, i.e. kubelet should check the machineid or something. But that isn't happening as far as I can tell (although I think I was testing with CoreOS, and maybe CoreOS has messed up whatever check kubelet is using). Anyway, I think deleting the node feels like the safe thing to do - it's just a little more predictable. |
|
/lgtm Ok cool- that makes sense to me, thanks for the fix and the info! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: geojaz, 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 |
This prevents a race where if the new node comes back with the same
name, it will still be cordoned. This seems to be more likely on GCE.