Delete node lease if the corresponding node is deleted #70034

wangzhen127 · 2018-10-19T18:42:04Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR ensures that node lease is deleted if the corresponding node is deleted. This is done in node lifecycle controller. OwnerReference approach is not feasible because an owning object must be in the same namespace as the owned object, and node does not have namespace.

This is part of KEP-0009, feature #589 and issue #14733.

Does this PR introduce a user-facing change?:
Yes. When node lease feature is enabled, with this PR, user will see node lease is deleted when the corresponding node is deleted.

Release note:

NodeLifecycleController: When node lease feature is enabled, node lease will be deleted when the corresponding node is deleted.

/sig node
/milestone v1.13

k8s-ci-robot · 2018-10-19T18:42:06Z

@wangzhen127: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR ensures that node lease is deleted if the corresponding node is deleted. This is done in node lifecycle controller. OwnerReference approach is not feasible because an owning object must be in the same namespace as the owned object, and node does not have namespace.

This is part of KEP-0009, feature #589 and issue #14733.

Does this PR introduce a user-facing change?:
Yes. When node lease feature is enabled, with this PR, user will see node lease is deleted when the corresponding node is deleted.

Release note:
NodeLifecycleController: When node lease feature is enabled, node lease will be deleted when the corresponding node is deleted.
/sig node
/milestone v1.13

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wangzhen127 · 2018-10-19T18:42:22Z

/assign @wojtek-t
/assign @yujuhong

wojtek-t · 2018-10-22T13:47:22Z

pkg/controller/nodelifecycle/node_lifecycle_controller.go

