Deep Review: 20260414-214710-pr-309

Date2026-04-14 21:47
Reporancher-sandbox/rancher-desktop-daemon
Round2 (of PR #309)
Author@jandubois
PR#309 — engine: mirror Docker container engine state into Kubernetes
Commits626c789 engine: mirror Docker container engine state into Kubernetes
ReviewersClaude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro
VerdictMerge with fixes — one BATS style fix and a controller-grouping decision
Wall-clock time19 min 21 s

Executive Summary

This PR adds the engine controller that mirrors Docker containers, images, and volumes into Kubernetes resources, plus supporting plumbing in rdd set (wait-for-desired-state with ObservedGeneration gating), a discovery ready annotation, and multi-writer safety on App status. The code is well-structured, thoroughly documented, and has strong integration test coverage. Findings are concentrated around one BATS style violation and a controller-grouping design question; the remaining items are minor robustness and ergonomics suggestions.

Structure: 0 critical, 2 important, 5 suggestions, plus design observations and a testing assessment.

Critical Issues

None.

Important Issues

I1. BATS file-scope command substitution for DOCKER_HOST Claude Codex Gemini
bats/tests/32-app-controllers/engine.bats
# containers, images, and volumes into Container, Image, and Volume
# resources, and that deletions and spec.state changes are forwarded
# to Docker.

NAMESPACE="rancher-desktop"
DOCKER_HOST="unix://$(rdd svc paths docker_socket)"
export DOCKER_HOST

local_setup_file() {
    # The Docker socket access pattern used by these tests is not yet wired
    # up for Windows/WSL2.
    skip_on_windows

The repo's BATS style rules forbid $(command ...) substitutions: when the inner command fails, BATS produces opaque errors instead of the command's output. This assignment runs at file-load time, outside the run/$output machinery, so a failure there aborts the whole suite with no diagnostic. In practice rdd svc paths docker_socket is a pure path computation that does not call into the service, so it is unlikely to fail — but the pattern still violates the project's standing rule and sets a bad example for later tests.

Fix: move the assignment into local_setup_file and use the canonical pattern.

 NAMESPACE="rancher-desktop"
-DOCKER_HOST="unix://$(rdd svc paths docker_socket)"
-export DOCKER_HOST

 local_setup_file() {
     skip_on_windows
+    run -0 rdd svc paths docker_socket
+    DOCKER_HOST="unix://${output}"
+    export DOCKER_HOST
     rdd svc delete
     rdd set running=true
 }
I2. Engine controller is excluded from --controllers=app Codex
pkg/controllers/app/engine/controller.go
	"github.com/rancher-sandbox/rancher-desktop-daemon/pkg/controllers/base"
)

func init() {
	// Windows lacks a Docker socket transport, so the reconciler would
	// hot-loop on connect errors. Skip registration until WSL2 support lands.
	if runtime.GOOS == "windows" {
		return
	}
	base.RegisterController(newController())
}

const (
	controllerName = "engine"
	apiGroup       = "app"
)

type controller struct{}

func newController() base.Controller {
	return &controller{}
}

var _ base.Controller = &controller{}

func (c *controller) GetName() string {
	return controllerName
}

The embedded control plane's controller selector in pkg/service/cmd/service.go:494-554 enables a controller only when the --controllers spec matches the controller's GetAPIGroup() or GetName(). Every other controller under pkg/controllers/app/ (app, demo) reports APIGroup = "app", but this new one uses "engine". As a direct consequence rdd svc start --controllers=app does not bring up the engine controller: ContainerEngineReady is never written and no mirror resources appear. That is why bats/tests/32-app-controllers/app.bats had to widen its selector from "app,limavm" to "*" in this PR.

pkg/service/cmd/service.go
	}
	return nil
}

// shouldEnableController determines if a controller should be enabled based on the controller's specification.
func shouldEnableController(controller base.Controller, spec string) bool {
	if controller.GetAPIGroup() == "builtin" {
		return true
	}
	// Empty spec: only builtin controllers are enabled
	if spec == "" {
		return false
	}

	controllerName := controller.GetName()
	apiGroup := controller.GetAPIGroup()

	var included bool
	var excluded bool

	parts := strings.Split(spec, ",")
	for _, part := range parts {
		part = strings.TrimSpace(part)
		if part == "" {
			continue
		}

		isExclusion := strings.HasPrefix(part, "-")
		if isExclusion {
			part = strings.TrimPrefix(part, "-")
		}

		// Check for wildcard
		if part == "*" {
			if isExclusion {
				excluded = true
			} else {
				included = true
			}
			continue
		}

		// Check if it matches the API group
		if part == apiGroup {
			if isExclusion {
				excluded = true
			} else {
				included = true
			}
			continue
		}

		// Check if it matches the specific controller
		if part == controllerName {
			if isExclusion {
				excluded = true
			} else {
				included = true
			}
			continue
		}
	}

	// Return true if included and not excluded
	return included && !excluded
}

// NewServeCommand creates a *cobra.Command object with default parameters.
func NewServeCommand(ctx context.Context) *cobra.Command {
	s := options.NewOptions(ctx, instance.Dir())

The --controllers help text (pkg/service/controllers/options.go:32) advertises app as an API group, so a user who narrows to it will hit this gap. This is a design choice, not a strict regression (the engine controller is new), but the current grouping makes selective starts silently incomplete. Codex additionally claims the standalone cmd/app-controller binary is affected — that part is incorrect: external.RunControllers at pkg/external/main.go:62 registers every controller returned by base.GetAllControllers() without filtering by API group, so the standalone binary does pick up engine.

pkg/service/controllers/options.go
func (o *Options) AddFlags(fs *pflag.FlagSet) {
	if o == nil {
		return
	}

	fs.StringVar(&o.Controllers, "controllers", "*", "Controllers to enable. Use '*' for all, or specify comma-separated list. API groups: 'rdd' (configmapreplicaset, notary), 'app' (demo). Prefix with '-' to exclude, e.g., '*,-demo'")
}

// Complete returns the completed configuration.
func (o *Options) Complete() CompletedOptions {
	return CompletedOptions{
pkg/external/main.go
	}

	// Check which controllers should start and filter the list
	var controllersToStart []base.Controller

	for _, controller := range base.GetAllControllers() {
		shouldStart, err := shouldStartController(ctx, config, controller.GetName(), setupLog)
		if err != nil {
			setupLog.Error(err, "Failed to check for running controller, starting anyway", "controller", controller.GetName())
			controllersToStart = append(controllersToStart, controller)
		} else if shouldStart {

Fix options (in order of preference):

  1. Change apiGroup to "app" so selective --controllers=app starts match the grouping convention used by app and demo.
  2. If engine needs to stay separately selectable, document that users must pass --controllers=app,engine and update the options.go help text to list engine as a known group.
  3. Add a dependency/alias mechanism on base.Controller so --controllers=app can pull engine in transitively.

Suggestions

S1. waitForDesiredState treats any non-"true" running value as "false" Claude
cmd/rdd/set.go
//
// TODO: changes that trigger a VM restart without changing "running"
// (for example, containerEngine.name alone) are still not waited on.
// A dedicated "Reconciled" condition on App would let the CLI detect
// when the full reconcile chain has settled for any property change.
func waitForDesiredState(ctx context.Context, config *rest.Config, properties map[string]string, timeout time.Duration, minGen int64) error {
	runningVal, ok := properties["running"]
	if !ok {
		return nil
	}

	ctx, cancel := context.WithTimeout(ctx, timeout)
	defer cancel()

	if runningVal == "true" {
		logrus.Info("Waiting for container engine to be ready")
		return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
			status, gen, present := conditionInfo(obj, "ContainerEngineReady")
			return present && gen >= minGen && status == metav1.ConditionTrue
		})
	}
	logrus.Info("Waiting for the VM to stop")
	return watchCondition(ctx, config, func(obj *unstructured.Unstructured) bool {
		// Absent Running means the VM was never started, so there's
		// nothing to wait for. Otherwise wait until the condition is
		// refreshed with our generation and reports a non-True status.
		status, gen, present := conditionInfo(obj, "Running")
		if !present {
			return true
		}
		return gen >= minGen && status != metav1.ConditionTrue
	})
}

// watchCondition watches the App singleton until the predicate
// returns true or the context expires. watchtools.UntilWithSync
// re-lists transparently on benign watch-channel closures (API
// timeouts, proxy disconnects, resource-version compaction).

Any value that is not "true" falls through to the "VM stop" wait path. The CRD enum at coerceValue (line 688) already constrains running to a boolean, so this is defensive territory rather than a reachable bug — but it contradicts the explicit intent in the surrounding code, and if coerceValue is ever bypassed or the schema widened the silent fall-through could mask user error.

cmd/rdd/set.go
			return nil, fmt.Errorf("valid values: %s", strings.Join(validValues, ", "))
		}
	}

	switch schema.Type {
	case "boolean":
		v, err := strconv.ParseBool(raw)
		if err != nil {
			return nil, errors.New("expected boolean; use \"true\" or \"false\"")
		}
		return v, nil

Fix:

 if runningVal == "true" {
     // wait for ContainerEngineReady
     return watchCondition(...)
 }
+if runningVal != "false" {
+    return nil
+}
 logrus.Info("Waiting for the VM to stop")
S2. syncAllContainers logs per-item Inspect errors but does not collect them Claude
pkg/controllers/app/engine/controllers/sync_containers.go
	// Log and skip per-item Inspect failures rather than failing the
	// whole startup: a single permanently-broken Inspect must not
	// prevent every other container from being mirrored, nor pin
	// ContainerEngineReady at ConnectFailed. Structural errors below
	// (K8s list, stale-mirror cleanup) are still fatal.
	var errs []error
	for _, dc := range listResult.Items {
		dockerIDs[dc.ID] = true
		if err := w.syncContainer(ctx, dc.ID); err != nil {
			log.Error(err, "Skipping container during full sync", "id", dc.ID)
		}
	}

	// Remove stale Container mirrors.
	var containerMirrors containersv1alpha1.ContainerList
	if err := w.k8s.List(ctx, &containerMirrors, client.InNamespace(apiNamespace)); err != nil {
		return fmt.Errorf("failed to list Containers: %w", err)

Per-item Inspect failures are logged but never appended to errs, while the stale-mirror-removal loop below does append. The asymmetry is deliberate (the comment at lines 49-53 explains why startup should not fail on one bad Inspect), but a reader expects parallel structure. The same asymmetry exists in syncAllImages and syncAllVolumes. Consider a one-line comment at the log call in each helper stating "not collected — see block comment above" so future readers do not "fix" it.

pkg/controllers/app/engine/controllers/sync_containers.go
	}

	// Track which Docker container IDs exist so stale mirrors can be pruned.
	dockerIDs := make(map[string]bool, len(listResult.Items))

	// Log and skip per-item Inspect failures rather than failing the
	// whole startup: a single permanently-broken Inspect must not
	// prevent every other container from being mirrored, nor pin
	// ContainerEngineReady at ConnectFailed. Structural errors below
	// (K8s list, stale-mirror cleanup) are still fatal.
	var errs []error
	for _, dc := range listResult.Items {
		dockerIDs[dc.ID] = true
		if err := w.syncContainer(ctx, dc.ID); err != nil {
			log.Error(err, "Skipping container during full sync", "id", dc.ID)
S3. fullSync continues after syncContainerNamespace failure Claude
pkg/controllers/app/engine/controllers/docker_watcher.go
	}
	return err
}

// fullSync lists all containers, images, and volumes from Docker and
// creates corresponding mirror resources. It also removes stale mirrors.
func (w *dockerWatcher) fullSync(ctx context.Context) error {
	log := logf.FromContext(ctx).WithName("docker-watcher")
	log.Info("Starting full sync")

	if err := w.ensureNamespace(ctx); err != nil {
		return fmt.Errorf("failed to ensure namespace: %w", err)
	}

	var errs []error

	if err := w.syncContainerNamespace(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync container namespace: %w", err))
	}
	if err := w.syncAllContainers(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync containers: %w", err))
	}
	if err := w.syncAllImages(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync images: %w", err))
	}
	if err := w.syncAllVolumes(ctx); err != nil {
		errs = append(errs, fmt.Errorf("failed to sync volumes: %w", err))
	}

	log.Info("Full sync complete")
	return errors.Join(errs...)
}

ensureNamespace returns early on failure (creating the K8s namespace is a prerequisite for everything that follows), but syncContainerNamespace failure is collected and execution continues. If writing the ContainerNamespace/moby object fails, the subsequent syncAllContainers/syncAllImages/syncAllVolumes calls will still run and may produce mirrors whose status.namespace points at an object that does not yet exist. Returning early on this failure (or collecting the error and skipping the dependent syncs) would produce cleaner failure semantics.

S4. Custom hasFinalizer/removeFinalizer helpers duplicate controllerutil Claude
pkg/controllers/app/engine/controllers/engine_reconciler.go
		Watches(&containersv1alpha1.Container{}, enqueueApp).
		Watches(&containersv1alpha1.Image{}, enqueueApp).
		Watches(&containersv1alpha1.Volume{}, enqueueApp).
		Complete(r)
}

sigs.k8s.io/controller-runtime/pkg/controller/controllerutil already provides ContainsFinalizer and RemoveFinalizer with identical semantics. The only friction is that controllerutil.RemoveFinalizer does not return a bool, but len(finalizers) != len(obj.GetFinalizers()) is a trivial post-check. Using the upstream helpers would match the pattern used elsewhere in the codebase and reduce surface area to maintain.

S5. parseContainerName produces non-"moby" status.namespace on the dockerd backend Codex
pkg/controllers/app/engine/controllers/sync_containers.go
}

// parseContainerName splits a Docker container name into namespace
// and name. Docker names start with "/" and may carry a namespace
// prefix like "/namespace/name".
func parseContainerName(fullName string) (namespace, name string) {
	fullName = strings.TrimPrefix(fullName, "/")
	namespace, name, found := strings.Cut(fullName, "/")
	if !found {
		return containerNamespace, namespace
	}
	return namespace, name
}

// mapDockerContainerState maps a free-form Docker State.Status string
// to the CRD enum. Unrecognised values fall through to
// ContainerStatusUnknown so a new Docker state string (added in a
// minor Docker release) does not fail SSA validation and silently

The unit test at sync_containers_test.go:25-29 explicitly expects /k8s.io/magical_gates to return ("k8s.io", "magical_gates"). The engine controller currently only runs when the backend is moby, and docs/design/api_containers.md:33-35 states unambiguously:

pkg/controllers/app/engine/controllers/sync_containers_test.go
		{
			input:         "plain",
			wantNamespace: containerNamespace,
			wantName:      "plain",
		},
		{
			input:         "/k8s.io/magical_gates",
			wantNamespace: "k8s.io",
			wantName:      "magical_gates",
		},
		{
			input:         "/ns/name/with/slashes",
			wantNamespace: "ns",
			wantName:      "name/with/slashes",
		},
docs/design/api_containers.md
the engine controller's role. The code uses the same terminology: the
finalizer is `engine.rancherdesktop.io/docker-mirror`, the cleanup
helper is `cleanupMirrorResources`, and the name helper is
`volumeMirrorName`.

When running `containerd`, the containerd namespace is listed as the `namespace`
label rather than re-using the Kubernetes namespace.  When running `dockerd`,
namespaces are not supported and we always use `moby` as the value for that label.

For the `*Request` resources, they use the `Complete` and `Failed` conditions to
express state; those are mutually exclusive (only one of the two can be set to
`True` at once).  Once either is set to `True`, the request object is considered
to be in a terminal state and will be removed after some timeout.  This will be

> When running dockerd, namespaces are not supported and we always use moby as the value for that label.

Standard Docker-CLI container names are single-segment, so in practice the found branch is dead code on dockerd — but if anything ever produces a multi-segment name, the resulting status.namespace would reference a ContainerNamespace object that this controller never creates (it only creates ContainerNamespace/moby in syncContainerNamespace). The unit test bakes the inconsistency in as intended behavior, which suggests the function is being shared with a future containerd implementation. Either: (a) hardcode status.namespace = "moby" in applyContainer for now and move the multi-segment parsing into the containerd code path when it lands, or (b) leave the parsing and drop the k8s.io test case so the function's dockerd behavior matches the design doc.

Design Observations

Strengths

Concerns

Testing Assessment

The new bats/tests/32-app-controllers/engine.bats suite (409 lines) exercises the end-to-end story thoroughly:

  1. Image pull / untag / dangling-image promotion lifecycle, including the reconcileImageByID path for events that carry only the image ID.
  2. Container create / stop / rm mirroring and bidirectional delete (K8s-side delete → Docker, Docker-side destroy → K8s).
  3. Volume create / rm, including names with uppercase and underscores that must be hashed into RFC-1123 subdomains.
  4. spec.state transitions across running, paused, created, and unknown, including the paused-plus-stop and paused-plus-unpause cases that cover the Docker state matrix at docker_watcher.go:320-367.
  5. In-use-image finalizer retention (processImageFinalizers keeps the finalizer until Docker permits the removal).
  6. Cleanup-on-VM-stop and containerd backend NotApplicable handling.
  7. rdd set wait semantics: --timeout=0 bypass, idempotent running=false on an already-stopped VM, restart-and-recreate-moby after running=true.

The pkg/service/controllers/discovery_test.go addition covers the full lifecycle of InitDiscovery → MarkControlPlaneReady → re-InitDiscovery clearing stale annotations. sync_containers_test.go unit-tests parseContainerName (though see S5 regarding the k8s.io cases).

Untested areas worth tracking (by decreasing risk):

  1. Watcher reconnect after a transient Docker disconnect. The goroutine-death → enqueueReconcile → new newDockerWatcher → fullSync path is only exercised implicitly. A test that kills Docker mid-stream and verifies mirror consistency would validate the most complex lifecycle branch.
  2. Selective --controllers startup paths (ties into I2). A test that runs rdd svc start --controllers=app and verifies ContainerEngineReady appears would have caught the grouping gap.
  3. mapDockerContainerState fall-through for an unrecognised Docker status string. A trivial unit test would confirm the SSA-safe unknown fallback.

Documentation Assessment

The design-doc updates are thorough and consistent with the implementation:

The one inconsistency is the S5 tension between api_containers.md's "dockerd always uses moby" statement and parseContainerName's ability to return other namespaces on dockerd input. Update whichever side you choose to reconcile.

Commit Structure

Reviewed, no issues. The PR is a single commit, which is appropriate for a feature whose parts (engine controller, rdd set wait, discovery readiness, spec.state=unknown, App multi-writer safety) are tightly coupled.

Acknowledged Limitations

  1. Transient handleEvent failures are dropped pending a periodic full resync. — docker_watcher.go:178-184

> "image pull and volume create events fire once, so a dropped apply leaves the mirror missing until the next full resync. A periodic fullSync tick is deferred until the rest of the mirroring work lands."

This change makes the limitation newly observable (image and volume events are newly mirrored), but the comment correctly describes it.

  1. reconcileContainerSpecs and processFinalizers are O(N) per reconcile. — engine_reconciler.go:180-184

> "The long-term fix is to split these into per-resource reconcilers with watch predicates."

Acknowledged; see the Design Observations concern.

  1. Changes that trigger a VM restart without changing running are not waited on. — cmd/rdd/set.go:460-464

> "for example, containerEngine.name alone — a dedicated 'Reconciled' condition on App would let the CLI detect when the full reconcile chain has settled for any property change."

Matches the existing TODO tracked in memory.

  1. Orphan window for Image mirrors with empty status.id. — sync_images.go:239-243

> "if applyImage wrote the metadata Apply but the status SubResource Apply failed transiently, the mirror has an empty status.id and this function will miss it. syncAllImages sweeps the orphan on the next watcher restart."

Narrow and self-healing via full sync on reconnect.

  1. Windows/WSL2 Docker socket access is not wired up. — engine.bats:17-19, PR summary.

The new BATS suite is skipped on Windows until WSL2 socket plumbing lands.

  1. containerEngine.name=containerd takes no mirroring action. — engine_reconciler.go:137-146

Reports ContainerEngineReady=True with reason NotApplicable so rdd set running=true containerEngine.name=containerd finishes waiting. The condition will be renamed when containerd mirroring lands.

Agent Performance Retro

[Claude] — Claude Opus 4.6

Claude delivered the most thorough file-by-file walkthrough and provided solid verification of subtle concerns (the stale NestedInt64 decoding path, the alreadyClean partial-cleanup self-healing, the running=false no-op generation flow). Those traces are genuinely useful context even when they did not produce findings. The downside of this approach is that the review document contains several "draft" findings Claude retracted in-place before writing the final version — the reader has to skip past the annotated dead-ends to find the actual conclusions. The surviving findings are all real but on the minor side; the one important item (BATS style) was also caught by the other two agents, and the waitForDesiredState fall-through was defensive territory that I downgraded to a suggestion.

[Codex] — Codex GPT 5.4

Codex was the only agent to identify the --controllers=app grouping gap (I2) and the parseContainerName/design-doc tension (S5), and both are substantive. Calibration was mixed: Codex also filed the apiNamespace = "rancher-desktop" constant as important, which is a direct misread of the design doc (rancher-desktop is the documented K8s namespace for mirror resources, not a stand-in for App.spec.namespace), and its I1 framing of cmd/app-controller being affected was factually wrong because the standalone binary registers every controller via base.GetAllControllers(). The signal/noise ratio was still better than any single-issue report — two out of three important claims landed as real findings.

[Gemini] — Gemini 3.1 Pro

Gemini produced a compact, high-precision review: one finding (the BATS style issue), a clean Coverage Summary with every file accounted for, and a tight set of design strengths. No false positives, no misreads, but also no unique findings: everything it raised was caught by at least one other agent. Gemini continues to skip git blame (daily quota), and that is acceptable given Claude and Codex cover regression attribution.

Summary

Claude Opus 4.6Codex GPT 5.4Gemini 3.1 Pro
Duration7m 58s9m 51s6m 30s
Findings1I 4S2I 1S1I
Tool calls49 (Read 28, Bash 11, Grep 9)91 (shell 87, stdin 3, plan 1)15 (runshellcommand 9, grepsearch 4, writefile 2)
Design observations634
False positives010
Unique insights420
Files reviewed333333
Coverage misses000
Totals1I 4S2I 1S1I
Downgraded1 (I→S)1 (I→S)0
Dropped010

All three agents surfaced the same important issue (I1). Codex contributed both of the unique insights that survived consolidation, at the cost of one false positive and one factually-wrong framing. Claude's coverage was the broadest but its contributions clustered in minor/suggestion territory after the retracted drafts were stripped out. Gemini was the most reliable per-word but had no unique finds.

Reconciliation. Codex P1 apiNamespace constant (important → rejected): rancher-desktop is the documented K8s namespace for mirror resources per docs/design/api_containers.md, not a stand-in for App.spec.namespace. Codex P1 parseContainerName (important → S5 suggestion): the "wrong" branch is effectively unreachable for normal Docker container names and the unit test bakes in the current behavior, making this a latent design question rather than an active bug. Codex P1 BATS $(...) (suggestion → I1 important): promoted to match Claude and Gemini's rating, reflecting the repo's standing rule against BATS command substitution. Claude P1 waitForDesiredState fall-through (important → S1 suggestion): unreachable given CRD enum validation in coerceValue, kept as a defensive suggestion.

Review Process Notes

Repo context updates