-
Notifications
You must be signed in to change notification settings - Fork 39.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
make a simple dynamic client that is easy to use #62913
Conversation
url = append(url, name) | ||
|
||
// subresources only work on things with names | ||
if len(c.subresource) > 0 { |
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 case should error, not construct a different URL... if I try to construct a request to delete a pod binding subresource and accidentally use an empty name var, I should error, not delete all pods in the namespace.
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 case should error, not construct a different URL... if I try to construct a request to delete a pod binding subresource and accidentally use an empty name var, I should error, not delete all pods in the namespace.
Fixed in callers with proper error. If someone uses an empty name, I put a panic below as a backstop, but its a programmer error.
fd07417
to
cd8d8d2
Compare
7ee2ac9
to
d6cb8fb
Compare
+1 this is what dynamic client should look like to consumers. |
5d8535a
to
cfead73
Compare
/retest |
note to self, gc sets incorrect namespace for cluster scoped resources here:
|
14b417b
to
c1a3d7a
Compare
/retest |
1 similar comment
/retest |
/retest kops seems to be having difficulty |
d030237
to
3632037
Compare
comments addressed. |
lgtm |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 63137, 62913). If you want to cherry-pick this change to another branch, please follow the instructions here. |
/assign @yliaog |
opts = &metav1.DeleteOptions{} | ||
} | ||
if opts == nil { | ||
opts = &metav1.DeleteOptions{} |
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.
duplicates?
if old ClientPool is dropped, what's the plan to enable sharing the REST clients? |
the previous implementation required constructing a new client per group/version, which led to the need for a client pool. with the implementation in this PR, a single restClient inside a single dynamic client can be used for requests to any Group/Version/Resource, so there's no need for a client pool. |
kubernetes#62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter.
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Bump QPS on namespace controller #62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter. Fixes #63240 ```release-note NONE ```
|
||
func (c *dynamicResourceClient) Create(obj *unstructured.Unstructured) (*unstructured.Unstructured, error) { | ||
if len(c.subresource) > 0 { | ||
return nil, fmt.Errorf("create not supported for subresources") |
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 support subresource creation for pods/binding
, pods/eviction
and some deployments/rollback
.
kubernetes#62913 switched from using a client pool, where each groupVersionResource got its own rest client, to a single client. This increases the QPS to account for increased requests using a single rest client rate limiter.
The dynamic client has annoyed me for the last time! The existing one takes arguments at odd levels, requires lots of information to instantiate, does some weird pool thing, and uses unusual types. This creates an interface like this:
You create it from just a
rest.Config
, no mapper, no path resolving func, no trying to set up codecs ahead of time, no unnecessary pool. It just works.I updated the namespace controller to use it and I updated the existing dynamic client to leverage it so that I get all their tests for "free".
@kubernetes/sig-api-machinery-pr-reviews
@liggitt @smarterclayton @bparees @sttts @ironcladlou I know each of us has struggled with the dynamic client in our time.
@lavalamp @caesarxuchao This is vastly simplifying. I'm eager to drop the old
ClientPool
. client-go will technically have another incompatible semver this release. I'm up for changing it in tree.