@@ -669,6 +669,11 @@ func (nc *Controller) monitorNodeHealth() error {
 		glog.V(1).Infof("Controller observed a Node deletion: %v", deleted[i].Name)
 		nodeutil.RecordNodeEvent(nc.recorder, deleted[i].Name, string(deleted[i].UID), v1.EventTypeNormal, "RemovingNode", fmt.Sprintf("Removing Node %v from Controller", deleted[i].Name))
 		delete(nc.knownNodeSet, deleted[i].Name)
+		if utilfeature.DefaultFeatureGate.Enabled(features.NodeLease) {
+			if err := nc.kubeClient.CoordinationV1beta1().Leases(v1.NamespaceNodeLease).Delete(deleted[i].Name, metav1.NewDeleteOptions(0)); err != nil {


This has a problem that if this fail (or if controller-manager will be killed/stopped in the meantime or sth like that), the lease object will be left.

Wouldn't it be possible to set ownerReference of the Lease object to be the corresponding Node object and rely on GC? @janetkuo - thought?

This has a problem that if this fail (or if controller-manager will be killed/stopped in the meantime or sth like that), the lease object will be left.

The alternative is to compare the existing nodes and leases and do the cleanup every time when monitorNodeHealth() runs.

Wouldn't it be possible to set ownerReference of the Lease object to be the corresponding Node object and rely on GC? Janet Kuo - thought?

IIUC, OwnerReference approach is not feasible because an owning object must be in the same namespace as the owned object, and node does not have namespace. See here. I also included this in the PR description.

The alternative is to compare the existing nodes and leases and do the cleanup every time when monitorNodeHealth() runs.

@wangzhen127 do the leases have information about which node acquired it ?

IIUC, OwnerReference approach is not feasible because an owning object must be in the same namespace as the owned object, and node does not have namespace. See here. I also included this in the PR description.

@janetkuo @caesarxuchao Does the GC work on non namespaced objects ?

I really think that we need something that is more robust that this.
Listing all leases (from local cache should be good enough) and comparing them with existing nodes sounds reasonable.

[If we won't be able to use GC - I pinged Janet and Chao about that offline].

GC support owner being non namespaced, while the dependents are namespaced. (see #65200).

@wangzhen127 - if that mechanism works, then we should use it.

SG. Will try owner reference and update later.

krmayankk · 2018-10-24T05:36:39Z

pkg/controller/nodelifecycle/node_lifecycle_controller_test.go

+	if actions[0].GetVerb() != "delete" {
+		t.Fatalf("unexpected action verb: %s", actions[0].GetVerb())
+	}
+	if actions[0].GetResource().Resource != "leases" {


is this the place you verify the action of delete was called for resource leases ?

We should also verify the name of the lease being deleted and if that was indeed held by the node that got deleted

wangzhen127 · 2018-10-30T18:44:43Z

@yujuhong, @wojtek-t, I have updated this PR to use owner reference. PTAL.

/test pull-kubernetes-node-e2e-alpha

wojtek-t · 2018-10-31T13:55:18Z

This LGTM.

wangzhen127 · 2018-10-31T17:45:49Z

/retest

guineveresaenger · 2018-11-05T16:42:44Z

Hi @wangzhen127 - I'm Enhancements Shadow for 1.13. This looks like it is nearing completion! Just a friendly reminder that code slush begins on 11/9 and code freeze is 11/15, so if you could follow up with your reviewers and get this in, that would be awesome.

Thank you!

wangzhen127 · 2018-11-05T19:31:56Z

Ping @yujuhong :)

yujuhong · 2018-11-07T21:20:00Z

test/e2e/lifecycle/node_lease.go

+		Expect(err).To(BeNil())
+		systemPodsNo = int32(len(systemPods))
+		if strings.Index(framework.TestContext.CloudConfig.NodeInstanceGroup, ",") >= 0 {
+			framework.Failf("Test dose not support cluster setup with more than one MIG: %s", framework.TestContext.CloudConfig.NodeInstanceGroup)


Should the test be skipped if there are more than one MIG?

I am not entirely sure about the reasoning here. I copied from resize_nodes tests:
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/lifecycle/resize_nodes.go#L57

yujuhong · 2018-11-07T21:23:34Z

test/e2e/lifecycle/node_lease.go

+				}
+				if pass {
+					return nil
+				} else {


nit: don't need the else

yujuhong · 2018-11-07T21:28:53Z

test/e2e/lifecycle/node_lease.go

+			Expect(deletedNodeName).NotTo(Equal(""))
+			Eventually(func() error {
+				if _, err := leaseClient.Get(deletedNodeName, metav1.GetOptions{}); err == nil {
+					return fmt.Errorf("node lease is not deleted yet")


Include the deleted node name in the error message for debugging.

yujuhong · 2018-11-07T21:31:18Z

test/e2e/lifecycle/node_lease.go

+					}
+				}
+				return nil
+			}, 5*time.Minute, 5*time.Second).Should(BeNil())


Do we really need to wait 5 minutes for this to be true? Seems excessive to me.

Changing to 1m now.

yujuhong · 2018-11-07T21:32:07Z

test/e2e/lifecycle/node_lease.go

+				} else {
+					return fmt.Errorf("some node lease is not ready")
+				}
+			}, 5*time.Minute, 5*time.Second).Should(BeNil())


Can we shorten the timeout here?

Changing to 1m now.

yujuhong · 2018-11-07T21:33:23Z

test/e2e/lifecycle/node_lease.go

+			targetNodes := framework.GetReadySchedulableNodesOrDie(c)
+			Expect(len(targetNodes.Items)).To(Equal(int(targetNumNodes)))
+
+			By("verify node lease is deleted for the deleted node")


What's the expected latency for the lease to be deleted?

That depends on GC. Based on my observation previously, it is deleted very soon after deleting nodes. Changing the timeout to 1m now.

yujuhong · 2018-11-07T21:35:34Z

pkg/kubelet/nodelease/controller.go

+				},
+			}
+		} else {
+			glog.Infof("failed to get node %q when trying to set owner ref to the node lease: %v", c.holderIdentity, err)


Why is this Infof as opposed to Errorf?

wangzhen127

Updated. PTAL

wangzhen127 · 2018-11-07T21:47:20Z

pkg/kubelet/nodelease/controller.go

+				},
+			}
+		} else {
+			glog.Infof("failed to get node %q when trying to set owner ref to the node lease: %v", c.holderIdentity, err)


wangzhen127 · 2018-11-07T21:50:19Z

test/e2e/lifecycle/node_lease.go

+		Expect(err).To(BeNil())
+		systemPodsNo = int32(len(systemPods))
+		if strings.Index(framework.TestContext.CloudConfig.NodeInstanceGroup, ",") >= 0 {
+			framework.Failf("Test dose not support cluster setup with more than one MIG: %s", framework.TestContext.CloudConfig.NodeInstanceGroup)


I am not entirely sure about the reasoning here. I copied from resize_nodes tests:
https://github.com/kubernetes/kubernetes/blob/master/test/e2e/lifecycle/resize_nodes.go#L57

wangzhen127 · 2018-11-07T21:51:12Z

test/e2e/lifecycle/node_lease.go

+				}
+				if pass {
+					return nil
+				} else {


wangzhen127 · 2018-11-07T21:52:08Z

test/e2e/lifecycle/node_lease.go

+				} else {
+					return fmt.Errorf("some node lease is not ready")
+				}
+			}, 5*time.Minute, 5*time.Second).Should(BeNil())


