-
Notifications
You must be signed in to change notification settings - Fork 39.9k
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
Plugin watcher getting notified of every Kubelet directory event #69015
Comments
/sig node |
@vladimirvivien I think you should only place the plugin socket file in the plugin registration directory but not the plugin data. |
agree with @tossmilestone, we can only place the registration file in the specified path instead of placing all data into it. On the other hand, you can place data into the /var/lib/kubelet/plugins/registration even if registration created, that's a no-end loop. |
@tossmilestone
And several more locations. The fix would be to have plugin watcher scan a subdirectory of plugins (say The watcher functions as intended. Except now it picks up all directory activities whether related or not to plugin registration. |
Well given the fact that the pluginwatcher is beta this is a bit of an issue. My understanding of the "beta" guarantees is that we want to be backwards compatible for all plugins published with the beta API. Meaning that even if we move the registration directory to a new location, we'll need to keep scanning the old directory (to pick up v1beta plugins). From a 10 000 feet point of view it seems easier to me if we move the in-tree volume plugin's data. Though it might be a lot of work. Another less painful option, but a bit grey is the following: Enforce plugins to end with What do you think @vladimirvivien ? |
@RenaudWasTaken I like the |
Hmm, I've just deployed a new k8s cluster on 1.12.0 and noticed this repeating in the logs on a few of our pods. Just for my own sanity, is this something I should be worried about or can I ignore it until a fix is released?
|
@cpressland you can ignore these errors. Another thing to keep in mind, it seems like CSI plugins puts a lot of data (and creates a lot of directories). It might be worth considering limiting the recursion to one level. I'm concerned we might exhaust the amount of fds by tracking the plugins data. |
@RenaudWasTaken yeah CSI keeps track of volume operation metadata in the plugins dir. This probably will not change. I understand this is beta. However, besides CSI and Device plugin, are there any other implementers out there for this feature ? Would it be possible to propose a new location (i.e. /reg/? |
@RenaudWasTaken let me know if you open a PR for this as discussed. |
@vladimirvivien thanks for open the issue with detail logs, i can see two problems by looking at logs and code.
I added a suggestion to @RenaudWasTaken fix of this issue #70494
plugin watcher still has activity when any file create/delete under the directory, but they become light weighted (CREATE file will only trigger file mode check, DELETE file event will trigger plugin searching) i am not sure we want to go further to limit directory depth where plugin can live.
|
These are definitely good suggestions!
It seems like there might be some confusion as to why we are thinking of doing this:
By limiting the search of the plugins to one directory, we would be severely limiting the ability of the module to unintentionally harm the overall Kubelet. It seems actually pretty easy to enforce, as it's a simple In terms of flexibility, is there a real need to create a plugin in a lower directory? Is there a plugin that has so many sockets that a single directory underneath the plugin directory isn't enough? Naively it seems to me that |
ping @figo on the above :) |
@RenaudWasTaken i can not speak from plugin developer, but both below looks reasonable to me |
@figo @RenaudWasTaken I like |
Is the suggestion to hard code the number of levels that the recursion algorithm will search to 3? Or just expect a socket only at a hard coded level (e.g. socket must be nested 3 deep)? Copying my comment from #70810 (comment) for consistency:
Another option I can think of:
Thoughts? |
i like this idea better if it is always the case that: volume data is mounted, we can modify the watcher code to not watch the mounted directory in the first place. |
Skipping mounted dir along with restructuring search path to known (shallow) dir sounds like a good plan. Though with the new suggested paths, sockets will be placed at a location where mounts should not exist. |
I think that would be a safe assumption. The plugin directory is only used as a target path for global mounts. |
Ephemeral volume types may not need to mount, it could just be a symlink or plain directory. Can we assume any ephemeral volume CSI driver won't implement global mount? |
Yes, in-tree ephemeral volumes today don't do a global mount so the mounts directory is For CSI the Maybe let's start with just checking for mount and ignoring mount directories. If it is still an issue we can consider adding a configurable black list of paths. Thoughts? |
That sounds good. Also, putting the socket files in a specific location that does not clash with mounts will also help (i.e. |
/milestone v1.13 |
I just want to point out that one of the issue we have with recursively watching all directories is that it will significantly increase the amount of fd consumed. Given that plugins actually store data there and in a dynamic way my fear is that it will exhaust the available fds for no real good reasons. Limiting the depth sounds like a reasonable solution to that issue. |
btw, because of this verbosity |
We can do both. Don't search mounted dirs, and max depth of search. Who is working on this? |
@gree-gorey That's pretty bad, can you open another issue to track that. |
@RenaudWasTaken how confident are we in getting this fixed by Code freeze tomorrow? 5pm PST |
I expect things to wind up today, changes in the above PR aren't very contentious and don't involve significant changes |
Temporal workaround for kubernetes/kubernetes#69015
Temporal workaround for kubernetes/kubernetes#69015 (cherry picked from commit 6d32980)
Temporal workaround for kubernetes/kubernetes#69015
Temporal workaround for kubernetes/kubernetes#69015 (cherry picked from commit b6d1341)
Is this a BUG REPORT or FEATURE REQUEST?:
/kind bug
This is more of an enhancement.
What happened:
The Plugin Watcher is setup to trigger on file activities when they occur in the subdirectory that it scans. During startup, the Watcher is passed (from within the Kubelet) the Kubelet's root plugin directory as a location to scan for newly added plugins -
kubernetes/pkg/kubelet/kubelet.go
Line 789 in 8346631
Because the kubelet's plugins subdirectory is also used by existing in-tree drivers (including csi) as a location to store plugins artifacts, the Plugin Watcher is getting triggered by directory/file activities occurring in that subdirectory that has nothing to do with plugin registration as shown in the output below:
What you expected to happen:
The Plugin Watcher should not be getting triggered by file/directory activities not related to plugin registration/unregistration. One possibility is to have the Watcher scan one directory below plugins like
/var/lib/kubelet/plugins/registration
or something similarHow to reproduce it (as minimally and precisely as possible):
Run plugin watcher with a CSI driver.
cc @RenaudWasTaken @vikaschoudhary16
The text was updated successfully, but these errors were encountered: