Aggregate ServiceExport conflict conditions #1624
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
tpantelis
requested review from
aswinsuryan,
Oats87,
skitt,
sridhargaddam and
vthapar
as code owners
|
🤖 Created branch: z_pr1624/tpantelis/conflict_condition |
tpantelis
force-pushed
the
conflict_condition
branch
from
3e4e92b
to
852a01e
Compare
Changed UpdateStatusConditions and RemoveStatusCondition public to facilitate. Signed-off-by: Tom Pantelis <[email protected]>
It's possible there could be more than one concurrent conflict however the MCS API only defines one conflict type so a second conflict would overwrite the first. We could define our own conflict types but the MCS spec does specifically reference the ServiceExportConflict type, defined as "Conflict", in the conflict resolution section. It doesn't mention what the behavior should be if there's more than one conflict. To represent multiple conflicts, We can still use the ServiceExportConflict type but aggregate the differing condition reasons and messages. For the Reason field, join multiple reasons separated by a comma and, for the Message field, join with a new line. When a particular conflict is resolved, remove it's reason and message. When there's no more conflicts, ie Reason is empty, set the condition Status to False. Signed-off-by: Tom Pantelis <[email protected]>
This was used to remove the Conflict condition when the conflict was resolved but we now update the the condition as there can be multiple conflicts. In general, the pattern is to update the condition status rather than removing a condition. Signed-off-by: Tom Pantelis <[email protected]>
tpantelis
force-pushed
the
conflict_condition
branch
from
852a01e
to
b6f1b06
Compare
vthapar
reviewed
Aug 28, 2024
| index := goslices.Index(reasons, *from.Reason) | ||
| if index >= 0 { | ||
| if from.Status == corev1.ConditionTrue { | ||
| if index < len(messages) { |
There was a problem hiding this comment.
When will this be true? Message is already present but not at end? Means this code is updating the message for this reason in case it changed?
There was a problem hiding this comment.
index should always be < len(messages) b/c it's size mirrors reason. This is just a sanity check.
If you're referring to if from.Status == corev1.ConditionTrue, yes, if the reason already exists but the message changed.
dfarrell07
approved these changes
Aug 28, 2024
vthapar
approved these changes
Aug 29, 2024
|
🤖 Closed branches: [z_pr1624/tpantelis/conflict_condition] |
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.
It's possible there could be more than one concurrent conflict however the MCS API only defines one conflict type so a second conflict would overwrite the first. We could define our own conflict types but the MCS spec does specifically reference the
ServiceExportConflicttype, defined as "Conflict", in the conflict resolution section. It doesn't mention what the behavior should be if there's more than one conflict.To represent multiple conflicts, we can still use the
ServiceExportConflicttype but aggregate the differing condition reasons and messages. For theReasonfield, join multiple reasons separated by a comma and, for theMessagefield, join with a new line. When a particular conflict is resolved, remove its reason and message. When there's no more conflicts, ieReasonis empty, set the conditionStatustoFalse.See individual commits for details.