Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
config/v1/types_cluster_version: Add capabilties properties
Roughly as described in [enhancement]. After discussion with Ben and
David, we've made the following changes:
# spec
## capabilities
The [enhancement] didn't have an opinion on whether or not this should
be a pointer. I've gone with pointer, because I'm fine allowing folks
to leave this unset. The docs for this pointer property point out
that there's no distinction between nil (Go, or for JSON, null) and an
empty object (&ClusterVersionCapabilitiesSpec{} in Go, {} in JSON), so
we don't have to rehash all the ClusterVersionCapabilitiesSpec
children defaults here, where they'd likely go stale as folks update
defaults within ClusterVersionCapabilitiesSpec or add new child
properties.
### baselineCapabilitySet
David prefered this name to the [enhancement]'s inclusionDefault, and
Ben and I are fine with that name.
David also prefered None to Exclude and vCurrent to Include, with
additional values like v4.11 for "give me the 4.11 stuff, but not new
4.12 stuff, until I have time to look that over after I update to
4.12". That seems overly complicated to me, and also like we coulld
add v4.11 later if folks felt None and vCurrent weren't convenient
enough, but David wanted v4.11 out of the gate. We can always see how
it plays out in production, and we can stop adding new v4.y forms if
they aren't popular enough to be worth maintaining. There's an enum
type to make it easy to validate, and hard to typo, these values.
There's also a map, so consumers like the cluster-version operator who
vendor the API repository can get the API-maintainer-intended
capability membership for each set, now that those semantics are more
complicated than all or nothing.
There were a few ways we could have taken the property default here:
a. Explicit +kubebuilder:default:=... . No option for folks to sit on
the fence, or to adjust existing clusters later without the admin's
explicit buy-in. But no ambiguity about what happens if the user
has no opinion.
b. omitempty, and docs for "we'll pick a sane default if you don't
care", that don't commit to a specific default. Great for when we
decide to change our default preference, because we don't need to
hunt down buy-in from admins that have deferred to us. Not great
for folks who are mildly curious about our current choice, but who
still trust us to evolve the default (I think this set is nearly
empty).
c. omitempty, and docs for "the current default is A, but who knows
tomorrow". Effectively (b), but also gives folks some information
that's not actionable which can go stale as soon as they close
their eyes.
(a) makes sense if we are confident we will never want to change our
default, and that seems plausible in this case. (b) makes sense when
we think we might change our default, and I'm fine with that in this
case too. I don't really understand the use case for (c), but as
David points out, even if we do change the default, we don't expect to
change it often, so maybe my personal take is off and there are a
bunch of folks who are mildly curious about our current choice, but
who still trust us to evolve the default. Anyhow, David's the
approver, so we're going with (c).
### additionalEnabledCapabilities
David prefered this name to the [enhancement]'s inclusionDefault, and
Ben and I are fine with that name.
In the [enhancement], Ben had intended to distribute the ability to
create new capabilities to all manifest-providing repositories, simply
by declaring the capability.openshift.io/name annotation. David was
worried about validation, and also possibly about insufficiently
scoped names between separate teams, so this pull request declares a
central enum where API maintainers can review and approve new
capability names, and work them into the appropriate entries in the
set maps. The installer and cluster-version operator will have to
bump their vendored API version after each addition to pick up the new
changes, but new capability additions shouldn't be too frequent to
make that particularly painful.
### exclude
The [enhancement] also provided a way to drop specific capabilities
from the selected set (inclusionDefault or baselineCapabilitySet).
Ben still feels like that will be popular, but David is skeptical, and
because we can always add a property in this space later without
breaking backwards compatibility, we're leaving it off for now.
# status
## capabilities
The [enhancement] didn't have an opinion on whether or not this should
be a pointer. I've gone with non-pointer, because we will always
declare at least some capabilities (e.g. knownCapabilities), so users
will be able to discover additional capabilities which they might want
to enable in their cluster.
### enabledCapabilities
David prefered this name to the [enhancement]'s include, and Ben's ok
with that name. I'm not wild about 'Capabilities' in:
status:
capabilities:
enabledCapabilities: ...
but David was committed, citing the example of:
FeatureGateSelection.FeatureSet
Although FeatureGateSelection is consumed with less context:
type FeatureGateSpec struct {
FeatureGateSelection `json:",inline"`
}
I'd pushed back against the stuttering in [stutterPrecedent], but
without success, and :shrug:, doesn't matter all that much if folks
have to type a few redundant characters to use this property.
### knownCapabilities
The [enhancement] had floated 'exclude'. There are a few capability
sets in play for the status listings:
* A is the set of all caps.
* I is the set of included caps.
* E is the set of excluded caps.
* Each cap must be either included or excluded, so I and E are disjoint, and the union of I and E is A.
So you can take any two of those three sets and construct the one
you're missing:
* A = I ∪ E
* E = A - I
* I = A - E
If we have to pick two to include in status, picking I and E gives us
all the data we need, and saves a few bytes by excluding the largest,
which is A. But David preferred picking I (as enabledCapabilities)
and A (as knownCapabilities) [knownCapabilities], so that's what we're
doing in this commit.
### inclusionDefault
The [enhancement] also provided a way to echo the spec set in an
inclusionDefault status property. I've left that out of the status
structure, because I'm using explicit, exhaustive list for
enabledCapabilities and knownCapabilities there. The exhaustive lists
will provide a convenient set (via A - I set subtraction) of "things
you don't have right now, but which you could choose to install right
now", so admins don't have to guess about their options there. With
the exhaustive lists, reflecting the default setting didn't seem to
add much useful information. And we can always add that property to
the status structure later if we do decide it would be useful.
## conditions
I have not created a constant with the status.conditions[] type that
will be used to declare "we are installing a capability you have not
asked for, because we don't support uninstalling capabilities, and
that one was tainted in via your cluster's history". We can come back
and declare that constant later if we want, although that's somewhat
complicated by the fact that we use ClusterOperatorStatusCondition,
and the new condition type would not be something that makes sense for
ClusterOperator.
[enhancement]: https://github.com/openshift/enhancements/blob/88cb7438f3ac0a8121e3cef87cb144097ece8cda/enhancements/installer/component-selection.md
[knownCapabilities]: openshift#1106 (comment)
[validation]: https://book.kubebuilder.io/reference/generating-crd.html#validation
[statusPropertyNames]: openshift#1106 (comment)
[stutterPrecedent]: openshift#1106 (comment)- Loading branch information
