Skip to content
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

Properly set object status #2726

Closed
ddysher opened this issue Dec 3, 2014 · 71 comments
Closed

Properly set object status #2726

ddysher opened this issue Dec 3, 2014 · 71 comments
Labels
area/api Indicates an issue on api area. area/apiserver area/usability priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@ddysher
Copy link
Contributor

ddysher commented Dec 3, 2014

As per discussion #2665 (comment), we currently haven't set object status for watch command. The status should be set properly before returning.

@bgrant0607 bgrant0607 added area/api Indicates an issue on api area. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Dec 3, 2014
@bgrant0607
Copy link
Member

We should be writing the status to etcd, and when the performance of accessing etcd becomes a bottleneck, we should serve the objects from an in-memory cache.

@bgrant0607 bgrant0607 changed the title Properly set object status in rest watch command Properly set object status Dec 3, 2014
@bgrant0607
Copy link
Member

Not only would this make it possible to WATCH status, but this would also address issues like #2882 and #2532 where the apiserver has different behavior from the object's implementation, and would facilitate plugins (#2594).

The apiserver should be dumb. Even the registry shouldn't have object-specific business logic.

Whatever component(s) implements the business logic for the object (e.g., Kubelet for pods, controller manager for replicationControllers) should proactively periodically compute the status and PUT it to the apiserver, which would update an in-memory cache and write through to etcd. GET should just return the latest cached value, not compute it on demand.

This provides another motivation for exposing sub-parts of objects at their own endpoints (.../status, .../spec), or PATCH.

/cc @lavalamp @smarterclayton @dchen1107

@bgrant0607
Copy link
Member

In case another issue doesn't cover this: We also shouldn't let the user POST/PUT operations trash status.

@lavalamp
Copy link
Member

So PUT needs to know who is doing the PUTing? If user, only spec may change, if particular service account(s), only status may change? That sounds hard and would block on auth policies.

Providing parts of objects at their own endpoints sounds hard, too...

@smarterclayton
Copy link
Contributor

Parts of resources doesn't sound that hard - it lets you policy them separately and define clear semantics.

For Derek's admission/resource controller he's separating the "spec" (this is what you're allowed) vs what you've observed in use "status", and status is set via a different endpoint. It seems cleaner so far than having it in spec and using policy to control attributes, because clients can reason about the rest resource as a unit rather than the attributes. My 0.02

On Dec 12, 2014, at 2:32 PM, Daniel Smith notifications@github.com wrote:

So PUT needs to know who is doing the PUTing? If user, only spec may change, if particular service account(s), only status may change? That sounds hard and would block on auth policies.

Providing parts of objects at their own endpoints sounds hard, too...


Reply to this email directly or view it on GitHub.

@davidopp
Copy link
Member

I may be hallucinating but I thought at Kubercon we decided to store these things as separate objects and make them be separate endpoints and then have some kind of virtual endpoint that joined them?

@smarterclayton
Copy link
Contributor

We certainly had a bunch of discussions to that end - probably want to lay it out and make sure everyone is happy

On Dec 12, 2014, at 3:54 PM, davidopp notifications@github.com wrote:

I may be hallucinating but I thought at Kubercon we decided to store these things as separate objects and make them be separate endpoints and then have some kind of virtual endpoint that joined them?


Reply to this email directly or view it on GitHub.

@bgrant0607
Copy link
Member

Ignoring auth, we wouldn't need to distinguish who was doing the PUTing if users only POSTed/PUT the Spec and components only POSTed/PUT the Status. The whole object could be a readonly view. Certainly this would be a major change.

@smarterclayton
Copy link
Contributor

The decomposed PUT certainly is more consistent across resources, and definitely allows dumber clients. Most users would set spec for sure (if designed well). Setting status seems like the exceptional case - where mutating status might just always be a differentiator.

----- Original Message -----

Ignoring auth, we wouldn't need to distinguish who was doing the PUTing if
users only POSTed/PUT the Spec and components only POSTed/PUT the Status.
The whole object could be a readonly view. Certainly this would be a major
change.


Reply to this email directly or view it on GitHub:
#2726 (comment)

@bgrant0607
Copy link
Member

/cc @satnam6502

@smarterclayton
Copy link
Contributor

I think one of the strong points of rest is that it's ok to have resources (identified by name) that return different representations of the same object and expose different mutation mechanisms. What complicates /pods/spec and /pods/status is that the vast majority of restful client libraries are simple - they can't handle things like nested sub resources, or if they do, they do it subtly different ways. Generally you gear your resources to what you expect users to transform. As an example, /pods works great because users mostly set the spec and mostly read the status. The intended users of /pods is pod consumers. The other use case for a pods endpoint is status owners (the people who own the status that the pod exposes). I don't know that there's as strong of a use case for /pod/spec (avoiding the cost to calculate status, maybe). Doing "/podspecs" and "/podstatus" simplifies client implementations a lot - you can more easily wire in "POST on /podstatus accepts PodStatus", whereas "POST on /pods accepts Pod" - you know what the type should be. It's harder to infer what "/pod/spec" returns in a generic fashion.

None of those are blocking, just considerations.

@bgrant0607
Copy link
Member

I was exactly thinking of different representations for the same resources.

I'd be fine with /podstatus rather than /pod/status.

I think the main motivation for /podspec would be to simplify read-modify-write sequences. PATCH and filtering/projection (#1459) would be alternatives to that. Internally, we use RPC rather than REST, and all read and write operations are inherently separate, as are reads of desired vs. current status (spec vs. status). I agree that separate operations are less amenable to generic meta-programming over the API, however, so probably we should just make slicing and dicing work easily and efficiently.

@smarterclayton
Copy link
Contributor

I certainly have no objects to /podspec as an endpoint. Is the sliced up resource just PodSpec, or must it have metadata? Can I POST to PodSpec (needs metadata), or just PUT? Can I delete PodSpec? Seems like just allowing PUT/PATCH/GET would be 90% of the use cases you mention.

----- Original Message -----

I was exactly thinking of different representations for the same resources.

I'd be fine with /podstatus rather than /pod/status.

I think the main motivation for /podspec would be to simplify
read-modify-write sequences. PATCH and filtering/projection (#1459) would be
alternatives to that. Internally, we use RPC rather than REST, and all read
and write operations are inherently separate, as are reads of desired vs.
current status (spec vs. status). I agree that separate operations are less
amenable to generic meta-programming over the API, however, so probably we
should just make slicing and dicing work easily and efficiently.


Reply to this email directly or view it on GitHub:
#2726 (comment)

@ddysher
Copy link
Contributor Author

ddysher commented Dec 17, 2014

Node status sync is blocked on this somehow. Agree that apiserver should be dumb, and I certainly want to do a PUT to /nodestatus or /node/status; however, this seems to be a larger change.

I think the convention here is that things under root path is a k8s resource; so to use /podstatus, are we going to make it a RESTStorage, with only Get/Update implemented? Also, since it's attached to Pod, should we make PodStatus a full-fleged API object (with ObjectMeta, etc), or just use Pod object?

@smarterclayton
Copy link
Contributor

Anything we accept or return probably needs to have an api version and kind embedded in it. We also at minimum need to have Name and Namespace somehow (unless we fix RESTStorage so that you can pass down the Name that the apiserver took off the URL). We probably need to return ResourceVersion in order to do atomic updates.

We don't want to add those to PodStatus directly I don't think. One option is:

type PodStatusObject struct {
  TypeMeta `inline`
  ObjectMeta `metadata`
  PodStatus `status`
}

Is objectmeta useful in this context? Automated processes may want to set annotations or labels.

On the other hand, if we defined very specific "facet" handlers (which is what /node/status /node/spec could be) you could define a different type of handler that is simpler and more targeted:

PUT /nodes/foo/status -> accepts PodStatus directly
PUT /status/nodes/foo -> accepts PodStatus directly

Changing semantics means clients have to be different, but clients could be more focused...

----- Original Message -----

Node status sync is blocked on this somehow. Agree that apiserver should be
dumb, and I certainly want to do a PUT to /nodestatus or /node/status;
however, this seems to be a larger change.

I think the convention here is that things under root path is a k8s resource;
so to use /podstatus, are we going to make it a RESTStorage, with only
Get/Update implemented? Also, since it's attached to Pod, should we make
PodStatus a full-fleged API object (with ObjectMeta, etc), or just use Pod
object?


Reply to this email directly or view it on GitHub:
#2726 (comment)

@ddysher
Copy link
Contributor Author

ddysher commented Dec 18, 2014

I think I'm leaning towards '/node/status' approach. NodeStatus is not a standalone resource, it's a subobject of Node. Namespace, SelfLink, etc do not make sense for NodeStatus, and even if some of them do make sense, they are potentially duplidate with that of Node. Further, we'll need to connect Node with NodeStatus externally (other than struct embedding), which could be non-trivial work.

Looking at our client implementation, it's not flexible to update status only. One option would be to change NodeInterface to include an extra status update field:

type NodeInterface interface {
  Get(name string) (result *api.Node, err error)
  Create(minion *api.Node) (*api.Node, error)
  List() (*api.NodeList, error)
  Delete(name string) error
  UpdateStatus(minion *api.Node) (*api.Node, error)
}

or create a new NodeStatusInterface.

type NodeStatusInterface interface {
  UpdateStatus(minion *api.Node) (*api.Node, error)
}

The latter might be better as adding UpdateStatus directly to NodeInterface sounds like status is updatable for client. No matter what, I'd think the change is less intrusive and cleaner than a new object.

As for server side, I think there is no need for another REST path. How about generalize context object?

@bgrant0607
Copy link
Member

I wouldn't add UpdateStatus. I'd add Patch.

@ddysher
Copy link
Contributor Author

ddysher commented Dec 18, 2014

Ah, yeah, not familiar with patch, looking now....

@smarterclayton
Copy link
Contributor

The challenge with PATCH is you still have define the type of object the server expects, and clients have to have a consistent object model as well. If that model is NodeStatus, the a separate resource path is appropriate because the path and resource is consistent.

I assume we want to leave open the door in the future to use patch for single atomic field updates: "change pod status phase to foo". To do that, you really don't want to use the full pod status field, since you need to distinguish between "nil this field" and "do nothing for this field". But to do that, you have to define a third type of model that isn't Pod or PodStatus.

On Dec 18, 2014, at 4:08 AM, Deyuan Deng notifications@github.com wrote:

Ah, yeah, not familiar with patch, looking now....


Reply to this email directly or view it on GitHub.

@ddysher
Copy link
Contributor Author

ddysher commented Dec 19, 2014

If we add another type for updating PodStatus, then we'll need another one for NodeStatus, then PodSpec, etc; is there a way we can avoid the pain?

To make it more flexible, can we do something like this:

PATCH /api/v1beta1/minions/10.1.1.1
{
    {"Operation": "Replace", "Field": "Phase", Type:"NodePhase", Value: "Running"},
    {"Operation": "Add", "Field": "Conditions", Type:"NodeCondition", Value: "{kind:reachable, status:full......}"},
    {"Operation": "Remove", "Field": "Conditions", Type:"NodeCondition", Value: "{kind:ready, status:none.....}"},
}

This may require a lot of reflection, not sure if this is doable.

@bgrant0607
Copy link
Member

Filed #5000 for /binding. There are already issues for /size and stop, and this issue is about /status. If we want to do something similar for /events and /endpoints, those would be 2 new issues.

@bgrant0607
Copy link
Member

Cool. Clone/instantiate is covered by #170.

@davidopp
Copy link
Member

Can someone (maybe Clayton?) summarize the status of this issue? If this has become a meta-issue, let's file any necessary sub-issues and close this. Otherwise, what remains to be done here for 1.0?

@smarterclayton
Copy link
Contributor

I don't know that we have a meta issue covering "cluster mutation is watchable, and the status of the cluster is observable from the API server without passthrough hacks" except this one. Will try to summarize if brian doesn't get here first.

@bgrant0607
Copy link
Member

Quick status AIUI:

@smarterclayton
Copy link
Contributor

PodTemplate would support /instantiate.

I need to do some prep work for /size to allow easy implementation.

----- Original Message -----

Quick status AIUI:


Reply to this email directly or view it on GitHub:
#2726 (comment)

@derekwaynecarr
Copy link
Member

Namespace Status is posted via a subresource

@davidopp
Copy link
Member

Thanks for the summary. So, concretely, what remains as a 1.0 blocker? Just the things from Brian's list minus the entries that say the item is done?

@bgrant0607
Copy link
Member

I had hoped that all status would be posted via subresources in the v1 API. Right now the API is in an intermediate state. I think finishing the remaining resources is mainly additive and therefore wouldn't need to be done for 1.0. Whether via a subresource or via posting the full resource is mostly an implementation detail. What matters for clients is the ability to watch status changes, which just requires status to be posted by some means.

Remaining things to be fixed for 1.0 can be driven by usability and performance problems.

@bgrant0607 bgrant0607 removed this from the v1.0 milestone Mar 27, 2015
@bgrant0607 bgrant0607 added priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. and removed priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 27, 2015
@bgrant0607 bgrant0607 added this to the v1.2-candidate milestone Sep 12, 2015
@bgrant0607 bgrant0607 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Sep 16, 2015
@bgrant0607
Copy link
Member

Using more specific issues for the remainder of the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Indicates an issue on api area. area/apiserver area/usability priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

No branches or pull requests

8 participants