Changing to 1m now.

wangzhen127 · 2018-11-07T21:54:26Z

test/e2e/lifecycle/node_lease.go

+			targetNodes := framework.GetReadySchedulableNodesOrDie(c)
+			Expect(len(targetNodes.Items)).To(Equal(int(targetNumNodes)))
+
+			By("verify node lease is deleted for the deleted node")


That depends on GC. Based on my observation previously, it is deleted very soon after deleting nodes. Changing the timeout to 1m now.

wangzhen127 · 2018-11-07T21:55:18Z

test/e2e/lifecycle/node_lease.go

+			Expect(deletedNodeName).NotTo(Equal(""))
+			Eventually(func() error {
+				if _, err := leaseClient.Get(deletedNodeName, metav1.GetOptions{}); err == nil {
+					return fmt.Errorf("node lease is not deleted yet")


wangzhen127 · 2018-11-07T21:55:31Z

test/e2e/lifecycle/node_lease.go

+					}
+				}
+				return nil
+			}, 5*time.Minute, 5*time.Second).Should(BeNil())


Changing to 1m now.

wangzhen127 · 2018-11-08T15:44:55Z

The failures in pull-kubernetes-e2e-gce-alpha-features seem not related to this PR, because the gci-gce-alpha-features CI jobs are also failing. Node lease tests are passing.

yujuhong · 2018-11-08T18:28:31Z

/lgtm

yujuhong · 2018-11-08T18:28:52Z

/approve

wojtek-t · 2018-11-09T07:20:37Z

/approve

k8s-ci-robot · 2018-11-09T07:21:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wangzhen127, wojtek-t, yujuhong

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:

~~pkg/kubelet/OWNERS~~ [wojtek-t,yujuhong]
~~test/OWNERS~~ [wojtek-t]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

wangzhen127 · 2018-11-09T07:49:07Z

/retest

fejta-bot · 2018-11-09T12:23:24Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

wangzhen127 · 2018-11-09T15:09:19Z

Do we have to wait until pull-kubernetes-e2e-gce-alpha-features fully pass before merging, which is not a required job? The failure is on CSI tests, which are unrelated to this PR. The CSI failures are tracked in #70760

Since the node lease tests are passing, can we merge ignoring pull-kubernetes-e2e-gce-alpha-features?
/cc @krzyzacy
/cc @AishSundar

fejta-bot · 2018-11-09T17:17:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

AishSundar · 2018-11-09T17:33:45Z

/priority important-soon

saad-ali · 2018-11-09T18:02:39Z

Do we have to wait until pull-kubernetes-e2e-gce-alpha-features fully pass before merging, which is not a required job? The failure is on CSI tests, which are unrelated to this PR. The CSI failures are tracked in #70760

Yes looks like these test are bad. They should not block 1.13. @mkimuram please open a PR to disable the tests and then fix them before reenabling.

krzyzacy · 2018-11-09T18:17:50Z

/override pull-kubernetes-e2e-gce-alpha-features

krzyzacy · 2018-11-09T18:20:01Z

gah... I think we need to wait for the test to finish before we can override the context

wangzhen127 · 2018-11-09T19:50:18Z

/override pull-kubernetes-e2e-gce-alpha-features

