From 15b1211bcc710cb27fb4aadef607792eff499a44 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 5 Sep 2025 21:01:15 +0000 Subject: [PATCH 1/3] refactor ensure job to remove named parameters (#3644) Signed-off-by: Per Goncalves da Silva Co-authored-by: Per Goncalves da Silva Upstream-repository: operator-lifecycle-manager Upstream-commit: ce2832a3b2fd0b70033ff96f0e3ec9d7ef605678 --- .../pkg/controller/bundle/bundle_unpacker.go | 24 ++++++++----------- .../pkg/controller/bundle/bundle_unpacker.go | 24 ++++++++----------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go index ad300b7c88..5042136bc9 100644 --- a/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go +++ b/staging/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go @@ -681,18 +681,18 @@ func (c *ConfigMapUnpacker) ensureConfigmap(csRef *corev1.ObjectReference, name return } -func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath string, secrets []corev1.LocalObjectReference, timeout time.Duration, unpackRetryInterval time.Duration) (job *batchv1.Job, err error) { +func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath string, secrets []corev1.LocalObjectReference, timeout time.Duration, unpackRetryInterval time.Duration) (*batchv1.Job, error) { fresh := c.job(cmRef, bundlePath, secrets, timeout) var jobs, toDelete []*batchv1.Job - jobs, err = c.jobLister.Jobs(fresh.GetNamespace()).List(k8slabels.ValidatedSetSelector{bundleUnpackRefLabel: cmRef.Name}) + jobs, err := c.jobLister.Jobs(fresh.GetNamespace()).List(k8slabels.ValidatedSetSelector{bundleUnpackRefLabel: cmRef.Name}) if err != nil { - return + return nil, err } // This is to ensure that we account for any existing unpack jobs that may be missing the label jobWithoutLabel, err := c.jobLister.Jobs(fresh.GetNamespace()).Get(cmRef.Name) if err != nil && !apierrors.IsNotFound(err) { - return + return nil, err } if jobWithoutLabel != nil { _, labelExists := jobWithoutLabel.Labels[bundleUnpackRefLabel] @@ -702,12 +702,11 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath } if len(jobs) == 0 { - job, err = c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) - return + return c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) } - maxRetainedJobs := 5 // TODO: make this configurable - job, toDelete = sortUnpackJobs(jobs, maxRetainedJobs) // choose latest or on-failed job attempt + maxRetainedJobs := 5 // TODO: make this configurable + job, toDelete := sortUnpackJobs(jobs, maxRetainedJobs) // choose latest or on-failed job attempt // only check for retries if an unpackRetryInterval is specified if unpackRetryInterval > 0 { @@ -716,7 +715,7 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath if cond, failed := getCondition(job, batchv1.JobFailed); failed { if time.Now().After(cond.LastTransitionTime.Time.Add(unpackRetryInterval)) { fresh.SetName(names.SimpleNameGenerator.GenerateName(fresh.GetName())) - job, err = c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) + return c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) } } @@ -724,18 +723,15 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath for _, j := range toDelete { _ = c.client.BatchV1().Jobs(j.GetNamespace()).Delete(context.TODO(), j.GetName(), metav1.DeleteOptions{}) } - return } } if equality.Semantic.DeepDerivative(fresh.GetOwnerReferences(), job.GetOwnerReferences()) && equality.Semantic.DeepDerivative(fresh.Spec, job.Spec) { - return + return job, nil } // TODO: Decide when to fail-out instead of deleting the job - err = c.client.BatchV1().Jobs(job.GetNamespace()).Delete(context.TODO(), job.GetName(), metav1.DeleteOptions{}) - job = nil - return + return nil, c.client.BatchV1().Jobs(job.GetNamespace()).Delete(context.TODO(), job.GetName(), metav1.DeleteOptions{}) } func (c *ConfigMapUnpacker) ensureRole(cmRef *corev1.ObjectReference) (role *rbacv1.Role, err error) { diff --git a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go index ad300b7c88..5042136bc9 100644 --- a/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go +++ b/vendor/github.com/operator-framework/operator-lifecycle-manager/pkg/controller/bundle/bundle_unpacker.go @@ -681,18 +681,18 @@ func (c *ConfigMapUnpacker) ensureConfigmap(csRef *corev1.ObjectReference, name return } -func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath string, secrets []corev1.LocalObjectReference, timeout time.Duration, unpackRetryInterval time.Duration) (job *batchv1.Job, err error) { +func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath string, secrets []corev1.LocalObjectReference, timeout time.Duration, unpackRetryInterval time.Duration) (*batchv1.Job, error) { fresh := c.job(cmRef, bundlePath, secrets, timeout) var jobs, toDelete []*batchv1.Job - jobs, err = c.jobLister.Jobs(fresh.GetNamespace()).List(k8slabels.ValidatedSetSelector{bundleUnpackRefLabel: cmRef.Name}) + jobs, err := c.jobLister.Jobs(fresh.GetNamespace()).List(k8slabels.ValidatedSetSelector{bundleUnpackRefLabel: cmRef.Name}) if err != nil { - return + return nil, err } // This is to ensure that we account for any existing unpack jobs that may be missing the label jobWithoutLabel, err := c.jobLister.Jobs(fresh.GetNamespace()).Get(cmRef.Name) if err != nil && !apierrors.IsNotFound(err) { - return + return nil, err } if jobWithoutLabel != nil { _, labelExists := jobWithoutLabel.Labels[bundleUnpackRefLabel] @@ -702,12 +702,11 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath } if len(jobs) == 0 { - job, err = c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) - return + return c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) } - maxRetainedJobs := 5 // TODO: make this configurable - job, toDelete = sortUnpackJobs(jobs, maxRetainedJobs) // choose latest or on-failed job attempt + maxRetainedJobs := 5 // TODO: make this configurable + job, toDelete := sortUnpackJobs(jobs, maxRetainedJobs) // choose latest or on-failed job attempt // only check for retries if an unpackRetryInterval is specified if unpackRetryInterval > 0 { @@ -716,7 +715,7 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath if cond, failed := getCondition(job, batchv1.JobFailed); failed { if time.Now().After(cond.LastTransitionTime.Time.Add(unpackRetryInterval)) { fresh.SetName(names.SimpleNameGenerator.GenerateName(fresh.GetName())) - job, err = c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) + return c.client.BatchV1().Jobs(fresh.GetNamespace()).Create(context.TODO(), fresh, metav1.CreateOptions{}) } } @@ -724,18 +723,15 @@ func (c *ConfigMapUnpacker) ensureJob(cmRef *corev1.ObjectReference, bundlePath for _, j := range toDelete { _ = c.client.BatchV1().Jobs(j.GetNamespace()).Delete(context.TODO(), j.GetName(), metav1.DeleteOptions{}) } - return } } if equality.Semantic.DeepDerivative(fresh.GetOwnerReferences(), job.GetOwnerReferences()) && equality.Semantic.DeepDerivative(fresh.Spec, job.Spec) { - return + return job, nil } // TODO: Decide when to fail-out instead of deleting the job - err = c.client.BatchV1().Jobs(job.GetNamespace()).Delete(context.TODO(), job.GetName(), metav1.DeleteOptions{}) - job = nil - return + return nil, c.client.BatchV1().Jobs(job.GetNamespace()).Delete(context.TODO(), job.GetName(), metav1.DeleteOptions{}) } func (c *ConfigMapUnpacker) ensureRole(cmRef *corev1.ObjectReference) (role *rbacv1.Role, err error) { From 75dd5ea600586ae398e7e1a9522712c85ad05223 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Sat, 6 Sep 2025 11:38:06 +0000 Subject: [PATCH 2/3] Make PSA catalog tests more resilient (#3645) Signed-off-by: Per Goncalves da Silva Co-authored-by: Per Goncalves da Silva Upstream-repository: operator-lifecycle-manager Upstream-commit: b9e6cce9f3f08aaffa305f3b3add0e71a34cee20 --- .../test/e2e/catalog_e2e_test.go | 46 ++++++------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/staging/operator-lifecycle-manager/test/e2e/catalog_e2e_test.go b/staging/operator-lifecycle-manager/test/e2e/catalog_e2e_test.go index 39fa0497d7..847e807f50 100644 --- a/staging/operator-lifecycle-manager/test/e2e/catalog_e2e_test.go +++ b/staging/operator-lifecycle-manager/test/e2e/catalog_e2e_test.go @@ -1467,29 +1467,19 @@ var _ = Describe("Starting CatalogSource e2e tests", Label("CatalogSource"), fun }) }) }) - When("The namespace is labled as Pod Security Admission policy enforce:restricted", func() { + When("The namespace is labeled as Pod Security Admission policy enforce:restricted", func() { BeforeEach(func() { - var err error - testNS := &corev1.Namespace{} Eventually(func() error { - testNS, err = c.KubernetesInterface().CoreV1().Namespaces().Get(context.TODO(), generatedNamespace.GetName(), metav1.GetOptions{}) + testNS, err := c.KubernetesInterface().CoreV1().Namespaces().Get(context.TODO(), generatedNamespace.GetName(), metav1.GetOptions{}) if err != nil { return err } - return nil - }).Should(BeNil()) - - testNS.ObjectMeta.Labels = map[string]string{ - "pod-security.kubernetes.io/enforce": "restricted", - "pod-security.kubernetes.io/enforce-version": "latest", - } - - Eventually(func() error { - _, err := c.KubernetesInterface().CoreV1().Namespaces().Update(context.TODO(), testNS, metav1.UpdateOptions{}) - if err != nil { - return err + testNS.ObjectMeta.Labels = map[string]string{ + "pod-security.kubernetes.io/enforce": "restricted", + "pod-security.kubernetes.io/enforce-version": "latest", } - return nil + _, err = c.KubernetesInterface().CoreV1().Namespaces().Update(context.TODO(), testNS, metav1.UpdateOptions{}) + return err }).Should(BeNil()) }) When("A CatalogSource built with opm v1.21.0 ( Date: Sat, 6 Sep 2025 10:05:32 +0200 Subject: [PATCH 3/3] UPSTREAM: : make downstream csv namespace labeler plugin e2e more resilient to race conditions Signed-off-by: Per Goncalves da Silva --- ...stream_csv_namespace_labeler_plugin_test.go | 18 ++++++++++++------ .../test/e2e/util/e2e_determined_client.go | 5 +++++ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/staging/operator-lifecycle-manager/test/e2e/downstream_csv_namespace_labeler_plugin_test.go b/staging/operator-lifecycle-manager/test/e2e/downstream_csv_namespace_labeler_plugin_test.go index c9ee8733ea..08700c5974 100644 --- a/staging/operator-lifecycle-manager/test/e2e/downstream_csv_namespace_labeler_plugin_test.go +++ b/staging/operator-lifecycle-manager/test/e2e/downstream_csv_namespace_labeler_plugin_test.go @@ -2,7 +2,6 @@ package e2e import ( "context" - "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -88,11 +87,18 @@ var _ = Describe("CSV Namespace Labeler Plugin", func() { }).Should(HaveKeyWithValue(plugins.NamespaceLabelSyncerLabelKey, "true")) // delete label - ns := &v1.Namespace{} - Expect(determinedE2eClient.Get(context.Background(), k8scontrollerclient.ObjectKeyFromObject(&testNamespace), ns)).To(Succeed()) - nsCopy := ns.DeepCopy() - delete(nsCopy.Annotations, plugins.NamespaceLabelSyncerLabelKey) - Expect(determinedE2eClient.Update(context.Background(), nsCopy)).To(Succeed()) + // NOTE: not using the determined client here because it shouldn't be used for update operations due to + // race conditions (the updated resource could change b/w 'get' and 'update' operations + c := ctx.Ctx().E2EClient() + Eventually(func() error { + ns := &v1.Namespace{} + if err := c.Get(context.Background(), k8scontrollerclient.ObjectKeyFromObject(&testNamespace), ns); err != nil { + return err + } + nsCopy := ns.DeepCopy() + delete(nsCopy.Annotations, plugins.NamespaceLabelSyncerLabelKey) + return c.Update(context.Background(), nsCopy) + }).Should(BeNil()) // namespace should be labeled Eventually(func() (map[string]string, error) { diff --git a/staging/operator-lifecycle-manager/test/e2e/util/e2e_determined_client.go b/staging/operator-lifecycle-manager/test/e2e/util/e2e_determined_client.go index f93df8cb95..03313ec817 100644 --- a/staging/operator-lifecycle-manager/test/e2e/util/e2e_determined_client.go +++ b/staging/operator-lifecycle-manager/test/e2e/util/e2e_determined_client.go @@ -28,6 +28,11 @@ func (m *DeterminedE2EClient) Create(context context.Context, obj k8scontrollerc return nil } +// Update retries update operation until success or timeout +// +// Deprecation: do not use this method as it's not resilient to the case where the resource has changed out of band +// it will conflict until it times out. +// There's no priority to fix this client implementation - please use regular client instead func (m *DeterminedE2EClient) Update(context context.Context, obj k8scontrollerclient.Object, options ...k8scontrollerclient.UpdateOption) error { m.keepTrying(func() error { return m.E2EKubeClient.Update(context, obj, options...)