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
1 change: 1 addition & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ linters:
- third_party$
- builtin$
- examples$
- tests/olmv1
issues:
max-issues-per-linter: 0
max-same-issues: 0
Expand Down
41 changes: 40 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ vet: check-go ## Run go vet against code.
.PHONY: test
test: check-go vet envtest ## Run unit tests; run Go linters checks; check if api and bundle folders are up to date; and check if go dependencies are valid
@make versions
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -mod=mod $(shell go list -mod=mod ./... | grep -v /tests/e2e) -coverprofile cover.out
KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(LOCALBIN) -p path)" go test -mod=mod $(shell go list -mod=mod ./... | grep -v /tests/e2e | grep -v /tests/olmv1) -coverprofile cover.out
@make lint
@make api-isupdated
@make bundle-isupdated
Expand Down 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 thread
weshayutin marked this conversation as resolved.

.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
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 thread
weshayutin marked this conversation as resolved.
})

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