From 6e1eb4a684ea8606f62d40d7f9dd9017f43ce6d7 Mon Sep 17 00:00:00 2001 From: Tafhim Ul Islam Date: Fri, 17 Jan 2025 20:58:12 +1100 Subject: [PATCH 1/2] OSD-26790 - Update logic to continue with upgrade despite failing to notify, update unit test to reflect the change --- pkg/upgraders/controlplanestep.go | 5 ++--- pkg/upgraders/controlplanestep_test.go | 11 +++++------ 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/upgraders/controlplanestep.go b/pkg/upgraders/controlplanestep.go index 01b99089..0187c892 100644 --- a/pkg/upgraders/controlplanestep.go +++ b/pkg/upgraders/controlplanestep.go @@ -14,7 +14,6 @@ import ( // CommenceUpgrade will update the clusterversion object to apply the desired version to trigger real OCP upgrade func (c *clusterUpgrader) CommenceUpgrade(ctx context.Context, logger logr.Logger) (bool, error) { - // We can reset the window breached metric if we're commencing c.metrics.UpdateMetricUpgradeWindowNotBreached(c.upgradeConfig.Name) @@ -29,7 +28,7 @@ func (c *clusterUpgrader) CommenceUpgrade(ctx context.Context, logger logr.Logge err = c.notifier.Notify(notifier.MuoStateControlPlaneUpgradeStartedSL) if err != nil { - return false, err + logger.Error(err, fmt.Sprintf("Failed to notify upon upgrade start %v", err)) } isComplete, err := c.cvClient.EnsureDesiredConfig(c.upgradeConfig) @@ -65,7 +64,7 @@ func (c *clusterUpgrader) ControlPlaneUpgraded(ctx context.Context, logger logr. } else { upgradeStartTime, err = time.Parse(time.RFC3339, c.upgradeConfig.Spec.UpgradeAt) if err != nil { - return false, err //error parsing time string + return false, err // error parsing time string } } diff --git a/pkg/upgraders/controlplanestep_test.go b/pkg/upgraders/controlplanestep_test.go index 2b15fecc..c0ab2162 100644 --- a/pkg/upgraders/controlplanestep_test.go +++ b/pkg/upgraders/controlplanestep_test.go @@ -238,16 +238,17 @@ var _ = Describe("ControlPlaneStep", func() { }) Context("When sending notification for control plane upgrade start fails", func() { - It("Should report error", func() { + It("Should continue with upgrade", func() { fakeError := fmt.Errorf("fake notification error") gomock.InOrder( mockMetricsClient.EXPECT().UpdateMetricUpgradeWindowNotBreached(gomock.Any()), mockCVClient.EXPECT().HasUpgradeCommenced(gomock.Any()).Return(false, nil), mockEMClient.EXPECT().Notify(gomock.Any()).Return(fakeError), + mockCVClient.EXPECT().EnsureDesiredConfig(gomock.Any()).Return(true, nil), ) result, err := upgrader.CommenceUpgrade(context.TODO(), logger) - Expect(err).To(HaveOccurred()) - Expect(result).To(BeFalse()) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(BeTrue()) }) }) @@ -263,7 +264,6 @@ var _ = Describe("ControlPlaneStep", func() { Expect(err).ToNot(HaveOccurred()) Expect(result).To(BeTrue()) }) - }) }) @@ -280,7 +280,7 @@ var _ = Describe("ControlPlaneStep", func() { }) Context("When the upgrader can't tell if the cluster's upgrade has commenced", func() { - var fakeError = fmt.Errorf("fake upgradeCommenced error") + fakeError := fmt.Errorf("fake upgradeCommenced error") It("will abort the commencing of an upgrade", func() { gomock.InOrder( mockMetricsClient.EXPECT().UpdateMetricUpgradeWindowNotBreached(gomock.Any()), @@ -292,5 +292,4 @@ var _ = Describe("ControlPlaneStep", func() { Expect(result).To(BeFalse()) }) }) - }) From af6af792b9962aeb8655cb5013adf2dc7d609f6d Mon Sep 17 00:00:00 2001 From: Tafhim Ul Islam Date: Fri, 17 Jan 2025 21:29:51 +1100 Subject: [PATCH 2/2] OSD-26790 - Update logic to continue with metrics update despite failing to notify, update unit test to reflect the change --- pkg/upgraders/controlplanestep.go | 2 +- pkg/upgraders/controlplanestep_test.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/upgraders/controlplanestep.go b/pkg/upgraders/controlplanestep.go index 0187c892..28bec093 100644 --- a/pkg/upgraders/controlplanestep.go +++ b/pkg/upgraders/controlplanestep.go @@ -51,7 +51,7 @@ func (c *clusterUpgrader) ControlPlaneUpgraded(ctx context.Context, logger logr. if isCompleted { err = c.notifier.Notify(notifier.MuoStateControlPlaneUpgradeFinishedSL) if err != nil { - return false, err + logger.Error(err, fmt.Sprintf("Failed to notify upon upgrade completion %v", err)) } c.metrics.ResetMetricUpgradeControlPlaneTimeout(c.upgradeConfig.Name, c.upgradeConfig.Spec.Desired.Version) return true, nil diff --git a/pkg/upgraders/controlplanestep_test.go b/pkg/upgraders/controlplanestep_test.go index c0ab2162..f1ddc7ef 100644 --- a/pkg/upgraders/controlplanestep_test.go +++ b/pkg/upgraders/controlplanestep_test.go @@ -103,10 +103,11 @@ var _ = Describe("ControlPlaneStep", func() { mockCVClient.EXPECT().GetClusterVersion().Return(nil, nil), mockCVClient.EXPECT().HasUpgradeCompleted(gomock.Any(), gomock.Any()).Return(true), mockEMClient.EXPECT().Notify(gomock.Any()).Return(fakeError), + mockMetricsClient.EXPECT().ResetMetricUpgradeControlPlaneTimeout(upgradeConfig.Name, upgradeConfig.Spec.Desired.Version), ) result, err := upgrader.ControlPlaneUpgraded(context.TODO(), logger) - Expect(err).To(HaveOccurred()) - Expect(result).To(BeFalse()) + Expect(err).NotTo(HaveOccurred()) + Expect(result).To(BeTrue()) }) })