Dynamic Client #56

fabianvf · 2018-04-09T18:33:22Z

Still TODO:

codecov-io · 2018-04-09T18:42:34Z

Codecov Report

Merging #56 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master      #56   +/-   ##
=======================================
  Coverage   91.85%   91.85%           
=======================================
  Files          11       11           
  Lines         994      994           
=======================================
  Hits          913      913           
  Misses         81       81

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 24a0ff2...788d114. Read the comment docs.

fabianvf · 2018-04-19T19:41:32Z

@mbohlool

mbohlool · 2018-05-04T18:15:42Z

@yliaog @roycaihw

roycaihw

Thanks for the contribution! I left some comments :)

roycaihw · 2018-06-07T01:49:29Z

dynamic/client.py

+from kubernetes.client.api_client import ApiClient
+from kubernetes.client.rest import ApiException
+
+from openshift.dynamic.exceptions import ResourceNotFoundError, ResourceNotUniqueError, api_exception


The module path needs to be changed

roycaihw · 2018-06-07T23:31:00Z

dynamic/client.py

+    def group_version(self):
+        if self.group:
+            return '{}/{}'.format(self.group, self.api_version)
+        return self.api_version


nit: '/'.join(filter(None, [self.group, self.api_version])) like Line112?

roycaihw · 2018-06-08T00:34:34Z

dynamic/client.py

+                kind: The kind of the resource
+                arbitrary arguments (see below), in random order
+
+            The arbitrary arguments can be any valid attribute for an openshift.dynamic.Resource object


ditto: update the module path

roycaihw · 2018-06-08T04:16:40Z

dynamic/client.py

+    def path(self, name=None, namespace=None):
+        url_type = []
+        path_params = {}
+        if self.namespaced and namespace:


should we catch the case where namespaced is set but namespace is None?

or does ensure_namespace rule out that case already?

if namespaced is set and namespace is None, then we can still construct a valid path (api/v1/pods for example)

roycaihw · 2018-06-08T04:22:19Z

dynamic/client.py

+            'base': '/{}/{}'.format(full_prefix, self.name),
+            'namespaced_base': '/{}/namespaces/{{namespace}}/{}'.format(full_prefix, self.name),
+            'full': '/{}/{}/{{name}}'.format(full_prefix, self.name),
+            'namespaced_full': '/{}/namespaces/{{namespace}}/{}/{{name}}'.format(full_prefix, self.name)


can we determine if the resource is cluster-scoped or namespaced using self.namespaced here and only return two urls base and full?

you can access namespaced resources without providing a namespace (ie, list all pods in every namespace, etc), so I don't think we should

roycaihw · 2018-06-08T04:41:53Z

dynamic/client.py

+        content_type = self.client.\
+            select_header_content_type(['application/json-patch+json', 'application/merge-patch+json', 'application/strategic-merge-patch+json'])
+
+        return self.request('patch', path, body=body, content_type=content_type)


pass **kwargs to self.request

dynamic/client.py

roycaihw · 2018-06-08T04:50:29Z

dynamic/client.py

+    @meta_request
+    def delete(self, resource, name=None, namespace=None, label_selector=None, field_selector=None, **kwargs):
+        if not (name or label_selector or field_selector):
+            raise ValueError("At least one of name|label_selector|field_selector is required")


From the design doc, the following actions are marked as TODO: update, watch, deletecollection, proxy. I see update has been implemented (replace). I'm just curious if we don't want to have deletecollection for the first iteration.

Yeah, the only reason I didn't yet implement deletecollection is that I'm not familiar with it, would love to get it in

I think it's fine to add deletecollection as a followup

roycaihw · 2018-06-08T04:53:48Z

dynamic/client.py

