client: update status subresource support #96

droot · 2018-07-30T21:52:28Z

Sending this (WIP) PR for an early feedback on the interface before I add tests.

droot · 2018-07-30T21:54:10Z

grantr · 2018-07-30T22:00:43Z

Seems fine to me. I'm curious what the plan is for other subresources. Scale is officially supported by Kubernetes apiserver, so will there be an UpdateScale function?

Intuitively, it seems fine to have a top-level UpdateStatus function (since this is used by almost every controller) and a separate "update any subresource" function that takes care of updating Scale. Though if you have the latter, why would you use the former?

droot · 2018-07-31T17:22:49Z

Seems fine to me. I'm curious what the plan is for other subresources. Scale is officially supported by Kubernetes apiserver, so will there be an UpdateScale function?

I think its important to capture resource behaviors in the interface. UpdateStatus(...), Scale(...) are important behaviors that they deserve their own methods IMO. Another thing I have realized, almost every behavior will have different set of args/validation/documentation requirements, so trying to fit all of them in some generic interface leads to a very poor UX.

CRDs have only two subresources at the moment, 'Status' and 'Scale'. Core resources have bind/exec/logs, so doesn't look like a whole lot.

DirectXMan12 · 2018-07-31T19:54:47Z

so trying to fit all of them in some generic interface leads to a very poor UX.

Yeah, that's what I was struggling with.

The dynamic scale client isn't terribly complicated (compared to the typed clients), so we may just want to consider providing an easy handle to it, or wrapping it lightly (I'd advise against re-implementing it here -- it'd be a decent amount of copied logic). The current invocation looks like client.Scales(ns).Get(groupResource, name) (similarly for update), so ours would probably look like client.GetScale(&kapi.Pod{}, scaleObj, reconcileKey) (although the duplicate types there might be confusing).

I'm also wary of having too many methods under the same "namespace" (interface), lest it get a bit confusing. We might want consider something like client.Get/Update/Delete/Whatever for the "normal" methods, and client.Scale(&kapi.Pod{}).Get(scaleInst, reconcileKey) for subresources. That way, if they ever introduce arbitrary CRUD-style subresource support (i.e. not things like exec and attach, but more like scale and status), we could have client.Subresource("mysubresource", &myapi.MyKind{}).Get(subresourceInst, reconcileKey) or what have you.

droot · 2018-07-31T22:25:30Z

so ours would probably look like client.GetScale(&kapi.Pod{}, scaleObj, reconcileKey) (although the duplicate types there might be confusing).

Why do we need to provide reconcileKey when we are providing &kapi.Pod{} which contains namespace and name ?

I'm also wary of having too many methods under the same "namespace" (interface), lest it get a bit confusing. We might want consider something like client.Get/Update/Delete/Whatever for the "normal" methods, and client.Scale(&kapi.Pod{}).Get(scaleInst, reconcileKey) for subresources. That way, if they ever introduce arbitrary CRUD-style subresource support (i.e. not things like exec and attach, but more like scale and status), we could have client.Subresource("mysubresource", &myapi.MyKind{}).Get(subresourceInst, reconcileKey) or what have you.

Same question why reconcileKey is needed ?

So going by the proposal above, Status subresource methods will look like:

Get: client.Status(&kapi.Pod{}).Get(....)
Update: client.Status(&kapi.Pod).Update(....)

Implementation in current PR looks like:

Get: didn't implement because client.Get retrieves Status as well
Update: client.UpdateStatus(ctx, &kapi.Pod{})

Implementation in the PR looks very intuitive because Status is special in the sense it accepts the API object itself as payload. I am sort of leaning towards special-casing Status and go with the simpler interface. WDYT ?

DirectXMan12

Code looks good.

Personally would prefer .Status().Update(ctx, object) (gives us a bit more flexibility in the future, IMO).

DirectXMan12 · 2018-08-02T19:33:46Z

pkg/client/fake/client.go

@@ -123,6 +123,11 @@ func (c *fakeClient) Update(ctx context.Context, obj runtime.Object) error {
 	return c.tracker.Update(gvr, obj, accessor.GetNamespace())
 }

