Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

🌱 fix TestClusterExtensionInstallReResolvesWhenNewCataloge2e test #1008

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 fix TestClusterExtensionInstallReResolvesWhenNewCataloge2e test #1008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ E2E_REGISTRY_NAMESPACE := operator-controller-e2e

export REG_PKG_NAME := registry-operator
export REGISTRY_ROOT := $(E2E_REGISTRY_NAME).$(E2E_REGISTRY_NAMESPACE).svc:5000
export CATALOG_IMG := $(REGISTRY_ROOT)/test-catalog:e2e
export CATALOG_IMG := $(REGISTRY_ROOT)/test-catalog:v1
export UPDATED_CATALOG_IMG := $(REGISTRY_ROOT)/test-catalog:v2
.PHONY: test-ext-dev-e2e
test-ext-dev-e2e: $(OPERATOR_SDK) $(KUSTOMIZE) $(KIND) #HELP Run extension create, upgrade and delete tests.
test/extension-developer-e2e/setup.sh $(OPERATOR_SDK) $(CONTAINER_RUNTIME) $(KUSTOMIZE) $(KIND) $(KIND_CLUSTER_NAME) $(E2E_REGISTRY_NAMESPACE)
Expand All @@ -142,6 +143,7 @@ image-registry: ## Setup in-cluster image registry

build-push-e2e-catalog: ## Build the testdata catalog used for e2e tests and push it to the image registry
./test/tools/build-push-e2e-catalog.sh $(E2E_REGISTRY_NAMESPACE) $(CATALOG_IMG)
./test/tools/build-push-e2e-catalog.sh $(E2E_REGISTRY_NAMESPACE) $(UPDATED_CATALOG_IMG)

# When running the e2e suite, you can set the ARTIFACT_PATH variable to the absolute path
# of the directory for the operator-controller e2e tests to store the artifacts, which
Expand Down
34 changes: 14 additions & 20 deletions test/e2e/cluster_extension_install_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func TestClusterExtensionInstallRegistry(t *testing.T) {
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "resolved to")
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle)
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle)
}, pollDuration, pollInterval)

t.Log("By eventually reporting a successful unpacked")
Expand Down Expand Up @@ -133,36 +133,27 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) {
PackageName: pkgName,
InstallNamespace: "default",
}

t.Log("By deleting the catalog first")
require.NoError(t, c.Delete(context.Background(), extensionCatalog))
require.EventuallyWithT(t, func(ct *assert.CollectT) {
err := c.Get(context.Background(), types.NamespacedName{Name: extensionCatalog.Name}, &catalogd.ClusterCatalog{})
assert.True(ct, errors.IsNotFound(err))
}, pollDuration, pollInterval)

t.Log("It resolves the specified package with correct bundle path")
t.Log("By creating the ClusterExtension resource")
require.NoError(t, c.Create(context.Background(), clusterExtension))

// TODO: this isn't a good precondition because a missing package results in
// exponential backoff retries. So we can't be sure that the re-reconcile is a result of
// the catalog becoming available because it could also be a retry of the initial failed
// resolution.
t.Log("By failing to find ClusterExtension during resolution")
t.Log("By reporting a successful resolution and bundle path")
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
assert.Len(ct, clusterExtension.Status.Conditions, len(conditionsets.ConditionTypes))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
if !assert.NotNil(ct, cond) {
return
}
assert.Equal(ct, metav1.ConditionFalse, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonResolutionFailed, cond.Reason)
assert.Contains(ct, cond.Message, fmt.Sprintf("no package %q found", pkgName))
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "resolved to")
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.1.2.0", Version: "1.2.0"}, clusterExtension.Status.ResolvedBundle)
}, pollDuration, pollInterval)

t.Log("By creating an ClusterExtension catalog with the desired package")
t.Log("By creating an ClusterExtension catalog with the updated ClusterCatalog")
var err error
extensionCatalog, err = createTestCatalog(context.Background(), testCatalogName, os.Getenv(testCatalogRefEnvVar))
extensionCatalog, err = patchTestCatalog(context.Background(), testCatalogName, os.Getenv(testUpdatedCatalogRefEnvVar))
require.NoError(t, err)
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: extensionCatalog.Name}, extensionCatalog))
Expand All @@ -174,15 +165,18 @@ func TestClusterExtensionInstallReResolvesWhenNewCatalog(t *testing.T) {
assert.Equal(ct, catalogd.ReasonUnpackSuccessful, cond.Reason)
}, pollDuration, pollInterval)

t.Log("By eventually resolving the package successfully")
t.Log("By eventually reporting a successful resolution and bundle path")
require.EventuallyWithT(t, func(ct *assert.CollectT) {
assert.NoError(ct, c.Get(context.Background(), types.NamespacedName{Name: clusterExtension.Name}, clusterExtension))
assert.Len(ct, clusterExtension.Status.Conditions, len(conditionsets.ConditionTypes))
cond := apimeta.FindStatusCondition(clusterExtension.Status.Conditions, ocv1alpha1.TypeResolved)
if !assert.NotNil(ct, cond) {
return
}
assert.Equal(ct, metav1.ConditionTrue, cond.Status)
assert.Equal(ct, ocv1alpha1.ReasonSuccess, cond.Reason)
assert.Contains(ct, cond.Message, "resolved to")
assert.Equal(ct, &ocv1alpha1.BundleMetadata{Name: "prometheus-operator.2.0.0", Version: "2.0.0"}, clusterExtension.Status.ResolvedBundle)
}, pollDuration, pollInterval)
}

Expand Down
25 changes: 23 additions & 2 deletions test/e2e/e2e_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@ var (
)

const (
testCatalogRefEnvVar = "CATALOG_IMG"
testCatalogName = "test-catalog"
testCatalogRefEnvVar = "CATALOG_IMG"
testUpdatedCatalogRefEnvVar = "UPDATED_CATALOG_IMG"
testCatalogName = "test-catalog"
)

