Avoid enqueue when status of k8s pods change #835

cwdsuzhou · 2020-06-06T07:45:41Z

No description provided.

cwdsuzhou · 2020-06-06T07:46:38Z

We should ignore podStatus change, it not. too many useless enqueue here.

sargun · 2020-06-08T18:05:07Z

node/podcontroller.go

-				pc.k8sQ.AddRateLimited(key)
+				// ignore ResourceVersion change
+				oldPod.ResourceVersion = newPod.ResourceVersion
+				if !cmp.Equal(oldPod.Spec, newPod.Spec) ||


If you're trying to achieve update suppression, you can use:

virtual-kubelet/node/pod.go

Line 103 in a67cfab

func podsEqual(pod1, pod2 *corev1.Pod) bool {

Also, you need to check deletion timestamp and phase change IIRC

No, wo should Ignore status update here. But we need check spec, deleteTime, Annotation, Labels and so on. So I exclude pod status comparison

Please add a more targeted equal method that tests the super set of the attributes of podequal. Something like:

podNeedsWork(old, new *v1.pod) bool { if !podsEqual(...) { return true }; if ... return true return false }

I'm curious, where is the perf concern?

Done, thanks, PTAL thanks

sargun · 2020-06-09T07:05:30Z

node/podcontroller.go

 			newPod := newObj.(*corev1.Pod)

 			// At this point we know that something in .metadata or .spec has changed, so we must proceed to sync the pod.
 			if key, err := cache.MetaNamespaceKeyFunc(newPod); err != nil {
 				log.G(ctx).Error(err)
 			} else {
-				pc.k8sQ.AddRateLimited(key)
+				// ignore ResourceVersion change
+				oldPod.ResourceVersion = newPod.ResourceVersion


aren't these cache references? So you're mutating the cache here?

sargun · 2020-06-09T07:06:56Z

node/podcontroller.go

-				pc.k8sQ.AddRateLimited(key)
+				// ignore ResourceVersion change
+				oldPod.ResourceVersion = newPod.ResourceVersion
+				if !cmp.Equal(oldPod.Spec, newPod.Spec) ||


Please add a more targeted equal method that tests the super set of the attributes of podequal. Something like:

podNeedsWork(old, new *v1.pod) bool { if !podsEqual(...) { return true }; if ... return true return false }

I'm curious, where is the perf concern?

sargun · 2020-06-09T07:08:32Z

Somehow my review got split. None the less, I think this is a fine idea.

cwdsuzhou · 2020-06-09T07:53:09Z

@sargun PR updated, thanks

sargun · 2020-06-09T16:53:18Z

node/pod.go

+	if !podsEqual(oldPod, newPod) {
+		return true
+	}
+	if !oldPod.DeletionTimestamp.Equal(newPod.DeletionTimestamp) {


In the earlier patch, you checked all of object meta. Any reason for this change?

I think Labels, Annotations, DeleteTimeStamp update should enqueue, the earlier patch check object meta for easier comparison. But I found podsEqual has check labels and annotations , so I thin this would be easier and effective.

I agree. Why didn't you add labels and annotations here? Honestly, we should probably make the update pod callback on label and annotation change. -- that's work for a different time.

@sargun podsEqual has already checked labels and annotations.

We should also check to make sure the deletion graceperiod is equal. and then I think it'll be good to go.

Yes, this is a case, but it would not affect the final delete time.

If you think it is necessary, I would like update this PR

It doesn't hurt to add this check, IMHO.

PR updated, PTAL

thanks

sargun · 2020-06-11T03:36:18Z

One last question, and then we're good to go.

shsjshentao · 2021-11-03T06:13:40Z

I think it should not ignore podStatus change when enqueue, if i set graceful deletion and delete pod, then all containers has dead in advance, it should kill immediately instead of waiting graceful deletion.

cwdsuzhou · 2021-11-04T03:19:20Z

I think it should not ignore podStatus change when enqueue, if i set graceful deletion and delete pod, then all containers has dead in advance, it should kill immediately instead of waiting graceful deletion.

You can take a look at these 2 PRs , may solve your issues.

#902
#874

shsjshentao · 2021-11-04T07:45:19Z

I think it should not ignore podStatus change when enqueue, if i set graceful deletion and delete pod, then all containers has dead in advance, it should kill immediately instead of waiting graceful deletion.

You can take a look at these 2 PRs , may solve your issues.

#902 #874

No, these 2 PRs cannot solve this problem.

For example:

00:00:00   
I delete a pod in k8s with 300 graceful seconds
vk provider delete real pod with 300 graceful seconds
vk enqueue k8s pod in deletion queue with delay 300 seconds

00:01:00    
all containers of vk has exited in advance
then real pod has deleted and missing
vk provider receive missing signal and update pod status to succeed with notifypods method
but nothing changed util 300 seconds

00:05:00   
vk delete pods in k8s

cwdsuzhou force-pushed the June/avoid_enqueue branch from 22b5202 to 3d60b97 Compare June 6, 2020 07:45

sargun reviewed Jun 8, 2020

View reviewed changes

sargun requested changes Jun 9, 2020

View reviewed changes

cwdsuzhou force-pushed the June/avoid_enqueue branch from 3d60b97 to 732b352 Compare June 9, 2020 07:52

cwdsuzhou requested a review from sargun June 9, 2020 07:52

sargun reviewed Jun 9, 2020

View reviewed changes

cwdsuzhou requested a review from sargun June 10, 2020 09:55

Avoid enqueue when status of k8s pods change

3db9ab9

cwdsuzhou force-pushed the June/avoid_enqueue branch from 732b352 to 3db9ab9 Compare June 13, 2020 05:20

sargun approved these changes Jun 13, 2020

View reviewed changes

sargun merged commit 05fc1a4 into virtual-kubelet:master Jun 15, 2020

cwdsuzhou mentioned this pull request Mar 16, 2021

Bump K8s and vk dependency virtual-kubelet/tensile-kube#68

Merged

shsjshentao mentioned this pull request Nov 3, 2021

graceful deletion bug caused by #835 #982

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid enqueue when status of k8s pods change #835

Avoid enqueue when status of k8s pods change #835

cwdsuzhou commented Jun 6, 2020

cwdsuzhou commented Jun 6, 2020

sargun Jun 8, 2020

cwdsuzhou Jun 9, 2020

sargun Jun 9, 2020

cwdsuzhou Jun 9, 2020

sargun Jun 9, 2020

sargun Jun 9, 2020

sargun commented Jun 9, 2020

cwdsuzhou commented Jun 9, 2020

sargun Jun 9, 2020

cwdsuzhou Jun 10, 2020

sargun Jun 11, 2020

cwdsuzhou Jun 11, 2020

sargun Jun 11, 2020

sargun Jun 11, 2020 •

edited

cwdsuzhou Jun 12, 2020

cwdsuzhou Jun 12, 2020

sargun Jun 12, 2020

cwdsuzhou Jun 13, 2020

sargun commented Jun 11, 2020

shsjshentao commented Nov 3, 2021 •

edited

cwdsuzhou commented Nov 4, 2021

shsjshentao commented Nov 4, 2021 •

edited

Avoid enqueue when status of k8s pods change #835

Avoid enqueue when status of k8s pods change #835

Conversation

cwdsuzhou commented Jun 6, 2020

cwdsuzhou commented Jun 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Jun 9, 2020

cwdsuzhou commented Jun 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun Jun 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sargun commented Jun 11, 2020

shsjshentao commented Nov 3, 2021 • edited

cwdsuzhou commented Nov 4, 2021

shsjshentao commented Nov 4, 2021 • edited

sargun Jun 11, 2020 •

edited

shsjshentao commented Nov 3, 2021 •

edited

shsjshentao commented Nov 4, 2021 •

edited