oc adm release new: add check for the existence of profile annotations in release manifests #663
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: csrwng The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pkg/cli/admin/release/new.go
Outdated
| } | ||
| } | ||
| if !hasProfileAnnotation { | ||
| return fmt.Errorf("%s: manifests must contain at least one profile annotation set", filename) |
There was a problem hiding this comment.
nit: ... must set at least one profile annotation or ... must have at least one profile annotation set or some such?
There was a problem hiding this comment.
updated error messages
|
|
That manifest is indeed in need of a profile declaration. |
csrwng
force-pushed
the
check_profile_annotations
branch
from
79e23bb
to
f698b36
Compare
There's a pr for that :) openshift/cluster-monitoring-operator#998 |
|
/test images |
1 similar comment
|
/test images |
|
@csrwng: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/test images |
|
openshift/cluster-kube-scheduler-operator#319 should fix the CI failure |
|
Before actually landing this, we probably need either a flag to turn the check off, or to wait a minor or two for the existing labels to sink in. If we land this as it stands right now, someone using a master |
|
This would break backwards compatibility between new cli and old releases. Why is absence of annotation not implied to mean "all profiles" in the CVO? |
|
/hold I have some concerns here. CLI tooling currently works on all releases of openshift. This changes that behavior. I agree the use case of verifying that users specify appropriate profiles is important. I would probably focus on adding a parameter which is an allowed set of profiles and if that is set, we reject typo'd profiles. However, you won't be able to add that flag to all invocations of the release tool without changing release controller and ci-operator slightly. I need to see discussion of how you'd roll that flag out there. The remaining question is whether no profile = all or none. I don't find the argument in the enhancement compelling (and certainly doesn't require that to prevent typos). I would probably argue that no profile = all, and we protect against typos with the allowed profiles flag in merge PRs. |
openshift-ci-robot
added
the
do-not-merge/hold
|
The core problem is that the enhancement change is not backwards compatible to the API of release construction, and I don't agree we should break that API. |
|
@smarterclayton are you referring to openshift/enhancements#200 (comment) ? |
|
Yes. However, what problem is THIS change solving? Someone accidentally omitted a profile? |
|
Yes that's the problem it's solving. Happy to condition it on a flag. |
|
Without a plan for how you roll that flag out to the CI infrastructure, the flag won't do much good. If we hadn't broken forwards behavior compatibilty on the API by requiring the profile annotation, you could have hardcoded the list of allowed profiles in oc adm release new (because old release would be the same as the new). |
|
A more effective guard (at least, more effective in being more localized and more concrete) for this would be to have the CVO refuse to start if any manifest contains ANY unspecified profile. That restricts the problem to static up front AND preserves compatibility of client tooling, and would have gated the CVO change from merging (because of the bug in scheduler operator). While I like detecting this in tooling, we should detect this where the object is silently ignored as well. We can then come back and decide whether we change the default behavior in CVO and release both to preserve compatibility of the payload API. |
|
I like that ... @wking ^^ |
|
openshift/cluster-kube-scheduler-operator#319 landed. /retest
But when you forget to label a manifest with a profile, or you typo the profile, or whatever, the result would be that the bootstrap CVO crashloops, install hangs waiting for bootstrap complete, and folks complain to the installer? Unless I can finally talk folks into landing automatic installer analysis of gathered log bundles (openshift/installer#2567) and find some non-brittle way to highlight the CVO issue there? I like guarding against this at release-build time, because it's a lot easier to report quickly to the person who called
Which object is silently ignored? Because neither this
I like the fact that the current "none" interpretation makes it easy to distinguish folks who have thought about it and decided that their manifest belongs in all profiles from folks who have not heard about profiles and may not have considered whether their manifest belongs in all of them. Of course, even the current schema does not record whether folks have considered each manifest vs. some new profile and then rejected that profile for that manifest, so maybe "I have thought about profiles" is too coarse to be worth recording. |
You wouldn't be able to merge your code change to any of the payload repos, because you wouldn't pass e2e-aws |
The current approach buries the signal in the noise. The signal is "this is different because it has the annotation". Right now you have to review everything to find that. I think the only way we can have adm release new validate profiles is to allow "" as "any", including in the CVO, and then validate one in oc adm release new the allowed set of profiles as "no annotation is valid", or "1..N annotations with known profile strings are valid". The release tooling then continues to work with all versions of openshift, and the noise around what is the delta is cleaned up, and we only allow a certain set of profiles at release creation time. |
|
/test images |
|
@csrwng: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
/close |
|
@csrwng: Closed this PR. In 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. |
With the introduction of cluster profiles, only manifests with a cluster profile annotation will be recognized/applied by the cluster version operator.
It should not be possible to create a release with manifests that are not annotated for at least one profile. This PR introduces a check for this.
Related enhancement:
openshift/enhancements#200