Add syncronization between the MPS control daemon and the device plugin #606

elezar · 2024-03-19T13:51:43Z

This change adds an init-container to the device plugin to ensure that the MPS daemon with the correct config has been started before the device plugin starts.

This init container logs:

I0319 13:48:40.749279      28 wait.go:76] Starting OS watcher.
E0319 13:48:40.749531      28 wait.go:87] "MPS is not ready; retrying ..." err="mps manager is not ready: failed to load .ready config: failed to open .ready file: open /mps/.ready: no such file or directory"
I0319 13:49:10.871775      28 wait.go:84] MPS is ready

Signed-off-by: Evan Lezar <[email protected]>

klueska · 2024-04-15T11:27:00Z

cmd/mps-control-daemon/mps/ready.go

+	ReadyFilePath = "/mps/.ready"
+)
+
+// ReadyFile represents a file used to store readyness of the MPS daemon.


Suggested change

// ReadyFile represents a file used to store readyness of the MPS daemon.

// ReadyFile represents a file used to store readiness of the MPS daemon.

klueska · 2024-04-15T11:27:54Z

cmd/mps-control-daemon/mps/ready.go

+// Matches checks whether the contents of the ready file matches the specified config.
+func (f ReadyFile) Matches(config *spec.Config) (bool, error) {


Maybe use a different verb than Matches to future proof this (since we don't plan on doing an exact match of the full config file in the future).

klueska · 2024-04-15T11:28:22Z

cmd/mps-control-daemon/mps/ready.go

+	return nil
+}
+
+// Load loads the contents of th read file.


Suggested change

// Load loads the contents of th read file.

// Load loads the contents of the ready file.

klueska · 2024-04-15T11:29:46Z

cmd/mps-control-daemon/wait/wait.go

+
+// NewCommand constructs a mount command.
+func NewCommand() *cli.Command {
+	// Create the 'generate-cdi' command


Suggested change

// Create the 'generate-cdi' command

// Create the 'wait' command

klueska · 2024-04-15T11:31:54Z

cmd/mps-control-daemon/wait/wait.go

+	if err != nil {
+		return nil, fmt.Errorf("unable to validate flags: %v", err)
+	}
+	config.Flags.GFD = nil


Is it worth a comment why we nil out the GFD config?

klueska · 2024-04-15T11:33:35Z

cmd/mps-control-daemon/main.go

+	readyFile := mps.ReadyFile{}
+	if err := readyFile.Remove(); err != nil {
+		klog.Warningf("Failed to remove .ready file: %v", err)
+	}


What if MPS is turned off in the config? Will a pevious ready file just be sitting around until MPS get reenabled at some point in the future?

Yes, with the current implementation. What options do we have to ensure that this is removed if the MPS daemonset is removed?

klueska · 2024-04-15T11:38:48Z

cmd/mps-control-daemon/wait/wait.go

+	klog.Info("Starting OS watcher.")
+	sigs := watch.Signals(syscall.SIGHUP, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT)
+
+	var retry <-chan time.Time
+
+restart:
+	err = assertMpsIsReady(config)
+	if err == nil {
+		klog.Infof("MPS is ready")
+		return nil
+	}
+	klog.ErrorS(err, "MPS is not ready; retrying ...")
+	retry = time.After(30 * time.Second)
+
+	for {
+		select {
+		case <-retry:
+			goto restart
+			// Watch for any signals from the OS. On SIGHUP, restart this loop,
+			// restarting all of the plugins in the process. On all other
+			// signals, exit the loop and exit the program.
+		case s := <-sigs:
+			switch s {
+			case syscall.SIGHUP:
+				klog.Info("Received SIGHUP, restarting.")
+				goto restart
+			default:
+				klog.Infof("Received signal \"%v\", shutting down.", s)
+				goto exit
+			}
+		}
+
+	}
+exit:
+	return nil


I'm not sure any of this restart logic is necessary. In fact, I'm not sure if this command should be called wait but rather just assert and then fail if it cannot assert that the MPS daemon is up and running. Since the intention is to run this as an init container, I actually don't want it to loop and instead just keep failing until it asserts that the daemon is up and running.

klueska · 2024-04-15T11:43:28Z

cmd/nvidia-device-plugin/main.go

+			if event.Name == mps.ReadyFilePath {
+				if config == nil || config.Sharing.SharingStrategy() != spec.SharingStrategyMPS {
+					klog.InfoS("Ignoring event", "event", event)
+					continue
+				}
+				switch {
+				case event.Op&fsnotify.Remove == fsnotify.Remove:
+					klog.Infof("%s removed; restarting", mps.ReadyFilePath)
+					goto restart
+				case event.Op&(fsnotify.Create|fsnotify.Write) != 0:
+					klog.Infof("%s created or modified; checking config", mps.ReadyFilePath)
+					readyFile := mps.ReadyFile{}
+
+					matches, err := readyFile.Matches(config)
+					if err != nil {
+						klog.ErrorS(err, "failed to check .ready file")
+						goto restart
+					}
+					if !matches {
+						klog.Info("mismatched MPS sharing config; restarting.")
+						goto restart
+					}
+					continue
+				}
+			}


I haven't read through this in detail, but at first glance it seems a bit convoluted. Can't we rely on the logic already in-place that handles config updates and hook into that somehow?

It may be possible to just rely on the config change restart logic. Let's make the following assumptions:

The MPS daemonset owns the lifecycle of the .ready file and this file contains the current config used to start the MPS control daemon.

If the config changes and both the MPS daemon and the device plugin are restared by their respective config managers, we should be able to fail plugin startup until: 1. A .ready file exists with the expected config, 2. The MPS pipe is ready from the perspective of the device plugin.

This should not require the additional error / watcher events here.

Note that since the config-manager sidecars are not always deployed, this seems to imply that there are configuration changes that we may miss in this case. For example, how do we capture the restart of the MPS daemon or some other configuration update that doesn't use the config manager? Do we expect the user to manually restart both pods in this case to ensure that we're in sync?

github-actions · 2024-07-15T04:27:41Z

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.

frittentheke · 2024-07-15T12:36:50Z

@elezar I suppose you don't want this change here to stale out?

elezar · 2024-08-12T20:17:19Z

@frittentheke you're right, I need to revisit this.

github-actions · 2024-11-12T04:27:46Z

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.

frittentheke · 2024-11-14T18:09:05Z

Please keep this alive @elezar

elezar added 8 commits March 19, 2024 14:17

Add content to mps/.ready file

eb5a22d

Signed-off-by: Evan Lezar <[email protected]>

Add watcher for /mps/.ready

7a9d9b7

Signed-off-by: Evan Lezar <[email protected]>

Only restart on config change

07e3cc5

Signed-off-by: Evan Lezar <[email protected]>

Add basic readyness sync

c60cc01

Signed-off-by: Evan Lezar <[email protected]>

Fix available configs

6a007a8

Signed-off-by: Evan Lezar <[email protected]>

Use app and command flags

925efbe

Signed-off-by: Evan Lezar <[email protected]>

fixup

66d5940

Signed-off-by: Evan Lezar <[email protected]>

Only load MPS config

ef73e17

Signed-off-by: Evan Lezar <[email protected]>

elezar requested review from klueska and cdesiniotis March 19, 2024 13:51

elezar self-assigned this Mar 19, 2024

fix imports

92719c3

Signed-off-by: Evan Lezar <[email protected]>

klueska reviewed Apr 15, 2024

View reviewed changes

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 15, 2024

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 16, 2024

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 12, 2024

github-actions bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add syncronization between the MPS control daemon and the device plugin #606

Add syncronization between the MPS control daemon and the device plugin #606

elezar commented Mar 19, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

elezar Aug 13, 2024

klueska Apr 15, 2024

klueska Apr 15, 2024

elezar Aug 13, 2024

github-actions bot commented Jul 15, 2024

frittentheke commented Jul 15, 2024

elezar commented Aug 12, 2024

github-actions bot commented Nov 12, 2024

frittentheke commented Nov 14, 2024 •

edited

Loading

	// ReadyFile represents a file used to store readyness of the MPS daemon.
	// ReadyFile represents a file used to store readiness of the MPS daemon.

		// Matches checks whether the contents of the ready file matches the specified config.
		func (f ReadyFile) Matches(config *spec.Config) (bool, error) {

	// Load loads the contents of th read file.
	// Load loads the contents of the ready file.

	// Create the 'generate-cdi' command
	// Create the 'wait' command

Add syncronization between the MPS control daemon and the device plugin #606

Are you sure you want to change the base?

Add syncronization between the MPS control daemon and the device plugin #606

Conversation

elezar commented Mar 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 15, 2024

frittentheke commented Jul 15, 2024

elezar commented Aug 12, 2024

github-actions bot commented Nov 12, 2024

frittentheke commented Nov 14, 2024 • edited Loading

frittentheke commented Nov 14, 2024 •

edited

Loading