+func (c *fakeClient) UpdateStatus(ctx context.Context, obj runtime.Object) error {
+	// TODO(droot): implement for *real*


we should prob implement this before merging.

I am calling c.Update now which will update the Status, but will also update the Spec part, which is wrong.

Still looking for a way to do it with underneath tracker.

droot · 2018-08-03T00:29:35Z

pkg/client/fake/client.go

+func (c *fakeClient) UpdateStatus(ctx context.Context, obj runtime.Object) error {
+	// TODO(droot): This results in full update of the obj (spec + status). Need
+	// a way to update status field only.
+	return c.Update(ctx, obj)


Couldn't figure out how to implement UpdateStatus behavior with tracker object.
@mengqiy any ideas ?

I don't see an easier way except implementing it upstream.
It seems no one uses fakeclient's UpdateStatus, I think this is OK to be a starting point.

Sounds good.

droot · 2018-08-06T17:28:34Z

@DirectXMan12 added partial implementation of the UpdateStatus, should be ok for now. (fixing it completely will require re-doing the fake implementation)

Personally would prefer .Status().Update(ctx, object) (gives us a bit more flexibility in the future, IMO).

Keeping flexibility aside for a moment, do you find UpdateStatus(...) is more intuitive over client.Status().Update(...) ?

droot · 2018-08-08T20:32:00Z

@DirectXMan12 revised the interface/implementation as per our last conversation. PTAL.

DirectXMan12

minor test-related nit inline, otherwise good

DirectXMan12 · 2018-08-08T20:35:26Z

pkg/client/client.go

+		return err
+	}
+	// TODO(droot): examine the returned error and check if it error needs to be
+	// wrapped to improve the UX ?


we could always check discovery to see if the /status subresource appeared in discovery first, if we really wanted to...

Thanks for the tip. Not critical for now, so not addressing this in this PR.

DirectXMan12 · 2018-08-08T20:36:14Z

pkg/client/client_test.go

+			close(done)
+		})
+
+		It("should fail if the object does not pass server-side validation", func() {


I don't think we need to explicitly test server-side behavior. The implication is that we're using the status subresource here.

+1, removed it.

droot · 2018-08-08T21:03:38Z

@DirectXMan12 addressed the comments. PTAL.

DirectXMan12

you've still got a couple of empty tests, otherwise looks good.

DirectXMan12 · 2018-08-09T16:10:27Z

pkg/client/client_test.go

+			close(done)
+		})
+
+		It("should fail if the GVK cannot be mapped to a Resource", func() {


you've still got a couple empty tests here.

I wanted to have these placeholders as TODOs so that it can act as good-first-issues for new contributors.

Lets wait for today, I might be able to add these later today.

Please mark them as pending then -- http://onsi.github.io/ginkgo/#pending-specs (if you're not going to add them now)

droot · 2018-08-14T00:26:56Z

@DirectXMan12 converted all TODO tests to pending specs (Thanks for the tip). PTAL whenever you get a chance.

DirectXMan12 · 2018-08-14T18:40:47Z

/lgtm
/approve

Let's try and fill in those existing gaps in the tests in a follow-up PR shortly

k8s-ci-robot · 2018-08-14T18:40:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, droot

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [DirectXMan12,droot]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…-subresource client: update status subresource support

droot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 30, 2018

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 30, 2018

droot requested a review from DirectXMan12 July 30, 2018 21:53

droot force-pushed the client/update-status-subresource branch from 7397811 to 927acf2 Compare July 31, 2018 21:26

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 31, 2018

droot changed the title ~~WIP: client: update status subresource support~~ client: update status subresource support Jul 31, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 31, 2018

DirectXMan12 suggested changes Aug 2, 2018

View reviewed changes

droot force-pushed the client/update-status-subresource branch from 927acf2 to 9d34003 Compare August 3, 2018 00:27

droot commented Aug 3, 2018

View reviewed changes

droot force-pushed the client/update-status-subresource branch from c547d2c to 25d0dce Compare August 8, 2018 20:30

DirectXMan12 suggested changes Aug 8, 2018

View reviewed changes

droot force-pushed the client/update-status-subresource branch from 25d0dce to 3dbc006 Compare August 8, 2018 21:00

DirectXMan12 suggested changes Aug 9, 2018

View reviewed changes

droot added 2 commits August 13, 2018 17:25

client: status subresource support

4ef4287

client: converted all TODOs test to pending specs

ad4fd79

droot force-pushed the client/update-status-subresource branch from 3dbc006 to ad4fd79 Compare August 14, 2018 00:25

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 14, 2018

DirectXMan12 approved these changes Aug 14, 2018

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Aug 14, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 14, 2018

k8s-ci-robot merged commit 3b338eb into kubernetes-sigs:master Aug 14, 2018

justinsb pushed a commit to justinsb/controller-runtime that referenced this pull request Dec 7, 2018

Merge pull request kubernetes-sigs#96 from droot/client/update-status…

38411a6

…-subresource client: update status subresource support

client: update status subresource support #96

client: update status subresource support #96

Uh oh!

Conversation

droot commented Jul 30, 2018

Uh oh!

droot commented Jul 30, 2018

Uh oh!

grantr commented Jul 30, 2018

Uh oh!

droot commented Jul 31, 2018

Uh oh!

DirectXMan12 commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droot commented Jul 31, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot commented Aug 6, 2018

Uh oh!

droot commented Aug 8, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot commented Aug 8, 2018

Uh oh!

DirectXMan12 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DirectXMan12 Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

droot commented Aug 14, 2018

Uh oh!

DirectXMan12 commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Aug 14, 2018

Uh oh!

Uh oh!

DirectXMan12 commented Jul 31, 2018 •

edited

Loading

DirectXMan12 Aug 9, 2018 •

edited

Loading

DirectXMan12 commented Aug 14, 2018 •

edited

Loading