func TestMain(m *testing.M) {
Expand Down Expand Up @@ -58,3 +59,23 @@ func createTestCatalog(ctx context.Context, name string, imageRef string) (*cata
err := c.Create(ctx, catalog)
return catalog, err
}

func patchTestCatalog(ctx context.Context, name string, newImageRef string) (*catalogd.ClusterCatalog, error) {
// Fetch the existing ClusterCatalog
catalog := &catalogd.ClusterCatalog{}
err := c.Get(ctx, client.ObjectKey{Name: name}, catalog)
if err != nil {
return nil, err
}

// Update the ImageRef
catalog.Spec.Source.Image.Ref = newImageRef

// Patch the ClusterCatalog
err = c.Update(ctx, catalog)
if err != nil {
return nil, err
}

return catalog, err
}
Comment on lines +63 to +81
Copy link
Member

@joelanford joelanford Jul 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is a valid test case (where we patch the spec of a ClusterCatalog), but it does not cover the case I had in mind (and a case I think we need to cover).

What I have in mind here specifically is that we need to prove that the polling behavior alone is enough to trigger an upgrade of a ClusterExtension. Forcing a change by changing the image in the ClusterCatalog spec is a side-effect foils the true intention of the test.

This "polling triggers an upgrade" test is one of the most important features of OLM, so we unfortunately can't "fake" it by touching the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was my first approach as well. Unfortunately, I can't find a way to change the latest image tag on a local registry. crane does not allow to change tags on local registries, hence I took this approach. Let me know if you you think this is still a valid test or we can approach this issue with a different solution. @joelanford

Copy link
Contributor

Choose a reason for hiding this comment

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

This "polling triggers an upgrade" test is one of the most important features of OLM, so we unfortunately can't "fake" it my touching the spec.

How is this accomplished in the current version of OLM? The use of make/sh/kinako(spelling), to get things into the cluster, the go to run tests, IMO blurs what is/isn't being tested, there aren't clear separation of duties in this model.

Correct me if I'm wrong, but I think you want an image loaded into the running cluster after the test has executed? Is that correct? Or do you just want this test to run longer to see if the polling picks up the the image already in the registry?

Copy link
Member

Choose a reason for hiding this comment

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

How is this accomplished in the current version of OLM?

I'm not sure

The use of make/sh/kinako(spelling), to get things into the cluster, the go to run tests, IMO blurs what is/isn't being tested, there aren't clear separation of duties in this model.

I agree that the number of moving parts is a problem. I'm fairly confident that if we wanted something a bit leaner, we could build all of this in Go.

  1. Code in go that runs during an e2e that deploys an image registry in the cluster.
  2. Client-go code that runs kubectl port-forward to give the e2e test a way to communicate with the registry.
  3. go-containerregistry code to push images, move tags, etc.

Correct me if I'm wrong, but I think you want an image loaded into the running cluster after the test has executed? Is that correct?

No, I don't think that is sufficient because the polling behavior of catalogd queries a remote registry. kind load would not make it visible to the polling process. What I'm after is $something that causes the digest of a tag to change mid way through the test. That could be one of:

  1. Pre-populating the registry with two catalog images as part of the setup, and then during the test re-tagging the second image over the first.
  2. Pushing the first image at repo:tag as part of the setup, and then pushing the second image at that same repo:tag during the test.

29 changes: 15 additions & 14 deletions test/tools/build-push-e2e-catalog.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,45 +21,46 @@ if [[ "$#" -ne 2 ]]; then
fi

namespace=$1
tag=$2
image=$2
tag=${image##*:}

echo "${namespace}" "${tag}"
echo "${namespace}" "${image}" "${tag}"

kubectl create configmap -n "${namespace}" --from-file=testdata/catalogs/test-catalog.Dockerfile operator-controller-e2e.dockerfile
kubectl create configmap -n "${namespace}" --from-file=testdata/catalogs/test-catalog operator-controller-e2e.build-contents
kubectl create configmap -n "${namespace}" --from-file=testdata/catalogs/test-catalog-${tag}.Dockerfile operator-controller-e2e-${tag}.dockerfile
kubectl create configmap -n "${namespace}" --from-file=testdata/catalogs/test-catalog-${tag} operator-controller-e2e-${tag}.build-contents

kubectl apply -f - << EOF
apiVersion: batch/v1
kind: Job
metadata:
name: kaniko
name: "kaniko-${tag}"
namespace: "${namespace}"
spec:
template:
spec:
containers:
- name: kaniko
- name: kaniko-${tag}
image: gcr.io/kaniko-project/executor:latest
args: ["--dockerfile=/workspace/test-catalog.Dockerfile",
args: ["--dockerfile=/workspace/test-catalog-${tag}.Dockerfile",
"--context=/workspace/",
"--destination=${tag}",
"--destination=${image}",
"--skip-tls-verify"]
volumeMounts:
- name: dockerfile
mountPath: /workspace/
- name: build-contents
mountPath: /workspace/test-catalog/
mountPath: /workspace/test-catalog-${tag}/
restartPolicy: Never
volumes:
- name: dockerfile
configMap:
name: operator-controller-e2e.dockerfile
name: operator-controller-e2e-${tag}.dockerfile
items:
- key: test-catalog.Dockerfile
path: test-catalog.Dockerfile
- key: test-catalog-${tag}.Dockerfile
path: test-catalog-${tag}.Dockerfile
- name: build-contents
configMap:
name: operator-controller-e2e.build-contents
name: operator-controller-e2e-${tag}.build-contents
EOF

kubectl wait --for=condition=Complete -n "${namespace}" jobs/kaniko --timeout=60s
kubectl wait --for=condition=Complete -n "${namespace}" jobs/kaniko-${tag} --timeout=60s
2 changes: 1 addition & 1 deletion testdata/catalogs/test-catalog.Dockerfile → testdata/catalogs/test-catalog-v1.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
FROM scratch
ADD test-catalog /configs
ADD test-catalog-v1 /configs

# Set DC-specific label for the location of the DC root directory
# in the image
Expand Down
0 testdata/catalogs/test-catalog/.indexignore → ...ata/catalogs/test-catalog-v1/.indexignore
View file
Open in desktop
File renamed without changes.
12 changes: 0 additions & 12 deletions testdata/catalogs/test-catalog/catalog.yaml → ...ata/catalogs/test-catalog-v1/catalog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ entries:
replaces: prometheus-operator.1.0.0
- name: prometheus-operator.1.2.0
replaces: prometheus-operator.1.0.1
- name: prometheus-operator.2.0.0
replaces: prometheus-operator.1.2.0
---
schema: olm.bundle
name: prometheus-operator.1.0.0
Expand Down Expand Up @@ -50,13 +48,3 @@ properties:
value:
packageName: prometheus
version: 1.2.0
---
schema: olm.bundle
name: prometheus-operator.2.0.0
package: prometheus
image: docker-registry.operator-controller-e2e.svc.cluster.local:5000/bundles/registry-v1/prometheus-operator:v2.0.0
properties:
- type: olm.package
value:
packageName: prometheus
version: 2.0.0
6 changes: 6 additions & 0 deletions testdata/catalogs/test-catalog-v2.Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
FROM scratch
ADD test-catalog-v2 /configs

# Set DC-specific label for the location of the DC root directory
# in the image
LABEL operators.operatorframework.io.index.configs.v1=/configs
2 changes: 2 additions & 0 deletions testdata/catalogs/test-catalog-v2/.indexignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/expected_all.json
..*
31 changes: 31 additions & 0 deletions testdata/catalogs/test-catalog-v2/catalog.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
---
schema: olm.package
name: prometheus
defaultChannel: beta
---
schema: olm.channel
name: beta
package: prometheus
entries:
- name: prometheus-operator.2.0.0
replaces: prometheus-operator.1.2.0
---
schema: olm.bundle
name: prometheus-operator.1.2.0
package: prometheus
image: docker-registry.operator-controller-e2e.svc.cluster.local:5000/bundles/registry-v1/prometheus-operator:v1.2.0
properties:
- type: olm.package
value:
packageName: prometheus
version: 1.2.0
---
schema: olm.bundle
name: prometheus-operator.2.0.0
package: prometheus
image: docker-registry.operator-controller-e2e.svc.cluster.local:5000/bundles/registry-v1/prometheus-operator:v2.0.0
properties:
- type: olm.package
value:
packageName: prometheus
version: 2.0.0
Loading