Move logic to run / check status of IMEX daemons into go binary #379

klueska · 2025-05-27T21:54:52Z

This PR is actually only the top-commit. The other commit is from #376 and will go away once this is merged.

The motivation for this is as follows:

We want to simplify the code in the helm chart to not rely on bash scripts
We want to add extra logic into this binary to eventually handle IMEX node IPs as hostname
We want to use this binary to start surfacing error conditions / metrics from running the IMEX daemon to the ComuteDomain object.

These new features will be added as follow-up PRs.

copy-pr-bot · 2025-05-27T21:54:55Z

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.

jgehrcke · 2025-05-29T12:30:21Z

cmd/compute-domain-daemon/main.go

+		signal.Notify(sigChan, syscall.SIGTERM)
+		go func() {
+			<-sigChan
+			cancel()


I like to emit a log message like "got SIGTERM, initiating shutdown" or something like that, to see how snappy and coordinated the shutdown procedure is

I'm fine with that. Will add

jgehrcke · 2025-05-29T12:30:50Z

cmd/compute-domain-daemon/main.go

+		return f(ctx)
+	}
+
+	// Create the app


Is that an AI code comment? :-) haha (sorry, not judging)

No, its the logical progression of comments from whats before and whats after (if all you did was read the comments to understand at a high level what this function is doing as you walk through it.

jgehrcke · 2025-05-29T12:31:46Z

cmd/compute-domain-daemon/main.go

+			},
+			{
+				Name:  "check",
+				Usage: "Check if the node is IMEX capable and if the IMEX daemon is ready",


opinion on "IMEX capable" vs. "IMEX-capable"?

jgehrcke · 2025-05-29T12:33:43Z

cmd/compute-domain-daemon/main.go

+	if !capable {
+		fmt.Println("ClusterUUID and CliqueId are NOT set for GPUs on this node.")
+		fmt.Println("The IMEX daemon will not be started.")
+		fmt.Println("Sleeping forever...")


should we emit these details within checkIMEXCapable(), closer to the code where we check for these things?

here, we could then log a neutral "not IMEX-capable"

To me it makes more sense to do it at the top level rather than buried in a helper function since this is info for the end user.

I was thinking about maintainability because the relationship between !capable and "ClusterUUID and CliqueId are NOT set" is defined elsewhere and might change (and we might forget changing the user output then).

(this is nit-level and I am OK with whatever you choose)

jgehrcke · 2025-05-29T12:35:17Z

cmd/compute-domain-daemon/nvml.go

+// It logs any errors that occur during shutdown but does not return them,
+// as this is typically called in a defer statement.
+func (l deviceLib) alwaysShutdown() {
+	ret := l.nvmllib.Shutdown()


I wonder what's in this library that needs to be shut down. Does it implement some kind of server or event handler / queue?

I don't know what all the underlying NVML library does, but (at a minimum) it drops the handle tit holds o the GPU driver (allowing e.g. a driver kernel module to be removed).

jgehrcke · 2025-05-29T12:46:42Z

cmd/compute-domain-daemon/main.go

+	cmd := exec.CommandContext(ctx, imexBinary, "-c", config)
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	return cmd.Run()


I forgot some details of what we talked about. But now when we wrap the IMEX daemon process like this, let's review some config settings. Maybe:

LOG_FILE_NAME="" -- according to docs, if empty, this emits to stderr

DAEMONIZE=0 -- maybe that's even required now to properly shut it down?

We currently also still have IMEX_WAIT_FOR_QUORUM=RECOVERY

(https://github.com/NVIDIA/k8s-dra-driver-gpu/blob/9732ac32c1e77dc9b08cf741830b5fb3529787d4/templates/compute-domain-daemon-config.tmpl.cfg#L106C1-L106C30)

Whereas I think in our conversations we explored setting this to

NONE: Do not wait for any quorum with other nodes.

Arguably, I have not fully understood RECOVERY yet -- docs start with "In case of unsafe IMEX termination", so this is probably about a previous crash. Probably RECOVERY is the same as NONE when there was no previous crash ('unsafe IMEX termination').

This is important stuff to consider, but I don't want to do that in this PR -- this is just about migrating what we had previously to a go binary instead of a bash script.

klueska · 2025-05-29T12:59:53Z

cmd/compute-domain-daemon/main.go

+// tail continuously reads and prints new lines from the specified file using the system's tail command.
+// It starts from the beginning of the file (-n +1) and follows new lines (-f).
+// It blocks until the context is cancelled or an error occurs.
+func tail(ctx context.Context, path string) error {
+	cmd := exec.CommandContext(ctx, "tail", "-n", "+1", "-f", path)
+	cmd.Stdout = os.Stdout
+	cmd.Stderr = os.Stderr
+	return cmd.Run()
+}


Instead of direct stdout / stderr, maybe I should intercept each line of output so that we can log it with klog and keep it consistent with other log messages being emitted by this program.

I kind of like having a tight process wrapper where we make the child process cleanly inherit parent's stdout/stderr, and make the child (well, the imex daemon) emit its log to stderr.

But I also like what you propose, we then have consistent timestamps and given the right needle one can isolate the daemon's output with grep.

We probably will iterate on this later anyway.

jgehrcke

As I'm going to be away for a bit: feel free to land this when it feels good to you.

Signed-off-by: Kevin Klues <[email protected]>

klueska · 2025-06-01T17:05:29Z

I'm going to land this as-is, as I have a number of follow-up PRs that build on this.

klueska requested a review from jgehrcke May 27, 2025 21:56

klueska force-pushed the compute-domain-daemon-cmd branch 4 times, most recently from 5309fbf to 4b5bc74 Compare May 27, 2025 23:21

jgehrcke reviewed May 29, 2025

View reviewed changes

klueska force-pushed the compute-domain-daemon-cmd branch from 4b5bc74 to 134e75e Compare May 29, 2025 12:55

klueska commented May 29, 2025

View reviewed changes

jgehrcke approved these changes May 29, 2025

View reviewed changes

Add envvar for CLIQUE_ID to be passed into compute-domain-daemon

024f379

Signed-off-by: Kevin Klues <[email protected]>

klueska force-pushed the compute-domain-daemon-cmd branch from 134e75e to 5382099 Compare May 29, 2025 21:39

Move logic to run / check status of IMEX daemons into go binary

eb75a1c

Signed-off-by: Kevin Klues <[email protected]>

klueska force-pushed the compute-domain-daemon-cmd branch from 5382099 to eb75a1c Compare May 29, 2025 22:05

klueska merged commit 7ded33d into NVIDIA:main Jun 1, 2025
7 checks passed

klueska added this to Planning Board: k8s-dra-driver-gpu Jun 16, 2025

github-project-automation bot moved this to Backlog in Planning Board: k8s-dra-driver-gpu Jun 16, 2025

jgehrcke mentioned this pull request Jul 3, 2025

Fix COPYing files during container image build #418

Merged

klueska added this to the v25.3.0 milestone Aug 13, 2025

klueska deleted the compute-domain-daemon-cmd branch August 20, 2025 21:26

Move logic to run / check status of IMEX daemons into go binary #379

Move logic to run / check status of IMEX daemons into go binary #379

Uh oh!

Conversation

klueska commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copy-pr-bot bot commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgehrcke left a comment

Choose a reason for hiding this comment

Uh oh!

klueska commented Jun 1, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

klueska commented May 27, 2025 •

edited

Loading

jgehrcke May 29, 2025 •

edited

Loading

jgehrcke May 29, 2025 •

edited

Loading