Merge pull request #250 from openshift-cherrypick-robot/cherry-pick-2…

…49-to-release-0.6

[release-0.6] Fix lease handling for healthy nodes in case not owned lease exists

controllers/nodehealthcheck_controller_test.go

@@ -620,6 +620,53 @@ var _ = Describe("Node Health Check CR", func() {
 						}, "2s", "100ms").Should(BeTrue(), "lease wasn't removed")
 
 					})
+
+					It("node lease not owned by us isn't removed, but status is updated (invalidate lease error is ignored)", func() {
+						cr := newRemediationCR(unhealthyNodeName, underTest)
+						err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(cr), cr)
+						Expect(err).ToNot(HaveOccurred())
+						//Verify lease exist
+						lease := &coordv1.Lease{}
+						err = k8sClient.Get(context.Background(), client.ObjectKey{Name: leaseName, Namespace: leaseNs}, lease)
+						Expect(err).ToNot(HaveOccurred())
+
+						// change lease owner
+						newLeaseOwner := "someone-else"
+						lease.Spec.HolderIdentity = pointer.String(newLeaseOwner)
+						Expect(k8sClient.Update(context.Background(), lease)).To(Succeed(), "failed to update lease owner")
+
+						//Mock node becoming healthy
+						node := &v1.Node{}
+						err = k8sClient.Get(context.Background(), client.ObjectKey{Name: unhealthyNodeName}, node)
+						Expect(err).ToNot(HaveOccurred())
+						for i, c := range node.Status.Conditions {
+							if c.Type == v1.NodeReady {
+								node.Status.Conditions[i].Status = v1.ConditionTrue
+							}
+						}
+						err = k8sClient.Status().Update(context.Background(), node)
+						Expect(err).ToNot(HaveOccurred())
+
+						//Remediation should be removed
+						Eventually(func() bool {
+							err := k8sClient.Get(context.Background(), client.ObjectKeyFromObject(cr), cr)
+							return errors.IsNotFound(err)
+						}, "2s", "100ms").Should(BeTrue(), "remediation CR wasn't removed")
+
+						// Status should be updated even though lease isn't owned by us anymore
+						Eventually(func(g Gomega) {
+							g.Expect(k8sClient.Get(context.Background(), client.ObjectKeyFromObject(underTest), underTest)).To(Succeed())
+							g.Expect(underTest.Status.InFlightRemediations).To(BeEmpty())
+							g.Expect(underTest.Status.UnhealthyNodes).To(BeEmpty())
+						}, "2s", "100ms").Should(Succeed(), "status update failed")
+
+						//Verify NHC didn't touch the lease
+						Consistently(func(g Gomega) {
+							g.Expect(k8sClient.Get(context.Background(), client.ObjectKey{Name: leaseName, Namespace: leaseNs}, lease)).To(Succeed(), "failed to get lease")
+							g.Expect(*lease.Spec.HolderIdentity).To(Equal(newLeaseOwner))
+						}, "2s", "100ms").Should(Succeed(), "lease was touched even though it's not owned by us")
+
+					})
 				})
 
 				When("an unhealthy node lease is already taken", func() {

controllers/resources/lease.go

@@ -156,6 +156,10 @@ func (m *nhcLeaseManager) InvalidateLease(ctx context.Context, nodeName string)
 	}
 	err := m.commonLeaseManager.InvalidateLease(ctx, node)
 	if err != nil {
+		if _, ok := err.(lease.AlreadyHeldError); ok {
+			// lease exists but isn't owned by us, can be ignored
+			return nil
+		}
 		m.log.Error(err, "failed to invalidate lease", "node name", nodeName)
 		return err
 	}

go.mod

@@ -4,7 +4,7 @@ go 1.20
 
 require (
 	github.com/go-logr/logr v1.2.4
-	github.com/medik8s/common v1.7.0
+	github.com/medik8s/common v1.8.0
 	github.com/onsi/ginkgo/v2 v2.9.5
 	github.com/onsi/gomega v1.27.7
 	github.com/openshift/api v0.0.0-20221013123534-96eec44e1979 // release-4.11

go.sum

@@ -235,8 +235,8 @@ github.com/mailru/easyjson v0.7.6 h1:8yTIVnZgCoiM1TgqoeTl+LfU5Jg6/xL3QhGQnimLYnA
 github.com/mailru/easyjson v0.7.6/go.mod h1:xzfreul335JAWq5oZzymOObrkdz5UnU4kGfJJLY9Nlc=
 github.com/matttproud/golang_protobuf_extensions v1.0.4 h1:mmDVorXM7PCGKw94cs5zkfA9PSy5pEvNWRP0ET0TIVo=
 github.com/matttproud/golang_protobuf_extensions v1.0.4/go.mod h1:BSXmuO+STAnVfrANrmjBb36TMTDstsz7MSK+HVaYKv4=
-github.com/medik8s/common v1.7.0 h1:JwimhWigPTAszGG2jYJmh+uVHq2nFUPRRljw7ZhlQhg=
-github.com/medik8s/common v1.7.0/go.mod h1:ZT/3hfMXJLmZEcqmxRWB5LGC8Wl+qKGGQ4zM8hOE7PY=
+github.com/medik8s/common v1.8.0 h1:7TOH+XQEvBEMpTZEewVtUzCpjGosaklWPt8E4gC2PNk=
+github.com/medik8s/common v1.8.0/go.mod h1:ZT/3hfMXJLmZEcqmxRWB5LGC8Wl+qKGGQ4zM8hOE7PY=
 github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
 github.com/moby/spdystream v0.2.0 h1:cjW1zVyyoiM0T7b6UoySUFqzXMoqRckQtXwGPiBhOM8=
 github.com/moby/spdystream v0.2.0/go.mod h1:f7i0iNDQJ059oMTcWxx8MA/zKFIuD/lY+0GqbN2Wy8c=

vendor/modules.txt

@@ -91,7 +91,7 @@ github.com/mailru/easyjson/jwriter
 # github.com/matttproud/golang_protobuf_extensions v1.0.4
 ## explicit; go 1.9
 github.com/matttproud/golang_protobuf_extensions/pbutil
-# github.com/medik8s/common v1.7.0
+# github.com/medik8s/common v1.8.0
 ## explicit; go 1.20
 github.com/medik8s/common/pkg/annotations
 github.com/medik8s/common/pkg/conditions