From 43eaaac8b0f33e964f913a891ced169f7aa0adfc Mon Sep 17 00:00:00 2001 From: Michael Johnson Date: Sun, 18 Oct 2026 08:48:22 -0400 Subject: [PATCH] fix: keep a save made while an install is still checking it stays up --- cmd/fjordd/address_clash_test.go | 35 +++++++++++ cmd/fjordd/keep_address.go | 103 +++++++++++++++++-------------- pkg/stack/stack.go | 34 +++++++++- 3 files changed, 123 insertions(+), 49 deletions(-) diff --git a/cmd/fjordd/address_clash_test.go b/cmd/fjordd/address_clash_test.go index 710453d..e184f95 100644 --- a/cmd/fjordd/address_clash_test.go +++ b/cmd/fjordd/address_clash_test.go @@ -75,6 +75,41 @@ func TestKeepAddressesSkipsATakenAddress(t *testing.T) { } } +// The type test on netlab: an install's stream runs on through its stay-up +// watch, and host networking saved meanwhile was overwritten at its end by +// the install-time copy, with the address pinned on top. +func TestKeepAddressesKeepsASaveMadeSince(t *testing.T) { + s := clashFixture(t) + s.backends["podman"].(liveEngine).held["web"] = map[string]map[string]string{"web": {"vlan5": "192.168.5.30"}} + dir := filepath.Join(s.manager.StacksDir, "web") + os.MkdirAll(dir, 0o755) + os.WriteFile(filepath.Join(dir, "compose.yaml"), []byte("services:\n web:\n image: x\n networks:\n - vlan5\nnetworks:\n vlan5:\n external: true\n"), 0o644) + os.WriteFile(filepath.Join(dir, "state.json"), []byte(`{"schema_version":1,"engine":"podman"}`), 0o644) + stale, _ := s.manager.Get("web") + onVlan := stale.Compose + + saved := "services:\n web:\n image: x\n network_mode: host\n" + if err := s.manager.Save(&stack.Stack{Name: "web", Compose: saved, Env: "A=1\n"}); err != nil { + t.Fatal(err) + } + if kept, _ := s.keepAddresses(context.Background(), stale); len(kept) != 0 { + t.Errorf("pinned on networks the stack has left: %v", kept) + } + if after, _ := s.manager.Get("web"); after.Compose != saved || after.Env != "A=1\n" { + t.Errorf("the save was overwritten:\n%s\nenv %q", after.Compose, after.Env) + } + + // Still on vlan5: the pin lands in the compose as saved, not in the copy. + s.manager.Save(&stack.Stack{Name: "web", Compose: strings.Replace(onVlan, "image: x", "image: y", 1)}) + stale.Compose = onVlan + if kept, _ := s.keepAddresses(context.Background(), stale); len(kept) != 1 { + t.Errorf("kept: %v", kept) + } + if after, _ := s.manager.Get("web"); !strings.Contains(after.Compose, "image: y") || !strings.Contains(after.Compose, "ipv4_address: 192.168.5.30") { + t.Errorf("pin not on the saved compose:\n%s", after.Compose) + } +} + // A stack pinning what another stack holds, pinned or not, is a clash -- // what pre-flight and Save refuse. func TestClashesPinnedAndLive(t *testing.T) { diff --git a/cmd/fjordd/keep_address.go b/cmd/fjordd/keep_address.go index 0e79a8d..125f63b 100644 --- a/cmd/fjordd/keep_address.go +++ b/cmd/fjordd/keep_address.go @@ -58,71 +58,80 @@ func (s *server) keepAddresses(ctx context.Context, st *stack.Stack) (kept, skip ids[c.Service] = c.ID } } - compose := st.Compose // What other stacks have, worked out once and only if something is to be // pinned: an address another stack has is never pinned here (see // takenAddresses), it is reported. var taken map[string]addrHolder - per := composepkg.ServiceAttachments(compose) - svcs := make([]string, 0, len(per)) - for svc := range per { - svcs = append(svcs, svc) - } - sort.Strings(svcs) - for _, svc := range svcs { - for _, a := range per[svc] { - if private[a.Network] { - continue - } - n, ok := hostnet.Get(a.Network) - if !ok || n.Static { - continue - } - if n.DHCP { - if a.MAC != "" { + // Pinned into the compose as it is on disk now, not st's copy: st was + // loaded when the action started, and an install's stream runs on through + // its stay-up watch. Saving that copy at the end undid a Save made + // meanwhile -- host networking set during the watch came back as the old + // networks with this MAC pinned (netlab, type test). + pin := func(compose string) string { + kept, skipped = nil, nil + per := composepkg.ServiceAttachments(compose) + svcs := make([]string, 0, len(per)) + for svc := range per { + svcs = append(svcs, svc) + } + sort.Strings(svcs) + for _, svc := range svcs { + for _, a := range per[svc] { + if private[a.Network] { + continue + } + n, ok := hostnet.Get(a.Network) + if !ok || n.Static { continue } - mac := hostnet.MACOf(a.Network, ids[svc]) - if mac == "" { + if n.DHCP { + if a.MAC != "" { + continue + } + mac := hostnet.MACOf(a.Network, ids[svc]) + if mac == "" { + continue + } + out, err := composepkg.PinMAC(compose, svc, a.Network, mac) + if err != nil || out == compose { + continue + } + compose = out + kept = append(kept, fmt.Sprintf("kept %s's MAC %s on %s, which its DHCP lease follows", svc, mac, a.Network)) continue } - out, err := composepkg.PinMAC(compose, svc, a.Network, mac) + if a.IP != "" { + continue + } + ip := held[svc][a.Network] + if parsed := net.ParseIP(ip); parsed == nil || parsed.To4() == nil { + continue + } + if taken == nil { + taken = s.takenAddresses(ctx, st.Name) + } + if h, ok := taken[a.Network+"|"+ip]; ok { + skipped = append(skipped, clashText(svc, ip, a.Network, h, fixOnStackPage)) + continue + } + out, err := composepkg.PinAddress(compose, svc, a.Network, ip) if err != nil || out == compose { continue } compose = out - kept = append(kept, fmt.Sprintf("kept %s's MAC %s on %s, which its DHCP lease follows", svc, mac, a.Network)) - continue - } - if a.IP != "" { - continue - } - ip := held[svc][a.Network] - if parsed := net.ParseIP(ip); parsed == nil || parsed.To4() == nil { - continue - } - if taken == nil { - taken = s.takenAddresses(ctx, st.Name) - } - if h, ok := taken[a.Network+"|"+ip]; ok { - skipped = append(skipped, clashText(svc, ip, a.Network, h, fixOnStackPage)) - continue - } - out, err := composepkg.PinAddress(compose, svc, a.Network, ip) - if err != nil || out == compose { - continue + kept = append(kept, fmt.Sprintf("kept %s on %s (%s)", svc, ip, a.Network)) } - compose = out - kept = append(kept, fmt.Sprintf("kept %s on %s (%s)", svc, ip, a.Network)) } + return compose } + out, err := s.manager.UpdateCompose(st.Name, pin) + if err != nil { + return []string{"could not keep addresses: " + err.Error()}, skipped + } + st.Compose = out if len(kept) == 0 { return nil, skipped } - st.Compose = compose - if err := s.manager.Save(st); err != nil { - return []string{"could not keep addresses: " + err.Error()}, skipped - } return kept, skipped } diff --git a/pkg/stack/stack.go b/pkg/stack/stack.go index c52bc45..d7f0200 100644 --- a/pkg/stack/stack.go +++ b/pkg/stack/stack.go @@ -181,6 +181,10 @@ type Manager struct { // records its rollback) otherwise both load the old file and the second // save drops the first one's field. locks sync.Map + // fileLocks holds a *sync.Mutex per stack, taken across each write of + // its files, so UpdateCompose's read-change-write cannot interleave with + // a Save. + fileLocks sync.Map } // NewManager initializes a new stack manager. @@ -271,6 +275,7 @@ func (m *Manager) Save(stack *Stack) error { if !ValidName(stack.Name) { return ErrInvalidName } + defer lockIn(&m.fileLocks, stack.Name)() dir := filepath.Join(m.StacksDir, stack.Name) if err := os.MkdirAll(dir, 0755); err != nil { return fmt.Errorf("failed to create stack directory: %w", err) @@ -529,9 +534,34 @@ func (m *Manager) UpdateState(name string, change func(st *State) *State) error return m.saveState(name, st) } +// UpdateCompose changes a stack's compose.yaml as it is on disk now, under +// the lock Save takes: change gets the compose and returns the one to write +// (the same text writes nothing). For fjord's own edits at the end of a long +// action -- the copy that action loaded can be minutes old, and saving it +// whole put back a compose the user had saved over meanwhile. +func (m *Manager) UpdateCompose(name string, change func(compose string) string) (string, error) { + if !ValidName(name) { + return "", ErrInvalidName + } + defer lockIn(&m.fileLocks, name)() + path := filepath.Join(m.StacksDir, name, "compose.yaml") + old, err := os.ReadFile(path) + if err != nil { + return "", err + } + out := change(string(old)) + if out == string(old) { + return out, nil + } + return out, writeFileAtomic(path, []byte(out), 0644) +} + // lock takes the stack's state lock and returns its release. -func (m *Manager) lock(name string) func() { - mu, _ := m.locks.LoadOrStore(name, &sync.Mutex{}) +func (m *Manager) lock(name string) func() { return lockIn(&m.locks, name) } + +// lockIn takes name's mutex in locks and returns its release. +func lockIn(locks *sync.Map, name string) func() { + mu, _ := locks.LoadOrStore(name, &sync.Mutex{}) mu.(*sync.Mutex).Lock() return mu.(*sync.Mutex).Unlock }