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.

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

✨ Add upgrade e2e tests #299

Closed

Conversation

ankitathomas
Copy link
Contributor

@ankitathomas ankitathomas commented Jul 9, 2024

Description

Testing upgrade from the latest release to the current commit.

Fixes #271

@ankitathomas ankitathomas requested a review from a team as a code owner July 9, 2024 08:56
@ankitathomas ankitathomas changed the title 🌱 Adding upgrade tests ✨ Add upgrade e2e tests Jul 9, 2024
Copy link

codecov bot commented Jul 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 33.53%. Comparing base (13002de) to head (82a9948).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #299   +/-   ##
=======================================
  Coverage   33.53%   33.53%           
=======================================
  Files          15       15           
  Lines         656      656           
=======================================
  Hits          220      220           
  Misses        413      413           
  Partials       23       23           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Install cert-manager
kubectl apply -f "https://github.com/cert-manager/cert-manager/releases/download/v1.13.1/cert-manager.yaml"
kubectl apply -f "https://github.com/cert-manager/cert-manager/releases/download/${CERT_MGR_VERSION}/cert-manager.yaml"
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of this completely. Please take a look: #300

- main

jobs:
upgrade-e2e:
Copy link
Member

Choose a reason for hiding this comment

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

Can we have this job in .github/workflows/e2e.yaml? It will be easier to consistently manage on triggers if we decide to change something.

Copy link
Member

@m1kola m1kola left a comment

Choose a reason for hiding this comment

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

Few questions / suggestions

Makefile Outdated
@@ -106,6 +106,28 @@ verify: tidy fmt vet generate ## Verify the current code generation and lint
lint: $(GOLANGCI_LINT) ## Run golangci linter.
$(GOLANGCI_LINT) run $(GOLANGCI_LINT_ARGS)

.PHONY: test-upgrade-e2e
test-upgrade-e2e: kind-cluster cert-manager build-container kind-load image-registry run-latest-release wait pre-upgrade-setup post-upgrade-checks deploy wait wait-cert ## Run upgrade e2e tests on a local kind cluster
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have post-upgrade-checks here (after pre-upgrade-setup) and below?

Makefile Outdated
Comment on lines 127 to 129
.PHONY: wait-cert
wait-cert:
kubectl wait --for=condition=Ready --namespace=olmv1-system certificate/catalogd-catalogserver-cert # Avoid upgrade test flakes when reissuing cert
Copy link
Member

Choose a reason for hiding this comment

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

Is wait-cert specific for upgrades? Or should it be included here?

catalogd/Makefile

Lines 186 to 187 in 7c0bcb4

wait:
kubectl wait --for=condition=Available --namespace=$(CATALOGD_NAMESPACE) deployment/catalogd-controller-manager --timeout=60s

Comment on lines +8 to +13
# delete any pods persisting from earlier tests
kubectl delete --ignore-not-found --wait -f test/tools/imageregistry/compare-catalog.pod.yaml
kubectl create -f test/tools/imageregistry/compare-catalog.pod.yaml
# wait till pod terminates
kubectl wait -f test/tools/imageregistry/compare-catalog.pod.yaml --for=jsonpath='{.status.containerStatuses[0].state.terminated}'
kubectl logs --all-containers=true -f -n catalogd-e2e catalogd-compare
Copy link
Member

Choose a reason for hiding this comment

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

Should we consider using kubectl port-forward for this? Sounds like it is going to be easier to request the catalog: no need for a temporary pod.

containers:
- image: radial/busyboxplus:curl
imagePullPolicy: IfNotPresent
command: ["sh", "-c", "curl -k https://catalogd-catalogserver.olmv1-system.svc.cluster.local/catalogs/test-catalog/all.json | diff build-contents/expected_all.json -"]
Copy link
Member

Choose a reason for hiding this comment

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

Would be good to get rid of hardcoded test-catalog

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

.PHONY: compare-catalog
compare-catalog:
test/tools/imageregistry/compare-catalog.sh # compare test catalog with expected contents
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could the actual upgrade comparisons be written in a separate Go test suite instead of using a shell script?

I think this would make it easier to communicate with the catalogd HTTP server and be easier to extend the test suite in the future.

Copy link
Member

Choose a reason for hiding this comment

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

@ankitathomas In operator-framework/operator-controller#1003 I started with simple bash, but we ended up extending checks there so I replacing post-upgrade-checks with go test.

You might want to take a similar approach.

I think this would make it easier to communicate with the catalogd HTTP server and be easier to extend the test suite in the future.

We run go test from outside of the cluster. Instead of kubectl port-forward & curl which I suggested earlier we will have to proxy somehow with client-go. We cam probably take some inspiration from kubectl port-forward

@ankitathomas
Copy link
Contributor Author

Closing in favour of #314

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[epic] Implement upgrade tests for Catalogd
3 participants