From 6c4d2be532c4213d060d4dcdb6d6a9f336e62584 Mon Sep 17 00:00:00 2001 From: TomerNewman Date: Sun, 23 Aug 2026 15:35:56 +0300 Subject: [PATCH] Retry build/sign resource when its pod was evicted During a cluster upgrade, node drains can delete a Build's pod before it completes, leaving the Build in Error(BuildPodDeleted) forever since Sync() only recreated resources whose spec had changed. Detect this recoverable failure and delete the resource so it gets retried. --- internal/buildsign/manager.go | 19 +++---- internal/buildsign/manager_test.go | 43 ++++++++++++++-- internal/buildsign/mock_resourcemanager.go | 28 +++++------ .../buildsign/resource/resourcemanager.go | 12 +++-- .../resource/resourcemanager_test.go | 50 +++++++++++++++---- internal/buildsign/resourcemanager.go | 2 +- 6 files changed, 110 insertions(+), 44 deletions(-) diff --git a/internal/buildsign/manager.go b/internal/buildsign/manager.go index 2aaf54316..977876f88 100644 --- a/internal/buildsign/manager.go +++ b/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 diff --git a/internal/buildsign/manager_test.go b/internal/buildsign/manager_test.go index 01019ea22..4b0605c91 100644 --- a/internal/buildsign/manager_test.go +++ b/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) } diff --git a/internal/buildsign/mock_resourcemanager.go b/internal/buildsign/mock_resourcemanager.go index fbd51608e..89e892846 100644 --- a/internal/buildsign/mock_resourcemanager.go +++ b/internal/buildsign/mock_resourcemanager.go @@ -129,32 +129,32 @@ func (mr *MockResourceManagerMockRecorder) HasResourcesCompletedSuccessfully(ctx return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasResourcesCompletedSuccessfully", reflect.TypeOf((*MockResourceManager)(nil).HasResourcesCompletedSuccessfully), ctx, obj) } -// IsResourceChanged mocks base method. -func (m *MockResourceManager) IsResourceChanged(existingObj, newObj v1.Object) (bool, error) { +// MakeResourceTemplate mocks base method. +func (m *MockResourceManager) MakeResourceTemplate(ctx context.Context, mld *api.ModuleLoaderData, owner v1.Object, pushImage bool, resourceType v1beta1.BuildOrSignAction) (v1.Object, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "IsResourceChanged", existingObj, newObj) - ret0, _ := ret[0].(bool) + ret := m.ctrl.Call(m, "MakeResourceTemplate", ctx, mld, owner, pushImage, resourceType) + ret0, _ := ret[0].(v1.Object) ret1, _ := ret[1].(error) return ret0, ret1 } -// IsResourceChanged indicates an expected call of IsResourceChanged. -func (mr *MockResourceManagerMockRecorder) IsResourceChanged(existingObj, newObj any) *gomock.Call { +// MakeResourceTemplate indicates an expected call of MakeResourceTemplate. +func (mr *MockResourceManagerMockRecorder) MakeResourceTemplate(ctx, mld, owner, pushImage, resourceType any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsResourceChanged", reflect.TypeOf((*MockResourceManager)(nil).IsResourceChanged), existingObj, newObj) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MakeResourceTemplate", reflect.TypeOf((*MockResourceManager)(nil).MakeResourceTemplate), ctx, mld, owner, pushImage, resourceType) } -// MakeResourceTemplate mocks base method. -func (m *MockResourceManager) MakeResourceTemplate(ctx context.Context, mld *api.ModuleLoaderData, owner v1.Object, pushImage bool, resourceType v1beta1.BuildOrSignAction) (v1.Object, error) { +// ShouldResourceBeRestarted mocks base method. +func (m *MockResourceManager) ShouldResourceBeRestarted(existingObj, newObj v1.Object) (bool, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "MakeResourceTemplate", ctx, mld, owner, pushImage, resourceType) - ret0, _ := ret[0].(v1.Object) + ret := m.ctrl.Call(m, "ShouldResourceBeRestarted", existingObj, newObj) + ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } -// MakeResourceTemplate indicates an expected call of MakeResourceTemplate. -func (mr *MockResourceManagerMockRecorder) MakeResourceTemplate(ctx, mld, owner, pushImage, resourceType any) *gomock.Call { +// ShouldResourceBeRestarted indicates an expected call of ShouldResourceBeRestarted. +func (mr *MockResourceManagerMockRecorder) ShouldResourceBeRestarted(existingObj, newObj any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "MakeResourceTemplate", reflect.TypeOf((*MockResourceManager)(nil).MakeResourceTemplate), ctx, mld, owner, pushImage, resourceType) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ShouldResourceBeRestarted", reflect.TypeOf((*MockResourceManager)(nil).ShouldResourceBeRestarted), existingObj, newObj) } diff --git a/internal/buildsign/resource/resourcemanager.go b/internal/buildsign/resource/resourcemanager.go index 1f1354c6c..bc0a6c7e6 100644 --- a/internal/buildsign/resource/resourcemanager.go +++ b/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, diff --git a/internal/buildsign/resource/resourcemanager_test.go b/internal/buildsign/resource/resourcemanager_test.go index 16264db84..176485b00 100644 --- a/internal/buildsign/resource/resourcemanager_test.go +++ b/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), diff --git a/internal/buildsign/resourcemanager.go b/internal/buildsign/resourcemanager.go index ecea3bfa1..38c869f74 100644 --- a/internal/buildsign/resourcemanager.go +++ b/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)