Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -1017,6 +1017,45 @@ test-e2e-cleanup: login-required
$(OC_CLI) delete ns mysql-persistent --ignore-not-found=true
rm -rf $(SETTINGS_TMP)

##@ OLMv1 Tests

OLMV1_PACKAGE ?= oadp-operator
OLMV1_NAMESPACE ?= $(OADP_TEST_NAMESPACE)
OLMV1_CHANNEL ?=
OLMV1_VERSION ?=
OLMV1_UPGRADE_VERSION ?=
OLMV1_CATALOG ?= oadp-olmv1-test-catalog
OLMV1_CATALOG_IMAGE ?=
OLMV1_SERVICE_ACCOUNT ?= oadp-olmv1-installer
OLMV1_FAIL_FAST ?= true

OLMV1_GINKGO_FLAGS = --vv \
--no-color=$(OPENSHIFT_CI) \
--label-filter="olmv1" \
--junit-report="$(ARTIFACT_DIR)/junit_olmv1_report.xml" \
--fail-fast=$(OLMV1_FAIL_FAST) \
--timeout=30m

.PHONY: test-olmv1
test-olmv1: login-required install-ginkgo ## Run OLMv1 lifecycle tests (install, verify, upgrade, cleanup) against a cluster with OLMv1 enabled.
ginkgo run -mod=mod $(OLMV1_GINKGO_FLAGS) $(GINKGO_ARGS) tests/olmv1/ -- \
-namespace=$(OLMV1_NAMESPACE) \
-package=$(OLMV1_PACKAGE) \
-channel=$(OLMV1_CHANNEL) \
-version=$(OLMV1_VERSION) \
-upgrade-version=$(OLMV1_UPGRADE_VERSION) \
-catalog=$(OLMV1_CATALOG) \
-catalog-image=$(OLMV1_CATALOG_IMAGE) \
-service-account=$(OLMV1_SERVICE_ACCOUNT) \
-artifact_dir=$(ARTIFACT_DIR)

.PHONY: test-olmv1-cleanup
test-olmv1-cleanup: login-required ## Cleanup resources created by OLMv1 tests.
$(OC_CLI) delete clusterextension oadp-operator --ignore-not-found=true
$(OC_CLI) delete clustercatalog $(OLMV1_CATALOG) --ignore-not-found=true
$(OC_CLI) delete clusterrolebinding $(OLMV1_SERVICE_ACCOUNT)-cluster-admin --ignore-not-found=true
$(OC_CLI) delete sa $(OLMV1_SERVICE_ACCOUNT) -n $(OLMV1_NAMESPACE) --ignore-not-found=true
Comment on lines +1052 to +1057
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Guard test-olmv1-cleanup from deleting non-test ClusterCatalogs.

This target always deletes $(OLMV1_CATALOG), but the suite only creates a ClusterCatalog when OLMV1_CATALOG_IMAGE is set. If someone points OLMV1_CATALOG at an existing/shared catalog, cleanup removes a resource the test did not create. Gate Line 1055 on the same creation condition, or persist a “created catalog” marker and only delete when that marker is present.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 1052 - 1057, The cleanup target test-olmv1-cleanup
unconditionally deletes ClusterCatalog $(OLMV1_CATALOG); guard that deletion so
we only remove a catalog the tests created by checking the same creation
condition or a creation marker. Modify test-olmv1-cleanup to only run the
$(OC_CLI) delete clustercatalog $(OLMV1_CATALOG) line when OLMV1_CATALOG_IMAGE
is set (or when a persisted marker like OLMV1_CATALOG_CREATED file/env var
exists), and update the Catalog creation step (the rule that creates the
catalog) to set that marker (e.g., touch a file or export a flag) so cleanup can
safely detect it before deleting.


.PHONY: update-non-admin-manifests
update-non-admin-manifests: NON_ADMIN_CONTROLLER_IMG?=quay.io/konveyor/oadp-non-admin:latest
update-non-admin-manifests: yq ## Update Non Admin Controller (NAC) manifests shipped with OADP, from NON_ADMIN_CONTROLLER_PATH
Expand Down
4 changes: 2 additions & 2 deletions config/manager/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
images:
- name: controller
newName: quay.io/konveyor/oadp-operator
newTag: latest
newName: ttl.sh/oadp-operator-7e53a850
newTag: 1h
Comment on lines +7 to +8
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Don't commit an expiring ttl.sh image as the default manager image.

ttl.sh/…:1h stops existing after an hour, so any flow that consumes this checked-in kustomization becomes non-reproducible and will break shortly after merge. Keep temporary test images in the test/deploy workflow, not in the repository’s default manifest.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@config/manager/kustomization.yaml` around lines 7 - 8, The checked-in
kustomization currently pins newName/newTag to the ephemeral ttl.sh image
(ttl.sh/oadp-operator-7e53a850:1h); replace this with a stable, non-expiring
default (e.g., the official oadp-operator image and a permanent tag or digest)
or remove the ttl.sh newName/newTag entries so the repository manifest does not
reference an expiring image; ensure any temporary ttl.sh usage is moved into
CI/deploy workflows that inject the test image dynamically rather than
committing it to kustomization.yaml.

1 change: 1 addition & 0 deletions tests/olmv1/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
tmp/
264 changes: 264 additions & 0 deletions tests/olmv1/olmv1_install_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,264 @@
package olmv1_test

import (
"context"
"fmt"
"log"
"time"

"github.com/onsi/ginkgo/v2"
"github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
)

const (
clusterExtensionName = "oadp-operator"

oadpCRDName = "dataprotectionapplications.oadp.openshift.io"
veleroCRDName = "backups.velero.io"
restoreCRDName = "restores.velero.io"

managerLabelSelector = "control-plane=controller-manager"
)

var _ = ginkgo.Describe("OADP OLMv1 lifecycle", ginkgo.Ordered, ginkgo.Label("olmv1"), func() {
ctx := context.Background()

ginkgo.BeforeAll(func() {
ginkgo.By("Cleaning up orphaned OADP/Velero CRDs from previous installs")
cleanupOrphanedCRDs(ctx)

ginkgo.By("Setting up namespace, ServiceAccount, and RBAC")
ensureNamespace(ctx, namespace)
ensureServiceAccount(ctx, serviceAccountName, namespace)
ensureClusterAdminBinding(ctx, serviceAccountName, namespace)

if catalogImage != "" {
ginkgo.By(fmt.Sprintf("Creating ClusterCatalog %s from image %s", catalogName, catalogImage))
ensureClusterCatalog(ctx, catalogName, catalogImage)
waitForClusterCatalogServing(ctx, catalogName)
}
})

ginkgo.AfterAll(func() {
ginkgo.By("Cleaning up OLMv1 test resources")
err := deleteClusterExtension(ctx, clusterExtensionName)
if err != nil {
log.Printf("Warning: failed to delete ClusterExtension: %v", err)
}

gomega.Eventually(func() bool {
_, err := getClusterExtension(ctx, clusterExtensionName)
return apierrors.IsNotFound(err)
}, 3*time.Minute, 5*time.Second).Should(gomega.BeTrue(), "ClusterExtension should be deleted")

if createdCatalog {
ginkgo.By(fmt.Sprintf("Deleting ClusterCatalog %s", catalogName))
deleteClusterCatalog(ctx, catalogName)
}

cleanupClusterRoleBinding(ctx, serviceAccountName)
})

ginkgo.It("should install OADP operator via ClusterExtension", func() {
ginkgo.By("Creating the ClusterExtension")
ce := buildClusterExtension(clusterExtensionName, packageName, namespace, serviceAccountName)
_, err := dynamicClient.Resource(clusterExtensionGVR).Create(ctx, ce, metav1.CreateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
log.Printf("Created ClusterExtension %s (package=%s, namespace=%s)", clusterExtensionName, packageName, namespace)

ginkgo.By("Waiting for ClusterExtension to be installed")
terminalReasons := map[string]bool{
"InvalidConfiguration": true,
"Failed": true,
}
gomega.Eventually(func(g gomega.Gomega) {
obj, err := getClusterExtension(ctx, clusterExtensionName)
g.Expect(err).NotTo(gomega.HaveOccurred(), "ClusterExtension should exist")

logAllConditions(obj)

progCond, progFound := getCondition(obj, "Progressing")
if progFound {
reason, _ := progCond["reason"].(string)
message, _ := progCond["message"].(string)
g.Expect(terminalReasons[reason]).NotTo(gomega.BeTrue(),
"ClusterExtension has terminal error on Progressing: reason=%s message=%s", reason, message)
}

instCond, instFound := getCondition(obj, "Installed")
g.Expect(instFound).To(gomega.BeTrue(), "Installed condition should be present")
status, _ := instCond["status"].(string)
g.Expect(status).To(gomega.Equal("True"), "Installed condition should be True")
}, 10*time.Minute, 10*time.Second).Should(gomega.Succeed())

ginkgo.By("Checking installed bundle info")
obj, err := getClusterExtension(ctx, clusterExtensionName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
bundleName, bundleVersion, found := getInstalledBundle(obj)
gomega.Expect(found).To(gomega.BeTrue(), "installed bundle should be present in status")
log.Printf("Installed bundle: name=%s version=%s", bundleName, bundleVersion)
})

ginkgo.It("should have the OADP controller-manager pod running", func() {
ginkgo.By("Waiting for controller-manager pod to be Running")
gomega.Eventually(func() (bool, error) {
pods, err := kubeClient.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
LabelSelector: managerLabelSelector,
})
if err != nil {
return false, err
}
for _, pod := range pods.Items {
if pod.Status.Phase == corev1.PodRunning {
log.Printf("Controller-manager pod %s is Running", pod.Name)
return true, nil
}
log.Printf("Controller-manager pod %s phase: %s", pod.Name, pod.Status.Phase)
}
return false, nil
}, 5*time.Minute, 10*time.Second).Should(gomega.BeTrue(), "controller-manager pod should be Running")
Comment on lines +107 to +122
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Check readiness, not only PodRunning, for controller-manager validation.

A pod can stay in Running while its containers are unready or crash-looping, so both checks can false-pass even though the operator is not usable. Assert PodReady or the Deployment’s Available condition instead.

Also applies to: 213-226

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/olmv1/olmv1_install_test.go` around lines 107 - 122, The current
gomega.Eventually loop only checks pod.Status.Phase == corev1.PodRunning which
can false-pass; update the check in the Eventually closure (the block using
kubeClient.CoreV1().Pods(...).List and iterating pods.Items) to verify readiness
instead: for each pod, inspect pod.Status.Conditions for condition.Type ==
corev1.PodReady with Status == corev1.ConditionTrue (or alternatively fetch the
Deployment via kubeClient.AppsV1().Deployments(namespace).Get and assert the
Deployment status has an Available condition == True / status.AvailableReplicas
> 0); apply the same change to the other occurrence mentioned (lines 213-226) so
tests assert PodReady or Deployment Available rather than just PodRunning.

})