krzyzacy · 2018-11-09T19:52:54Z

/override pull-kubernetes-e2e-gce-alpha-features

krzyzacy · 2018-11-09T19:53:13Z

/skip

wangzhen127 · 2018-11-09T22:19:20Z

/test pull-kubernetes-e2e-gce-100-performance

wangzhen127 · 2018-11-09T23:48:38Z

/test pull-kubernetes-e2e-gce-100-performance

k8s-ci-robot · 2018-11-10T02:19:35Z

@wangzhen127: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-alpha-features	`b8b1279`	link	`/test pull-kubernetes-e2e-gce-alpha-features`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2018-11-10T04:50:39Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

k8s-ci-robot assigned wojtek-t and yujuhong Oct 19, 2018

k8s-ci-robot requested review from aveshagarwal and k82cn October 19, 2018 18:42

wangzhen127 force-pushed the cleanup-node-lease branch from 5823290 to 868f884 Compare October 19, 2018 20:24

wojtek-t reviewed Oct 22, 2018

View reviewed changes

krmayankk reviewed Oct 24, 2018

View reviewed changes

wangzhen127 force-pushed the cleanup-node-lease branch from 868f884 to 9fb1285 Compare October 30, 2018 18:25

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/kubelet sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 30, 2018

wangzhen127 force-pushed the cleanup-node-lease branch from 9fb1285 to cd09483 Compare October 30, 2018 20:44

wangzhen127 mentioned this pull request Nov 1, 2018

Make periodic NodeStatus updates cheaper #14733

Closed

yujuhong added this to the v1.13 milestone Nov 7, 2018

yujuhong reviewed Nov 7, 2018

View reviewed changes

wangzhen127 force-pushed the cleanup-node-lease branch from cd09483 to 691f95c Compare November 7, 2018 21:57

wangzhen127 commented Nov 7, 2018

View reviewed changes

wangzhen127 mentioned this pull request Nov 8, 2018

Move frequent Kubelet heartbeats to Lease API kubernetes/enhancements#589

Closed

6 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 8, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2018

k8s-ci-robot requested review from AishSundar and krzyzacy November 9, 2018 15:09

wangzhen127 mentioned this pull request Nov 9, 2018

CSI alpha tests are failing #70760

Closed

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 9, 2018

k8s-ci-robot merged commit be71fbe into kubernetes:master Nov 10, 2018

wangzhen127 deleted the cleanup-node-lease branch November 10, 2018 07:22

cartermckinnon mentioned this pull request Jul 29, 2023

Handle errors when preparing lease for update #119661

Merged

Delete node lease if the corresponding node is deleted #70034

Delete node lease if the corresponding node is deleted #70034

Conversation

wangzhen127 commented Oct 19, 2018

k8s-ci-robot commented Oct 19, 2018

wangzhen127 commented Oct 19, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen127 commented Oct 30, 2018

wojtek-t commented Oct 31, 2018

wangzhen127 commented Oct 31, 2018

guineveresaenger commented Nov 5, 2018

wangzhen127 commented Nov 5, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen127 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangzhen127 commented Nov 8, 2018 • edited Loading

yujuhong commented Nov 8, 2018

yujuhong commented Nov 8, 2018

wojtek-t commented Nov 9, 2018

k8s-ci-robot commented Nov 9, 2018

wangzhen127 commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

wangzhen127 commented Nov 9, 2018

fejta-bot commented Nov 9, 2018

AishSundar commented Nov 9, 2018

saad-ali commented Nov 9, 2018

krzyzacy commented Nov 9, 2018

krzyzacy commented Nov 9, 2018

wangzhen127 commented Nov 9, 2018

krzyzacy commented Nov 9, 2018

krzyzacy commented Nov 9, 2018

wangzhen127 commented Nov 9, 2018

wangzhen127 commented Nov 9, 2018

k8s-ci-robot commented Nov 10, 2018 • edited Loading

fejta-bot commented Nov 10, 2018

wangzhen127 commented Nov 8, 2018 •

edited

Loading

k8s-ci-robot commented Nov 10, 2018 •

edited

Loading