-
Notifications
You must be signed in to change notification settings - Fork 5.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: TPR to beta proposal #524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k can you outline how the on-the-wire API changes in order to accommodate these suggested changes? e.g. http://127.0.0.1:8080/apis/etcd.coreos.com/v1beta1/namespaces/default/clusters/example-etcd-cluster
becomes ...?
1. handle TPR-registration value-space conflicts | ||
1. [stop using the extensions API group](https://github.com/kubernetes/kubernetes/issues/43214) | ||
|
||
We can create a type `ThirdPartyResource.apiextension.k8s.io`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: kubernetes.io?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API groups are k8s.io
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gah, right. consistency FTW.
ListKind string `json:"listKind,omitempty" protobuf:"bytes,7,opt,name=listKind"` | ||
|
||
// ClusterScoped indicates that this resource is cluster scoped as opposed to namespace scoped | ||
ClusterScoped bool `json:"clusterScoped" protobuf:"bytes,8,opt,name=clusterScoped"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest namespaced
to match discovery doc field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be Scope = {Cluster, Namespaced} enum
1. handle TPR-registration value-space conflicts | ||
1. [stop using the extensions API group](https://github.com/kubernetes/kubernetes/issues/43214) | ||
|
||
We can create a type `ThirdPartyResource.apiextension.k8s.io`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API groups are k8s.io
That format will not change. |
versions. A TPR does not have the ability to convert between versions, which | ||
focuses on the primary role of TPR as an easily extensible and simple mechanism | ||
for adding new APIs. Conversion primarily allows structural, but not backwards | ||
incompatible, changes. By not supporting conversion, all TPR use cases are |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: I thought the version conversions was a good thing, but you mention this is a bad thing and should be avoided because it does not preserve behavior, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: I thought the version conversions was a good thing, but you mention this is a bad thing and should be avoided because it does not preserve behavior, right?
Conversions can allow a change of expression over time (rename fieldA to fieldB for example), but they can't change behavior (do something different with fieldA). Someone who owns a TPR could change expression over time client-side by being tolerant for a couple releases and rewriting the objects periodically over that time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks
|
||
// Items individual ThirdParties | ||
Items []ThirdPartyResource `json:"items" protobuf:"bytes,2,rep,name=items"` | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
specs LGTM
GroupVersionResource information to match it to a ThirdPartyResource. The ThirdPartyResource | ||
will be checked to make sure its valid enough in .Status to serve and will | ||
response appropriated. If there is no ThirdPartyResource defined, it will delegate | ||
to the next handler in the chain. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In issue kubernetes/enhancements#95 it mentions that it could take up to 10 seconds to see a new TPR. How will Create help reduce that? Maybe not one of the goals for this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In issue kubernetes/enhancements#95 it mentions that it could take up to 10 seconds to see a new TPR. How will Create help reduce that? Maybe not one of the goals for this change?
That's more of an implementation detail, but given a spec/status split, it's possible write a series of small, "normal", controllers that are easy to reason about and write a different kind of RESTHandler (re-using our impl code) to drive it all from a shared cache that would end up more consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool
// Singular is the singular name of the resource. Defaults to lowercased <kind> | ||
Singular string `json:"singular,omitempty" protobuf:"bytes,4,opt,name=singular"` | ||
// ShortNames are short names for the resource. | ||
ShortNames []string `json:"shortNames,omitempty" protobuf:"bytes,5,opt,name=shortNames"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the plumbing exist yet to actually make this work? i.e. kubectl knows about them, and there's a central registry to prevent conflicts between short names from other sources besides TPR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the plumbing exist yet to actually make this work?
Yes, kubectl
respects it. Since shortNames are group scoped, there's no need for a central registry (each group owns their own) and given status
TPRs do the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right I forgot that long names are allowed to overlap too, and kubectl guesses which one you mean if you don't specify a group (based on discovery order?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the short names for the exsiting resource types are in kubectl for the default resources. I remember having a conversation with @bgrant0607 about moving these out into the discovery API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the short names for the exsiting resource types are in kubectl for the default resources. I remember having a conversation with @bgrant0607 about moving these out into the discovery API.
A community member, @p0lyn0mial , moved the shortNames to the api discovery endpoint in 1.6.
There are a couple things that you'll need to consider: | ||
1. Garbage collection. You may have created links that weren't respected by | ||
the GC collector in 1.6. Since you orphaned your dependents, you'll probably | ||
want to re-adopt them like the Kubernetes controllers do with their resources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unlikely but possible that someone else may have adopted them in the meantime. For example, suppose my TPR-based controller created ReplicaSets that happen to match the selector of some other Deployment.
There may not be an easy way to systematically prevent this (hard ways include reparenting to a dummy object), but it should probably be mentioned as a caveat for anyone following these migration instructions.
single TPR must have the same version or the Kubernetes API semantic of always | ||
returning a resource encoded to the matching version will not be maintained. | ||
Since conversions (even native Kubernetes conversions) cannot be used to handle | ||
behavioral changes, the same effect can be achieved for TPRs client-side with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be helpful to write out an example flow for handling version upgrades of a TPR client-side, similar to the tpr.extensions->tpr.apiextension migration at the bottom.
One thing that may be worth having a better plan for is how TPR clients (e.g. Operators) can upgrade TPR data safely. For the initial migration to tpr.apiextension, it's perhaps reasonable to require offline migration, but version bumps within a given TPR (e.g. EtcdCluster v1beta1 -> v1beta2) will be more common.
For example, I'd want to make sure the framework exists to let an Operator do its own auto-upgrade of the stored TPR data. If that must be done by downloading all TPR data before deregistering the old TPR version, where is it going to store that data to survive crashes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version bumps within a given TPR (e.g. EtcdCluster v1beta1 -> v1beta2) will be more common.
The point of my description is that its actually possible to mutate within a rolling version of your choosing which preserves backwards compatibility. That is essentially what we do with server-side conversions, but with just slightly more sugar.
If you need to change behavior or wish to break backwards compatibility, TPRs end up in the same boat as normal resources have to deal with a logical move to a different resource or some kind of off-line change. It's the same regardless of TPR-ness or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure I understand, is the following what you're proposing?
- Create TPR with some arbitrary k8s-level API version string, but don't change it for anything that's backwards compatible, if you want to be able to upgrade seamlessly (automated by the Operator).
- If you need to track data version to help you automatically migrate between backwards-compatible representations, you should record it in your own field within the TPR data.
Kubernetes APIs. | ||
2. Enable ThirdPartyResources to specify how they will appear in API | ||
discovery to be consistent with other resources and avoid naming confilcts | ||
3. Move TPR into their own API group to allow the extensions group to be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
arguably "extensions" is a good place for TPRs? Move everything else out instead? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I've had this exact same thought
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on kubernetes/kubernetes#43214 (comment) seems to imply that there's something special about extensions
such that "it doesn't support multiple API versions." Is that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all third party resources will live in the same group?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all third party resources will live in the same group?
No, this is just the description of the TPR you want to store. TPR-registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on kubernetes/kubernetes#43214 (comment) seems to imply that there's something special about extensions such that "it doesn't support multiple API versions." Is that true?
It's a really confusing group. Not everything it's version is actually beta. I agree with that issue where Brian says we should just retire it.
|
||
Non-goals | ||
1. Solve automatic conversion of TPR between versions or automatic migration of | ||
existing TPR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me if this and previous references to TPR migration are talking about TPR data objects or TPR spec objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I have the same question.
incompatible, changes. By not supporting conversion, all TPR use cases are | ||
preserved, but a large amount of complexity is avoided for consumers of TPR. | ||
|
||
Allowing a single, user specified version for a given TPR will provide this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shrug I really don't have a problem with allowing the user to register v1 and v2 and manually convert. I think we will need to do that for some resources eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes TPR behave differently than every other kube resource, and breaks things like garbage collection and namespace cleanup, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
manual conversion means we stored two different distinct values? i hope we don't do that... i could see offering some transformation hook, but hope we do not start persisting distinct entities per version. if we did that how would resource version work? possible i am misunderstanding what @lavalamp was implying.
|
||
Conflicts with other parts of the value-space can not be detected with static | ||
validation, so there will be a spec/status split with `status.conditions` that | ||
reflect the acceptance status of a TPR-registration. For instance, you cannot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I would expect to do this with an admission controller / initializer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I would expect to do this with an admission controller / initializer.
Admission is opt-in by the cluster-admin. This behavior shouldn't be enable/disable-able (admission is). Also, I prefer the locality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you implement these in their own apiserver, that doesn't need to be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you implement these in their own apiserver, that doesn't need to be true.
Technically true, but very confusing. Especially as we look to compose API servers.
In addition, most resources (all except RBAC where its a security problem?), handle "any chance of working" validation synchronously and "is it working now" asynchronously. I think the model works well and applies cleanly here.
2. singular resource-type name - like "configmap" | ||
3. short names - like "cm" | ||
2. Kind-type value space - for group "example.com" | ||
1. Kind name - like "ConfigMap" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not important for this to be unique, since multiple TPRs could use the same kind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not important for this to be unique, since multiple TPRs could use the same kind.
Given how a restmapper works with serialized objects, the kind must be unique per group or weird/bad things will happen.
|
||
Parts of the value-space will be "claimed" by making an entry in TPR.status to | ||
include the accepted names which will be served. This prevents a new TPR from | ||
disabling an existing TPR's name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
short name is not important because no one should write scripts using the short name, if two objects collide then the spec/discovery assembly code can just serve neither short name (or both and let clients complain).
Where is the singular resource name used? I am not convinced we need to write a big system to keep it unique.
Honestly this is a lot of work/complexity all to handle a case where two different plural resource types happen to try and use the same singular name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plural resource (for URLs) and kind (for object specs) must both be unique.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the singular resource name used? I am not convinced we need to write a big system to keep it unique.
Singular is used on the CLI. The size is subjective, this ends up being pretty small and keeps us consistent with the first party resources.
ShortNames []string `json:"shortNames,omitempty" protobuf:"bytes,5,opt,name=shortNames"` | ||
// Kind is the serialized kind of the resource | ||
Kind string `json:"kind" protobuf:"bytes,6,opt,name=kind"` | ||
// ListKind is the serialized kind of the list for this resource. Defaults to <kind>List |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any exceptions to this derivation? I'm not sure we want to allow customizing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any exceptions to this derivation? I'm not sure we want to allow customizing this?
Non-english comes to mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't think it makes a lot of sense to internationalize the type names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I don't think it makes a lot of sense to internationalize the type names.
Derived, uninspectable, unchangeable, unfixable field tend to bite people. This is information we need to properly serve the endpoint. I don't mind defaulting it (noted in the doc), but I really don't want to say that there's no way for a user to specify it if there is some need.
// Version is the version this resource belongs in | ||
Version string `json:"version" protobuf:"bytes,2,opt,name=version"` | ||
// Plural is the plural name of the resource | ||
Plural string `json:"plural" protobuf:"bytes,3,opt,name=plural"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must match name of resource
ResourceNameConflict ThirdPartyResourceConditionType = "ResourceNameConflict" | ||
// KindNameConflict means the kind names chosen for this ThirdPartyResource conflict with others in the group. | ||
// The first TPR in the group to have the name reflected in status "wins" the name. | ||
KindNameConflict ThirdPartyResourceConditionType = "KindNameConflict" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just one name conflict condition, and the reason/message can say which specifically is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe just one name conflict condition, and the reason/message can say which specifically is wrong.
done
const ( | ||
ConditionTrue ConditionStatus = "True" | ||
ConditionFalse ConditionStatus = "False" | ||
ConditionUnknown ConditionStatus = "Unknown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stuff not defined by the v1 api?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this stuff not defined by the v1 api?
I think I would encourage API groups to define their own types unless they actually need to interoperate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I think we want conditions to be as close to being generically programmatically consumable as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree, I think we want conditions to be as close to being generically programmatically consumable as possible.
That sounds more like a polymorphic /conditions
subresource that could be programatically discovered and used like HPA than attempting to force a particular serialization format on kinds.
Conditions []ThirdPartyResourceCondition `json:"conditions" protobuf:"bytes,1,opt,name=conditions"` | ||
|
||
// Singular is the singular name of the resource. Defaults to lowercased <kind> | ||
Singular string `json:"singular,omitempty" protobuf:"bytes,2,opt,name=singular"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider grouping all of these into a "naming" struct, which can be present in both status & spec instead of copying everything individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider grouping all of these into a "naming" struct, which can be present in both status & spec instead of copying everything individually.
done
discovery to be consistent with other resources and avoid naming confilcts | ||
3. Move TPR into their own API group to allow the extensions group to be | ||
[removed](https://github.com/kubernetes/kubernetes/issues/43214) | ||
4. Support cluster scoped TPR resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did want this, not every resource is namespace scoped, does this mean we may want a generic mechanism to define cluster scoped resources?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we did want this, not every resource is namespace scoped, does this mean we may want a generic mechanism to define cluster scoped resources?
Yes, its described in this doc.
3. Move TPR into their own API group to allow the extensions group to be | ||
[removed](https://github.com/kubernetes/kubernetes/issues/43214) | ||
4. Support cluster scoped TPR resources | ||
5. Identify other features required for TPR to become beta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like multi-version support, more custom specific validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we don't plan to support multi version :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we don't plan to support multi version
I'm not fundamentally opposed, but I did lay out the REST API semantics that prevent it from working like the first-party kube APIs. @lavalamp indicated that he wanted to change that restriction in first-party kube APIs, but if that's the case I think the first-party APIs should move first and the third-party APIs can change afterwards.
struct updated for comments. |
1. Ensure ThirdPartyResource APIs operate consistently with first party | ||
Kubernetes APIs. | ||
2. Enable ThirdPartyResources to specify how they will appear in API | ||
discovery to be consistent with other resources and avoid naming confilcts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/confilcts/conflicts
/cc |
const ( | ||
// NameConflict means the resource or kind names chosen for this ThirdPartyResource conflict with others in the group. | ||
// The first TPR in the group to have the name reflected in status "wins" the name. | ||
NameConflict ThirdPartyResourceConditionType = "NameConflict" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect in most cases, the real Condition clients care about is, "Is this TPR actually installed in the API server?". A name conflict seems more like a particular Reason for the Installed
condition to have a status of ConditionFalse
. In the future, there may be other Reasons besides name conflict that a TPR would have condition Installed:False
, and clients shouldn't need to know to check for those specifically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect in most cases, the real Condition clients care about is, "Is this TPR actually installed in the API server?". A name conflict seems more like a particular Reason for the Installed condition to have a status of ConditionFalse. In the future, there may be other Reasons besides name conflict that a TPR would have condition Installed:False, and clients shouldn't need to know to check for those specifically.
Not all name conflicts prevent the TPR from being served. ShortNames are a good example. If you added a conflicting ShortName, your resource is still servable, you just can't use the ShortName to refer to it.
I'm fine adding an Available
condition, but this isn't it and available doesn't have sufficient power to describe the states.
// Group is the group this resource belongs in | ||
Group string `json:"group" protobuf:"bytes,1,opt,name=group"` | ||
// Version is the version this resource belongs in | ||
Version string `json:"version" protobuf:"bytes,2,opt,name=version"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we later support multiple versions (e.g. because the first-party APIs evolve to allow the content of different versions to differ), can this Spec be changed to allow multiple versions in a backwards-compatible way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this Spec be changed to allow multiple versions in a backwards-compatible way?
AdditionalVersions []string
along with a new TPR version (v1alpha2?) which combines them into a single field. It is definitely extensible.
disabling an existing TPR's name. | ||
|
||
|
||
## New API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are moving to a new API group, this seems like the time to ask whether everyone is happy with the name ThirdPartyResource. Is it going to make sense given all the ways we now expect this to be used in the future?
For example, I've seen TPR proposed as a way to implement new first-party (included in Kubernetes) objects like BatchJob in order to avoid bloating the core API. TPR has also been proposed as a way to store data for objects implemented by aggregated API servers, in cases when those aggregated servers don't want to run their own datastore.
Given that, the defining feature of TPR is not that the resources are necessarily third-party, but rather that they are not compiled in, and can be added and removed on a running cluster.
Just brainstorming:
- DynamicResource (cf. dynamic client)
- ExtensionResource
- PluginResource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While a new name might make sense, I'd recommend we defer any discussion about it until all the other issues are resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense to me as well and we should discuss it after the other parts of this proposal are agreed on.
I'd like to add 'APIExtension' to the list of options. (I'm in favor of dropping the 'Resource' suffix.)
On Tue, Apr 11, 2017 at 4:46 AM David Eads ***@***.***> wrote:
@deads2k <https://github.com/deads2k> can you outline how the on-the-wire
API changes in order to accommodate these suggested changes? e.g.
http://127.0.0.1:8080/apis/etcd.coreos.com/v1beta1/namespaces/default/clusters/example-etcd-cluster
becomes ...?
That format will not change.
What would prevent the API server from not doing the data dump and restore
internally to do a migration?
Brandon
|
I'm not sure I understand the question, but I think you're asking why the migration steps are manual instead of automatic? Because of the interaction with controllers, there needs to be a coordinated change. Coordinating with external processes is beyond the scope of what our API server does. I want to make sure the migration is crisply described, but I don't think it can be baked into the API server itself. |
I think we've made it through the structural concerns. I think we can continue hashing out names when the types pull gets opened up. We get all the way until code freeze for changes there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM overall from a sig-apps perspective (specifically focusing on Operators and custom controllers that use TPR). I think it will be a good base that we can build on.
I do want to put more thought into how we can ease the migration for users of alpha TPR (more automation), but I don't foresee solutions there requiring changes at the level of this proposal - migration helpers could be a separate proposal.
Merging the proposal so we can propose working on it in the 1.7 cycle. We can continue to iterate on the details in later reviews. |
Because of the changes required to meet the goals, there is not a silent | ||
auto-migration from the existing TPR to the new TPR. It will be possible, but | ||
it will be manual. At a high level, you simply: | ||
1. Stop all clients from writing to TPR (revoke edit rights for all users) and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deads2k is it going to be a requirement to coordinate this cluster wide for all TPRs, or will individual controllers be able to migrate on a per-TPR basis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, the plan is that existing TPR (extensions/v1beta1
) will continue to work side-by-side with the new TPR (apiextensions/v1beta1
) in v1.7. So, it will be possible to migrate on a per-TPR or per-controller basis, assuming you don't have interdependencies between multiple TPRs/controllers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, the plan is that existing TPR (extensions/v1beta1) will continue to work side-by-side with the new TPR (apiextensions/v1beta1) in v1.7. So, it will be possible to migrate on a per-TPR or per-controller basis, assuming you don't have interdependencies between multiple TPRs/controllers.
Exactly right.
I think the biggest pain point is likely to be the migration from original TPR to new TPR. @enisoc is out this week, so we won't be able to coordinate fully, but I was hoping he could take the lead on making the transition easier. You have significant user experience expertise too, so help with the design (I just sketched it in this doc) and implementation of actually transitioning a TPR from old to new is an open area. I've got most of an implementation running and I've started making small-ish pulls to bring it in. If you have review time, getting 90% types kubernetes/kubernetes#45115 and iterating on them would be useful. |
Get it and thanks very much @deads2k :) |
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
…ources Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Proposal: TPR to beta proposal
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
Automatic merge from submit-queue. Proposal: SubResources for CustomResources [CustomResourceDefinitions](kubernetes/community#524) (CRDs) were introduced in 1.7. The objects defined by CRDs are called CustomResources (CRs). Currently, we do not provide subresources for CRs. However, it is one of the [most requested features](kubernetes/kubernetes#38113) and this proposal seeks to add `/status` and `/scale` subresources for CustomResources. cc @sttts @deads2k @enisoc @bgrant0607 @erictune @lavalamp @brendandburns @philips @liggitt @mbohlool @fabxc @adohe @munnerz
* Add self as developer * Fix sort
There are a number of important issues with the alpha version of ThirdPartyResources that we wish to address to move TPR to beta. The list is tracked here, and also includes feedback from existing Kubernetes ThirdPartyResource users. This proposal covers the steps that are necessary to move TPR to beta and to prevent future challenges in upgrading.
@kubernetes/sig-api-machinery-misc @erictune @lavalamp @brendandburns @philips