Fix Condition check for Reconcile InstallPlan status #3153
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
approved
grokspawn
reviewed
Jan 11, 2024
test/e2e/subscription_e2e_test.go
Outdated
| // The Reason check: | ||
| // CatalogSources[Added|Deleted] could overwrite AllCatalogSourcesHealthy, but leave Status/Message preserved | ||
| // Added/Deleted could subesequently be overwritten by Healthy | ||
| // This all depends on when reconciliation occurs and if the number of catalog sources changed between reconcilliations |
There was a problem hiding this comment.
Suggested change
| // This all depends on when reconciliation occurs and if the number of catalog sources changed between reconcilliations | |
| // This all depends on when reconciliation occurs and if the number of catalog sources changed between reconciliations |
There was a problem hiding this comment.
Ugh... no spell checker in my code editor!
grokspawn
reviewed
Jan 11, 2024
| if lastCond.Status != cond.Status && lastCond.Reason != cond.Reason && lastCond.Message == cond.Message { | ||
| GinkgoT().Logf("waited %s subscription condition met %v\n", time.Since(lastTime), cond) | ||
| lastTime = time.Now() | ||
| lastCond = cond | ||
| } | ||
| return true | ||
| } | ||
|
|
||
| fmt.Printf("subscription condition not met: %v\n", cond) | ||
| if lastCond.Status != cond.Status && lastCond.Reason != cond.Reason && lastCond.Message == cond.Message { | ||
| GinkgoT().Logf("waited %s subscription condition not met: %v\n", time.Since(lastTime), cond) | ||
| lastTime = time.Now() | ||
| lastCond = cond | ||
| } | ||
| return false |
There was a problem hiding this comment.
I would love if there were a way to write this for greater readability.
Maybe a comment something like
// if status/reason/message meet expectations, then subscription state is considered met/true IFF this is the result of a recent change of status/reason
// else, cache the current status/reason for next loop/comparisonI also have a concern that this makes this test edge-set instead of level-set. I'm not sure how purist we need to be for an e2e test.
There was a problem hiding this comment.
This is the general trend to reduce repetitive logging, and I admit, the comparisons are ugly.
There was a problem hiding this comment.
BUT... that's not the reason for the change... The change reduces the logging when conditions don't change, the reason for getting out of the loop doesn't change, just the output.
test/e2e/subscription_e2e_test.go
Outdated
| require.FailNowf(GinkgoT(), "failed", "subscription contains unexpected installplan condition: %v", cond) | ||
| case operatorsv1alpha1.SubscriptionInstallPlanMissing: | ||
| require.Equal(GinkgoT(), operatorsv1alpha1.ReferencedInstallPlanNotFound, cond.Reason) | ||
| case operatorsv1alpha1.SubscriptionCatalogSourcesUnhealthy: |
There was a problem hiding this comment.
The way this test is written makes it sensitive to any change in the cluster state or the reconciliation flow of the controller. Since we're trying to test a negative ("all of the non-InstallPlan conditions can change between the time we looked at them in the beginning of the test and now"), I don't know that this test is super powerful, and, since that kind of approach is very fragile, I wonder if a different approach exists (maybe just to delete the test? what is it testing?).
Consider the case where a CatalogSource happens to blip unhealthy for a moment during this check. Why should the test fail if the reconciler has correctly exposed that downtime in status?
test/e2e/subscription_e2e_test.go
Outdated
|
|
||
| By(`Ensure original non-InstallPlan status conditions remain after InstallPlan transitions`) | ||
| hashEqual := comparison.NewHashEqualitor() | ||
| By(`Ensure original, specific, non-InstallPlan status conditions remain after InstallPlan transitions`) |
There was a problem hiding this comment.
Suggested change
| By(`Ensure original, specific, non-InstallPlan status conditions remain after InstallPlan transitions`) | |
| By(`Ensure InstallPlan-related status conditions match what we're expecting`) |
Fix operator-framework#3151 Remove non-InstallPlan related checks for this test. Also: * Clean up some looping log messages * Clean up some logging added when comments were converted These comments/logs are at the beginning of the test, and are also part of the test sequence, so they are redundant (and possibly confusing) Signed-off-by: Todd Short <todd.short@me.com>
stevekuznetsov
approved these changes
Jan 12, 2024
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: stevekuznetsov, 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 |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
The test failed in that e2e run @tmshort |
|
That should be including the (new) finalizer code in the merge. AFAICT, it's failing on the delete of the CSV (not on the removal of the CRBs/CRs/SA), as in the original failure. The merge queue was also failing on unit test, so something was wonky. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix #3151
The Reason check for CatalogSourcesUnhealthy does not take into account that CatalogSourcesAdded may overwrite the AllCatalogSourcesHealthy reason because the number of catalog sources changed. So, depending on when reconcilliation happens relative to the catalog sources being added, the initial Reason may be either Added or Healthy.
At the end of the test, also depending on when reconcilliation happens, the new Reason may be Added or Healthy.
New can't be Added after the original is Healthy because no additional catalog sources are added once the test starts.
However, the Status and Message (which is the real indicator of health) must match. And because of the possible update, don't check the timestamps.
Also:
These comments/logs are at the beginning of the test, and are also part of the test sequence, so they are redundant (and possibly confusing)
Description of the change:
Motivation for the change:
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue