Mirror order conditions as False, Unknown, True #204
Merged
Conversation
This file contains 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
gibizer
added a commit
to gibizer/nova-operator
that referenced
this pull request
Mar 6, 2023
The lib-common PR openstack-k8s-operators/lib-common#204 fixed the behavior of Conditions.Mirror(). So this patch checks if the fix needs any adaptation from nova-operator.
|
/hold |
gibizer
added a commit
to gibizer/nova-operator
that referenced
this pull request
Mar 6, 2023
The lib-common PR openstack-k8s-operators/lib-common#204 fixed the behavior of Conditions.Mirror(). So it now returns any Unknown condition before any True condition (except ReadyCondition True). It seems that our tests incorrectly assumed that NovaAPI becomes Ready after the StatefulSet is Ready. But in reality NovaAPI creates the KeystoneEndpoint *after* the StatefulSet is ready. So these tests passed only because of the original bug in the Mirror code returned a True condition from NovaAPI when it had DeploymentReady=True and KeystoneEndpointReady=Unknown status. As the bug is fixed now the test needs to make sure the KeystoneEndpoints are ready before it can expect that NovaAPI is ready.
|
/unhold |
5 tasks
stuggi
reviewed
Mar 6, 2023
bogdando
reviewed
Mar 6, 2023
bogdando
reviewed
Mar 6, 2023
This is a breaking change on Conditions.Mirror(). Originally this call ordered conditions as False, True, Unknown then picked the first of that list to reflect. However all the use cases (we see) assume that if there is reasourceA that depends on the status of resourceB then resourceA reconciler calls: resourceB.Status.Mirror(ResourceBSpecificSubConditionOfA) and expects that the call returns the most sever non True condition if any or return a True condition otherwise. So the original behavior that returned True even if there was Unknown condition was unexpected. This patch changes the order of the status to False, Unknown, True. But keeps the original logic that if the ReadyCondition is True then the rest of the conditions are ignored even if there are False conditions in the list.
gibizer
force-pushed
the
conditions-ordering
branch
from
9ba5716 to
3c60f35
Compare
gibizer
added a commit
to gibizer/nova-operator
that referenced
this pull request
Mar 7, 2023
The lib-common PR openstack-k8s-operators/lib-common#204 fixed the behavior of Conditions.Mirror(). So it now returns any Unknown condition before any True condition (except ReadyCondition True). It seems that our tests incorrectly assumed that NovaAPI becomes Ready after the StatefulSet is Ready. But in reality NovaAPI creates the KeystoneEndpoint *after* the StatefulSet is ready. So these tests passed only because of the original bug in the Mirror code returned a True condition from NovaAPI when it had DeploymentReady=True and KeystoneEndpointReady=Unknown status. As the bug is fixed now the test needs to make sure the KeystoneEndpoints are ready before it can expect that NovaAPI is ready.
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.
This is a breaking change on Conditions.Mirror(). Originally this call ordered conditions as False, True, Unknown then picked the first of that list to reflect. However all the use cases (we see) assume that if there is reasourceA that depends on the status of resourceB then resourceA reconciler calls:
and expects that the call returns the most sever non True condition if any or return a True condition otherwise. So the original behavior that returned True even if there was Unknown condition was unexpected.
This patch changes the order of the status to False, Unknown, True. But keeps the original logic that if the ReadyCondition is True then the rest of the conditions are ignored even if there are False conditions in the list.