From 9c73155e177c935932f68664a8b053276877b765 Mon Sep 17 00:00:00 2001 From: Thales Maciel Date: Tue, 21 Apr 2026 15:59:09 -0300 Subject: [PATCH] daemon split (7/n): narrow capability interfaces, wire deps at construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stop passing *Daemon into capability hooks. Each capability implementation is now a struct with explicit service-pointer fields populated at wireServices time; the six dynamic-dispatch interfaces (AddStartPreflight, PrepareHost, PostStart, Cleanup, ApplyConfigChange, AddDoctorChecks) no longer have a *Daemon parameter. Capability methods reach their dependencies through struct fields, not through d.vm / d.ws / d.net. - workDiskCapability carries {vm, ws, store, defaultImageName} - dnsCapability carries {net} - natCapability carries {vm, net, logger} Daemon.defaultCapabilities() builds the production list from the already-constructed services and is called from wireServices so d.vmCaps is populated eagerly. Tests that preinstall d.vmCaps with stubs still work — wireServices only overwrites an empty slice. registeredCapabilities() is gone (every dispatch loop now reads d.vmCaps directly). capabilities_test.go's testCapability fake drops *Daemon from its method set to match the new interfaces. This finishes the daemon service split: capability implementations no longer reach through the composition root, there's no path back to *Daemon from any service or capability, and test construction goes through one explicit wireServices call instead of lazy getters. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/daemon/capabilities.go | 169 +++++++++++++++++---------- internal/daemon/capabilities_test.go | 44 +++---- internal/daemon/daemon.go | 3 + internal/daemon/vm_service.go | 17 +-- 4 files changed, 143 insertions(+), 90 deletions(-) diff --git a/internal/daemon/capabilities.go b/internal/daemon/capabilities.go index 59f104d..319df39 100644 --- a/internal/daemon/capabilities.go +++ b/internal/daemon/capabilities.go @@ -3,6 +3,7 @@ package daemon import ( "context" "errors" + "log/slog" "net" "os" "strings" @@ -10,16 +11,23 @@ import ( "banger/internal/firecracker" "banger/internal/guestconfig" "banger/internal/model" + "banger/internal/store" "banger/internal/system" "banger/internal/vmdns" ) +// vmCapability is the base capability tag. Actual behaviour lives on +// optional sub-interfaces (startPreflight / guestConfig / machineConfig +// / prepareHost / postStart / cleanup / configChange / doctor); a +// capability implements whichever subset it cares about. None of them +// take *Daemon — each capability is a struct constructed with its +// explicit service-pointer dependencies at wireServices time. type vmCapability interface { Name() string } type startPreflightCapability interface { - AddStartPreflight(context.Context, *Daemon, *system.Preflight, model.VMRecord, model.Image) + AddStartPreflight(context.Context, *system.Preflight, model.VMRecord, model.Image) } type guestConfigCapability interface { @@ -31,46 +39,48 @@ type machineConfigCapability interface { } type prepareHostCapability interface { - PrepareHost(context.Context, *Daemon, *model.VMRecord, model.Image) error + PrepareHost(context.Context, *model.VMRecord, model.Image) error } type postStartCapability interface { - PostStart(context.Context, *Daemon, model.VMRecord, model.Image) error + PostStart(context.Context, model.VMRecord, model.Image) error } type cleanupCapability interface { - Cleanup(context.Context, *Daemon, model.VMRecord) error + Cleanup(context.Context, model.VMRecord) error } type configChangeCapability interface { - ApplyConfigChange(context.Context, *Daemon, model.VMRecord, model.VMRecord) error + ApplyConfigChange(context.Context, model.VMRecord, model.VMRecord) error } type doctorCapability interface { - AddDoctorChecks(context.Context, *Daemon, *system.Report) + AddDoctorChecks(context.Context, *system.Report) } -func (d *Daemon) registeredCapabilities() []vmCapability { - if len(d.vmCaps) > 0 { - return d.vmCaps - } +// defaultCapabilities builds the production capability list from +// already-constructed services. Called from wireServices once d.vm / +// d.ws / d.net are populated, so every capability ships with the +// concrete service pointers it needs and none of them reach through +// *Daemon at dispatch time. +func (d *Daemon) defaultCapabilities() []vmCapability { return []vmCapability{ - workDiskCapability{}, - dnsCapability{}, - natCapability{}, + newWorkDiskCapability(d.vm, d.ws, d.store, d.config.DefaultImageName), + newDNSCapability(d.net), + newNATCapability(d.vm, d.net, d.logger), } } func (d *Daemon) addCapabilityStartPrereqs(ctx context.Context, checks *system.Preflight, vm model.VMRecord, image model.Image) { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { if hook, ok := capability.(startPreflightCapability); ok { - hook.AddStartPreflight(ctx, d, checks, vm, image) + hook.AddStartPreflight(ctx, checks, vm, image) } } } func (d *Daemon) contributeGuestConfig(builder *guestconfig.Builder, vm model.VMRecord, image model.Image) { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { if hook, ok := capability.(guestConfigCapability); ok { hook.ContributeGuest(builder, vm, image) } @@ -78,7 +88,7 @@ func (d *Daemon) contributeGuestConfig(builder *guestconfig.Builder, vm model.VM } func (d *Daemon) contributeMachineConfig(cfg *firecracker.MachineConfig, vm model.VMRecord, image model.Image) { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { if hook, ok := capability.(machineConfigCapability); ok { hook.ContributeMachine(cfg, vm, image) } @@ -86,13 +96,13 @@ func (d *Daemon) contributeMachineConfig(cfg *firecracker.MachineConfig, vm mode } func (d *Daemon) prepareCapabilityHosts(ctx context.Context, vm *model.VMRecord, image model.Image) error { - prepared := make([]vmCapability, 0, len(d.registeredCapabilities())) - for _, capability := range d.registeredCapabilities() { + prepared := make([]vmCapability, 0, len(d.vmCaps)) + for _, capability := range d.vmCaps { hook, ok := capability.(prepareHostCapability) if !ok { continue } - if err := hook.PrepareHost(ctx, d, vm, image); err != nil { + if err := hook.PrepareHost(ctx, vm, image); err != nil { d.cleanupPreparedCapabilities(context.Background(), vm, prepared) return err } @@ -102,7 +112,7 @@ func (d *Daemon) prepareCapabilityHosts(ctx context.Context, vm *model.VMRecord, } func (d *Daemon) postStartCapabilities(ctx context.Context, vm model.VMRecord, image model.Image) error { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { switch capability.Name() { case "dns": vmCreateStage(ctx, "apply_dns", "publishing vm dns record") @@ -112,7 +122,7 @@ func (d *Daemon) postStartCapabilities(ctx context.Context, vm model.VMRecord, i } } if hook, ok := capability.(postStartCapability); ok { - if err := hook.PostStart(ctx, d, vm, image); err != nil { + if err := hook.PostStart(ctx, vm, image); err != nil { return err } } @@ -121,7 +131,7 @@ func (d *Daemon) postStartCapabilities(ctx context.Context, vm model.VMRecord, i } func (d *Daemon) cleanupCapabilityState(ctx context.Context, vm model.VMRecord) error { - return d.cleanupPreparedCapabilities(ctx, &vm, d.registeredCapabilities()) + return d.cleanupPreparedCapabilities(ctx, &vm, d.vmCaps) } func (d *Daemon) cleanupPreparedCapabilities(ctx context.Context, vm *model.VMRecord, capabilities []vmCapability) error { @@ -131,15 +141,15 @@ func (d *Daemon) cleanupPreparedCapabilities(ctx context.Context, vm *model.VMRe if !ok { continue } - err = joinErr(err, hook.Cleanup(ctx, d, *vm)) + err = joinErr(err, hook.Cleanup(ctx, *vm)) } return err } func (d *Daemon) applyCapabilityConfigChanges(ctx context.Context, before, after model.VMRecord) error { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { if hook, ok := capability.(configChangeCapability); ok { - if err := hook.ApplyConfigChange(ctx, d, before, after); err != nil { + if err := hook.ApplyConfigChange(ctx, before, after); err != nil { return err } } @@ -148,18 +158,37 @@ func (d *Daemon) applyCapabilityConfigChanges(ctx context.Context, before, after } func (d *Daemon) addCapabilityDoctorChecks(ctx context.Context, report *system.Report) { - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { if hook, ok := capability.(doctorCapability); ok { - hook.AddDoctorChecks(ctx, d, report) + hook.AddDoctorChecks(ctx, report) } } } -type workDiskCapability struct{} +// workDiskCapability provisions a per-VM work disk (image-seeded or +// freshly formatted) and syncs host-side authorised keys + git +// identity + file_sync entries onto it. Holds pointers to the VM and +// workspace services because PrepareHost orchestrates across both, +// plus the store + default image name for its doctor check. +type workDiskCapability struct { + vm *VMService + ws *WorkspaceService + store *store.Store + defaultImageName string +} + +func newWorkDiskCapability(vm *VMService, ws *WorkspaceService, st *store.Store, defaultImageName string) workDiskCapability { + return workDiskCapability{ + vm: vm, + ws: ws, + store: st, + defaultImageName: defaultImageName, + } +} func (workDiskCapability) Name() string { return "work-disk" } -func (workDiskCapability) AddStartPreflight(_ context.Context, _ *Daemon, checks *system.Preflight, vm model.VMRecord, image model.Image) { +func (workDiskCapability) AddStartPreflight(_ context.Context, checks *system.Preflight, vm model.VMRecord, image model.Image) { if exists(vm.Runtime.WorkDiskPath) { return } @@ -198,23 +227,23 @@ func (workDiskCapability) ContributeMachine(cfg *firecracker.MachineConfig, vm m }) } -func (workDiskCapability) PrepareHost(ctx context.Context, d *Daemon, vm *model.VMRecord, image model.Image) error { - prep, err := d.vm.ensureWorkDisk(ctx, vm, image) +func (c workDiskCapability) PrepareHost(ctx context.Context, vm *model.VMRecord, image model.Image) error { + prep, err := c.vm.ensureWorkDisk(ctx, vm, image) if err != nil { return err } - if err := d.ws.ensureAuthorizedKeyOnWorkDisk(ctx, vm, image, prep); err != nil { + if err := c.ws.ensureAuthorizedKeyOnWorkDisk(ctx, vm, image, prep); err != nil { return err } - if err := d.ws.ensureGitIdentityOnWorkDisk(ctx, vm); err != nil { + if err := c.ws.ensureGitIdentityOnWorkDisk(ctx, vm); err != nil { return err } - return d.ws.runFileSync(ctx, vm) + return c.ws.runFileSync(ctx, vm) } -func (workDiskCapability) AddDoctorChecks(_ context.Context, d *Daemon, report *system.Report) { - if d.store != nil && strings.TrimSpace(d.config.DefaultImageName) != "" { - if image, err := d.store.GetImageByName(context.Background(), d.config.DefaultImageName); err == nil && strings.TrimSpace(image.WorkSeedPath) != "" && exists(image.WorkSeedPath) { +func (c workDiskCapability) AddDoctorChecks(_ context.Context, report *system.Report) { + if c.store != nil && strings.TrimSpace(c.defaultImageName) != "" { + if image, err := c.store.GetImageByName(context.Background(), c.defaultImageName); err == nil && strings.TrimSpace(image.WorkSeedPath) != "" && exists(image.WorkSeedPath) { checks := system.NewPreflight() checks.RequireFile(image.WorkSeedPath, "default image work-seed", `rebuild the default image to regenerate the /root seed`) report.AddPreflight("feature /root work disk", checks, "seeded /root work disk artifact available") @@ -229,19 +258,27 @@ func (workDiskCapability) AddDoctorChecks(_ context.Context, d *Daemon, report * report.AddWarn("feature /root work disk", "default image has no work-seed artifact; new VM creates will be slower until the image is rebuilt") } -type dnsCapability struct{} +// dnsCapability publishes + removes .vm records on the in-process +// DNS server. Only needs HostNetwork. +type dnsCapability struct { + net *HostNetwork +} + +func newDNSCapability(net *HostNetwork) dnsCapability { + return dnsCapability{net: net} +} func (dnsCapability) Name() string { return "dns" } -func (dnsCapability) PostStart(ctx context.Context, d *Daemon, vm model.VMRecord, _ model.Image) error { - return d.net.setDNS(ctx, vm.Name, vm.Runtime.GuestIP) +func (c dnsCapability) PostStart(ctx context.Context, vm model.VMRecord, _ model.Image) error { + return c.net.setDNS(ctx, vm.Name, vm.Runtime.GuestIP) } -func (dnsCapability) Cleanup(_ context.Context, d *Daemon, vm model.VMRecord) error { - return d.net.removeDNS(vm.Runtime.DNSName) +func (c dnsCapability) Cleanup(_ context.Context, vm model.VMRecord) error { + return c.net.removeDNS(vm.Runtime.DNSName) } -func (dnsCapability) AddDoctorChecks(_ context.Context, _ *Daemon, report *system.Report) { +func (dnsCapability) AddDoctorChecks(_ context.Context, report *system.Report) { conn, err := net.ListenPacket("udp", vmdns.DefaultListenAddr) if err != nil { if strings.Contains(strings.ToLower(err.Error()), "address already in use") { @@ -255,57 +292,69 @@ func (dnsCapability) AddDoctorChecks(_ context.Context, _ *Daemon, report *syste report.AddPass("feature vm dns", "listener can bind "+vmdns.DefaultListenAddr) } -type natCapability struct{} +// natCapability sets up host-side NAT so guest traffic can reach the +// outside world. Needs VMService (tap lookup + aliveness) and +// HostNetwork (NAT rules), plus the daemon logger for the cleanup +// short-circuit note. +type natCapability struct { + vm *VMService + net *HostNetwork + logger *slog.Logger +} + +func newNATCapability(vm *VMService, net *HostNetwork, logger *slog.Logger) natCapability { + return natCapability{vm: vm, net: net, logger: logger} +} func (natCapability) Name() string { return "nat" } -func (natCapability) AddStartPreflight(ctx context.Context, d *Daemon, checks *system.Preflight, vm model.VMRecord, _ model.Image) { +func (c natCapability) AddStartPreflight(ctx context.Context, checks *system.Preflight, vm model.VMRecord, _ model.Image) { if !vm.Spec.NATEnabled { return } - d.net.addNATPrereqs(ctx, checks) + c.net.addNATPrereqs(ctx, checks) } -func (natCapability) PostStart(ctx context.Context, d *Daemon, vm model.VMRecord, _ model.Image) error { +func (c natCapability) PostStart(ctx context.Context, vm model.VMRecord, _ model.Image) error { if !vm.Spec.NATEnabled { return nil } - return d.net.ensureNAT(ctx, vm.Runtime.GuestIP, d.vm.vmHandles(vm.ID).TapDevice, true) + return c.net.ensureNAT(ctx, vm.Runtime.GuestIP, c.vm.vmHandles(vm.ID).TapDevice, true) } -func (natCapability) Cleanup(ctx context.Context, d *Daemon, vm model.VMRecord) error { +func (c natCapability) Cleanup(ctx context.Context, vm model.VMRecord) error { if !vm.Spec.NATEnabled { return nil } - tap := d.vm.vmHandles(vm.ID).TapDevice + tap := c.vm.vmHandles(vm.ID).TapDevice if strings.TrimSpace(vm.Runtime.GuestIP) == "" || strings.TrimSpace(tap) == "" { - if d.logger != nil { - d.logger.Debug("skipping nat cleanup without runtime network handles", append(vmLogAttrs(vm), "guest_ip", vm.Runtime.GuestIP, "tap_device", tap)...) + if c.logger != nil { + c.logger.Debug("skipping nat cleanup without runtime network handles", append(vmLogAttrs(vm), "guest_ip", vm.Runtime.GuestIP, "tap_device", tap)...) } return nil } - return d.net.ensureNAT(ctx, vm.Runtime.GuestIP, tap, false) + return c.net.ensureNAT(ctx, vm.Runtime.GuestIP, tap, false) } -func (natCapability) ApplyConfigChange(ctx context.Context, d *Daemon, before, after model.VMRecord) error { +func (c natCapability) ApplyConfigChange(ctx context.Context, before, after model.VMRecord) error { if before.Spec.NATEnabled == after.Spec.NATEnabled { return nil } - if !d.vm.vmAlive(after) { + if !c.vm.vmAlive(after) { return nil } - return d.net.ensureNAT(ctx, after.Runtime.GuestIP, d.vm.vmHandles(after.ID).TapDevice, after.Spec.NATEnabled) + return c.net.ensureNAT(ctx, after.Runtime.GuestIP, c.vm.vmHandles(after.ID).TapDevice, after.Spec.NATEnabled) } -func (natCapability) AddDoctorChecks(ctx context.Context, d *Daemon, report *system.Report) { +func (c natCapability) AddDoctorChecks(ctx context.Context, report *system.Report) { checks := system.NewPreflight() checks.RequireCommand("ip", toolHint("ip")) - d.net.addNATPrereqs(ctx, checks) + c.net.addNATPrereqs(ctx, checks) if len(checks.Problems()) > 0 { report.Add(system.CheckStatusFail, "feature nat", checks.Problems()...) return } - uplink, err := d.net.defaultUplink(ctx) + uplink, err := c.net.defaultUplink(ctx) if err != nil { report.AddFail("feature nat", err.Error()) return diff --git a/internal/daemon/capabilities_test.go b/internal/daemon/capabilities_test.go index 2799795..35cc888 100644 --- a/internal/daemon/capabilities_test.go +++ b/internal/daemon/capabilities_test.go @@ -14,27 +14,27 @@ import ( type testCapability struct { name string - prepare func(context.Context, *Daemon, *model.VMRecord, model.Image) error - cleanup func(context.Context, *Daemon, model.VMRecord) error + prepare func(context.Context, *model.VMRecord, model.Image) error + cleanup func(context.Context, model.VMRecord) error contribute func(*guestconfig.Builder, model.VMRecord, model.Image) contributeFC func(*firecracker.MachineConfig, model.VMRecord, model.Image) - configChange func(context.Context, *Daemon, model.VMRecord, model.VMRecord) error - doctor func(context.Context, *Daemon, *system.Report) - startPreflight func(context.Context, *Daemon, *system.Preflight, model.VMRecord, model.Image) + configChange func(context.Context, model.VMRecord, model.VMRecord) error + doctor func(context.Context, *system.Report) + startPreflight func(context.Context, *system.Preflight, model.VMRecord, model.Image) } func (c testCapability) Name() string { return c.name } -func (c testCapability) PrepareHost(ctx context.Context, d *Daemon, vm *model.VMRecord, image model.Image) error { +func (c testCapability) PrepareHost(ctx context.Context, vm *model.VMRecord, image model.Image) error { if c.prepare != nil { - return c.prepare(ctx, d, vm, image) + return c.prepare(ctx, vm, image) } return nil } -func (c testCapability) Cleanup(ctx context.Context, d *Daemon, vm model.VMRecord) error { +func (c testCapability) Cleanup(ctx context.Context, vm model.VMRecord) error { if c.cleanup != nil { - return c.cleanup(ctx, d, vm) + return c.cleanup(ctx, vm) } return nil } @@ -51,22 +51,22 @@ func (c testCapability) ContributeMachine(cfg *firecracker.MachineConfig, vm mod } } -func (c testCapability) ApplyConfigChange(ctx context.Context, d *Daemon, before, after model.VMRecord) error { +func (c testCapability) ApplyConfigChange(ctx context.Context, before, after model.VMRecord) error { if c.configChange != nil { - return c.configChange(ctx, d, before, after) + return c.configChange(ctx, before, after) } return nil } -func (c testCapability) AddDoctorChecks(ctx context.Context, d *Daemon, report *system.Report) { +func (c testCapability) AddDoctorChecks(ctx context.Context, report *system.Report) { if c.doctor != nil { - c.doctor(ctx, d, report) + c.doctor(ctx, report) } } -func (c testCapability) AddStartPreflight(ctx context.Context, d *Daemon, checks *system.Preflight, vm model.VMRecord, image model.Image) { +func (c testCapability) AddStartPreflight(ctx context.Context, checks *system.Preflight, vm model.VMRecord, image model.Image) { if c.startPreflight != nil { - c.startPreflight(ctx, d, checks, vm, image) + c.startPreflight(ctx, checks, vm, image) } } @@ -78,27 +78,27 @@ func TestPrepareCapabilityHostsRollsBackPreparedCapabilitiesInReverseOrder(t *te vmCaps: []vmCapability{ testCapability{ name: "first", - prepare: func(context.Context, *Daemon, *model.VMRecord, model.Image) error { + prepare: func(context.Context, *model.VMRecord, model.Image) error { return nil }, - cleanup: func(context.Context, *Daemon, model.VMRecord) error { + cleanup: func(context.Context, model.VMRecord) error { cleanupOrder = append(cleanupOrder, "first") return nil }, }, testCapability{ name: "second", - prepare: func(context.Context, *Daemon, *model.VMRecord, model.Image) error { + prepare: func(context.Context, *model.VMRecord, model.Image) error { return nil }, - cleanup: func(context.Context, *Daemon, model.VMRecord) error { + cleanup: func(context.Context, model.VMRecord) error { cleanupOrder = append(cleanupOrder, "second") return nil }, }, testCapability{ name: "broken", - prepare: func(context.Context, *Daemon, *model.VMRecord, model.Image) error { + prepare: func(context.Context, *model.VMRecord, model.Image) error { return errors.New("boom") }, }, @@ -146,11 +146,11 @@ func TestContributeHooksPopulateGuestAndMachineConfig(t *testing.T) { } } -func TestRegisteredCapabilitiesInOrder(t *testing.T) { +func TestDefaultCapabilitiesInOrder(t *testing.T) { d := &Daemon{} wireServices(d) var names []string - for _, capability := range d.registeredCapabilities() { + for _, capability := range d.vmCaps { names = append(names, capability.Name()) } want := []string{"work-disk", "dns", "nat"} diff --git a/internal/daemon/daemon.go b/internal/daemon/daemon.go index 8ed545e..3651c1c 100644 --- a/internal/daemon/daemon.go +++ b/internal/daemon/daemon.go @@ -662,6 +662,9 @@ func wireServices(d *Daemon) { beginOperation: d.beginOperation, }) } + if len(d.vmCaps) == 0 { + d.vmCaps = d.defaultCapabilities() + } } func marshalResultOrError(v any, err error) rpc.Response { diff --git a/internal/daemon/vm_service.go b/internal/daemon/vm_service.go index 4783b9a..0557d89 100644 --- a/internal/daemon/vm_service.go +++ b/internal/daemon/vm_service.go @@ -32,10 +32,10 @@ import ( // services remain unexported within the package so nothing outside // the daemon can see them. // -// Capability invocation still runs through Daemon because the hook -// interfaces take *Daemon directly. VMService calls back via the -// capHooks seam rather than holding a *Daemon pointer, to keep the -// dependency graph acyclic. +// Capability dispatch goes through the capHooks seam rather than a +// *Daemon pointer, so VMService has no path back to the composition +// root. Daemon.buildCapabilityHooks() populates the seam at wiring +// time with the registered-capabilities loops from capabilities.go. type VMService struct { runner system.CommandRunner logger *slog.Logger @@ -67,10 +67,11 @@ type VMService struct { guestWaitForSSH func(context.Context, string, string, time.Duration) error guestDial func(context.Context, string, string) (guestSSHClient, error) - // Capability hook dispatch. Capabilities themselves live on - // *Daemon (their interface takes *Daemon as receiver); VMService - // invokes them via these seams so it doesn't need a *Daemon - // pointer. + // Capability hook dispatch. VMService invokes capabilities via + // these seams, populated by Daemon.buildCapabilityHooks() at + // wiring time. Capability implementations themselves are + // structs with explicit service-pointer fields (see capabilities.go); + // VMService never reaches back to *Daemon. capHooks capabilityHooks beginOperation func(name string, attrs ...any) *operationLog