From bbc0c25832179c26efeac3dc2d4d75636bf9bbd0 Mon Sep 17 00:00:00 2001 From: Marc Sluiter Date: Thu, 31 Aug 2023 09:57:05 +0200 Subject: [PATCH 1/3] Update medik8s/common to v1.8.0 Signed-off-by: Marc Sluiter --- go.mod | 2 +- go.sum | 4 ++-- vendor/github.com/medik8s/common/pkg/lease/manager.go | 7 +++++-- vendor/modules.txt | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index f81d46b1..fd5d12d3 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index aa506a0a..d5f9c128 100644 --- a/go.sum +++ b/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= diff --git a/vendor/github.com/medik8s/common/pkg/lease/manager.go b/vendor/github.com/medik8s/common/pkg/lease/manager.go index 2edbd5c1..d68b5c66 100644 --- a/vendor/github.com/medik8s/common/pkg/lease/manager.go +++ b/vendor/github.com/medik8s/common/pkg/lease/manager.go @@ -45,13 +45,13 @@ type manager struct { log logr.Logger } -// AlreadyHeldError is returned in case the lease that is requested is already held by a different holder +// AlreadyHeldError is returned in case the lease is already held by a different holder type AlreadyHeldError struct { holderIdentity string } func (e AlreadyHeldError) Error() string { - return fmt.Sprintf("can't update valid lease held by different owner: %s", e.holderIdentity) + return fmt.Sprintf("can't update or invalidate the lease because it is held by different owner: %s", e.holderIdentity) } func (l *manager) RequestLease(ctx context.Context, obj client.Object, leaseDuration time.Duration) error { @@ -227,6 +227,9 @@ func (l *manager) invalidateLease(ctx context.Context, obj client.Object) error } return err } + if lease.Spec.HolderIdentity != nil && l.holderIdentity != *lease.Spec.HolderIdentity { + return AlreadyHeldError{*lease.Spec.HolderIdentity} + } if err := l.Client.Delete(ctx, lease); err != nil { log.Error(err, "failed to delete lease to be invalidated") return err diff --git a/vendor/modules.txt b/vendor/modules.txt index 26737496..e5f6007c 100644 --- a/vendor/modules.txt +++ b/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 From 3154c51bc0cbabb82e85a4f8ca4463eddbabf1ab Mon Sep 17 00:00:00 2001 From: Marc Sluiter Date: Thu, 31 Aug 2023 10:38:34 +0200 Subject: [PATCH 2/3] Ignore error in case we try to invalidate a lease not owned by us Signed-off-by: Marc Sluiter --- controllers/resources/lease.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/controllers/resources/lease.go b/controllers/resources/lease.go index ce296180..7722a216 100644 --- a/controllers/resources/lease.go +++ b/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 } From 12e24c536db94ce5366e3568d340afe9887b8887 Mon Sep 17 00:00:00 2001 From: Marc Sluiter Date: Thu, 31 Aug 2023 10:39:42 +0200 Subject: [PATCH 3/3] Add unit test for healthy nodes with not owned leases - lease should not be touched - status update should still succeed Signed-off-by: Marc Sluiter --- .../nodehealthcheck_controller_test.go | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/controllers/nodehealthcheck_controller_test.go b/controllers/nodehealthcheck_controller_test.go index e59ef45e..3f6ad30c 100644 --- a/controllers/nodehealthcheck_controller_test.go +++ b/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() {