Allow MPS control daemonset to be explicitly disabled

[marinoborges]

This change allows the deployment of the MPS daemon to be explicitly disabled by adding --set mps.enabled=false to a Helm install / upgrade command.

The default behaviour of the plugin is to deploy the MPS control daemonset even if not MPS sharing is configured. However, the actual MPS daemon is only started if a GPU is replicated using MPS. The mps.enabled Helm value allows this to be explicitly disabled.

Fixes #1177 with backward compatibility so this should be a minor change.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[ArangoGutierrez]

@elezar / @tariq1890 Thoughts?

[marinoborges]

Unsure if we need also to change NVIDIA_DRIVER_CAPABILITIES env var value when running without MPS.

[chipzoller]

You may also consider renaming this field so its purpose is more clear. Something more explicit like createDaemonset would be an example.

[chipzoller]

As a user, I think at a minimum there needs to be comments placed above this line to explain its use. If I see this in a values file, I may interpret mps.enabled: true to mean I, as a user, want MPS enabled right out of the gate. But that's not what this does. When set to false MPS will never work even if I later activate an MPS configuration by labeling a node.

[marinoborges]

I added a comment clarifying this

[sidewinder12s]

@chipzoller can you re-review this?

[chipzoller]

The comment itself looks fine, but this will need to be reviewed by a maintainer (I am not one).

[github-actions]

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 30 days unless new comments are made or the stale label is removed.

[elezar]

Sorry for the delay here @marinoborges. I think the opt-out makes sense. I did have one question though:

What if a user is upgrading the device plugin and as such this value is not defined in the set that has already been applied? Does the default value of true function as expected here, or should we check for non-nil values explicitly?

[marinoborges]

@elezar if a user is upgrading device plugin via helm chart upgrade, the default value of true will be applied so no undesired changes are expected.
Also, if a user is upgrading device plugin via image value, then the helm chart change i'm proposing isn't taken into consideration so again no undesired changes are expected.