Add section about discovery to compatibility version KEP #13
Conversation
jpbetz
force-pushed
the
discovery-compat-version
branch
from
1b70967 to
68ae673
Compare
jpbetz
force-pushed
the
discovery-compat-version
branch
from
68ae673 to
3435ba2
Compare
| Also note that we that show information about unavailable features in discovery | ||
| today. We introduce fields into APIs for disabled-by-default features and make | ||
| no attempt to hide those fields in discovery. | ||
|
|
There was a problem hiding this comment.
This makes a lot of sense. The only extra thing I think is worth mentioning here that I'd be concerned about is what happens when an old version of API server deserializes an object at rest with unrecognized fields. I am not sure about our handling of that situation. I think if we can expect N+3 skip level upgrades in the future it may be reasonable to design for N-3 skip level downgrades to rollback such an update.
EDIT: I wrote this comment in the context of general API/field enablement, and not specifically related to OpenAPI/discovery
There was a problem hiding this comment.
skip level downgrades a whole can of worms. I think round-trip-ability is the rub. Today we require all Beta and GA fields exist in the API for the N-1 version so that if we do a downgrade, we can still round trip data without data loss.
Taking the same approach for N-3 would imply that a feature can't be promoted to GA until it's been in alpha for 3 releases.. I don't want to ask the community to do that.
There was a problem hiding this comment.
Taking the same approach for N-3 would imply that a feature can't be promoted to GA until it's been in alpha for 3 releases.. I don't want to ask the community to do that.
Hmm maybe you can correct my understanding, elsewhere this KEP it says an N+3 upgrade is actually three N+1 upgrades using the N+3 binary.
- Migrate to N+1 (using N+3 binary)
- Migrate to N+2 (using N+3 binary)
- Migrate to N+3 (using N+3 binary)
Wouldnt a downgrade be the same in reverse? Meaning we still would only need the N-1 release to know about the fields? If thats that case I think my original comment was asking if it's possible for the the N+3 binary to know which fields to drop when using --compatibility=N+1 (if N+1 introduced the fields and N+2 was the first to use them) so that the final binary swap to N doesn't have unknown fields in storage.
There was a problem hiding this comment.
A good example might be the sidecars KEP. A new field is introduced, and if that field is ever dropped, the pod becomes entirely invalid and won't do what it was suppose to do. If you want to rollback a cluster using that feature, it's fundamentally unsafe to roll it back to a cluster version that is not aware of that field, because round tripping will drop the field and break the sidecar pods.
There was a problem hiding this comment.
Ah that makes sense, I think we had a great discussion here and offline about rollbacks that may benefit the KEP if summarized and put somewhere
| @@ -507,6 +508,27 @@ compatibility version can be set to. | |||
|
|
|||
| Alpha APIs may not be enabled in conjunction with compatibility version. | |||
|
|
|||
| ### Discovery | |||
|
|
|||
| Discovery will [enable](https://github.com/kubernetes/kubernetes/blob/7080b51ee92f67623757534f3462d8ae862ef6fe/staging/src/k8s.io/apiserver/pkg/util/openapi/enablement.go#L32) the group versions matching the compatibility version. | |||
There was a problem hiding this comment.
Something possibly tangential that affects my interpretation of this statement: is APIDiscovery intended to return a different response depending on which apiserver you talk to? In the context of other features like StorageVersionProxy would this change?
There was a problem hiding this comment.
This is being discussed right now on the UVIP KEP: kubernetes#4441 (review). I think UVIP is a good forum for this one.
| API fields that were introduced after the compatibility version will **not** be | ||
| pruned. There is a tradoff here: |
There was a problem hiding this comment.
AFAIK discovery doesn't expose any api fields. Is this referring to the OpenAPI schemas?
There was a problem hiding this comment.
Yes, apologies if my terminology is off.. I think of discovery including both api discovery and openapi schemas.. Happy to take suggestions on the correct use of terms.
There was a problem hiding this comment.
Gotcha, since they are separate documents with different sources of truth and usages I think it would makes sense to separate them in the KEP.
Discovery refers to document hosted at /api[/version] and /apis[/version] it just lists GVRs, their subresources, names, categories and respective GVKs.
OpenAPI contains the description of all REST endpoints at lower level of abstraction .e.g. this endopint responds to GET, and returns 200 status with a response using X schema with Y fields.
IMO my gut instinct is that it should be fine to leave inert fields exposed in the OpenAPI schema, unless they are listed as "required". Though I think in general we do not make a habit out of adding new required fields. It would be nice if there was a way to mark the field as ignored.
There was a problem hiding this comment.
not make a habit out of adding new required fields. It would be nice if there was a way to mark the field as ignored
Yep, we can design with the expectation that all of https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api_changes.md is respected. In particular these rules:
- Any API call (e.g. a structure POSTed to a REST endpoint) that succeeded before your change must succeed after your change.
- Any API call that does not use your change must behave the same as it did before your change.
There was a problem hiding this comment.
Offline it sounded like we have the ability to enumerate these fields that are exposed-but-ignored due to their features being disabled by the compatibiltiy version flag. Not saying we should, but if we wanted to, would we be able to mark such fields in the schema in an x-extension?
There was a problem hiding this comment.
I like the idea. It would be non-trivial since we don't track feature gated fields in a declarative way. Maybe handle as a separate enhancement?
@alexzielenski @siyuanfoundation does this look OK to you?