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 ironic and bmo cleanup for upgrade BMO E2E tests #1553

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

mquhuy
Copy link
Member

@mquhuy mquhuy commented Feb 13, 2024

This PR adds Ironic and BMO to the cleanup phase in upgrade tests. This is needed as a preparation for multiple upgrade tests (multiple versions of BMO upgrade + ironic upgrade), as the test has to start from a clean state (with no Ironic nor BMO).
We also move installation of Ironic and BMO to the It{}, as it's more related to the test itself, rather than a preparation.

Notice that this change has allowed the test to be ran multiple times using the same cluster, as followed:

export GINKGO_FOCUS=upgrade
hack/ci-e2e.sh # first time
export UPGRADE_DEPLOY_CERT_MANAGER=false
<extra exports normally handled by ci-e2e.sh>
make test-e2e # As many times as needed

We could apply this to the required e2e tests as well if running multiple times is a requirement. That's not the main focus of the PR though.

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 784e6e0 to 0d4f90a Compare February 13, 2024 09:28
@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 13, 2024
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 0d4f90a to 319fdfc Compare February 13, 2024 09:30
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/retest

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch 2 times, most recently from 21f2174 to f898fd4 Compare February 13, 2024 13:50
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/retest

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch 2 times, most recently from 473382a to 044dac6 Compare February 13, 2024 13:54
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 044dac6 to 2b5f9d0 Compare February 13, 2024 14:05
@mquhuy
Copy link
Member Author

mquhuy commented Feb 13, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
test/e2e/upgrade_test.go Outdated Show resolved Hide resolved
@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 2b5f9d0 to 464540a Compare February 14, 2024 15:40
@mquhuy mquhuy requested a review from lentzi90 February 14, 2024 15:40
@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 464540a to 38d26fd Compare February 14, 2024 15:43
@mquhuy
Copy link
Member Author

mquhuy commented Feb 14, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull


// BuildAndRemoveKustomize builds the provided kustomization to resources and removes them from the cluster
// provided by clusterProxy.
func BuildAndRemoveKustomize(ctx context.Context, kustomization string, clusterProxy framework.ClusterProxy) error {
Copy link
Member

Choose a reason for hiding this comment

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

nit

Suggested change
func BuildAndRemoveKustomize(ctx context.Context, kustomization string, clusterProxy framework.ClusterProxy) error {
func BuildAndRemoveKustomization(ctx context.Context, kustomization string, clusterProxy framework.ClusterProxy) error {

@@ -247,6 +254,10 @@ var _ = Describe("BMO Upgrade", func() {
WaitIntervals: e2eConfig.GetIntervals("default", "wait-deployment"),
})
Expect(err).NotTo(HaveOccurred())
DeferCleanup(func() {
By(fmt.Sprintf("Removing BMO from %s on the upgrade cluster", bmoKustomization))
BuildAndRemoveKustomize(ctx, bmoKustomization, upgradeClusterProxy)
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit confusing and dangerous. The variable bmoKustomization is used both for the old and upgraded BMO kustomization so it is easy to make a mistake when reasoning about it. Could you change it so we have separate variables?

Also, it seems strange to delete the old kustomization after the upgrade. At that point it should be the upgraded BMO we delete. If there is an issue and the test fails early, we should anyway stop. I don't think there is much point trying to clean up at that point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the point here is to clean up so the next test can start, so no matter at what stage the test stops, the cluster will be restored to its original state. If the upgrade was done, then we would delete the upgraded deployment, but even in case, for some reason, it was skipped, then we still cleanup the upgrade-from deployment. Of course, it would mean we may do a bit extra in our cleanup, but imo that's acceptable, and shouldn't cause any harm (similarly, we always clean up both vbmc and sushy-tools containers, even though only one of them should exist at one point).

In the latest patch I renamed the param bmoKustomization in one place as you suggested. I also changed all reference of kustomize in function names to kustomization, for consistency.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks the kustomization functions and variables look much better now! I don't think the defer makes sense in tests though. We should use the AfterEach instead, that is what it is for and where we clean up everything else from a test. This way we also make sure that we can collect relevant logs before we remove the containers.

If you really want to do the double deletion of both the old and upgraded I can accept that, but please note that it clutters the logs with a lot of errors and the test should anyway not continue if there was an error. If the upgrade from 0.4 fails for example then we stop and collect all data we can. No more tests should run at that point so cleaning up or not does not matter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you! My intention is to add ironic Upgrade into the same suite, so imo full cleanup, even doubling ones, is necessary.

I don't understand, however, why DeferCleanup is not suitable? It's listed as a way to clean up in Ginkgo docs, and compared to AfterEach, I think it is safer in this case: we only need to make sure to put one DeferCleanup after each installation, and we're fine. Otherwise, to put these cleanups into AfterEach, we would have to keep track of what was installed and what was not (for e.g. ironic is there, but it was preinstalled so shouldn't be cleaned, etc.)

I don't think missing logs is an issue either. DeferCleanup should be triggered only after the It block has ended. In our case it means the trigger phase will be similar to as if we put that cleanup in AfterEach. (In fact, it runs AFTER AfterEach, so it should be safe even if we want to put any extra log collection in AfterEach)

Copy link
Member

Choose a reason for hiding this comment

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

Ah I learned something new. Thanks for that! I was not aware of how this DeferCleanup worked. This looks good to me now :)

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 38d26fd to 79027f1 Compare February 19, 2024 12:22
@mquhuy
Copy link
Member Author

mquhuy commented Feb 19, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

@mquhuy mquhuy force-pushed the mquhuy/add-e2e-upgrade-cleanup branch from 79027f1 to 0309d9d Compare February 19, 2024 12:25
@mquhuy
Copy link
Member Author

mquhuy commented Feb 19, 2024

/test metal3-bmo-e2e-test-pull
/test metal3-bmo-e2e-test-optional-pull

@mquhuy mquhuy requested a review from lentzi90 February 19, 2024 12:27
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 20, 2024
@mquhuy
Copy link
Member Author

mquhuy commented Feb 20, 2024

/cc @kashifest

Copy link
Member

@kashifest kashifest left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kashifest

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 20, 2024
@mquhuy
Copy link
Member Author

mquhuy commented Feb 20, 2024

/test-centos-e2e-integration-main

@metal3-io-bot metal3-io-bot merged commit 3cd8e38 into metal3-io:main Feb 20, 2024
20 checks passed
@metal3-io-bot metal3-io-bot deleted the mquhuy/add-e2e-upgrade-cleanup branch February 20, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants