gpu: attach to an already-running process, and keep its modules (#124)

cmd/gpu-cuda-profile/main_test.go

@@ -0,0 +1,183 @@
+package main
+
+import (
+	"flag"
+	"io"
+	"os"
+	"strings"
+	"testing"
+	"time"
+)
+
+// The guard this whole classification exists for.
+//
+// Attach mode's contract is that a flag which cannot take effect is refused
+// rather than ignored, and that contract is only as good as the coverage of
+// the table behind it. A flag added to defineFlags and classified nowhere
+// would fall through refusedLaunchFlags and be accepted in attach mode while
+// doing nothing at all -- which is precisely the failure the refusal exists
+// to prevent, reintroduced by omission rather than by decision.
+//
+// So the test is over the REGISTERED set, not over a list written here: it
+// enumerates what defineFlags actually installed and fails until every name
+// has been put on one side or the other. Adding a flag without thinking about
+// attach mode is not possible; adding one and deciding it is attach-safe
+// takes one line.
+func TestEveryFlagIsClassifiedForAttachMode(t *testing.T) {
+	fs := flag.NewFlagSet("gpu-cuda-profile", flag.ContinueOnError)
+	defineFlags(fs)
+
+	var unclassified, both []string
+	registered := map[string]bool{}
+	fs.VisitAll(func(f *flag.Flag) {
+		registered[f.Name] = true
+		_, launch := launchOnlyInAttachMode[f.Name]
+		safe := attachSafeFlags[f.Name]
+		switch {
+		case launch && safe:
+			both = append(both, f.Name)
+		case !launch && !safe:
+			unclassified = append(unclassified, f.Name)
+		}
+	})
+	if len(unclassified) > 0 {
+		t.Errorf("these flags are registered but classified neither launch-only nor "+
+			"attach-safe, so -pid would accept them and they would do nothing: %s",
+			strings.Join(unclassified, ", "))
+	}
+	if len(both) > 0 {
+		t.Errorf("these flags are in both tables, which cannot be true of any flag: %s",
+			strings.Join(both, ", "))
+	}
+	// The tables must not outlive the flags they describe either: a name left
+	// behind after a flag is removed makes the classification look more
+	// complete than it is.
+	for name := range launchOnlyInAttachMode {
+		if !registered[name] {
+			t.Errorf("launchOnlyInAttachMode names %q, which defineFlags does not register", name)
+		}
+	}
+	for name := range attachSafeFlags {
+		if !registered[name] {
+			t.Errorf("attachSafeFlags names %q, which defineFlags does not register", name)
+		}
+	}
+}
+
+// A reason that does not say why is a refusal the operator cannot act on.
+func TestEveryRefusalExplainsItself(t *testing.T) {
+	for name, why := range launchOnlyInAttachMode {
+		if len(why) < 20 {
+			t.Errorf("-%s is refused with %q, which does not tell the operator what to do "+
+				"instead", name, why)
+		}
+	}
+}
+
+func TestOnlyTheFlagsActuallySetAreRefused(t *testing.T) {
+	fs := flag.NewFlagSet("t", flag.ContinueOnError)
+	fs.SetOutput(io.Discard)
+	defineFlags(fs)
+	if err := fs.Parse([]string{"-pid", "1234", "-period", "4", "-out", "x.pb.gz"}); err != nil {
+		t.Fatal(err)
+	}
+	got := refusedLaunchFlags(fs)
+	if len(got) != 1 || !strings.HasPrefix(got[0], "-period:") {
+		t.Fatalf("want exactly -period refused, got %v", got)
+	}
+	// -iters and -linger-ms are launch-only and have non-zero DEFAULTS. If
+	// refusal keyed on the value rather than on the flag having been set,
+	// every attach run would be refused for flags nobody typed.
+	for _, r := range got {
+		if strings.HasPrefix(r, "-iters") || strings.HasPrefix(r, "-linger-ms") {
+			t.Fatalf("a launch-only flag was refused on its default value: %q", r)
+		}
+	}
+}
+
+// waitForShimIn must distinguish "I looked and it is not there" from "I could
+// not look", because they send the reader to completely different places: the
+// first to how the target was started, the second to their own command line
+// or to their capabilities. A single generic failure would send everyone to
+// the driver.
+func TestWaitForShimInSeparatesNotMappedFromCannotLook(t *testing.T) {
+	t.Run("a process that maps the file", func(t *testing.T) {
+		// Whatever libc this test binary is linked against is mapped into
+		// this very process by definition, so this is the positive case with
+		// no GPU, no shim and no capabilities involved.
+		libc := findMappedLibrary(t)
+		if err := waitForShimIn(os.Getpid(), libc, time.Second); err != nil {
+			t.Fatalf("self maps %s but waitForShimIn says: %v", libc, err)
+		}
+	})
+
+	t.Run("a process that does not map the file", func(t *testing.T) {
+		err := waitForShimIn(os.Getpid(), "/bin/true", 50*time.Millisecond)
+		if err == nil {
+			t.Fatal("want an error: this process does not map /bin/true")
+		}
+		// The message must name the mechanism, or the operator has no way to
+		// know that restarting the target is the fix and re-running this
+		// command is not.
+		if !strings.Contains(err.Error(), "cuInit") {
+			t.Errorf("the refusal does not explain that injection happens at cuInit "+
+				"and cannot be added later: %v", err)
+		}
+	})
+
+	t.Run("a pid that cannot be read", func(t *testing.T) {
+		err := waitForShimIn(1<<21, "/bin/true", 50*time.Millisecond)
+		if err == nil {
+			t.Fatal("want an error for a pid that does not exist")
+		}
+		if !strings.Contains(err.Error(), "could not read") {
+			t.Errorf("a pid that cannot be inspected is reported as though it had been "+
+				"inspected and found wanting: %v", err)
+		}
+	})
+}
+
+func findMappedLibrary(t *testing.T) string {
+	t.Helper()
+	maps, err := os.ReadFile("/proc/self/maps")
+	if err != nil {
+		t.Skipf("no /proc/self/maps: %v", err)
+	}
+	for _, line := range strings.Split(string(maps), "\n") {
+		i := strings.Index(line, " /")
+		if i < 0 {
+			continue
+		}
+		path := strings.TrimSpace(line[i+1:])
+		if strings.Contains(path, ".so") && !strings.Contains(path, "(deleted)") {
+			return path
+		}
+	}
+	t.Skip("this binary maps no shared library to test against")
+	return ""
+}
+
+// The launch cache's counters are cumulative for the life of the run --
+// Snapshot drains the timeline's rings but not the cache -- so an eviction
+// total is re-reported, larger, in every subsequent snapshot. Collapsing a
+// line to its kind is what stops one ongoing condition from being announced
+// as a fresh anomaly on every drain interval.
+func TestAnOngoingConditionIsOneAnomalyAndNotOnePerInterval(t *testing.T) {
+	kind := func(s string) string { return anomalyDigits.ReplaceAllString(s, "#") }
+
+	// The exact lines a 20s attach produced on the 3090, four intervals apart.
+	a := "gpu join ANOMALY: launch cache evicted 10198 launches at capacity (65536 live) — too small for the launch rate"
+	b := "gpu join ANOMALY: launch cache evicted 45034 launches at capacity (65536 live) — too small for the launch rate"
+	if kind(a) != kind(b) {
+		t.Errorf("the same growing condition reads as two different anomalies:\n  %q\n  %q",
+			kind(a), kind(b))
+	}
+
+	// And it must not over-collapse: two genuinely different findings that
+	// happen to differ only in their numbers are still different findings,
+	// but two different SENTENCES must stay apart.
+	c := "gpu join ANOMALY: 353 of 65536 executions unmatched — GPU time arrived with no launch"
+	if kind(a) == kind(c) {
+		t.Errorf("two unrelated anomalies collapsed to one kind: %q", kind(a))
+	}
+}

docs/gpu-injection.md

@@ -146,13 +146,57 @@ because the `perf_uprobe` PMU path requires `CAP_SYS_ADMIN` and that is what
 gets a per-pod agent rejected by admission policy. It costs a **Linux 6.6**
 floor.
 
+## Profiling a process that is already running
+
+A sidecar or a node collector cannot launch what it profiles: the kubelet
+started it. `-pid` attaches to a process that is already running.
+
+```
+gpu-cuda-profile -pid 12345 -duration 60s -out gpu.pb.gz
+```
+
+The target must have been started with `CUDA_INJECTION64_PATH` already in its
+environment — the driver loads the shim during `cuInit` and never again, and
+nothing (not `-pid`, not `ptrace`) can add it to a live process afterwards.
+That is why the init container sets the variable on the *application* pod
+rather than on the profiler. If the shim is not in the target's mappings the
+command refuses at startup and says so, rather than collecting nothing for a
+full `-duration`.
+
+Three things behave differently from a launched run, and all three are
+consequences of not being the parent:
+
+- **Flags that configure the target are refused, not ignored.** `-period` and
+  `-gpu-pc-sampling` are read by the adapter out of the target's environment at
+  `cuInit`. Accepting them here would change nothing while making the profile
+  look as though it had been taken at a rate it was not.
+- **The run ends when the target exits.** A sampled launch's stack is
+  symbolized against `/proc/<pid>/maps`, which the kernel destroys the instant
+  the process leaves; anything collected past that point has stacks that can
+  never be resolved.
+- **The timeline is drained on an interval** (`-drain-every`, default 2s)
+  rather than once at the end. Its rings hold one interval, not a whole run —
+  a 20 s attach to a process issuing ~5,500 launches/s overruns a run-length
+  ring and loses GPU time outright.
+
+Capabilities are the same set as a launched run; no `privileged`, no
+`CAP_SYS_ADMIN`. A node collector additionally needs `hostPID: true` to see
+the pids it is attaching to, which is a genuine privilege increase over the
+sidecar and should be stated in a pod spec rather than absorbed quietly.
+
 ## What this does not yet cover
 
 - **No published container image** for the init-container pattern. CI uploads
   the portable shim as a build artifact; there is no registry image to name in
   a pod spec yet.
-- **The agent cannot attach to an already-running process** on the GPU path:
-  `cmd/gpu-cuda-profile` launches the workload itself. A Kubernetes sidecar has
-  to attach to a container the kubelet started, so the sidecar shape in
-  `examples/kubernetes/` cannot run end to end until that exists. Tracked in
-  issue #124.
+- **Retention costs the target memory.** The shim captures every module's
+  bytes whether or not a consumer has attached, because the driver offers them
+  exactly once and there is no way to ask again — so a process that is never
+  profiled can hold up to 512 modules or 64 MiB (the consumer's own module
+  store bounds) on a profiler's behalf. Past those bounds captures are dropped
+  and counted; nothing grows without limit. `module_retained_unattached` in the
+  adapter's exit report is what it is currently holding.
+- **The collector (DaemonSet) shape is not built yet** — `-pid` is its
+  prerequisite, not the whole of it. One profile per pod versus one profile
+  labelled by pod, attaching to pods that start later, and whether the shim is
+  one inode per node or one per pod are all open. Tracked in issue #124.

shim/core/cubinqueue.cc

@@ -138,9 +138,11 @@ size_t CubinQueue::drain(CubinOfferFn offer, unsigned timeout_ms) {
         } else {
             // Broader than cubin.cc's cubins_send_failed(), on purpose: that
             // counter excludes "nobody was listening", because an unprofiled
-            // process must not accumulate failures. Here the queue only ever
-            // holds bytes when a consumer was believed present, so a
-            // no-listener result IS a module the consumer will not have.
+            // process must not accumulate failures. Here the caller only
+            // drains when it believes a consumer is present -- the adapter
+            // retains rather than drains while unattached, precisely so an
+            // offer is never made into the void -- so a no-listener result IS
+            // a module the consumer will not have.
             send_failed_.fetch_add(1, std::memory_order_relaxed);
         }
     }