ginkgo.It("should have OADP CRDs installed", func() {
expectedCRDs := []string{
oadpCRDName,
veleroCRDName,
restoreCRDName,
"schedules.velero.io",
"backupstoragelocations.velero.io",
"volumesnapshotlocations.velero.io",
}

for _, crdName := range expectedCRDs {
ginkgo.By(fmt.Sprintf("Checking CRD %s exists", crdName))
exists, err := crdExists(ctx, crdName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
gomega.Expect(exists).To(gomega.BeTrue(), fmt.Sprintf("CRD %s should exist", crdName))
log.Printf("CRD %s exists", crdName)
}
})

ginkgo.It("should not report deprecation warnings", func() {
obj, err := getClusterExtension(ctx, clusterExtensionName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

for _, condType := range []string{"Deprecated", "PackageDeprecated", "ChannelDeprecated", "BundleDeprecated"} {
cond, found := getCondition(obj, condType)
if found {
status, _ := cond["status"].(string)
gomega.Expect(status).To(gomega.Equal("False"),
fmt.Sprintf("%s condition should be False, got %s", condType, status))
}
}
})

ginkgo.When("upgrading the operator", func() {
ginkgo.BeforeAll(func() {
if upgradeVersion == "" {
ginkgo.Skip("No --upgrade-version specified, skipping upgrade tests")
}
})

ginkgo.It("should upgrade the ClusterExtension to the target version", func() {
ginkgo.By(fmt.Sprintf("Patching ClusterExtension version to %s", upgradeVersion))
obj, err := getClusterExtension(ctx, clusterExtensionName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

previousBundleName, previousVersion, _ := getInstalledBundle(obj)
log.Printf("Current installed bundle: name=%s version=%s", previousBundleName, previousVersion)

catalogSpec, _, _ := unstructuredNestedMap(obj.Object, "spec", "source", "catalog")
gomega.Expect(catalogSpec).NotTo(gomega.BeNil())
catalogSpec["version"] = upgradeVersion
catalogSpec["upgradeConstraintPolicy"] = "SelfCertified"
err = unstructuredSetNestedMap(obj.Object, catalogSpec, "spec", "source", "catalog")
gomega.Expect(err).NotTo(gomega.HaveOccurred())

_, err = dynamicClient.Resource(clusterExtensionGVR).Update(ctx, obj, metav1.UpdateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
Comment on lines +167 to +181
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Patch ClusterExtension.spec instead of updating the whole object.

ClusterExtension is controller-managed, so this read/modify/write Update can race status changes and intermittently fail with a 409 conflict. Patch just the version/policy fields, or wrap the update in retry.RetryOnConflict.

Example using a merge patch
-           catalogSpec, _, _ := unstructuredNestedMap(obj.Object, "spec", "source", "catalog")
-           gomega.Expect(catalogSpec).NotTo(gomega.BeNil())
-           catalogSpec["version"] = upgradeVersion
-           catalogSpec["upgradeConstraintPolicy"] = "SelfCertified"
-           err = unstructuredSetNestedMap(obj.Object, catalogSpec, "spec", "source", "catalog")
-           gomega.Expect(err).NotTo(gomega.HaveOccurred())
-
-           _, err = dynamicClient.Resource(clusterExtensionGVR).Update(ctx, obj, metav1.UpdateOptions{})
+           patch := []byte(fmt.Sprintf(
+               `{"spec":{"source":{"catalog":{"version":%q,"upgradeConstraintPolicy":"SelfCertified"}}}}`,
+               upgradeVersion,
+           ))
+           _, err = dynamicClient.Resource(clusterExtensionGVR).Patch(
+               ctx,
+               clusterExtensionName,
+               types.MergePatchType,
+               patch,
+               metav1.PatchOptions{},
+           )
            gomega.Expect(err).NotTo(gomega.HaveOccurred())

Add:

import "k8s.io/apimachinery/pkg/types"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
obj, err := getClusterExtension(ctx, clusterExtensionName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
previousBundleName, previousVersion, _ := getInstalledBundle(obj)
log.Printf("Current installed bundle: name=%s version=%s", previousBundleName, previousVersion)
catalogSpec, _, _ := unstructuredNestedMap(obj.Object, "spec", "source", "catalog")
gomega.Expect(catalogSpec).NotTo(gomega.BeNil())
catalogSpec["version"] = upgradeVersion
catalogSpec["upgradeConstraintPolicy"] = "SelfCertified"
err = unstructuredSetNestedMap(obj.Object, catalogSpec, "spec", "source", "catalog")
gomega.Expect(err).NotTo(gomega.HaveOccurred())
_, err = dynamicClient.Resource(clusterExtensionGVR).Update(ctx, obj, metav1.UpdateOptions{})
gomega.Expect(err).NotTo(gomega.HaveOccurred())
obj, err := getClusterExtension(ctx, clusterExtensionName)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
previousBundleName, previousVersion, _ := getInstalledBundle(obj)
log.Printf("Current installed bundle: name=%s version=%s", previousBundleName, previousVersion)
patch := []byte(fmt.Sprintf(
`{"spec":{"source":{"catalog":{"version":%q,"upgradeConstraintPolicy":"SelfCertified"}}}}`,
upgradeVersion,
))
_, err = dynamicClient.Resource(clusterExtensionGVR).Patch(
ctx,
clusterExtensionName,
types.MergePatchType,
patch,
metav1.PatchOptions{},
)
gomega.Expect(err).NotTo(gomega.HaveOccurred())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/olmv1/olmv1_install_test.go` around lines 167 - 181, The current code
reads the ClusterExtension via getClusterExtension and then calls
dynamicClient.Resource(clusterExtensionGVR).Update after changing catalogSpec
(using unstructuredNestedMap/unstructuredSetNestedMap), which can race
controller status updates and yield 409s; instead patch only the
spec.source.catalog fields (or wrap the update in retry.RetryOnConflict) rather
than updating the whole object: construct a minimal merge patch containing
spec.source.catalog.version and spec.source.catalog.upgradeConstraintPolicy and
call dynamicClient.Resource(clusterExtensionGVR).Patch with types.MergePatchType
(add import "k8s.io/apimachinery/pkg/types"), or if you prefer keep Update, wrap
the read/modify/write in retry.RetryOnConflict to retry on conflicts. Ensure
references to getClusterExtension, unstructuredNestedMap,
unstructuredSetNestedMap, and dynamicClient.Resource(clusterExtensionGVR) are
updated accordingly.

log.Printf("Patched ClusterExtension version to %s", upgradeVersion)

ginkgo.By("Waiting for upgrade to complete")
gomega.Eventually(func() string {
updated, err := getClusterExtension(ctx, clusterExtensionName)
if err != nil {
return ""
}

cond, found := getCondition(updated, "Installed")
if !found {
return ""
}
status, _ := cond["status"].(string)
if status != "True" {
reason, _ := cond["reason"].(string)
message, _ := cond["message"].(string)
log.Printf("Installed condition: status=%s reason=%s message=%s", status, reason, message)
return ""
}

_, bundleVer, found := getInstalledBundle(updated)
if !found {
return ""
}
log.Printf("Installed bundle version: %s", bundleVer)
return bundleVer
}, 10*time.Minute, 10*time.Second).ShouldNot(gomega.Equal(previousVersion),
"Installed bundle version should change after upgrade")

ginkgo.By("Verifying controller-manager pod is running after upgrade")
gomega.Eventually(func() (bool, error) {
pods, err := kubeClient.CoreV1().Pods(namespace).List(ctx, metav1.ListOptions{
LabelSelector: managerLabelSelector,
})
if err != nil {
return false, err
}
for _, pod := range pods.Items {
if pod.Status.Phase == corev1.PodRunning {
return true, nil
}
}
return false, nil
}, 5*time.Minute, 10*time.Second).Should(gomega.BeTrue())
})
})
})

func unstructuredNestedMap(obj map[string]interface{}, fields ...string) (map[string]interface{}, bool, error) {
var current interface{} = obj
for _, field := range fields {
m, ok := current.(map[string]interface{})
if !ok {
return nil, false, fmt.Errorf("expected map at field %s", field)
}
current, ok = m[field]
if !ok {
return nil, false, nil
}
}
result, ok := current.(map[string]interface{})
if !ok {
return nil, false, fmt.Errorf("final value is not a map")
}
return result, true, nil
}

func unstructuredSetNestedMap(obj map[string]interface{}, value map[string]interface{}, fields ...string) error {
if len(fields) == 0 {
return fmt.Errorf("no fields specified")
}
current := obj
for _, field := range fields[:len(fields)-1] {
next, ok := current[field].(map[string]interface{})
if !ok {
return fmt.Errorf("expected map at field %s", field)
}
current = next
}
current[fields[len(fields)-1]] = value
return nil
}
Loading