Retry build/sign resource when its pod was evicted

internal/buildsign/manager.go

@@ -103,18 +103,19 @@ func (m *manager) Sync(ctx context.Context, mld *api.ModuleLoaderData, pushImage
 		return nil
 	}
 
-	changed, err := m.resourceManager.IsResourceChanged(resource, resourceTemplate)
+	shouldRestart, err := m.resourceManager.ShouldResourceBeRestarted(resource, resourceTemplate)
 	if err != nil {
-		return fmt.Errorf("could not determine if the resource has changed: %v", err)
+		return fmt.Errorf("could not determine if the resource should be restarted: %v", err)
 	}
 
-	if changed {
-		logger.Info("The module's spec has been changed, deleting the current resource so a new one can be created",
-			"name", resource.GetName(), "action", action)
-		err = m.resourceManager.DeleteResource(ctx, resource)
-		if err != nil {
-			logger.Info(utils.WarnString(fmt.Sprintf("failed to delete %s resource %s: %v", action, resource.GetName(), err)))
-		}
+	if !shouldRestart {
+		return nil
+	}
+
+	logger.Info("Deleting the current resource so a new one can be created",
+		"name", resource.GetName(), "action", action)
+	if err = m.resourceManager.DeleteResource(ctx, resource); err != nil {
+		return fmt.Errorf("could not delete %s resource %s: %w", action, resource.GetName(), err)
 	}
 
 	return nil

internal/buildsign/manager_test.go

@@ -184,7 +184,7 @@ var _ = Describe("Sync", func() {
 		Expect(err).To(BeNil())
 	})
 
-	It("IsResourceChanged failed", func() {
+	It("ShouldResourceBeRestarted failed", func() {
 		testTemplate := buildv1.Build{}
 		testBuild := buildv1.Build{}
 		gomock.InOrder(
@@ -193,13 +193,13 @@ var _ = Describe("Sync", func() {
 			mockResourceManager.EXPECT().GetResourceByKernel(ctx, mbscName, mbscNamespace, kernelVersion,
 				kmmv1beta1.BuildImage, &testMBSC).
 				Return(&testBuild, nil),
-			mockResourceManager.EXPECT().IsResourceChanged(&testBuild, &testTemplate).Return(false, fmt.Errorf("some error")),
+			mockResourceManager.EXPECT().ShouldResourceBeRestarted(&testBuild, &testTemplate).Return(false, fmt.Errorf("some error")),
 		)
 		err := mgr.Sync(ctx, testMLD, true, kmmv1beta1.BuildImage, &testMBSC)
 		Expect(err).To(HaveOccurred())
 	})
 
-	It("DeleteResource failed should not cause failure", func() {
+	It("DeleteResource failed should cause failure so the restart is retried", func() {
 		testTemplate := buildv1.Build{}
 		testBuild := buildv1.Build{}
 		gomock.InOrder(
@@ -208,10 +208,43 @@ var _ = Describe("Sync", func() {
 			mockResourceManager.EXPECT().GetResourceByKernel(ctx, mbscName, mbscNamespace, kernelVersion,
 				kmmv1beta1.BuildImage, &testMBSC).
 				Return(&testBuild, nil),
-			mockResourceManager.EXPECT().IsResourceChanged(&testBuild, &testTemplate).Return(true, nil),
+			mockResourceManager.EXPECT().ShouldResourceBeRestarted(&testBuild, &testTemplate).Return(true, nil),
 			mockResourceManager.EXPECT().DeleteResource(ctx, &testBuild).Return(fmt.Errorf("some error")),
 		)
 		err := mgr.Sync(ctx, testMLD, true, kmmv1beta1.BuildImage, &testMBSC)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("DeleteResource returning NotFound should also cause failure", func() {
+		testTemplate := buildv1.Build{}
+		testBuild := buildv1.Build{}
+		notFoundErr := k8serrors.NewNotFound(buildv1.Resource("builds"), "some-name")
+		gomock.InOrder(
+			mockResourceManager.EXPECT().MakeResourceTemplate(ctx, testMLD, &testMBSC, true, kmmv1beta1.BuildImage).
+				Return(&testTemplate, nil),
+			mockResourceManager.EXPECT().GetResourceByKernel(ctx, mbscName, mbscNamespace, kernelVersion,
+				kmmv1beta1.BuildImage, &testMBSC).
+				Return(&testBuild, nil),
+			mockResourceManager.EXPECT().ShouldResourceBeRestarted(&testBuild, &testTemplate).Return(true, nil),
+			mockResourceManager.EXPECT().DeleteResource(ctx, &testBuild).Return(notFoundErr),
+		)
+		err := mgr.Sync(ctx, testMLD, true, kmmv1beta1.BuildImage, &testMBSC)
+		Expect(err).To(HaveOccurred())
+	})
+
+	It("unchanged but recoverable resource gets deleted so it can be retried", func() {
+		testTemplate := buildv1.Build{}
+		testBuild := buildv1.Build{}
+		gomock.InOrder(
+			mockResourceManager.EXPECT().MakeResourceTemplate(ctx, testMLD, &testMBSC, true, kmmv1beta1.BuildImage).
+				Return(&testTemplate, nil),
+			mockResourceManager.EXPECT().GetResourceByKernel(ctx, mbscName, mbscNamespace, kernelVersion,
+				kmmv1beta1.BuildImage, &testMBSC).
+				Return(&testBuild, nil),
+			mockResourceManager.EXPECT().ShouldResourceBeRestarted(&testBuild, &testTemplate).Return(true, nil),
+			mockResourceManager.EXPECT().DeleteResource(ctx, &testBuild).Return(nil),
+		)
+		err := mgr.Sync(ctx, testMLD, true, kmmv1beta1.BuildImage, &testMBSC)
 		Expect(err).To(BeNil())
 	})
 
@@ -240,7 +273,7 @@ var _ = Describe("Sync", func() {
 			mockResourceManager.EXPECT().CreateResource(ctx, &testBuildTemplate).Return(nil)
 			goto executeTestFunction
 		}
-		mockResourceManager.EXPECT().IsResourceChanged(&existingTestBuild, &testBuildTemplate).Return(buildChanged, nil)
+		mockResourceManager.EXPECT().ShouldResourceBeRestarted(&existingTestBuild, &testBuildTemplate).Return(buildChanged, nil)
 		if buildChanged {
 			mockResourceManager.EXPECT().DeleteResource(ctx, &existingTestBuild).Return(nil)
 		}

internal/buildsign/resource/resourcemanager.go

@@ -111,7 +111,7 @@ func (rm *resourceManager) GetResourceStatus(obj metav1.Object) (buildsign.Statu
 	}
 }
 
-func (rm *resourceManager) IsResourceChanged(existingObj metav1.Object, newObj metav1.Object) (bool, error) {
+func (rm *resourceManager) ShouldResourceBeRestarted(existingObj metav1.Object, newObj metav1.Object) (bool, error) {
 
 	existingResource, ok := existingObj.(*buildv1.Build)
 	if !ok {
@@ -122,15 +122,17 @@ func (rm *resourceManager) IsResourceChanged(existingObj metav1.Object, newObj m
 		return false, errors.New("the new resource cannot be converted to the corect resource")
 	}
 
+	if existingResource.Status.Phase == buildv1.BuildPhaseError &&
+		existingResource.Status.Reason == buildv1.StatusReasonBuildPodDeleted {
+		return true, nil
+	}
+
 	existingAnnotations := existingResource.GetAnnotations()
 	newAnnotations := newResource.GetAnnotations()
 	if existingAnnotations == nil {
 		return false, fmt.Errorf("annotations are not present in the existing resource %s", existingResource.Name)
 	}
-	if existingAnnotations[constants.ResourceHashAnnotation] == newAnnotations[constants.ResourceHashAnnotation] {
-		return false, nil
-	}
-	return true, nil
+	return existingAnnotations[constants.ResourceHashAnnotation] != newAnnotations[constants.ResourceHashAnnotation], nil
 }
 
 func (rm *resourceManager) GetModuleResources(ctx context.Context, modName, namespace string,

internal/buildsign/resource/resourcemanager_test.go

@@ -309,7 +309,7 @@ var _ = Describe("GetResourceStatus", func() {
 	)
 })
 
-var _ = Describe("IsResourceChanged", func() {
+var _ = Describe("ShouldResourceBeRestarted", func() {
 	var (
 		ctrl                   *gomock.Controller
 		mockKubeClient         *client.MockClient
@@ -325,26 +325,56 @@ var _ = Describe("IsResourceChanged", func() {
 		rm = NewResourceManager(mockKubeClient, mockBuildArgOverrider, mockKernelOSDTKMapping, scheme)
 	})
 
-	DescribeTable("should detect if a build has changed",
-		func(annotation map[string]string, expectchanged bool, expectsErr bool) {
+	newBuild := buildv1.Build{
+		ObjectMeta: metav1.ObjectMeta{
+			Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"},
+		},
+	}
+
+	DescribeTable("should flag a build as needing a restart when its pod was deleted, regardless of spec changes",
+		func(existingBuild *buildv1.Build, expectedRestart bool) {
+			res, err := rm.ShouldResourceBeRestarted(existingBuild, &newBuild)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(res).To(Equal(expectedRestart))
+		},
+		Entry("error phase, pod deleted", &buildv1.Build{
+			ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"}},
+			Status:     buildv1.BuildStatus{Phase: buildv1.BuildPhaseError, Reason: buildv1.StatusReasonBuildPodDeleted},
+		}, true),
+		Entry("error phase, other reason", &buildv1.Build{
+			ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"}},
+			Status:     buildv1.BuildStatus{Phase: buildv1.BuildPhaseError, Reason: buildv1.StatusReasonGenericBuildFailed},
+		}, false),
+		Entry("failed phase, pod deleted reason is irrelevant to Failed phase", &buildv1.Build{
+			ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"}},
+			Status:     buildv1.BuildStatus{Phase: buildv1.BuildPhaseFailed, Reason: buildv1.StatusReasonBuildPodDeleted},
+		}, false),
+		Entry("completed", &buildv1.Build{
+			ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"}},
+			Status:     buildv1.BuildStatus{Phase: buildv1.BuildPhaseComplete},
+		}, false),
+	)
+
+	It("errors out on an unexpected resource type", func() {
+		_, err := rm.ShouldResourceBeRestarted(&metav1.ObjectMeta{}, &newBuild)
+		Expect(err).To(HaveOccurred())
+	})
+
+	DescribeTable("should detect if a build's spec has changed",
+		func(annotation map[string]string, expectRestart bool, expectsErr bool) {
 			existingBuild := buildv1.Build{
 				ObjectMeta: metav1.ObjectMeta{
 					Annotations: annotation,
 				},
 			}
-			newBuild := buildv1.Build{
-				ObjectMeta: metav1.ObjectMeta{
-					Annotations: map[string]string{constants.ResourceHashAnnotation: "some hash"},
-				},
-			}
 
-			changed, err := rm.IsResourceChanged(&existingBuild, &newBuild)
+			restart, err := rm.ShouldResourceBeRestarted(&existingBuild, &newBuild)
 
 			if expectsErr {
 				Expect(err).To(HaveOccurred())
 				return
 			}
-			Expect(expectchanged).To(Equal(changed))
+			Expect(restart).To(Equal(expectRestart))
 		},
 
 		Entry("should error if build has no annotations", nil, false, true),

internal/buildsign/resourcemanager.go

@@ -30,7 +30,7 @@ type ResourceManager interface {
 	GetResourceByKernel(ctx context.Context, name, namespace, targetKernel string, resourceType kmmv1beta1.BuildOrSignAction,
 		owner metav1.Object) (metav1.Object, error)
 	GetResourceStatus(obj metav1.Object) (Status, error)
-	IsResourceChanged(existingObj metav1.Object, newObj metav1.Object) (bool, error)
+	ShouldResourceBeRestarted(existingObj metav1.Object, newObj metav1.Object) (bool, error)
 	GetModuleResources(ctx context.Context, modName, namespace string, resourceType kmmv1beta1.BuildOrSignAction,
 		owner metav1.Object) ([]metav1.Object, error)
 	HasResourcesCompletedSuccessfully(ctx context.Context, obj metav1.Object) (bool, error)