shim/core/cubinqueue.h

@@ -116,7 +116,22 @@ typedef CubinOfferResult (*CubinOfferFn)(const void *bytes, size_t len, uint64_t
 // Config.CubinMaxBytes default so a cubin this end copies is not one the
 // other end was always going to refuse.
 struct CubinQueueLimits {
-    size_t max_entries = 32;                     // queued offers
+    // Sized for RETENTION, not for a 100 ms hop.
+    //
+    // These bounds used to describe a queue that was drained every drain
+    // tick, so 32 entries was generous: nothing could accumulate. Since the
+    // adapter retains captures while no consumer is attached (a late attach
+    // would otherwise find every module already gone -- measured 100% loss),
+    // the queue holds a process's modules for as long as nobody has arrived,
+    // and 32 is not a process's worth of modules.
+    //
+    // 512 and 64 MiB are the CONSUMER's ModuleStore defaults, taken
+    // deliberately rather than invented: there is no value in retaining more
+    // modules than the store on the other end can hold, and no reason to
+    // retain fewer. max_queued_bytes is the real bound -- it is the memory an
+    // opted-in process can be made to hold -- and max_entries is the count
+    // guard beside it.
+    size_t max_entries = 512;                    // queued offers
     size_t max_queued_bytes = 64u * 1024 * 1024; // their total
     size_t max_cubin_bytes = 8u * 1024 * 1024;   // one module, the memcpy bound
     size_t max_interned = 4096;                  // distinct CRCs remembered

shim/core/cubinqueue_test.cc

@@ -397,6 +397,52 @@ void test_destruction_releases_undrained_copies() {
     printf("  destroying a non-empty queue releases its copies\n");
 }
 
+// Retention: a queue nobody drains must HOLD, and hold the bytes intact.
+//
+// This is the property the adapter's late-attach fix rests on. It retains
+// captures while no consumer is attached and drains only once one arrives,
+// because drain() pops and frees whatever it takes regardless of whether the
+// offer landed -- so draining into a socket nobody is listening on is
+// indistinguishable from deleting the modules, which is exactly how a late
+// attach came to find zero of them.
+//
+// The failure this guards against is a plausible "optimisation": making the
+// queue self-trim, or drop its oldest entry on a timer, on the reasoning that
+// a queue is a transient thing. It is not one any more.
+void test_an_undrained_queue_retains_everything_intact() {
+    reset_offers();
+    reset_captured();
+    CubinQueue q;
+    const char *payloads[] = {"module-alpha", "module-bravo", "module-charlie"};
+    for (int i = 0; i < 3; i++) {
+        const CubinView v(payloads[i], strlen(payloads[i]));
+        assert(q.capture(v, fnv1a, nullptr, nullptr));
+    }
+    // Many ticks' worth of doing nothing. Nothing may age out on its own:
+    // there is no deadline in this policy, only the bounds.
+    assert(q.depth() == 3);
+    assert(q.modules_captured() == 3);
+    assert(q.cubin_queue_full() == 0);
+    assert(g_offers.empty());
+
+    // And when a consumer finally arrives, every one of them is offered --
+    // with its BYTES, not just its CRC. A retention scheme that kept the
+    // entries but lost the contents would satisfy every counter above and
+    // still deliver nothing a consumer could parse.
+    assert(q.drain(record_offer, 0) == 3);
+    assert(g_offers.size() == 3);
+    for (int i = 0; i < 3; i++) assert(g_offers[i].bytes == payloads[i]);
+    assert(q.depth() == 0);
+    printf("  an undrained queue retains every capture, bytes intact\n");
+}
+
+void test_the_retention_bounds_are_the_consumers_store_bounds() {
+    const CubinQueueLimits d;
+    assert(d.max_entries == 512);                     // gpu.ModuleStoreConfig.Capacity
+    assert(d.max_queued_bytes == 64u * 1024 * 1024);  // gpu.ModuleStoreConfig.MaxBytes
+    printf("  the retention bounds are the consumer's module-store bounds\n");
+}
+
 }  // namespace
 
 int main() {
@@ -413,6 +459,8 @@ int main() {
     test_a_slow_offer_does_not_block_a_capture();
     test_a_healthy_run_reads_zero_on_every_drop_counter();
     test_destruction_releases_undrained_copies();
+    test_an_undrained_queue_retains_everything_intact();
+    test_the_retention_bounds_are_the_consumers_store_bounds();
     printf("cubinqueue_test: OK\n");
     return 0;
 }