Skip to content
This repository was archived by the owner on Sep 18, 2020. It is now read-only.

Fix pod deletion to be compatible with Kubernetes 1.9#164

Merged
dghubble merged 1 commit into
coreos:masterfrom
dghubble:fix-pod-drain
Jan 17, 2018
Merged

Fix pod deletion to be compatible with Kubernetes 1.9#164
dghubble merged 1 commit into
coreos:masterfrom
dghubble:fix-pod-drain

Conversation

@dghubble

@dghubble dghubble commented Jan 13, 2018

Copy link
Copy Markdown
Contributor
  • Check each pod's OwnerReferences to determine whether it is owned by an existing Daemonset. If so, exclude it from deletion as the pod would just be recreated by the Daemonset controller
  • Drop usage of CreatedByAnnotation to deserialize the owner object of a pod when listing pods to delete during agent drains

Testing

Running locksmithctl send-need-reboot is not enough to reproduce the original issue. I deployed a Typhoon 1.9.1 cluster with an intentionally old version of Container Linux. CLUO v0.4.1 reproduced the issue in which one node became unscheduable at a time, with no nodes able to actually upgrade.

Edited to test image and Container Linux upgrades were able to be applied.

  • quay.io/dghubble/container-linux-update-operator:77ac24a86c4e74d30e1a04bc6aa808371a1deb8e

A proper release will be forthcoming.

See #163

@dghubble
dghubble requested review from crawford and sdemos January 13, 2018 00:54
@dghubble dghubble added the bug label Jan 13, 2018
@dghubble dghubble removed the bug label Jan 13, 2018
@dghubble
dghubble requested review from ericchiang and removed request for crawford January 13, 2018 01:04
Comment thread pkg/drain/drain.go Outdated
switch controllerRef.Kind {
case "DaemonSet":
return kc.ExtensionsV1beta1().DaemonSets(sr.Reference.Namespace).Get(sr.Reference.Name, v1meta.GetOptions{})
return kc.Extensions().DaemonSets(namespace).Get(controllerRef.Name, v1meta.GetOptions{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if drain uses Extensions, I think the versioned client is probably correct.

@ericchiang ericchiang Jan 16, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, DaemonSet is now also in the apps API group. I don't know if Extensions() will list everything.

* Check each pod's OwnerReferences to determine whether it is
owned by an existing Daemonset. If so, exclude it from deletion
as the pod would just be recreated by the Daemonset controller
* Drop usage of CreatedByAnnotation to deserialize the owner
object of a pod when listing pods to delete during agent drains
@dghubble

Copy link
Copy Markdown
Contributor Author

Retested on another v1.9.1 cluster with quay.io/dghubble/container-linux-update-operator:77ac24a86c4e74d30e1a04bc6aa808371a1deb8e and it was able to apply updates to all nodes.

Comment thread pkg/drain/drain.go
switch controllerRef.Kind {
case "DaemonSet":
return kc.ExtensionsV1beta1().DaemonSets(sr.Reference.Namespace).Get(sr.Reference.Name, v1meta.GetOptions{})
return kc.ExtensionsV1beta1().DaemonSets(namespace).Get(controllerRef.Name, v1meta.GetOptions{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still concerned this won't identify daemonsets installed through the apps API group.

@dghubble dghubble Jan 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what its worth, kubernetes/kubernetes drain (master) only seems to use client.ExtensionsV1beta1 in detecting DaemonSets.

https://github.com/kubernetes/kubernetes/blob/48f69ac964b9a96b55351a3541f285b4cb5611bb/pkg/kubectl/cmd/drain.go#L341
https://github.com/kubernetes/kubernetes/blob/48f69ac964b9a96b55351a3541f285b4cb5611bb/pkg/kubectl/cmd/drain.go#L362

Referring to a specific hash (post 1.9) so the links don't break in future.

@dghubble dghubble Jan 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. A typical cluster has a few daemonsets to experiment with:

NAMESPACE            NAME                           DESIRED   CURRENT   READY     UP-TO-DATE   AVAILABLE   NODE SELECTOR                     AGE
kube-system          calico-node                    4         4         4         4            4           <none>                            19h
kube-system          kube-apiserver                 1         1         1         1            1           node-role.kubernetes.io/master=   19h
kube-system          kube-proxy                     4         4         4         4            4           <none>                            19h
kube-system          pod-checkpointer               1         1         1         1            1           node-role.kubernetes.io/master=   19h
monitoring           node-exporter                  4         4         4         4            4           <none>                            13h
reboot-coordinator   container-linux-update-agent   4         4         4         4            4           <none>                            13h

At present, these daemonsets are all defined as "extensions/v1beta1 DaemonSet" objects. We can

  • Switch a few to "apps/v1 DaemonSet"
  • Intentionally orphan the DaemonSet pods with kubectl delete daemonset node-exporter --cascade=false
  • Watch update-agent logs to see the orphaned pod is deleted (log line)
  • Watch update-agent logs to see the non-orphaned pod is not deleted

@dghubble dghubble Jan 17, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed node-exporter to apps/v1 DaemonSet, it was correctly drained. Change update-agent itself to apps/v1 DaemonSet, it was correctly skipped because it is owned by an existing DaemonSet.

I0117 19:48:14.635108       1 agent.go:130] Setting annotations map[string]string{"container-linux-update.v1.coreos.com/reboot-in-progress":"true"}
I0117 19:48:14.655798       1 agent.go:142] Marking node as unschedulable                      
I0117 19:48:14.673086       1 agent.go:147] Getting pod list for deletion                      
I0117 19:48:14.690520       1 agent.go:156] Deleting 1 pods                                    
I0117 19:48:14.690619       1 agent.go:159] Terminating pod "node-exporter-rsgbn"...           
I0117 19:48:14.715594       1 agent.go:171] Waiting for pod "node-exporter-rsgbn" to terminate 
I0117 19:48:24.731064       1 agent.go:374] Deleted pod "node-exporter-rsgbn"                  
I0117 19:48:24.731089       1 agent.go:180] Node drained, rebooting        

@ericchiang ericchiang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API server is magic

$ curl -s --cert cluster/tls/admin.crt --key cluster/tls/admin.key -k https://172.17.4.100:6443/apis/extensions/v1beta1/namespaces/kube-system/daemonsets | jq .items[].metadata.name
"kube-apiserver"
"kube-flannel"
"kube-proxy"
"pod-checkpointer"
$ curl -s --cert cluster/tls/admin.crt --key cluster/tls/admin.key -k https://172.17.4.100:6443/apis/apps/v1/namespaces/kube-system/daemonsets | jq .items[].metadata.name
"kube-apiserver"
"kube-flannel"
"kube-proxy"
"pod-checkpointer"

Sorry for not testing that earlier.

lgtm

@ericchiang

Copy link
Copy Markdown
Contributor

merge whenever

@dghubble
dghubble merged commit e327c57 into coreos:master Jan 17, 2018
@dghubble
dghubble deleted the fix-pod-drain branch January 17, 2018 20:47
@dghubble dghubble mentioned this pull request Jan 17, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants