🌱 Prevent duplicate log watchers in E2E tests

[lentzi90]

What this PR does / why we need it:
We sometimes use BuildAndApplyKustomization multiple times, e.g. inside
FlakeAttempt. This is an issue for the log watchers, since we create a
new watcher each time. Each watcher writes logs to the files so we end
up with the logs duplicated. To fix this, keep track of which watchers
we have and skip setting up duplicates. Additionally, clean up the
watchers after the test is done, to avoid leaking and spamming.

I think we use FlakeAttempts only during upgrade tests, so don't expect to see a difference in the normal tests.

Part of #2927

Checklist:

• Documentation has been updated, if necessary.
• Unit tests have been added, if necessary.
• E2E tests have been added, if necessary.
• Integration tests have been added, if necessary.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign lentzi90 for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[lentzi90]

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

[lentzi90]

/hold
BMO and Ironic logs are missing here. I must have missed something.

[lentzi90]

Now the logs look correct
/hold cancel

[lentzi90]

I forgot to run upgrade tests...
/hold
/test metal3-bmo-e2e-test-optional-pull

[lentzi90]

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

[lentzi90]

Log duplication fixes are working. There are more duplications still that we cannot fix here though. See #2927 (comment).
The test failure is unrelated. The issue behind that is fixed in #3033.

[lentzi90]

/hold cancel

[lentzi90]

/test metal3-bmo-e2e-test-optional-pull
Still expected to fail often, until #3033 is merged.

[lentzi90]

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

[adilGhaffarDev]

I think this can be make simpler if we just return if WaitForDeploymentsAvailable fails,
and call WatchDeploymentLogsByName only when Deployment is successful.
What do you think?

[lentzi90]

Hmm not sure I understand. I think WaitForDeploymentAvailable already fails the test if it fails, so in that case we do not continue.
The thing I am trying to fix here is when we use FlakeAttempts, here:

baremetal-operator/test/e2e/upgrade_test.go

Line 88 in 45602b7

err := FlakeAttempt(2, func() error {

[adilGhaffarDev]

Does FlakeAttempt always run twice even if first run is successful?
I think it would be better to use Eventually here. In Eventually as soon as there is success it will not try again.

[lentzi90]

No it should only run 2 times if the first fails. Hmm maybe I could remove this actually. I think we added it when IPA downloader was very flaky. It may not be needed anymore.
/hold
Let me check this

[lentzi90]

Looks like we do need FlakeAttempts still. I think it is not really flaky, rather the first fails every time. Maybe we could clean that up in some other way, but for now I think this log watcher fix is ok.
/hold cancel

[lentzi90]

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

[lentzi90]

Ok it looks like we still need the FlakeAttempts. From what I understand we need it at least when applying BMO, because we create namespaces together with resources in the namespaces, so things fail the first time because the namespaces are missing.

[lentzi90]

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

[lentzi90]

/ok-to-test
Comon... run the tests

[lentzi90]

/hold
Discussed with @adilGhaffarDev . We will hold for now so we can get the CAPI fix in first. Then it will be easier to say what duplication is coming from where.