WIP: 🌱 Remove BMO in the preCleanupManagementCluster function. - #3535
WIP: 🌱 Remove BMO in the preCleanupManagementCluster function.#3535UncivilizedGhost wants to merge 1 commit into
Conversation
metal3-io-bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test ? |
metal3-io-bot
added
the
size/XS
|
/test metal3-e2e-clusterctl-upgrade-test-main |
3823ba3 to
11e41c4
Compare
metal3-io-bot
added
size/S
|
/test metal3-e2e-clusterctl-upgrade-test-main |
|
/copilot-review |
|
Copilot code review has been requested by @UncivilizedGhost. Please allow a few moments for the review to be added. |
There was a problem hiding this comment.
Pull request overview
This PR updates CAPM3’s E2E test helpers/hooks to reduce noisy log-collection errors during clusterctl-upgrade scenarios by avoiding long-lived deployment log watchers and explicitly removing BMO before the management cluster is torn down.
Changes:
- Added a
WatchLogstoggle toInstallIRSOso callers can disable deployment log watching. - Disabled deployment log watching for IRSO and BMO installs in
clusterctl-upgradetests. - Added an explicit BMO Deployment deletion step in
preCleanupManagementClusterto reduce log-collector errors when the management cluster is being deleted.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
test/e2e/common.go |
Adds InstallIRSOInput.WatchLogs and wires it into BuildAndApplyKustomization to control deployment log watching. |
test/e2e/upgrade_clusterctl_test.go |
Disables IRSO/BMO deployment log watching during upgrade tests and removes the BMO Deployment during management-cluster pre-cleanup. |
test/e2e/pivoting.go |
Updates IRSO installs to explicitly set WatchLogs: true after the new input field was introduced. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
11e41c4 to
71e6a3f
Compare
metal3-io-bot
added
size/M
|
/test metal3-e2e-clusterctl-upgrade-test-main |
71e6a3f to
8fd71ce
Compare
|
/test metal3-e2e-clusterctl-upgrade-test-main |
8fd71ce to
8131a11
Compare
|
/test metal3-e2e-clusterctl-upgrade-test-main |
8131a11 to
96df11e
Compare
|
/test metal3-e2e-clusterctl-upgrade-test-main |
96df11e to
2b65bc0
Compare
|
Trying to remove BMO doesn't work since watchers still exist. I'm trying to manually suppress the errors to see if that can do something. /test metal3-e2e-clusterctl-upgrade-test-main |
2b65bc0 to
6b2f173
Compare
metal3-io-bot
added
size/L
|
/test metal3-e2e-clusterctl-upgrade-test-main |
6b2f173 to
c7f33c9
Compare
|
/test metal3-e2e-clusterctl-upgrade-test-main |
c7f33c9 to
dc4c9ad
Compare
Prevents error spam in the console log when the log collectors are trying to get logs from the non-existing cluster in clusterctl-upgrade tests Signed-off-by: Saad Zia <saad.zia@est.tech>
dc4c9ad to
4cd92b9
Compare
|
/test metal3-e2e-clusterctl-upgrade-test-main |
|
\copilot-review |
|
/copilot-review |
|
Copilot code review has been requested by @UncivilizedGhost. Please allow a few moments for the review to be added. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (3)
test/e2e/pivoting.go:330
- Foreground propagation controls garbage-collection order, but the DELETE request still returns as soon as deletion is accepted; it does not wait for the Deployment or its Pods to disappear. Consequently the new cleanup callers can continue to management-cluster teardown while BMO is still running, so this does not guarantee the behavior needed to prevent the log spam. After issuing the delete, poll until the Deployment is
NotFound(using a configured cleanup interval) before returning.
// Use foreground propagation so the deployment's pods are deleted as part of
// the delete instead of being orphaned and cleaned up asynchronously.
foregroundDeletion := metav1.DeletePropagationForeground
err := input.ClusterProxy.GetClientSet().AppsV1().
Deployments(input.Namespace).
Delete(ctx, input.Name, metav1.DeleteOptions{
PropagationPolicy: &foregroundDeletion,
})
test/e2e/upgrade_clusterctl_test.go:53
- This unrelated replacement removes the v1.13/BMO 0.13/Ironic 35 → current upgrade scenario, while the following spec already exercises v1.12 → main. The PR and issue only describe teardown log spam, so this silently drops the newer release-to-current coverage. Restore the v1.13 scenario, or move and explain this test-matrix change separately.
// Ironic 33.0 -> 35.0.
var _ = Describe("When testing cluster upgrade from releases (v1.12=>current)", Label("clusterctl-upgrade"), func() {
minorVersion := "1.12"
bmoFromRelease := "0.12"
ironicFromRelease := "33.0"
bmoToRelease := "0.13"
ironicToRelease := "35.0"
scripts/ci-e2e.sh:300
- This filter wraps the entire E2E run, not just teardown, and generic alternatives such as
reflector.go,Unhandled Error, andclient connection lostwill hide genuine API/informer failures during provisioning or test execution. That removes precisely the diagnostics needed to investigate flakes. Preserve the unfiltered output (or remove this wrapper now that the caches are explicitly cancelled) and only filter a derived teardown-specific view if necessary.
E2E_LOG_FILTER='reflector\.go|Failed to watch \*v1\.Pod|failed to list \*v1\.Pod|watch of \*v1\.Pod ended|Unhandled Error|client connection lost|Error starting logs stream for pod'
What this PR does / why we need itPrevents error spam in the console log when the log collectors are trying to get logs from the non-existing cluster in clusterctl-upgrade tests:
Fixes #2460
Checklist: