diff --git a/cmd/manager-hub/main.go b/cmd/manager-hub/main.go index 017113101..e65a1b69b 100644 --- a/cmd/manager-hub/main.go +++ b/cmd/manager-hub/main.go @@ -161,7 +161,14 @@ func main() { mcmr := hub.NewManagedClusterModuleReconciler( client, manifestwork.NewCreator(client, scheme, kernelAPI, registryAPI, authFactory, cache, operatorNamespace), - cluster.NewClusterAPI(client, module.NewKernelMapper(buildHelperAPI, sign.NewSignerHelper()), buildAPI, signAPI, operatorNamespace), + cluster.NewClusterAPI( + client, + module.NewKernelMapper(buildHelperAPI, sign.NewSignerHelper()), + buildAPI, + signAPI, + operatorNamespace, + cfg.Job.GCDelay, + ), statusupdater.NewManagedClusterModuleStatusUpdater(client), filterAPI, operatorNamespace, diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 264558245..e8bea5fe5 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -170,7 +170,8 @@ func main() { buildAPI, signAPI, kernelAPI, - filterAPI) + filterAPI, + cfg.Job.GCDelay) if err = bsc.SetupWithManager(mgr, constants.KernelLabel); err != nil { cmd.FatalError(setupLogger, err, "unable to create controller", "name", controllers.BuildSignReconcilerName) } diff --git a/internal/build/manager.go b/internal/build/manager.go index 9b1a9ca4c..f41ad9053 100644 --- a/internal/build/manager.go +++ b/internal/build/manager.go @@ -2,6 +2,7 @@ package build import ( "context" + "time" "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,7 +13,7 @@ import ( //go:generate mockgen -source=manager.go -package=build -destination=mock_manager.go type Manager interface { - GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object) ([]string, error) + GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object, delay time.Duration) ([]string, error) ShouldSync(ctx context.Context, mld *api.ModuleLoaderData) (bool, error) diff --git a/internal/build/mock_manager.go b/internal/build/mock_manager.go index 97dc87594..582d68c2b 100644 --- a/internal/build/mock_manager.go +++ b/internal/build/mock_manager.go @@ -11,6 +11,7 @@ package build import ( context "context" reflect "reflect" + time "time" api "github.com/rh-ecosystem-edge/kernel-module-management/internal/api" ocpbuild "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" @@ -42,18 +43,18 @@ func (m *MockManager) EXPECT() *MockManagerMockRecorder { } // GarbageCollect mocks base method. -func (m *MockManager) GarbageCollect(ctx context.Context, modName, namespace string, owner v1.Object) ([]string, error) { +func (m *MockManager) GarbageCollect(ctx context.Context, modName, namespace string, owner v1.Object, delay time.Duration) ([]string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GarbageCollect", ctx, modName, namespace, owner) + ret := m.ctrl.Call(m, "GarbageCollect", ctx, modName, namespace, owner, delay) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) return ret0, ret1 } // GarbageCollect indicates an expected call of GarbageCollect. -func (mr *MockManagerMockRecorder) GarbageCollect(ctx, modName, namespace, owner any) *gomock.Call { +func (mr *MockManagerMockRecorder) GarbageCollect(ctx, modName, namespace, owner, delay any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GarbageCollect", reflect.TypeOf((*MockManager)(nil).GarbageCollect), ctx, modName, namespace, owner) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GarbageCollect", reflect.TypeOf((*MockManager)(nil).GarbageCollect), ctx, modName, namespace, owner, delay) } // ShouldSync mocks base method. diff --git a/internal/build/ocpbuild/manager.go b/internal/build/ocpbuild/manager.go index 9e3ea4d8d..a19543dba 100644 --- a/internal/build/ocpbuild/manager.go +++ b/internal/build/ocpbuild/manager.go @@ -4,9 +4,11 @@ import ( "context" "errors" "fmt" + "time" buildv1 "github.com/openshift/api/build/v1" "github.com/rh-ecosystem-edge/kernel-module-management/internal/build" + "github.com/rh-ecosystem-edge/kernel-module-management/internal/constants" ocpbuildutils "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -32,7 +34,8 @@ func NewManager( maker Maker, ocpBuildsHelper ocpbuildutils.OCPBuildsHelper, authFactory auth.RegistryAuthGetterFactory, - registry registry.Registry) build.Manager { + registry registry.Registry, +) build.Manager { return &manager{ client: client, maker: maker, @@ -42,7 +45,7 @@ func NewManager( } } -func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object) ([]string, error) { +func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object, delay time.Duration) ([]string, error) { moduleBuilds, err := m.ocpBuildsHelper.GetModuleOCPBuilds(ctx, modName, namespace, owner) if err != nil { return nil, fmt.Errorf("failed to get OCP builds for module's builds %s: %v", modName, err) @@ -51,11 +54,19 @@ func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, deleteNames := make([]string, 0, len(moduleBuilds)) for _, moduleBuild := range moduleBuilds { if moduleBuild.Status.Phase == buildv1.BuildPhaseComplete { - err = m.ocpBuildsHelper.DeleteOCPBuild(ctx, &moduleBuild) - if err != nil { - return nil, fmt.Errorf("failed to delete OCP build %s: %v", moduleBuild.Name, err) + if moduleBuild.DeletionTimestamp == nil { + if err = m.ocpBuildsHelper.DeleteOCPBuild(ctx, &moduleBuild); err != nil { + return nil, fmt.Errorf("failed to delete build %s: %v", moduleBuild.Name, err) + } + } + + if moduleBuild.DeletionTimestamp.Add(delay).Before(time.Now()) { + if err = m.ocpBuildsHelper.RemoveFinalizer(ctx, &moduleBuild, constants.GCDelayFinalizer); err != nil { + return nil, fmt.Errorf("could not remove the GC delay finalizer from build %s/%s: %v", moduleBuild.Namespace, moduleBuild.Name, err) + } + + deleteNames = append(deleteNames, moduleBuild.Name) } - deleteNames = append(deleteNames, moduleBuild.Name) } } return deleteNames, nil diff --git a/internal/build/ocpbuild/manager_test.go b/internal/build/ocpbuild/manager_test.go index 708b72ad5..3e7f733d1 100644 --- a/internal/build/ocpbuild/manager_test.go +++ b/internal/build/ocpbuild/manager_test.go @@ -4,10 +4,13 @@ import ( "context" "errors" "fmt" + "time" + "github.com/budougumi0617/cmpmock" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" buildv1 "github.com/openshift/api/build/v1" + "github.com/rh-ecosystem-edge/kernel-module-management/internal/constants" "go.uber.org/mock/gomock" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -271,7 +274,7 @@ var _ = Describe("GarbageCollect", func() { It("GetModuleBuilds failed", func() { mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return(nil, fmt.Errorf("some error")) - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil, 0) Expect(err).To(HaveOccurred()) Expect(deleted).To(BeEmpty()) @@ -287,40 +290,137 @@ var _ = Describe("GarbageCollect", func() { mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return([]buildv1.Build{ocpBuild}, nil), mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild).Return(fmt.Errorf("some error")), ) - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil, 0) Expect(err).To(HaveOccurred()) Expect(deleted).To(BeEmpty()) }) - DescribeTable("should return the correct error and names of the collected OCP builds", - func(buildPhase1 buildv1.BuildPhase, buildPhase2 buildv1.BuildPhase, numSuccessfulBuilds int) { - ocpBuild1 := buildv1.Build{ - Status: buildv1.BuildStatus{ - Phase: buildPhase1, - }, + mld := api.ModuleLoaderData{ + Name: "moduleName", + Owner: &kmmv1beta1.Module{}, + } + + type testCase struct { + podPhase1, podPhase2 buildv1.BuildPhase + gcDelay time.Duration + expectsErr bool + resShouldContainPod1, resShouldContainPod2 bool + } + + DescribeTable("should return the correct error and names of the collected pods", + func(tc testCase) { + const ( + build1Name = "pod-1" + build2Name = "pod-2" + ) + + ctx := context.Background() + + build1 := buildv1.Build{ + ObjectMeta: metav1.ObjectMeta{Name: build1Name}, + Status: buildv1.BuildStatus{Phase: tc.podPhase1}, } - ocpBuild2 := buildv1.Build{ - Status: buildv1.BuildStatus{ - Phase: buildPhase2, - }, + build2 := buildv1.Build{ + ObjectMeta: metav1.ObjectMeta{Name: build2Name}, + Status: buildv1.BuildStatus{Phase: tc.podPhase2}, } - mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return([]buildv1.Build{ocpBuild1, ocpBuild2}, nil) - if buildPhase1 == buildv1.BuildPhaseComplete { - mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild1).Return(nil) + + returnedError := fmt.Errorf("some error") + if !tc.expectsErr { + returnedError = nil } - if buildPhase2 == buildv1.BuildPhaseComplete { - mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild2).Return(nil) + + buildList := []buildv1.Build{build1, build2} + + calls := []any{ + mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, mld.Name, mld.Namespace, mld.Owner).Return(buildList, returnedError), } - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + if !tc.expectsErr { + now := metav1.Now() + + for i := 0; i < len(buildList); i++ { + build := &buildList[i] + + if build.Status.Phase == buildv1.BuildPhaseComplete { + c := mockOCPBuildsHelper. + EXPECT(). + DeleteOCPBuild(ctx, cmpmock.DiffEq(build)). + Do(func(_ context.Context, b *buildv1.Build) { + b.DeletionTimestamp = &now + build.DeletionTimestamp = &now + }) + + calls = append(calls, c) + + if time.Now().After(now.Add(tc.gcDelay)) { + calls = append( + calls, + mockOCPBuildsHelper.EXPECT().RemoveFinalizer(ctx, cmpmock.DiffEq(build), constants.GCDelayFinalizer), + ) + } + } + } + } + + gomock.InOrder(calls...) + + names, err := m.GarbageCollect(ctx, mld.Name, mld.Namespace, mld.Owner, tc.gcDelay) + + if tc.expectsErr { + Expect(err).To(HaveOccurred()) + return + } Expect(err).NotTo(HaveOccurred()) - Expect(len(deleted)).To(Equal(numSuccessfulBuilds)) + + if tc.resShouldContainPod1 { + Expect(names).To(ContainElements(build1Name)) + } + + if tc.resShouldContainPod2 { + Expect(names).To(ContainElements(build2Name)) + } }, - Entry("0 successfull builds", buildv1.BuildPhaseRunning, buildv1.BuildPhaseRunning, 0), - Entry("1 successfull builds", buildv1.BuildPhaseRunning, buildv1.BuildPhaseComplete, 1), - Entry("2 successfull builds", buildv1.BuildPhaseComplete, buildv1.BuildPhaseComplete, 2), + Entry( + "all pods succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseComplete, + resShouldContainPod1: true, + resShouldContainPod2: true, + }, + ), + Entry( + "1 pod succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseFailed, + resShouldContainPod1: true, + }, + ), + Entry( + "0 pod succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseFailed, + podPhase2: buildv1.BuildPhaseFailed, + }, + ), + Entry( + "error occurred", + testCase{expectsErr: true}, + ), + Entry( + "GC delayed", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseComplete, + gcDelay: time.Hour, + resShouldContainPod1: false, + resShouldContainPod2: false, + }, + ), ) }) diff --git a/internal/cluster/cluster.go b/internal/cluster/cluster.go index 6142ea5cd..b2206cc94 100644 --- a/internal/cluster/cluster.go +++ b/internal/cluster/cluster.go @@ -6,6 +6,7 @@ import ( "fmt" "sort" "strings" + "time" ocpbuildutils "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -38,6 +39,7 @@ type clusterAPI struct { client client.Client kernelAPI module.KernelMapper buildAPI build.Manager + gcDelay time.Duration signAPI sign.SignManager namespace string } @@ -47,11 +49,13 @@ func NewClusterAPI( kernelAPI module.KernelMapper, buildAPI build.Manager, signAPI sign.SignManager, - defaultJobNamespace string) ClusterAPI { + defaultJobNamespace string, + gcDelay time.Duration) ClusterAPI { return &clusterAPI{ client: client, kernelAPI: kernelAPI, buildAPI: buildAPI, + gcDelay: gcDelay, signAPI: signAPI, namespace: defaultJobNamespace, } @@ -143,12 +147,12 @@ func (c *clusterAPI) BuildAndSign( } func (c *clusterAPI) GarbageCollectBuildsAndSigns(ctx context.Context, mcm hubv1beta1.ManagedClusterModule) ([]string, error) { - deletedBuilds, err := c.buildAPI.GarbageCollect(ctx, mcm.Name, c.namespace, &mcm) + deletedBuilds, err := c.buildAPI.GarbageCollect(ctx, mcm.Name, c.namespace, &mcm, c.gcDelay) if err != nil { return nil, fmt.Errorf("failed to garbage collect build object: %v", err) } - deletedSigns, err := c.signAPI.GarbageCollect(ctx, mcm.Name, c.namespace, &mcm) + deletedSigns, err := c.signAPI.GarbageCollect(ctx, mcm.Name, c.namespace, &mcm, c.gcDelay) if err != nil { return nil, fmt.Errorf("failed to garbage collect sign object: %v", err) } diff --git a/internal/cluster/cluster_test.go b/internal/cluster/cluster_test.go index 62db9c92a..59d5003af 100644 --- a/internal/cluster/cluster_test.go +++ b/internal/cluster/cluster_test.go @@ -5,6 +5,7 @@ import ( "errors" "sort" "strings" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -31,7 +32,7 @@ var _ = Describe("RequestedManagedClusterModule", func() { var c ClusterAPI BeforeEach(func() { - c = NewClusterAPI(clnt, mockKM, nil, nil, "") + c = NewClusterAPI(clnt, mockKM, nil, nil, "", 0) }) It("should return the requested ManagedClusterModule", func() { @@ -93,7 +94,7 @@ var _ = Describe("SelectedManagedClusters", func() { var c ClusterAPI BeforeEach(func() { - c = NewClusterAPI(clnt, mockKM, nil, nil, "") + c = NewClusterAPI(clnt, mockKM, nil, nil, "", 0) }) It("should return the ManagedClusters matching the ManagedClusterModule selector", func() { @@ -155,7 +156,7 @@ var _ = Describe("KernelVersions", func() { var c ClusterAPI BeforeEach(func() { - c = NewClusterAPI(clnt, mockKM, nil, nil, "") + c = NewClusterAPI(clnt, mockKM, nil, nil, "", 0) }) It("should return an error when no cluster claims are found", func() { @@ -209,7 +210,7 @@ var _ = Describe("BuildAndSign", func() { var c ClusterAPI BeforeEach(func() { - c = NewClusterAPI(clnt, mockKM, mockBM, mockSM, namespace) + c = NewClusterAPI(clnt, mockKM, mockBM, mockSM, namespace, 0) }) var ( @@ -386,10 +387,12 @@ var _ = Describe("BuildAndSign", func() { }) var _ = Describe("GarbageCollectBuildsAndSigns", func() { + const gcDelay = 1 * time.Hour + var c ClusterAPI BeforeEach(func() { - c = NewClusterAPI(clnt, nil, mockBM, mockSM, namespace) + c = NewClusterAPI(clnt, nil, mockBM, mockSM, namespace, gcDelay) }) DescribeTable("return deleted builds and signs if no failure", func(buildsToDeleteFound, signsToDeleteFound bool) { @@ -408,8 +411,8 @@ var _ = Describe("GarbageCollectBuildsAndSigns", func() { ctx := context.Background() gomock.InOrder( - mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm).Return(deletedBuilds, nil), - mockSM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm).Return(deletedSigns, nil), + mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm, gcDelay).Return(deletedBuilds, nil), + mockSM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm, gcDelay).Return(deletedSigns, nil), ) collected, err := c.GarbageCollectBuildsAndSigns(ctx, mcm) @@ -431,8 +434,8 @@ var _ = Describe("GarbageCollectBuildsAndSigns", func() { collectedBuilds := []string{"test-build"} gomock.InOrder( - mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm).Return(collectedBuilds, nil), - mockSM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm).Return(nil, errors.New("test")), + mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm, gcDelay).Return(collectedBuilds, nil), + mockSM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm, gcDelay).Return(nil, errors.New("test")), ) collected, err := c.GarbageCollectBuildsAndSigns(ctx, mcm) @@ -448,7 +451,7 @@ var _ = Describe("GarbageCollectBuildsAndSigns", func() { ctx := context.Background() gomock.InOrder( - mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm).Return(nil, errors.New("test")), + mockBM.EXPECT().GarbageCollect(ctx, mcm.Name, namespace, &mcm, gcDelay).Return(nil, errors.New("test")), ) collected, err := c.GarbageCollectBuildsAndSigns(ctx, mcm) diff --git a/internal/config/config.go b/internal/config/config.go index ec5f6cf2a..867a4e072 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "fmt" "os" + "time" "github.com/go-logr/logr" "github.com/rh-ecosystem-edge/kernel-module-management/internal/http" @@ -14,6 +15,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" ) +type Job struct { + GCDelay time.Duration `yaml:"gcDelay,omitempty"` +} + type Webhook struct { DisableHTTP2 bool `yaml:"disableHTTP2"` Port int `yaml:"port"` @@ -39,6 +44,7 @@ type Metrics struct { type Config struct { HealthProbeBindAddress string `yaml:"healthProbeBindAddress"` + Job Job `yaml:"job"` LeaderElection LeaderElection `yaml:"leaderElection"` Metrics Metrics `yaml:"metrics"` Webhook Webhook `yaml:"webhook"` diff --git a/internal/config/config_test.go b/internal/config/config_test.go index aee927ad9..dcc115b72 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -1,6 +1,8 @@ package config import ( + "time" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/utils/ptr" @@ -16,6 +18,9 @@ var _ = Describe("ParseFile", func() { It("should parse the file correctly", func() { expected := &Config{ HealthProbeBindAddress: ":8081", + Job: Job{ + GCDelay: time.Hour, + }, LeaderElection: LeaderElection{ Enabled: true, ResourceID: "some-resource-id", diff --git a/internal/config/testdata/config.yaml b/internal/config/testdata/config.yaml index 98689266f..1d9d1cd56 100644 --- a/internal/config/testdata/config.yaml +++ b/internal/config/testdata/config.yaml @@ -3,6 +3,8 @@ metricsBindAddress: 127.0.0.1:8080 webhook: disableHTTP2: true port: 9443 +job: + gcDelay: 1h leaderElection: enabled: true resourceID: some-resource-id diff --git a/internal/constants/constants.go b/internal/constants/constants.go index 13f4c5564..184955fb4 100644 --- a/internal/constants/constants.go +++ b/internal/constants/constants.go @@ -15,6 +15,7 @@ const ( DevicePluginVersionLabelPrefix = "beta.kmm.node.kubernetes.io/version-device-plugin" ModuleVersionLabelPrefix = "kmm.node.kubernetes.io/version-module" + GCDelayFinalizer = "kmm.node.kubernetes.io/gc-delay" ModuleFinalizer = "kmm.node.kubernetes.io/module-finalizer" JobEventFinalizer = "kmm.node.kubernetes.io/job-event-finalizer" diff --git a/internal/controllers/build_sign_reconciler.go b/internal/controllers/build_sign_reconciler.go index 2db0fbac4..60825d5b5 100644 --- a/internal/controllers/build_sign_reconciler.go +++ b/internal/controllers/build_sign_reconciler.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "strings" + "time" buildv1 "github.com/openshift/api/build/v1" kmmv1beta1 "github.com/rh-ecosystem-edge/kernel-module-management/api/v1beta1" @@ -53,8 +54,9 @@ func NewBuildSignReconciler( signAPI sign.SignManager, kernelAPI module.KernelMapper, filter *filter.Filter, + gcDelay time.Duration, ) *BuildSignReconciler { - reconHelperAPI := newBuildSignReconcilerHelper(client, buildAPI, signAPI, kernelAPI) + reconHelperAPI := newBuildSignReconcilerHelper(client, buildAPI, signAPI, kernelAPI, gcDelay) return &BuildSignReconciler{ reconHelperAPI: reconHelperAPI, filter: filter, @@ -110,7 +112,7 @@ func (r *BuildSignReconciler) Reconcile(ctx context.Context, mod *kmmv1beta1.Mod } logger.Info("run garbage collector for build/sign pods") - err = r.reconHelperAPI.garbageCollect(ctx, mod, mldMappings) + err = r.reconHelperAPI.garbageCollect(ctx, mod) if err != nil { return res, fmt.Errorf("failed to run garbage collection: %v", err) } @@ -127,12 +129,13 @@ type buildSignReconcilerHelperAPI interface { getRelevantKernelMappings(ctx context.Context, mod *kmmv1beta1.Module, targetedNodes []v1.Node) (map[string]*api.ModuleLoaderData, error) handleBuild(ctx context.Context, mld *api.ModuleLoaderData) (bool, error) handleSigning(ctx context.Context, mld *api.ModuleLoaderData) (bool, error) - garbageCollect(ctx context.Context, mod *kmmv1beta1.Module, mldMappings map[string]*api.ModuleLoaderData) error + garbageCollect(ctx context.Context, mod *kmmv1beta1.Module) error } type buildSignReconcilerHelper struct { client client.Client buildAPI build.Manager + gcDelay time.Duration signAPI sign.SignManager kernelAPI module.KernelMapper } @@ -140,10 +143,12 @@ type buildSignReconcilerHelper struct { func newBuildSignReconcilerHelper(client client.Client, buildAPI build.Manager, signAPI sign.SignManager, - kernelAPI module.KernelMapper) buildSignReconcilerHelperAPI { + kernelAPI module.KernelMapper, + gcDelay time.Duration) buildSignReconcilerHelperAPI { return &buildSignReconcilerHelper{ client: client, buildAPI: buildAPI, + gcDelay: gcDelay, signAPI: signAPI, kernelAPI: kernelAPI, } @@ -270,13 +275,11 @@ func (bsrh *buildSignReconcilerHelper) handleSigning(ctx context.Context, mld *a return completedSuccessfully, nil } -func (bsrh *buildSignReconcilerHelper) garbageCollect(ctx context.Context, - mod *kmmv1beta1.Module, - mldMappings map[string]*api.ModuleLoaderData) error { +func (bsrh *buildSignReconcilerHelper) garbageCollect(ctx context.Context, mod *kmmv1beta1.Module) error { logger := log.FromContext(ctx) // Garbage collect for successfully finished build pods - deleted, err := bsrh.buildAPI.GarbageCollect(ctx, mod.Name, mod.Namespace, mod) + deleted, err := bsrh.buildAPI.GarbageCollect(ctx, mod.Name, mod.Namespace, mod, bsrh.gcDelay) if err != nil { return fmt.Errorf("could not garbage collect build objects: %v", err) } @@ -284,7 +287,7 @@ func (bsrh *buildSignReconcilerHelper) garbageCollect(ctx context.Context, logger.Info("Garbage-collected Build objects", "names", deleted) // Garbage collect for successfully finished sign pods - deleted, err = bsrh.signAPI.GarbageCollect(ctx, mod.Name, mod.Namespace, mod) + deleted, err = bsrh.signAPI.GarbageCollect(ctx, mod.Name, mod.Namespace, mod, bsrh.gcDelay) if err != nil { return fmt.Errorf("could not garbage collect sign objects: %v", err) } diff --git a/internal/controllers/build_sign_reconciler_test.go b/internal/controllers/build_sign_reconciler_test.go index e223a5264..406dfb5e2 100644 --- a/internal/controllers/build_sign_reconciler_test.go +++ b/internal/controllers/build_sign_reconciler_test.go @@ -3,6 +3,7 @@ package controllers import ( "context" "fmt" + "time" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -71,7 +72,7 @@ var _ = Describe("BuildSignReconciler_Reconcile", func() { goto executeTestFunction } mockReconHelper.EXPECT().handleSigning(ctx, mappings["kernelVersion"]).Return(true, nil) - mockReconHelper.EXPECT().garbageCollect(ctx, &mod, mappings).Return(returnedError) + mockReconHelper.EXPECT().garbageCollect(ctx, &mod).Return(returnedError) executeTestFunction: res, err := bsr.Reconcile(ctx, &mod) @@ -93,7 +94,7 @@ var _ = Describe("BuildSignReconciler_Reconcile", func() { mockReconHelper.EXPECT().getNodesListBySelector(ctx, mod).Return(selectNodesList, nil), mockReconHelper.EXPECT().getRelevantKernelMappings(ctx, mod, selectNodesList).Return(mappings, nil), mockReconHelper.EXPECT().handleBuild(ctx, mappings["kernelVersion"]).Return(false, nil), - mockReconHelper.EXPECT().garbageCollect(ctx, mod, mappings).Return(nil), + mockReconHelper.EXPECT().garbageCollect(ctx, mod).Return(nil), ) res, err := bsr.Reconcile(ctx, mod) @@ -110,7 +111,7 @@ var _ = Describe("BuildSignReconciler_Reconcile", func() { mockReconHelper.EXPECT().getRelevantKernelMappings(ctx, mod, selectNodesList).Return(mappings, nil), mockReconHelper.EXPECT().handleBuild(ctx, mappings["kernelVersion"]).Return(true, nil), mockReconHelper.EXPECT().handleSigning(ctx, mappings["kernelVersion"]).Return(false, nil), - mockReconHelper.EXPECT().garbageCollect(ctx, mod, mappings).Return(nil), + mockReconHelper.EXPECT().garbageCollect(ctx, mod).Return(nil), ) res, err := bsr.Reconcile(ctx, mod) @@ -127,7 +128,7 @@ var _ = Describe("BuildSignReconciler_Reconcile", func() { mockReconHelper.EXPECT().getRelevantKernelMappings(ctx, mod, selectNodesList).Return(mappings, nil), mockReconHelper.EXPECT().handleBuild(ctx, mappings["kernelVersion"]).Return(true, nil), mockReconHelper.EXPECT().handleSigning(ctx, mappings["kernelVersion"]).Return(true, nil), - mockReconHelper.EXPECT().garbageCollect(ctx, mod, mappings).Return(nil), + mockReconHelper.EXPECT().garbageCollect(ctx, mod).Return(nil), ) res, err := bsr.Reconcile(ctx, mod) @@ -147,7 +148,7 @@ var _ = Describe("BuildSignReconciler_getNodesListBySelector", func() { BeforeEach(func() { ctrl = gomock.NewController(GinkgoT()) clnt = client.NewMockClient(ctrl) - bsrh = newBuildSignReconcilerHelper(clnt, nil, nil, nil) + bsrh = newBuildSignReconcilerHelper(clnt, nil, nil, nil, 0) }) It("list failed", func() { @@ -203,7 +204,7 @@ var _ = Describe("BuildSignReconciler_getRelevantKernelMappings", func() { BeforeEach(func() { ctrl = gomock.NewController(GinkgoT()) mockKM = module.NewMockKernelMapper(ctrl) - bsrh = newBuildSignReconcilerHelper(nil, nil, nil, mockKM) + bsrh = newBuildSignReconcilerHelper(nil, nil, nil, mockKM, 0) }) node1 := v1.Node{ @@ -273,7 +274,7 @@ var _ = Describe("BuildSignReconciler_handleBuild", func() { BeforeEach(func() { ctrl = gomock.NewController(GinkgoT()) mockBM = build.NewMockManager(ctrl) - bsrh = newBuildSignReconcilerHelper(nil, mockBM, nil, nil) + bsrh = newBuildSignReconcilerHelper(nil, mockBM, nil, nil, 0) }) const ( @@ -345,7 +346,7 @@ var _ = Describe("BuildSignReconciler_handleSigning", func() { BeforeEach(func() { ctrl = gomock.NewController(GinkgoT()) mockSM = sign.NewMockSignManager(ctrl) - bsrh = newBuildSignReconcilerHelper(nil, nil, mockSM, nil) + bsrh = newBuildSignReconcilerHelper(nil, nil, mockSM, nil, 0) }) const ( @@ -435,6 +436,8 @@ var _ = Describe("BuildSignReconciler_handleSigning", func() { }) var _ = Describe("ModuleReconciler_garbageCollect", func() { + const gcDelay = time.Hour + var ( ctrl *gomock.Controller mockBM *build.MockManager @@ -446,7 +449,7 @@ var _ = Describe("ModuleReconciler_garbageCollect", func() { ctrl = gomock.NewController(GinkgoT()) mockBM = build.NewMockManager(ctrl) mockSM = sign.NewMockSignManager(ctrl) - bsrh = newBuildSignReconcilerHelper(nil, mockBM, mockSM, nil) + bsrh = newBuildSignReconcilerHelper(nil, mockBM, mockSM, nil, gcDelay) }) mod := &kmmv1beta1.Module{ @@ -457,32 +460,26 @@ var _ = Describe("ModuleReconciler_garbageCollect", func() { } It("good flow", func() { - mldMappings := map[string]*api.ModuleLoaderData{ - "kernelVersion1": &api.ModuleLoaderData{}, "kernelVersion2": &api.ModuleLoaderData{}, - } gomock.InOrder( - mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod).Return(nil, nil), - mockSM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod).Return(nil, nil), + mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod, gcDelay).Return(nil, nil), + mockSM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod, gcDelay).Return(nil, nil), ) - err := bsrh.garbageCollect(context.Background(), mod, mldMappings) + err := bsrh.garbageCollect(context.Background(), mod) Expect(err).NotTo(HaveOccurred()) }) DescribeTable("check error flows", func(buildError bool) { returnedError := fmt.Errorf("some error") - mldMappings := map[string]*api.ModuleLoaderData{ - "kernelVersion1": &api.ModuleLoaderData{}, "kernelVersion2": &api.ModuleLoaderData{}, - } if buildError { - mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod).Return(nil, returnedError) + mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod, gcDelay).Return(nil, returnedError) goto executeTestFunction } - mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod).Return(nil, nil) - mockSM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod).Return(nil, returnedError) + mockBM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod, gcDelay).Return(nil, nil) + mockSM.EXPECT().GarbageCollect(context.Background(), mod.Name, mod.Namespace, mod, gcDelay).Return(nil, returnedError) executeTestFunction: - err := bsrh.garbageCollect(context.Background(), mod, mldMappings) + err := bsrh.garbageCollect(context.Background(), mod) Expect(err).To(HaveOccurred()) }, diff --git a/internal/controllers/mock_build_sign_reconciler.go b/internal/controllers/mock_build_sign_reconciler.go index c52a43b0e..6bf63165b 100644 --- a/internal/controllers/mock_build_sign_reconciler.go +++ b/internal/controllers/mock_build_sign_reconciler.go @@ -42,17 +42,17 @@ func (m *MockbuildSignReconcilerHelperAPI) EXPECT() *MockbuildSignReconcilerHelp } // garbageCollect mocks base method. -func (m *MockbuildSignReconcilerHelperAPI) garbageCollect(ctx context.Context, mod *v1beta1.Module, mldMappings map[string]*api.ModuleLoaderData) error { +func (m *MockbuildSignReconcilerHelperAPI) garbageCollect(ctx context.Context, mod *v1beta1.Module) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "garbageCollect", ctx, mod, mldMappings) + ret := m.ctrl.Call(m, "garbageCollect", ctx, mod) ret0, _ := ret[0].(error) return ret0 } // garbageCollect indicates an expected call of garbageCollect. -func (mr *MockbuildSignReconcilerHelperAPIMockRecorder) garbageCollect(ctx, mod, mldMappings any) *gomock.Call { +func (mr *MockbuildSignReconcilerHelperAPIMockRecorder) garbageCollect(ctx, mod any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "garbageCollect", reflect.TypeOf((*MockbuildSignReconcilerHelperAPI)(nil).garbageCollect), ctx, mod, mldMappings) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "garbageCollect", reflect.TypeOf((*MockbuildSignReconcilerHelperAPI)(nil).garbageCollect), ctx, mod) } // getNodesListBySelector mocks base method. diff --git a/internal/sign/manager.go b/internal/sign/manager.go index 067397378..97ab99982 100644 --- a/internal/sign/manager.go +++ b/internal/sign/manager.go @@ -2,6 +2,7 @@ package sign import ( "context" + "time" "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -12,7 +13,7 @@ import ( //go:generate mockgen -source=manager.go -package=sign -destination=mock_manager.go type SignManager interface { - GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object) ([]string, error) + GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object, delay time.Duration) ([]string, error) ShouldSync(ctx context.Context, mld *api.ModuleLoaderData) (bool, error) diff --git a/internal/sign/mock_manager.go b/internal/sign/mock_manager.go index 2dee3cc2b..b6aee0fda 100644 --- a/internal/sign/mock_manager.go +++ b/internal/sign/mock_manager.go @@ -11,6 +11,7 @@ package sign import ( context "context" reflect "reflect" + time "time" api "github.com/rh-ecosystem-edge/kernel-module-management/internal/api" ocpbuild "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" @@ -42,18 +43,18 @@ func (m *MockSignManager) EXPECT() *MockSignManagerMockRecorder { } // GarbageCollect mocks base method. -func (m *MockSignManager) GarbageCollect(ctx context.Context, modName, namespace string, owner v1.Object) ([]string, error) { +func (m *MockSignManager) GarbageCollect(ctx context.Context, modName, namespace string, owner v1.Object, delay time.Duration) ([]string, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GarbageCollect", ctx, modName, namespace, owner) + ret := m.ctrl.Call(m, "GarbageCollect", ctx, modName, namespace, owner, delay) ret0, _ := ret[0].([]string) ret1, _ := ret[1].(error) return ret0, ret1 } // GarbageCollect indicates an expected call of GarbageCollect. -func (mr *MockSignManagerMockRecorder) GarbageCollect(ctx, modName, namespace, owner any) *gomock.Call { +func (mr *MockSignManagerMockRecorder) GarbageCollect(ctx, modName, namespace, owner, delay any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GarbageCollect", reflect.TypeOf((*MockSignManager)(nil).GarbageCollect), ctx, modName, namespace, owner) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GarbageCollect", reflect.TypeOf((*MockSignManager)(nil).GarbageCollect), ctx, modName, namespace, owner, delay) } // ShouldSync mocks base method. diff --git a/internal/sign/ocpbuild/manager.go b/internal/sign/ocpbuild/manager.go index 6778d7e21..122d6a708 100644 --- a/internal/sign/ocpbuild/manager.go +++ b/internal/sign/ocpbuild/manager.go @@ -4,8 +4,10 @@ import ( "context" "errors" "fmt" + "time" buildv1 "github.com/openshift/api/build/v1" + "github.com/rh-ecosystem-edge/kernel-module-management/internal/constants" "github.com/rh-ecosystem-edge/kernel-module-management/internal/sign" ocpbuildutils "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,7 +44,7 @@ func NewManager( } } -func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object) ([]string, error) { +func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, owner metav1.Object, delay time.Duration) ([]string, error) { moduleSigns, err := m.ocpBuildsHelper.GetModuleOCPBuilds(ctx, modName, namespace, owner) if err != nil { return nil, fmt.Errorf("failed to get OCP builds for module's signs %s: %v", modName, err) @@ -51,11 +53,19 @@ func (m *manager) GarbageCollect(ctx context.Context, modName, namespace string, deleteNames := make([]string, 0, len(moduleSigns)) for _, moduleSign := range moduleSigns { if moduleSign.Status.Phase == buildv1.BuildPhaseComplete { - err = m.ocpBuildsHelper.DeleteOCPBuild(ctx, &moduleSign) - if err != nil { - return nil, fmt.Errorf("failed to delete OCP build %s: %v", moduleSign.Name, err) + if moduleSign.DeletionTimestamp == nil { + if err = m.ocpBuildsHelper.DeleteOCPBuild(ctx, &moduleSign); err != nil { + return nil, fmt.Errorf("failed to delete signing pod %s: %v", moduleSign.Name, err) + } + } + + if moduleSign.DeletionTimestamp.Add(delay).Before(time.Now()) { + if err = m.ocpBuildsHelper.RemoveFinalizer(ctx, &moduleSign, constants.GCDelayFinalizer); err != nil { + return nil, fmt.Errorf("could not remove the GC delay finalizer from pod %s/%s: %v", moduleSign.Namespace, moduleSign.Name, err) + } + + deleteNames = append(deleteNames, moduleSign.Name) } - deleteNames = append(deleteNames, moduleSign.Name) } } return deleteNames, nil diff --git a/internal/sign/ocpbuild/manager_test.go b/internal/sign/ocpbuild/manager_test.go index 72e924102..2e55e62ba 100644 --- a/internal/sign/ocpbuild/manager_test.go +++ b/internal/sign/ocpbuild/manager_test.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "time" + "github.com/budougumi0617/cmpmock" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" buildv1 "github.com/openshift/api/build/v1" @@ -12,6 +14,7 @@ import ( "github.com/rh-ecosystem-edge/kernel-module-management/internal/api" "github.com/rh-ecosystem-edge/kernel-module-management/internal/auth" "github.com/rh-ecosystem-edge/kernel-module-management/internal/client" + "github.com/rh-ecosystem-edge/kernel-module-management/internal/constants" "github.com/rh-ecosystem-edge/kernel-module-management/internal/registry" signmanager "github.com/rh-ecosystem-edge/kernel-module-management/internal/sign" "github.com/rh-ecosystem-edge/kernel-module-management/internal/utils/ocpbuild" @@ -253,7 +256,7 @@ var _ = Describe("GarbageCollect", func() { It("GetModuleOCPBuilds failed", func() { mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return(nil, fmt.Errorf("some error")) - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil, 0) Expect(err).To(HaveOccurred()) Expect(deleted).To(BeEmpty()) @@ -269,40 +272,137 @@ var _ = Describe("GarbageCollect", func() { mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return([]buildv1.Build{ocpBuild}, nil), mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild).Return(fmt.Errorf("some error")), ) - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil, 0) Expect(err).To(HaveOccurred()) Expect(deleted).To(BeEmpty()) }) - DescribeTable("should return the correct error and names of the collected OCP builds", - func(buildPhase1 buildv1.BuildPhase, buildPhase2 buildv1.BuildPhase, numSuccessfulBuilds int) { - ocpBuild1 := buildv1.Build{ - Status: buildv1.BuildStatus{ - Phase: buildPhase1, - }, + mld := api.ModuleLoaderData{ + Name: "moduleName", + Owner: &kmmv1beta1.Module{}, + } + + type testCase struct { + podPhase1, podPhase2 buildv1.BuildPhase + gcDelay time.Duration + expectsErr bool + resShouldContainPod1, resShouldContainPod2 bool + } + + DescribeTable("should return the correct error and names of the collected pods", + func(tc testCase) { + const ( + build1Name = "pod-1" + build2Name = "pod-2" + ) + + ctx := context.Background() + + build1 := buildv1.Build{ + ObjectMeta: metav1.ObjectMeta{Name: build1Name}, + Status: buildv1.BuildStatus{Phase: tc.podPhase1}, } - ocpBuild2 := buildv1.Build{ - Status: buildv1.BuildStatus{ - Phase: buildPhase2, - }, + build2 := buildv1.Build{ + ObjectMeta: metav1.ObjectMeta{Name: build2Name}, + Status: buildv1.BuildStatus{Phase: tc.podPhase2}, } - mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, moduleName, namespace, nil).Return([]buildv1.Build{ocpBuild1, ocpBuild2}, nil) - if buildPhase1 == buildv1.BuildPhaseComplete { - mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild1).Return(nil) + + returnedError := fmt.Errorf("some error") + if !tc.expectsErr { + returnedError = nil } - if buildPhase2 == buildv1.BuildPhaseComplete { - mockOCPBuildsHelper.EXPECT().DeleteOCPBuild(ctx, &ocpBuild2).Return(nil) + + buildList := []buildv1.Build{build1, build2} + + calls := []any{ + mockOCPBuildsHelper.EXPECT().GetModuleOCPBuilds(ctx, mld.Name, mld.Namespace, mld.Owner).Return(buildList, returnedError), } - deleted, err := m.GarbageCollect(ctx, moduleName, namespace, nil) + if !tc.expectsErr { + now := metav1.Now() + + for i := 0; i < len(buildList); i++ { + build := &buildList[i] + + if build.Status.Phase == buildv1.BuildPhaseComplete { + c := mockOCPBuildsHelper. + EXPECT(). + DeleteOCPBuild(ctx, cmpmock.DiffEq(build)). + Do(func(_ context.Context, b *buildv1.Build) { + b.DeletionTimestamp = &now + build.DeletionTimestamp = &now + }) + + calls = append(calls, c) + + if time.Now().After(now.Add(tc.gcDelay)) { + calls = append( + calls, + mockOCPBuildsHelper.EXPECT().RemoveFinalizer(ctx, cmpmock.DiffEq(build), constants.GCDelayFinalizer), + ) + } + } + } + } + + gomock.InOrder(calls...) + + names, err := m.GarbageCollect(ctx, mld.Name, mld.Namespace, mld.Owner, tc.gcDelay) + + if tc.expectsErr { + Expect(err).To(HaveOccurred()) + return + } Expect(err).NotTo(HaveOccurred()) - Expect(len(deleted)).To(Equal(numSuccessfulBuilds)) + + if tc.resShouldContainPod1 { + Expect(names).To(ContainElements(build1Name)) + } + + if tc.resShouldContainPod2 { + Expect(names).To(ContainElements(build2Name)) + } }, - Entry("0 successfull builds", buildv1.BuildPhaseRunning, buildv1.BuildPhaseRunning, 0), - Entry("1 successfull builds", buildv1.BuildPhaseRunning, buildv1.BuildPhaseComplete, 1), - Entry("2 successfull builds", buildv1.BuildPhaseComplete, buildv1.BuildPhaseComplete, 2), + Entry( + "all pods succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseComplete, + resShouldContainPod1: true, + resShouldContainPod2: true, + }, + ), + Entry( + "1 pod succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseFailed, + resShouldContainPod1: true, + }, + ), + Entry( + "0 pod succeeded", + testCase{ + podPhase1: buildv1.BuildPhaseFailed, + podPhase2: buildv1.BuildPhaseFailed, + }, + ), + Entry( + "error occurred", + testCase{expectsErr: true}, + ), + Entry( + "GC delayed", + testCase{ + podPhase1: buildv1.BuildPhaseComplete, + podPhase2: buildv1.BuildPhaseComplete, + gcDelay: time.Hour, + resShouldContainPod1: false, + resShouldContainPod2: false, + }, + ), ) }) diff --git a/internal/utils/ocpbuild/helper.go b/internal/utils/ocpbuild/helper.go index b8406da3e..bf6dc08c5 100644 --- a/internal/utils/ocpbuild/helper.go +++ b/internal/utils/ocpbuild/helper.go @@ -9,6 +9,7 @@ import ( "github.com/rh-ecosystem-edge/kernel-module-management/internal/api" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) var ErrNoMatchingBuild = errors.New("no matching Build") @@ -19,6 +20,7 @@ type OCPBuildsHelper interface { GetModuleOCPBuildByKernel(ctx context.Context, mld *api.ModuleLoaderData, owner metav1.Object) (*buildv1.Build, error) GetModuleOCPBuilds(ctx context.Context, moduleName, moduleNamespace string, owner metav1.Object) ([]buildv1.Build, error) DeleteOCPBuild(ctx context.Context, build *buildv1.Build) error + RemoveFinalizer(ctx context.Context, build *buildv1.Build, finalizer string) error } type ocpBuildsHelper struct { @@ -84,6 +86,18 @@ func (o *ocpBuildsHelper) DeleteOCPBuild(ctx context.Context, build *buildv1.Bui return o.client.Delete(ctx, build, opts...) } +func (o *ocpBuildsHelper) RemoveFinalizer(ctx context.Context, build *buildv1.Build, finalizer string) error { + if !controllerutil.RemoveFinalizer(build, finalizer) { + return nil + } + + podCopy := build.DeepCopy() + + controllerutil.RemoveFinalizer(build, finalizer) + + return o.client.Patch(ctx, build, client.MergeFrom(podCopy)) +} + func filterOCPBuildsByOwner(builds []buildv1.Build, owner metav1.Object) []buildv1.Build { ownedBuilds := []buildv1.Build{} for _, build := range builds { diff --git a/internal/utils/ocpbuild/mock_helper.go b/internal/utils/ocpbuild/mock_helper.go index c37ef4846..8ab23665f 100644 --- a/internal/utils/ocpbuild/mock_helper.go +++ b/internal/utils/ocpbuild/mock_helper.go @@ -84,3 +84,17 @@ func (mr *MockOCPBuildsHelperMockRecorder) GetModuleOCPBuilds(ctx, moduleName, m mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetModuleOCPBuilds", reflect.TypeOf((*MockOCPBuildsHelper)(nil).GetModuleOCPBuilds), ctx, moduleName, moduleNamespace, owner) } + +// RemoveFinalizer mocks base method. +func (m *MockOCPBuildsHelper) RemoveFinalizer(ctx context.Context, build *v1.Build, finalizer string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "RemoveFinalizer", ctx, build, finalizer) + ret0, _ := ret[0].(error) + return ret0 +} + +// RemoveFinalizer indicates an expected call of RemoveFinalizer. +func (mr *MockOCPBuildsHelperMockRecorder) RemoveFinalizer(ctx, build, finalizer any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveFinalizer", reflect.TypeOf((*MockOCPBuildsHelper)(nil).RemoveFinalizer), ctx, build, finalizer) +}