Is there any documentation that explains that DRA plugin is a part of Kubelet and DRA driver is an external component to which DRA plugin is connecting?

The terminology used elsewhere is that a DRA driver "provides (or deploys) a kubelet plugin".

I don't think we say that "DRA plugin is a part of Kubelet" or "DRA driver is an external component to which DRA plugin is connecting".

"Kubelet plugin" sounds like an external entity to me. I think it should be clearly explained somewhere. CSI has similar terminology, do they have an explanation somewhere?

I am fine with / actually prefer the term plugin here. The driver is the whole thing -- this is the kubelet plugin part of that.

What this function does is "give me some plugin for this driver name, I don't care which one". In that sense, "driver name" is more appropriate than "plugin name" because the name is not identifying a specific plugin.

Update: this wasn't about the parameter. As @klueska said, what gets returned here is the local proxy for one plugin, which is represented by one *Plugin instance here.

This sounds very confusing to me. It sounds like DRA driver doesn't register itself, but registers a plugin. Can you explain or refrase?

A DRA driver is a higher-level concept. It does register one or more plugins. A driver is identified by its name. A plugin is identified by that same name and the endpoint.

I don't think we we should say that a driver registers itself, because what is "it" in that case?

That sounds logical to me, thanks for the explanations. It still contradicts with Device Plugins terminology and (may be) with the CSI one though.

this explanation / comment matches my mental model and sounds good to me.

is it version or service?

It's the version of the API which defines the service.

Would it make sense to use service in the error message to match the variable name or rename the API to validateSupportedVersions?

The error message must match what users know about e.g. from the KEP, which is "gRPC versions". We cannot rename the method because it is part of the plugin registration handler interface.

Again, this sounds confusing, at least to me. DeRegisterPlugin doesn't deregister DRA driver, but a plugin with the driver name.

I can change this particular parameter to "pluginName" and then map it to "driverName" elsewhere in the code, with the explanation that DRA drivers must register themselves with their driver name.

Would that be better?

I'm not sure it will be better. After your explanation above we need to decide if we want to stick with the concept of "driver registers one or more plugins" or we want to keep using the terms that were used before for other types of plugins.

Registering multiple endpoints under the same plugin name (using terminology of the registration API here) is a new thing. We cannot rely on other types of plugins for guidance and have to come up with some consistent terminology ourselves.

That makes sense to me now, thanks for the explanations!

:+1

This seems to also panic on line instances := s.store[pluginName]

This was added to simplify mocking in the unit tests, but seems to not used. I agree, it can be removed, unless we come up with unit tests that want to mock Manager, e.g. if they don't want to run gRPC server.

+1

This is now embedded in the Manager type -- great -- I have never liked that this was a global variable.

it feels awkward to have this take an already initialized (but empty) *Store object which it then caches locally and populates. Does it make sense to have this return (*RegistrationHandler, *Store) so that the component that actually populates this object owns this object and the one that consumes it is the one that caches a reference to it.

The way I see it, RegistrationHandler is one layer above the central store. It would be possible to use a store without a registration handler, or with some other way of registering endpoints. With that in mind, constructing the store separately seems consistent.

If this feels like overkill, then let's eliminate the RegistrationHandler. The store can implement cache.PluginHandler directly.

I do not particularly care about RegistrationHandler being separate. I kept it because it was that way before and only clarified the layering.

I like the idea of the store implementing cache.PluginHandler directly

This is easier to implement on top of b92cf11 because that commit already moves some of the code from the RegistrationHandler into the Store.

I've pulled that commit into this PR and completed the removal of RegistrationHandler. Definitely the right choice, the end result is less complex.

@klueska

I like the idea of the store implementing cache.PluginHandler directly

Do you also like an idea of the store implementing plugin connection management?

Well at this point, I'd just argue that maybe store isn't the right name for this abstraction, but rather something like PluginManager.

Sounds okay to me. You can never have too many managers... okay, perhaps you can.

I did the rename in a separate commit because then the file renaming is handled better in a diff because the files are similar enough: dec98e8

There are still some concerns with the approach taken here to introduce an overall DRAPluginMananger abstraction as a replacement for the separate PluginStore and PluginRegistration abstractions: #132058 (comment)

However, I find merging them into a single entity both logical and elegant, since it now puts all Plugin management in a single place and removes the need for cross-component locking to ensure specific operations happen atomically.

As such, I am inclined to approve this PR as-is.

We can always revisit this and separate them again in the future if we find a good reason to do so (e.g. testing becomes too convoluted).