+        full_prefix = '{}/{}'.format(self.prefix, self.group_version)
+        return {
+            'full': '/{}/{}/{{name}}/{}'.format(full_prefix, self.parent.name, self.subresource),
+            'namespaced_full': '/{}/namespaces/{{namespace}}/{}/{{name}}/{}'.format(full_prefix, self.parent.name, self.subresource)


ditto: if we could check self.namespaced and combine the two urls into one

see above response

roycaihw · 2018-06-08T04:57:16Z

dynamic/client.py

+    def _load_server_info(self):
+        self.__version = {'kubernetes': load_json(self.request('get', '/version'))}
+        try:
+            self.__version['openshift'] = load_json(self.request('get', '/version/openshift'))


I suppose this is not specific to kubernetes cluster api discovery. Not sure how we want organize the code. Looping in @yliaog @mbohlool

I can go ahead and take it out, this belongs in openshift specific code not upstream

fabianvf · 2018-07-30T17:41:16Z

@roycaihw rebased to keep up to date with branch, any additional comments?

fabianvf · 2018-08-08T18:46:43Z

ping @roycaihw

fabianvf · 2018-08-08T18:47:44Z

We've also had some community interest in the other discussed dynamic implementation (the one that uses the OpenAPI spec for discovery), I haven't had time to look into it again yet but I'm hoping I'll get a chance for a second pass soon.

roycaihw

It looks good to me in general. I left one nit and one question for clarification.

roycaihw · 2018-08-13T00:00:22Z

dynamic/client.py

+        return self.request('post', path, body=body, **kwargs)
+
+    @meta_request
+    def delete(self, resource, name=None, namespace=None, label_selector=None, field_selector=None, **kwargs):


k8s delete requests can have body of type DeleteOptions as alternative of some query parameters, although it's not a good practice to send delete request with body I suppose.

Good call, this should definitely support delete options.

roycaihw · 2018-08-13T00:22:23Z

dynamic/client.py

+        self.client = client
+        self.configuration = client.configuration
+        self._load_server_info()
+        self.__resources = ResourceContainer(self.parse_api_groups())


IIUC people need to create another ResourceContainer with client.parse_api_groups() when they want to re-discover new CRDs and aggregated APIs - self.__resources only records the resources that exist when the dynamic client gets created - is that correct?

You're right, I'll break this stuff out into a discovery() method so that it can be called repeatedly

I'll break this stuff out into a discovery() method so that it can be called repeatedly

This can be done in a separate PR.

roycaihw · 2018-08-17T22:54:33Z

I think this PR will be good to go after @fabianvf adds body to delete requests. @yliaog @mbohlool Any other comments?

This dynamic client should also be e2e tested (something like test_client.py), probably in another PR.

yliaog · 2018-08-17T23:14:48Z

dynamic/client.py

+from functools import partial
+import yaml
+from pprint import pformat
+from kubernetes.client.rest import ApiException


better to have the import above be ordered?

Yeah I can clean that up

yliaog · 2018-08-17T23:55:54Z

dynamic/exceptions.py

+        503: ServiceUnavailableError,
+        504: ServerTimeoutError,
+    }.get(e.status, DynamicApiError)(e, tb)
+


if just want to add traceback, why do you need to add all these specific HTTP errors (BadRequestError, UnauthorizedError, ...)? Is DynamicApiError alone not enough for the purpose?

I prefer to expose errors that correspond to the HTTP errors we receive, it provides a cleaner way to handle errors when consuming the client (since you can just catch a NotFoundError rather than catching a DynamicApiError and checking the status code). Catching a generic DynamicApiError and checking the code still works if the user prefers to handle it that way.

yliaog · 2018-08-18T00:19:40Z

dynamic/client.py

