OCPBUGS-18454: Match restrictions on ConditionalUpdateRisk name with Condition reason #1577
Conversation
Conditional risk names are used as a `Reason` field values when the risk applies and CVO sets up the `Recommended` condition. Hence, the risk names need to be valid Reason field values.
openshift-ci-robot
added
jira/severity-moderate
|
@petr-muller: This pull request references Jira Issue OCPBUGS-18454, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Hello @petr-muller! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/S
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petr-muller The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@petr-muller: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
| // +kubebuilder:validation:Required | ||
| // +kubebuilder:validation:MaxLength=1024 | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:Pattern=`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$` |
There was a problem hiding this comment.
I'm conflicted about this. If we accept that the update service can serve risks with non-compliant names, then we can:
a. Land this regexp. Drop the risk (silently for now, eventually triggering CannotEvaluateConditionalUpdates to warn the admin). You'd have to do this for all clusters, not just clusters where the risk applied.
b. Land this regexp. Squash any non-compliant names into compliant names before writing the risk.
c. Drop this API change. Accept the risk with the non-compliant name, and squash it into something compliant when we translate to the Recommended reason (if we end up wanting to involve the risk's name because we assess it as the only valid risk).
I'm personally leaning towards (c), because fewer clusters are impacted (only clusters where the non-compliant risk is the only risk assessed to apply), and because I like complaining in status instead of relying on alerts alone. But this pull request's current (a) is more aligned with our existing status enum precedent, where the Kube API server doesn't trust the CVO to write solid data. And (b) hides the "hey, we wish the update service was using a better name for this risk" condition while (c) makes that more obvious just from status.
I'm not terribly committed though, so I'm ok with what you have here if my cost/benefit breakdown isn't convincing ;)
There was a problem hiding this comment.
I agree, I currently like (c) the best too. I filed openshift/cluster-version-operator#962 that follows that path (including some refactors so that the change is easier to test). That PR just operates in the same area like openshift/cluster-version-operator#964 which is more important to merge and I will need to rebase #962 on top of it.
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@petr-muller: This pull request references Jira Issue OCPBUGS-18454, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/close We'll probably don't want to do this and only merge openshift/cluster-version-operator#962. Closing for now, I can always reopen. |
|
@petr-muller: This pull request references Jira Issue OCPBUGS-18454. The bug has been updated to no longer refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@petr-muller: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
3 participants
Conditional risk names are used as a
Reasonfield values when the riskapplies and CVO sets up the
Recommendedcondition. Hence, the risknames need to be valid Reason field values.
xref: openshift/cincinnati-graph-data#4059