Dynamic client supports subresources #56717

roycaihw · 2017-12-01T21:15:20Z

What this PR does / why we need it:
Allows resource.name to be a subresource which contains "/" in

kubernetes/staging/src/k8s.io/client-go/dynamic/client.go

Line 143 in db2977f

resource *metav1.APIResource

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #49588

Special notes for your reviewer:
The change is backward compatible.

Release note:

NONE

/sig api-machinery

caesarxuchao · 2017-12-08T00:07:22Z

staging/src/k8s.io/client-go/dynamic/client.go

@@ -151,9 +151,18 @@ func (rc *ResourceClient) List(opts metav1.ListOptions) (runtime.Object, error)
 	if parameterEncoder == nil {
 		parameterEncoder = defaultParameterEncoder
 	}
+	resourceName := ""


Make this block of code a private method of ResourceClient to avoid code duplication.

Updated. I also put a if logic into Create() instead of having two Create functions.

caesarxuchao

Users usually creates a client_pool. I think it client_pool will only work with some subresources, e.g., it will work with apps/v1/replicaset/status, but not apps/v1/replicaset/scale, because in the latter case, the gvk of scale is autoscaling/v1/scale, different from the group version of the main resource. Could you manually test these cases, and then write e2e tests?

caesarxuchao · 2017-12-08T00:11:35Z

staging/src/k8s.io/client-go/dynamic/client_test.go

@@ -114,10 +117,47 @@ func TestList(t *testing.T) {
 				},
 			},
 		},
+		{
+			resource: "rtest/srtest",
+			name:     "normal_list",


Name should be unique for each test.

non_namespaced_list? "normal" doesn't make sense. That could be another PR.

Have updated Name to be unique for each test.

caesarxuchao · 2017-12-08T22:54:01Z

staging/src/k8s.io/client-go/dynamic/client.go

+			NamespaceIfScoped(rc.ns, rc.resource.Namespaced).
+			Resource(resourceName).
+			SubResource(subresourceName...).
+			Name(obj.GetName()).


obj here is the subresource, but the name you want is the name of the parent resource, right?

It seems subresource name is implicitly always the same as the resource name.

caesarxuchao · 2018-02-06T19:46:48Z

staging/src/k8s.io/client-go/dynamic/client.go

@@ -66,6 +66,8 @@ type ResourceInterface interface {
 	DeleteCollection(deleteOptions *metav1.DeleteOptions, listOptions metav1.ListOptions) error
 	// Create creates the provided resource.
 	Create(obj *unstructured.Unstructured) (*unstructured.Unstructured, error)
+	// CreateSubresource creates the provided resource.
+	CreateSubresource(name string, obj *unstructured.Unstructured) (*unstructured.Unstructured, error)


Could you remind me why CreateSubresource requires a name, instead of using obj.GetName()? And why we don't need an UpdateSubresource as well?

We could use obj.GetName(), as it seems subresource name is implicitly always the same as the resource name. CreateSubresource has more code change because it's adding both {name} and subresourceName into the request:

Create posts to .../resourceName
CreateSubresource posts to .../resourceName/{name}/subresourceName
compared with other operations:
Update puts to .../resourceName/{name}
UpdateSubresource puts to .../resourceName/{name}/subresourceName

We have a separate CreateSubresource instead of merging the logic into Create, because we want the change to be more backward compatible.
(FYI, subresources that support create operation are: pods/binding, pods/eviction and some deployments/rollback)

Currently there are subresources supporting create/get/patch/update operations. Probably we don't need to change code in list/delete/deletecollection/watch operations.

Currently there are subresources supporting create/get/patch/update operations. Probably we don't need to change code in list/delete/deletecollection/watch operations.

I prefer not changing the methods that we are not supporting.

Can you clarify why something like CreateSubresource is not a good idea?

Having a separate CreateSubresource is for allowing user to input a resource {name} (resourceName/{name}/subresourceName) that is different from obj.GetName(). We think currently our system assumes every subresource object has the same name as the parent resource object. E.g. a pods/binding object having metadada.name "foo" means pod "foo" is being bound. So we can keep the dynamic client concise and restricted.

cc @lavalamp

roycaihw · 2018-02-08T00:13:06Z

@caesarxuchao As we discussed offline, our system assumes every subresource object has the same name as the parent resource object. E.g. a pods/binding object having metadada.name "foo" means pod "foo" is being bound. This PR only changes the verbs create/get/update/patch.

caesarxuchao

Please address the nits and then LGTM.

cc @sttts if you care about dynamic client supporting subresources.

caesarxuchao · 2018-02-13T00:28:34Z

staging/src/k8s.io/client-go/dynamic/client.go

@@ -145,6 +145,19 @@ type ResourceClient struct {
 	parameterCodec runtime.ParameterCodec
 }

+func (rc *ResourceClient) parseResourceSubresourceName() (string, []string) {
+	resourceName := ""
+	subresourceName := []string{}


nit: change to

var resourceName var subresourceName []string

to avoid the memory allocation caused by []string{}.

caesarxuchao · 2018-02-13T00:35:03Z

staging/src/k8s.io/client-go/dynamic/client.go

+	if len(subresourceName) > 0 {
+		// If the provided resource is a subresource, the POST request should contain
+		// object name. Examples of subresources that support Create operation:
+		//	core/v1/pods/binding


How about updating it to core/v1/pods/{name}/binding so it's clear where the name is in the url?

caesarxuchao · 2018-02-13T00:36:32Z

staging/src/k8s.io/client-go/dynamic/client.go

+		//	core/v1/pods/eviction
+		//	extensions/v1beta1/deployments/rollback
+		//	apps/v1beta1/deployments/rollback
+		// NOTE: Currently our system assumes every subresource object has the same


The Update() function has the same assumption. Could you copy-paste this NOTE to there as well?

roycaihw · 2018-02-13T01:27:35Z

/retest

nikhita · 2018-02-13T07:22:13Z

staging/src/k8s.io/client-go/dynamic/client.go

@@ -166,9 +179,11 @@ func (rc *ResourceClient) Get(name string, opts metav1.GetOptions) (*unstructure
 		parameterEncoder = defaultParameterEncoder
 	}
 	result := new(unstructured.Unstructured)
+	resourceName, subresourceName := rc.parseResourceSubresourceName()


Which subresources support List?

I'm not aware of any based on api discovery. This change is in Get (github has some weird display problem :) ).

Oops, yes. :)

nikhita · 2018-02-13T07:24:19Z

staging/src/k8s.io/client-go/dynamic/client.go

 	err := rc.cl.Put().
 		NamespaceIfScoped(rc.ns, rc.resource.Namespaced).
-		Resource(rc.resource.Name).
+		Resource(resourceName).
+		SubResource(subresourceName...).


How about adding explicit methods like UpdateStatus(), etc instead of deriving the name? It would keep it backward compatible too.

I had added UpdateStatus and modified Patch to support subresources in c961ac4#diff-781d9a2143af0fe2be85e527bf53626a.

That's how we do in typed clients:

kubernetes/pkg/client/clientset_generated/internalclientset/typed/apps/internalversion/statefulset.go

Lines 35 to 47 in ae0f45e

// StatefulSetInterface has methods to work with StatefulSet resources.

type StatefulSetInterface interface {

Create(*apps.StatefulSet) (*apps.StatefulSet, error)

Update(*apps.StatefulSet) (*apps.StatefulSet, error)

UpdateStatus(*apps.StatefulSet) (*apps.StatefulSet, error)

Delete(name string, options *v1.DeleteOptions) error

DeleteCollection(options *v1.DeleteOptions, listOptions v1.ListOptions) error

Get(name string, options v1.GetOptions) (*apps.StatefulSet, error)

List(opts v1.ListOptions) (*apps.StatefulSetList, error)

Watch(opts v1.ListOptions) (watch.Interface, error)

Patch(name string, pt types.PatchType, data []byte, subresources ...string) (result *apps.StatefulSet, err error)

GetScale(statefulSetName string, options v1.GetOptions) (*autoscaling.Scale, error)

UpdateScale(statefulSetName string, scale *autoscaling.Scale) (*autoscaling.Scale, error)

We also need to support Update for other subresources like certificates.k8s.io/v1beta1/certificatesigningrequests/{name}/approval and v1/namespaces/{name}/finalize.

nikhita · 2018-02-13T07:25:35Z

staging/src/k8s.io/client-go/dynamic/client.go

 	err := rc.cl.Patch(pt).
 		NamespaceIfScoped(rc.ns, rc.resource.Namespaced).
-		Resource(rc.resource.Name).
+		Resource(resourceName).
+		SubResource(subresourceName...).


Would be better to have subresources as an argument...that's how we do in typed clients.

kubernetes/pkg/client/clientset_generated/internalclientset/typed/apps/internalversion/statefulset.go

Lines 164 to 176 in ae0f45e

// Patch applies the patch and returns the patched statefulSet.

func (c *statefulSets) Patch(name string, pt types.PatchType, data []byte, subresources ...string) (result *apps.StatefulSet, err error) {

result = &apps.StatefulSet{}

err = c.client.Patch(pt).

Namespace(c.ns).

Resource("statefulsets").

SubResource(subresources...).

Name(name).

Body(data).

Do().

Into(result)

return

}

I agree that we should emulate typed clients behavior in dynamic client.

However, I think rc.resource.Name (which is a metav1.APIResource.Name) should allow names containing '/', e.g. pods/binding, to reflect what we are serving in api discovery:
{
"name": "pods/binding",
"singularName": "",
"namespaced": true,
"kind": "Binding",
"verbs": [
"create"
]
},

In that case, should we parse rc.resource.Name and change Resource(rc.resource.Name). into Resource(resourceName).?

@roycaihw can you try what happens if user creates a clientpool, and then call clientpool.ClientForGroupVersionResource() with a subresource? Can the restmapper handle subresource?

Yes, the restmapper can handle subresource: 9f9ec17#diff-b9e0984a44b8dbba37804e9a762ea6cbR89. It seems to return a client for GV if Group and Version exist

kubernetes/staging/src/k8s.io/client-go/dynamic/client_pool.go

Line 86 in 92e4d3d

return c.ClientForGroupVersionKind(schema.GroupVersionKind{Group: resource.Group, Version: resource.Version})

@nikhita a common use case is constructing the dynamic client with the result returned by the discovery client. In this case, it's convenient if the client.Resource(resource, namespace) allows resource to be in the form of resource/subresource. Typed clients don't have this use case so I think we can tolerate the discrepancy.

caesarxuchao · 2018-02-14T18:29:52Z

All the comments from @nikhita have been resolved. Thank you for reviewing it.

/lgtm
/approve

k8s-ci-robot · 2018-02-14T18:30:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, roycaihw

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 OWNERS Files:

~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao]

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

k8s-github-robot · 2018-02-14T18:30:28Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-14T19:14:43Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-02-14T19:42:37Z

@roycaihw: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-device-plugin-gpu	`e10cdb3`	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-github-robot assigned erictune and caesarxuchao Dec 1, 2017

roycaihw changed the title ~~Dynamic client support subresource~~ Dynamic client supports subresources Dec 1, 2017

caesarxuchao reviewed Dec 8, 2017

View reviewed changes

roycaihw force-pushed the dynamic_subresource branch from 7f1ebbe to bd4a014 Compare December 8, 2017 22:05

kubernetes deleted a comment from k8s-github-robot Dec 8, 2017

caesarxuchao reviewed Dec 8, 2017

View reviewed changes

roycaihw force-pushed the dynamic_subresource branch from bd4a014 to 75d0dba Compare December 14, 2017 18:37

caesarxuchao reviewed Feb 6, 2018

View reviewed changes

roycaihw force-pushed the dynamic_subresource branch from 75d0dba to cbd105f Compare February 8, 2018 00:04

caesarxuchao reviewed Feb 13, 2018

View reviewed changes

Dynamic client support subresource create/get/update/patch verbs

e10cdb3

roycaihw force-pushed the dynamic_subresource branch from cbd105f to e10cdb3 Compare February 13, 2018 00:56

nikhita reviewed Feb 13, 2018

View reviewed changes

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

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

k8s-github-robot merged commit f33e0b3 into kubernetes:master Feb 14, 2018

roycaihw deleted the dynamic_subresource branch February 15, 2018 18:42

enisoc mentioned this pull request Feb 23, 2018

Support status subresource and ObservedGeneration. GoogleCloudPlatform/metacontroller#26

Closed

shyamjvs mentioned this pull request Apr 3, 2018

'PATCH node-status' latency slo violations #62064

Closed

	// StatefulSetInterface has methods to work with StatefulSet resources.
	type StatefulSetInterface interface {
	Create(apps.StatefulSet) (apps.StatefulSet, error)
	Update(apps.StatefulSet) (apps.StatefulSet, error)
	UpdateStatus(apps.StatefulSet) (apps.StatefulSet, error)
	Delete(name string, options *v1.DeleteOptions) error
	DeleteCollection(options *v1.DeleteOptions, listOptions v1.ListOptions) error
	Get(name string, options v1.GetOptions) (*apps.StatefulSet, error)
	List(opts v1.ListOptions) (*apps.StatefulSetList, error)
	Watch(opts v1.ListOptions) (watch.Interface, error)
	Patch(name string, pt types.PatchType, data []byte, subresources ...string) (result *apps.StatefulSet, err error)
	GetScale(statefulSetName string, options v1.GetOptions) (*autoscaling.Scale, error)
	UpdateScale(statefulSetName string, scale autoscaling.Scale) (autoscaling.Scale, error)

	// Patch applies the patch and returns the patched statefulSet.
	func (c statefulSets) Patch(name string, pt types.PatchType, data []byte, subresources ...string) (result apps.StatefulSet, err error) {
	result = &apps.StatefulSet{}
	err = c.client.Patch(pt).
	Namespace(c.ns).
	Resource("statefulsets").
	SubResource(subresources...).
	Name(name).
	Body(data).
	Do().
	Into(result)
	return
	}

Dynamic client supports subresources #56717

Dynamic client supports subresources #56717

Conversation

roycaihw commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw commented Feb 8, 2018

caesarxuchao left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw commented Feb 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw Feb 13, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Feb 14, 2018

k8s-ci-robot commented Feb 14, 2018

k8s-github-robot commented Feb 14, 2018

k8s-github-robot commented Feb 14, 2018

k8s-ci-robot commented Feb 14, 2018

caesarxuchao left a comment •

edited

roycaihw Feb 13, 2018 •

edited