[DNM] (review-only) WIP: OLM v1 API review #2067
Conversation
Signed-off-by: everettraven <[email protected]>
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @everettraven! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven 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 |
everettraven
changed the title
Signed-off-by: everettraven <[email protected]>
openshift-ci
bot
added
size/XXL
Signed-off-by: everettraven <[email protected]>
| // When set to "Enabled", the CRD Upgrade Safety pre-flight check will be run when | ||
| // performing an upgrade operation. | ||
| // | ||
| //+kubebuilder:validation:Enum:="Enabled";"Disabled" |
There was a problem hiding this comment.
Enabled/Disabled should be replaced with descriptive levels like
- InformationalOnly
- Strict
- Could later add: CriticalOnly.
There was a problem hiding this comment.
We talked about this for awhile yesterday and got hung up on InformationalOnly because we didn't think a user would actually be able see the information on the API.
We ended up settling on: CRDUpgradeSafetyEnforcement with None and Strict as options, and we could later add Permissive, CriticalOnly, etc. WDYT?
For InformationalOnly is there recommended way to provide that information to a user? Since this is an edge-based check, we couldn't immediately think of a way to put this in the API in a way that it would "stick". We mentioned recording an Event, but those are ephemeral.
For now, we landed on "None", which means "don't even run the checks", and in our controller we will log what the enforcement enum was set to when we reconcile so that we have more information to help reconstruct why a cluster might be borked when a customer reports an issue with a ClusterExtension.
| // When omitted, the image will not be polled for new content. | ||
| // +kubebuilder:validation:Format:=duration | ||
| // +optional | ||
| PollInterval *metav1.Duration `json:"pollInterval,omitempty"` |
There was a problem hiding this comment.
lower bound on the poll interval
There was a problem hiding this comment.
1 minute as the lower bound sounds good as discussed.
There was a problem hiding this comment.
1 minute means we have an e2e that will take ~1 minute to run, instead of the current configuration, which sets a poll interval as 1 second, and the test finishes in a couple of seconds.
That's not a good motivation for the design of an API, but just something for us to consider on the e2e side of things. Maybe we could run this test in parallel with others?
There was a problem hiding this comment.
Testing at a different poll interval than you'd expect real life users to leverage is likely to either expose bugs or mask them, so I'd encourage testing at a real-ish poll interval where possible
Out of interest, what do you expect the "default" or average to be? Every hour?
There was a problem hiding this comment.
The "default" that we set on the ClusterCatalog resources that ship as part of the OCP payload will be 10 minutes, which is the same as what the existing OLM v0 CatalogSource resources that ship as part of the OCP payload.
There was a problem hiding this comment.
There are a lot of places where my comments would/are duplicated, I didn't capture them all as I didn't want to constantly repeat myself.
For the most part I think the structure looks good, though I've left some comments where we might need more thought about structure.
The majority of what I've left is suggestions on the godocs and what I would consider missing validations.
| ) | ||
|
|
||
| // ClusterExtensionSpec defines the desired state of ClusterExtension | ||
| type ClusterExtensionSpec struct { |
There was a problem hiding this comment.
Source and Install could well be interpreted to be quite similar, eg, why is the source not part of the installation configuration, I wonder if changing the names might make this more obvious, or perhaps, source should be within install? Or the fields from install should just be promoted to the spec?
There was a problem hiding this comment.
The loose idea is that there are stages:
- source the bundle from $somewhere (for now, from a catalog. in the future, perhaps from a direct reference or from a helm repo)
- (not implemented yet) template/render the bundle using cluster-admin defined configuration
- install it (i.e. change the cluster to reflect the result of (2))
So the idea was to have logical groupings of fields in the API so that fields from different concepts would not be interspersed.
There was a problem hiding this comment.
Just a note that a couple of the install fields have been promoted to the spec.
There was a problem hiding this comment.
Right, I wonder if the details of that flow can somehow be represented in the API field documentation so that it's easier to understand, but agreed, the logical grouping makes sense
| //+kubebuilder:validation:MaxLength:=64 | ||
| //+kubebuilder:validation:Pattern=`^(\s*(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|[x|X|\*])(\.(0|[1-9]\d*|x|X|\*]))?(\.(0|[1-9]\d*|x|X|\*))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)((?:\s+|,\s*|\s*\|\|\s*)(=||!=|>|<|>=|=>|<=|=<|~|~>|\^)\s*(v?(0|[1-9]\d*|x|X|\*])(\.(0|[1-9]\d*|x|X|\*))?(\.(0|[1-9]\d*|x|X|\*]))?(-([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?(\+([0-9A-Za-z\-]+(\.[0-9A-Za-z\-]+)*))?)\s*)*$` | ||
| //+optional | ||
| Version string `json:"version,omitempty"` |
There was a problem hiding this comment.
Would this be easier to reason about, if you created a structure to capture the version constraints, rather than trying to regex match and explain this massive validation?
For example, just splitting this into a list of strings, and saying that a match of any item in the list is considered a match, would remove the need to account for the OR in this.
Thinking about the ~ and ^ usage, is that not effectively a form of pinning to either the major, minor or patch versions of the version?
Could a field that was an enum represent that choice? upgradePinning: Major | Minor represent something equivalent? (I don't think you need patch)
One of my concerns about the validation here at the moment, is that there are many ways to express the same thing
What if this was something like
version:
- base: 1.2.0 # This would be ">=1.2.0, < 1.3.0" or "1.2.x" or "~1.2.0" in the current form
operator: GreaterOrEqual
pinnedAt: Minor
There was a problem hiding this comment.
Jordan, Lala, Bryce, and I chatted about this for a while yesterday. Our conclusion is that we believe masterminds implements a standard version range syntax used in other well-known parts of the Kubernetes ecosystem (e.g. helm install --version) and handles edge cases well (e.g. pre-release versions are only included in the range if the range itself specifies versions with pre-release values), so we were all agreed that we desired for the implementation to remain.
With that settled, the next questions we discussed were:
- Can we confidently translate from a syntax like the one you are proposing back into masterminds syntax?
- Are we sure that our pre-translation API would be expressive enough to cover all of the possible masterminds constraint strings? If it was, would that API still be more intuitive than a version range string?
For (1), we thought that we probably could, but it would require an extensive test suite that we would need to maintain. For (2), we were not confident.
Lastly, we talked about whether we thought there was a significant enough UX gain by going with a structured syntax, and we didn't think there was. We believe that the GoDoc on the field, the alignment with a well-known library, and the simplicity of a single string might actually be overall more intuitive than a verbose structure would be.
There was a problem hiding this comment.
Ok, that's fair, as this is already relatively well used, I can see the argument for keeping it.
I would encourage though, to try breaking down the pattern validation into many CEL rules that not only would make it easier to maintain (eg you can split on comma/|| and the process the constraints for the strings within) but would also allow more specific error messages potentially
There was a problem hiding this comment.
@grokspawn Had mentioned running into some issues with breaking this down into individual CEL rules. I don't recall the exact issues off the top of my head.
| // # OR Comparisons | ||
| // You can use the "||" character to represent an OR operation in the version | ||
| // range. Some examples: | ||
| // - ">=1.2.3, <2.0.0 || >3.0.0" |
There was a problem hiding this comment.
If I wanted to exclude a specific version from being valid, do I then just have to create ORs on a range around the version I want to exclude?
There was a problem hiding this comment.
You could do something like this:
version: "2.x, !=2.1.2"Signed-off-by: everettraven <[email protected]>
|
here is example output for describing the ClusterExtension CRD |
Signed-off-by: everettraven <[email protected]>
Signed-off-by: everettraven <[email protected]>
Signed-off-by: everettraven <[email protected]>
|
Hey @JoelSpeed , @deads2k just pinging you here to let you know that @everettraven updated this PR with our latest set of changes, and to reflect our understanding vis-a-vis resolved comments. Please take another look when you have a chance. I'll ping elsewhere as well, but covering all my bases. Thanks! |
The Operator Framework group has been working on a re-vamp of the Operator Lifecycle Manager project that we have dubbed "OLM v1". Our upstream project has adopted the OpenShift API conventions and since we intend for it to go GA in OpenShift 4.18 we wanted to get the APIs reviewed before GA.
This PR adds a new
olmdirectory containing the Go types for the two APIs that would be introduced:ClusterExtensionClusterCatalogEach API is under a separate sub-directory in this PR because while they are fairly coupled, the APIs live in separate upstream projects:
ClusterExtensionAPI lives in the upstream project https://github.com/operator-framework/operator-controller (which has a downstream equivalent at https://github.com/openshift/operator-framework-operator-controller) and as such I named the subdirectoryoperator-controllerClusterCatalogAPI lives in the upstream project https://github.com/operator-framework/catalogd (which has a downstream equivalent at https://github.com/openshift/operator-framework-catalogd) and as such I named the subdirectorycatalogdAlso of note for reviewers, there is a portion of theClusterCatalogAPI that is still a work-in-progress, but was included in this PR with what we anticipate it to look like. I will leave PR comments on the exact locations with references to the design document we have in place for the API change and functionality intended.