update-agent: wait for pods to terminate before rebooting by hankjacobs · Pull Request #137 · coreos/container-linux-update-operator

hankjacobs · 2017-09-29T23:42:22Z

This change makes the agent wait until all pods are terminated before rebooting. Prior to this change, the agent would reboot immediately after the pod delete request was made and, consequently, pods were not given the time needed to terminate gracefully.

coreosbot · 2017-09-29T23:42:25Z

Can one of the admins verify this patch?

hankjacobs · 2017-10-05T19:33:26Z

Just a friendly ping on this

dghubble

Minor tweaks related to polling interval, timeout, and not terminating on transient errors.

dghubble · 2017-10-09T20:51:30Z


+// waitForPodDeletion waits for a pod to be deleted
+func (k *Klocksmith) waitForPodDeletion(pod v1.Pod) error {
+	return wait.PollInfinite(time.Second, func() (bool, error) {


Let's not poll every second. We already have issues filed about CLUO having too aggressive call rates. Other code in agent polls every 10 seconds.

const reapInterval = 10 * time.Second ... wait.PollUntil(reapInterval, func() (bool, error) {...})

Forever is a long time to wait as well. We should probably set a sane maximum time we're willing to wait for a troublesome pod before proceeding with the shutdown anyway (e.g. 10 min). Otherwise one app developer with a misbehaving app and an unreasonable grace period will stall Container Linux updates for the cluster.

const ( reapInterval = 10 * time.Second reapTimeout = 10 * time.Minute ) ... wait.Poll(reapInterval, reapTimeout, func() (bool, error) {...})

Any opposition to making the reap timeout configurable? 10 minutes seems reasonable for most use cases but there are some that require a much longer timeout. Specifically, some of our pods running in our cluster can take up to 24 hours to terminate gracefully in certain scenarios.

I'm not greatly in favor of new flags, unless really needed. Apps that require 24 hours to terminate gracefully, on clusters designed around the idea that individual nodes can go down and workloads move flexibly?

If you've been using the original PR on clusters (and we mentioned that on any transient call error, the wait would exit), it sounds like your apps are already subject to ungraceful shutdown?

We haven't been using it on our clusters for the very reason this PR was opened. The fact that pods don't terminate gracefully before reboot showed up in our testing prior to rolling this out.

Yep, it's most definitely not ideal that these jobs can take so long (most of the time they don't) and it's something that is being worked on but we have to work around it for the time being.

24 hours is most definitely an outlier but I'd argue that 11 minutes isn't. 15 minutes still sounds pretty reasonable to me depending on what's running in the cluster. The point I'm trying to make is that the reap timeout is heavily dependent on the type of workload that runs on your cluster. Because it could be so variable, I'd say it's worth making into a flag.

Ah sorry, misunderstood that last question. We’ve used this PR on our test cluster running a similar workload but with not nearly as long of running.

Hm, I'm not excited to add config flags (flags cascade into higher-level components), but I can see a --grace-period flag on update-agent (mirroring the --grace-period flag of kubectl drain) being warranted here. It does also set expectations that there has to be some period of time beyond which we give up on waiting (i.e. its best-effort, we can't guarantee we'll satisfy every pod grace period).

dghubble · 2017-10-09T21:01:35Z

+			return false, nil
+		case errors.IsNotFound(err):
+			return true, nil
+		default:


Handling this more naively could improve logs and retry. Compared with logging at the top level where the wait return error is just a loose approximation of what happened.

_, err := k.kc.CoreV1().Pods(pod.Namespace).Get(pod.Name, v1meta.GetOptions{}) if errors.IsNotFound(err) { glog.Infof("deleted pod %s", pod.Name) // success log return true, nil } if err != nil { glog.Errorf("failed to get pod %v", err) // some API call error, likely transient } return false, nil

So then success looks like:

Waiting for pod POD to terminate failed to get pod: transient error failed to get pod: transient error deleted pod POD

And "failure" gets logged (by the caller) as skipping the wait.

Waiting for pod POD to terminate failed to get pod: transient error failed to get pod: transient error Skipping wait on pod POD: timeout error

dghubble · 2017-10-09T21:13:58Z

+	for _, pod := range pods {
+		glog.Infof("Waiting for pod %q to terminate", pod.Name)
+		if err := k.waitForPodDeletion(pod); err != nil {
+			glog.Errorf("failed waiting for pod %q to terminate: %v", pod.Name, err)


In practice, many of these log lines will be about transient errors to make API calls and we shouldn't stop the wait for them (e.g. "failed waiting for pod xx to terminate: can't reach 10.3.0.1"). Other uses of wait in agent allow the anon function to log errors and return nil to continue trying.

On timeout, we can say we're skipping waiting for the pod to terminate.

hankjacobs · 2017-10-10T22:57:23Z

@dghubble feedback implemented. thanks!

FaKod · 2017-10-11T04:53:24Z

Just a comment on this. I am using Rook (a Ceph based cluster filesystem) and failed with locksmith and this PR to reboot updated nodes.
This PR doesn't help because Rooks Pods (OSDs) are killed before the Pods of f.e. ElasticSearch are killed. So the ES Pods cannot be terminated at all by K8s and the agent waits forever.

hankjacobs · 2017-10-11T05:07:35Z

@FaKod If graceful termination of a pod is dependent on the order in which other pods are terminated, this PR won't help much. That being said, the agent shouldn't hang with the changes I pushed ~6 hours ago. The agent should timeout after 10 minutes of waiting for a pod to terminate. Did you try with that version? If so, could you send me the agent logs? If not, could you try with that version? My testing proved successful in our test cluster.

FaKod · 2017-10-11T05:16:13Z

@hankjacobs In case of rook: Although pods are terminated eventually, the mounts are still valid on the node. That prevents a node from restarting. See rook/rook#1018

This may be a specific issue of Rook. What would help is to call "drain" before and wait for the non DaemonSet Pods to terminate.

hankjacobs · 2017-10-11T05:32:10Z

This is effectively a "drain" as implemented by kubectl (albeit a rather simplified version -- doesn't use the eviction APIs). The hanging you saw was an issue with my original implementation. That should be fixed with the current version of this PR.

hankjacobs · 2017-10-11T05:39:23Z

Additionally, DaemonSet pods are skipped. To me, this points to something with your setup. This is outside the scope of this PR but I'd double check that you're running rook as a DS.

FaKod · 2017-10-11T05:49:58Z

Right, its a ReplicaSet (created by an Operator) - sorry for the confusion. So I'll issue a feature request to exclude some Resources from deletion by labels or so.

dghubble

Minor nits. Can another maintainer agree on the flag addition? @sdemos

Otherwise, I'll approve after these fixes.

dghubble · 2017-10-16T18:50:17Z

 	nc   v1core.NodeInterface
 	ue   *updateengine.Client
 	lc   *login1.Conn
+	ri   time.Duration


remove, this isn't customized anywhere

dghubble · 2017-10-16T18:51:21Z

 	ue   *updateengine.Client
 	lc   *login1.Conn
+	ri   time.Duration
+	rt   time.Duration


Can you name this reapTimeout or gracePeriod? No need to emulate these (poor imo) existing field names

dghubble · 2017-10-16T18:52:00Z

+	rt   time.Duration
 }

+const reapInterval = 10 * time.Second


Let's have this be defaultInterval or something more general to replace the other occurrences of 10 second intervals

dghubble · 2017-10-16T18:53:06Z

 	}

-	return &Klocksmith{node, kc, nc, ue, lc}, nil
+	return &Klocksmith{node, kc, nc, ue, lc, reapInterval, reapTimeout}, nil


No need to configure with the constant

sdemos · 2017-10-16T20:29:57Z

adding the --grace-period flag seems reasonable to me.

This change makes the agent wait until all pods are terminated before rebooting. Prior to this change, the agent would reboot immediately after the pod delete request was made and, consequently, pods were not given the time needed to terminate gracefully.

…en waiting for pod deletion

hankjacobs · 2017-10-17T21:57:54Z

nits picked and an image to go with it: quay.io/dollarshaveclub/container-linux-update-operator:8015b109a973f0e8497ae894b90f66b6e47e5455-dirty

hankjacobs force-pushed the wait-for-pod-deletion branch from e6180d4 to 8376483 Compare September 29, 2017 23:51

galexrt mentioned this pull request Oct 4, 2017

Problems reboot node with rook rook/rook#1018

Closed

dghubble reviewed Oct 9, 2017

View reviewed changes

dghubble mentioned this pull request Oct 9, 2017

Wait for pods to come up on other nodes before rebooting #134

Closed

dghubble reviewed Oct 16, 2017

View reviewed changes

Hank Jacobs added 4 commits October 17, 2017 13:08

update-agent: use longer poll interval and a concrete poll timeout wh…

a2bbedb

…en waiting for pod deletion

update-agent: ignore transient errors when waiting for pod deletion

2d6065f

update-agent: clean up Klocksmith fields and defaults

8015b10

hankjacobs force-pushed the wait-for-pod-deletion branch from 8f1b849 to 8015b10 Compare October 17, 2017 20:08

dghubble approved these changes Oct 17, 2017

View reviewed changes

dghubble merged commit 0325d63 into coreos:master Oct 17, 2017

This was referenced Oct 20, 2017

examples: Add RBAC permission to get individual pods #147

Merged

pkg/agent: Consider NotFound or changed UID pods deleted #151

Merged

Uh oh!

Conversation

hankjacobs commented Sep 29, 2017

Uh oh!

coreosbot commented Sep 29, 2017

Uh oh!

hankjacobs commented Oct 5, 2017

Uh oh!

dghubble left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hankjacobs Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dghubble Oct 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hankjacobs commented Oct 10, 2017

Uh oh!

FaKod commented Oct 11, 2017

Uh oh!

hankjacobs commented Oct 11, 2017

Uh oh!

FaKod commented Oct 11, 2017

Uh oh!

hankjacobs commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hankjacobs commented Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FaKod commented Oct 11, 2017

Uh oh!

dghubble left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdemos commented Oct 16, 2017

Uh oh!

hankjacobs commented Oct 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hankjacobs Oct 9, 2017 •

edited

Loading

dghubble Oct 9, 2017 •

edited

Loading

hankjacobs commented Oct 11, 2017 •

edited

Loading

hankjacobs commented Oct 11, 2017 •

edited

Loading

dghubble left a comment •

edited

Loading