+        header_params['Accept'] = self.client.select_header_accept([
+            'application/json',
+            'application/yaml',
+            'application/vnd.kubernetes.protobuf'


i don't think protobuf is supported in python client, better to remove it here.

ack, good call

fabianvf · 2018-09-05T15:27:17Z

sorry, I've been on PTO + planning meetings the last few weeks, I'll try and carve up some time to clean all this up sometime this week or next

fabianvf · 2018-09-24T16:19:10Z

We're actually doing a little bit of a rework of the dynamic client in openshift, driven by user feedback. I think I might hold off a bit on updating this PR, so that I can bring those improvements in

micw523 · 2019-03-19T05:56:32Z

@fabianvf I see that you're working on a dynamic client for OpenShift. Are there any plans to backport the dynamic client to this library?

fabianvf · 2019-03-19T12:27:18Z

Yes, there's actually almost nothing specific to openshift in the client. There was a lot of churn so I put this PR on hold while we stabilized a bit, now it's mostly a matter of finding the time to bring in the relevant new stuff.

fejta-bot · 2019-06-17T12:51:14Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

micw523 · 2019-06-17T13:36:37Z

/remove-lifecycle stale

roycaihw · 2019-08-06T22:44:01Z

/lgtm
/approve

Thanks a lot for working on this! Will merge after the test failure gets resolved kubernetes-client/python#919

k8s-ci-robot · 2019-08-06T22:44:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabianvf, 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 files:

~~OWNERS~~ [roycaihw]

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

yliaog · 2019-08-07T21:42:44Z

i have a meta request, could you please squash the commits to a few logical commits?

roycaihw · 2019-08-12T18:00:34Z

/lgtm

fabianvf changed the title ~~[WIP] Dynamic Client~~ Dynamic Client May 4, 2018

roycaihw reviewed Jun 8, 2018

View reviewed changes

fabianvf force-pushed the dynamic-client branch from 284318f to c1933bf Compare June 8, 2018 16:59

fabianvf mentioned this pull request Jul 11, 2018

Validate resource definitions openshift/openshift-restclient-python#181

Closed

fabianvf force-pushed the dynamic-client branch from 62565de to c56f569 Compare July 30, 2018 17:29

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

fabianvf force-pushed the dynamic-client branch from c56f569 to 788d114 Compare July 30, 2018 17:40

roycaihw reviewed Aug 13, 2018

View reviewed changes

yliaog reviewed Aug 18, 2018

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 17, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 17, 2019

fabianvf force-pushed the dynamic-client branch from 788d114 to a9c0f20 Compare June 28, 2019 19:42

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 28, 2019

roycaihw mentioned this pull request Jul 16, 2019

Discover api classes kubernetes-client/python#871

Closed

fabianvf force-pushed the dynamic-client branch from 0c54bfc to 6299e29 Compare July 24, 2019 17:46

fabianvf force-pushed the dynamic-client branch from 6299e29 to 61d676c Compare July 24, 2019 17:47

k8s-ci-robot assigned roycaihw Aug 6, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2019

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

fabianvf added 3 commits August 9, 2019 21:38

Add dynamic client

461a3bf

Add tests for dynamic client

5b21d65

Add boilerplate

53c4cb2

fabianvf force-pushed the dynamic-client branch from 61d676c to 53c4cb2 Compare August 10, 2019 01:39

k8s-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2019

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2019

k8s-ci-robot merged commit 4b8e89f into kubernetes-client:master Aug 12, 2019

fabianvf mentioned this pull request Oct 8, 2019

REQUEST: New membership for fabianvf kubernetes/org#1276

Closed

6 tasks

Dynamic Client #56

Dynamic Client #56

Conversation

fabianvf commented Apr 9, 2018 • edited Loading

codecov-io commented Apr 9, 2018 • edited Loading

Codecov Report

fabianvf commented Apr 19, 2018

mbohlool commented May 4, 2018

roycaihw 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

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

fabianvf commented Jul 30, 2018

fabianvf commented Aug 8, 2018

fabianvf commented Aug 8, 2018

roycaihw 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

roycaihw commented Aug 17, 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

fabianvf commented Sep 5, 2018

fabianvf commented Sep 24, 2018

micw523 commented Mar 19, 2019

fabianvf commented Mar 19, 2019

fejta-bot commented Jun 17, 2019

micw523 commented Jun 17, 2019 via email • edited Loading

roycaihw commented Aug 6, 2019

k8s-ci-robot commented Aug 6, 2019

yliaog commented Aug 7, 2019

roycaihw commented Aug 12, 2019

fabianvf commented Apr 9, 2018 •

edited

Loading

codecov-io commented Apr 9, 2018 •

edited

Loading

micw523 commented Jun 17, 2019 via email •

edited

Loading