-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fix dynamic client watch of named resource #2076
Fix dynamic client watch of named resource #2076
Conversation
Signed-off-by: Bob Haddleton <bob.haddleton@nokia.com>
Welcome @bobh66! |
@@ -195,10 +195,13 @@ def watch(self, resource, namespace=None, name=None, label_selector=None, field_ | |||
""" | |||
if not watcher: watcher = watch.Watch() | |||
|
|||
# Use field selector to query for named instance so the watch parameter is handled properly. | |||
if name: | |||
field_selector = f"metadata.name={name}" |
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.
If in addition to name
, the user also specified field_selector
when calling watch()
, shall we also persist the user-specified value instead of overwriting it?
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.
I thought about that, but according to this: https://kubernetes.io/docs/concepts/overview/working-with-objects/field-selectors/#chained-selectors multiple field selectors are and-ed together, and since name
is unique including additional field selectors didn't seem to make any sense.
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.
Sorry for the super late response. That makes sense. Thanks!
Thanks for sending the fix! I have one question. Otherwise LGTM |
Any news on this? I tested the proposed solution and works like a charm! |
@kubealex - if it helps this fix is in the kubernetes_asyncio package DynamicClient watch implementation. I found the issue when I was porting DynamicClient to kubernetes_asyncio, and this PR is the backport of that fix: |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
@roycaihw did you have any more concerns? Thanks |
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.
/lgtm
/approve
@@ -195,10 +195,13 @@ def watch(self, resource, namespace=None, name=None, label_selector=None, field_ | |||
""" | |||
if not watcher: watcher = watch.Watch() | |||
|
|||
# Use field selector to query for named instance so the watch parameter is handled properly. | |||
if name: | |||
field_selector = f"metadata.name={name}" |
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.
Sorry for the super late response. That makes sense. Thanks!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bobh66, 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:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
This PR fixes a problem the DynamicClient
watch
method which has aname
parameter to specify a specific resource to watch. This causes KeyError exceptions in watch.unmarshal_event because the API returns the named resource and ignores thewatch
parameter. The change converts thename
parameter tofieldSelector="metadata.name=foo"
before calling thewatch.stream()
method.Which issue(s) this PR fixes:
Fixes #1484
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: