Fix e2e Install Plan creation with permission #3129
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: tmshort 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 |
openshift-ci
bot
added
the
approved
|
/hold Even if this happens to ameliorate the symptom, not adding a finalizer means that the system remains capable of producing incorrect output. I don't suggest that. |
openshift-ci
bot
added
the
do-not-merge/hold
I consider the Finalizer/Deletion issue separate from the Install Plan test flake. I'm not ignoring that! I'm not actually 100% sure it's a finalizer issue, as the CSV is not actually removed when deleted; the delete succeeds, but the CSV remains. I created #3130 to track the Finalizer/Deletion issue. This gets us past the test issue so we can continue to contribute code. |
|
The only mechanism that exists in Kubernetes to ensure that some action is taken before an object is fully deleted is to use a finalizer. The controller attempts to ensure that a cleanup action is taken, but it cannot do that without using a finalizer. No finalizer is added to the CSV object. I don't think there is room for uncertainty about whether or not "it's a finalizer issue". |
|
Seeing some weird behavior here. |
|
The problem with the test is that the CSV ends up being re-created by the Subscription (I've discovered that the UID on the CSVs are different). According to @awgreene, this test worked previously because of the poor performance. With improved performance, the CSV is being recreated in time for the CR/CRBs to remain (or possibly be re-created, I did not check those UIDs). The Subscription should be deleted before the CSV, to ensure the CSV remains deleted. This still does not obviate the potential need for a finalizer, however. |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
test/e2e/installplan_e2e_test.go
Outdated
| GinkgoT().Logf("deleting csv %s/%s", generatedNamespace.GetName(), stableCSVName) | ||
| By("Explicitly delete the CSV") | ||
| By("Waiting on CSV to succeed before deleting") | ||
| _, err = fetchCSV(crc, generatedNamespace.GetName(), stableCSVName, csvSucceededChecker) |
There was a problem hiding this comment.
Are we going to remove this step?
Fix operator-framework#3108 The Subscription needs to be deleted before deleting the CSV, otherwise the Subscription will recreate the CSV, and subsequently, the CR/CRBs are not deleted. Update some test logging as well. Signed-off-by: Todd Short <[email protected]>
8215c7f
Fix #3108
The Subscription needs to be deleted before deleting the CSV, otherwise the Subscription will recreate the CSV, and subsequently, the CR/CRBs are not deleted.
Update some test logging as well.
Signed-off-by: Todd Short [email protected]
Description of the change:
Motivation for the change:
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue