diff --git a/docs/terminal-interface.mdx b/docs/terminal-interface.mdx index c1addb5ca..fe0cffe2a 100644 --- a/docs/terminal-interface.mdx +++ b/docs/terminal-interface.mdx @@ -245,7 +245,7 @@ the rest are declined and their senders are told. | `d` | Decline an inbound pairing request | | `f` | Find a machine by address, without pairing with it | | `c` | Cancel the pairing request you sent | -| `r` | Remove the selected machine from the cluster (asks you to confirm). For a machine you added with `f`, forget it straight away | +| `r` | Remove the selected machine from the cluster (asks you to confirm), forgetting it too if you added it with `f`. A machine you added with `f` that is not a member is forgotten straight away | | `l` | Leave the cluster (asks you to confirm) | | `/` | Filter the list by name or address; `esc` clears it | diff --git a/services/nvpair-tui/ui/nodes.go b/services/nvpair-tui/ui/nodes.go index 41f68b3b8..647b7bfbb 100644 --- a/services/nvpair-tui/ui/nodes.go +++ b/services/nvpair-tui/ui/nodes.go @@ -4,9 +4,11 @@ package ui import ( + "context" "fmt" "log/slog" "net" + "slices" "sort" "strconv" "strings" @@ -1261,7 +1263,8 @@ func (v *nodesView) declineWaiting() []tea.Cmd { } // removeSelected drops the selected node's strongest relationship: cluster -// membership if it is a member, otherwise the manual entry that added it. +// membership if it is a member, along with the manual entry that added it, +// otherwise that manual entry alone. func (v *nodesView) removeSelected() tea.Cmd { row := v.selectedRow() if row == nil { @@ -1282,11 +1285,11 @@ func (v *nodesView) removeSelected() tea.Cmd { row.name) return nil - case row.manualID != "": - return call(v.client, "node/remove", map[string]string{"id": row.manualID}, - func(_ *rpc.Message, err error) tea.Msg { - return nodeActionMsg{what: "remove manual entry " + row.name, err: err} - }) + case len(row.manualIDs) > 0: + name, ids := row.name, row.manualIDs + return func() tea.Msg { + return nodeActionMsg{what: "remove manual entry " + name, err: forgetManualEntries(v.client, ids)} + } default: v.status.info("%s is only discovered - nothing to remove", row.name) @@ -1358,17 +1361,23 @@ func (v *nodesView) handleInviteCancelled(msg inviteCancelledMsg) tea.Cmd { return nil } -// removeMember un-pairs a confirmed peer. +// removeMember un-pairs a confirmed peer, then forgets every manual entry that +// added it. // // Keyed by the stable nodeUuid rather than the display name: a member that // renamed its PC keeps its UUID, so matching on a possibly stale name would // silently fail. The row is looked up again by key so a list that re-sorted // between arming and confirming cannot redirect the removal at another node. +// +// The broker keeps manual entries on disk and re-adds them whenever it starts, +// so one left behind brings the removed peer back as a manual row, here and in +// the desktop app. They are forgotten only once the un-pairing succeeds: a peer +// still in the cluster keeps the addresses that reach it. func (v *nodesView) removeMember(key string) tea.Cmd { - name := key + name, manualIDs := key, []string(nil) for _, n := range v.all { if n.key == key { - name = n.name + name, manualIDs = n.name, n.manualIDs break } } @@ -1377,10 +1386,31 @@ func (v *nodesView) removeMember(key string) tea.Cmd { v.status.busy("removing %s from the cluster...", name) return call(v.client, "nodes:remove", map[string]string{"nodeUuid": key}, func(_ *rpc.Message, err error) tea.Msg { - return nodeActionMsg{what: "remove " + name, err: err} + if err != nil { + return nodeActionMsg{what: "remove " + name, err: err} + } + // Still on the command's goroutine, so the follow-ups run in line. + if err := forgetManualEntries(v.client, manualIDs); err != nil { + return nodeActionMsg{what: "remove manual entry " + name, err: err} + } + return nodeActionMsg{what: "remove " + name} }) } +// forgetManualEntries removes each manual entry in turn, stopping at the first +// that fails. Each request gets its own callTimeout, as it would through call. +func forgetManualEntries(client *rpc.Client, ids []string) error { + for _, id := range ids { + ctx, cancel := context.WithTimeout(context.Background(), callTimeout) + _, err := client.Call(ctx, "node/remove", map[string]string{"id": id}) + cancel() + if err != nil { + return err + } + } + return nil +} + // leaveCluster unjoins this node. The cluster-manager tears down local trust and // pushes cluster:identity-changed and nodes:changed, which refresh the view. func (v *nodesView) leaveCluster() tea.Cmd { @@ -1444,9 +1474,9 @@ func (v *nodesView) followDetail() { return } } - if id := v.detail.node.manualID; id != "" { + if ids := v.detail.node.manualIDs; len(ids) > 0 { for _, n := range v.all { - if n.manualID == id { + if slices.Contains(n.manualIDs, ids[0]) { v.detail.followNode(n, true) return } diff --git a/services/nvpair-tui/ui/nodes_test.go b/services/nvpair-tui/ui/nodes_test.go new file mode 100644 index 000000000..834584827 --- /dev/null +++ b/services/nvpair-tui/ui/nodes_test.go @@ -0,0 +1,147 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package ui + +import ( + "context" + "encoding/json" + "net" + "testing" + "time" + + "nvpair-tui/rpc" + + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// removalCall is one request removalBroker received. +type removalCall struct { + method string + params json.RawMessage +} + +// removalBroker is a broker that records every call and answers it, failing +// each call to the method named by fail. +func removalBroker(t *testing.T, fail string) (*rpc.Client, <-chan removalCall) { + t.Helper() + c1, c2 := net.Pipe() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(func() { + cancel() + c1.Close() + c2.Close() + }) + client := rpc.NewClient(c1, c1) + go client.Run(ctx) + + calls := make(chan removalCall, 8) + broker := rpc.NewCodec(c2, c2) + go func() { + for { + req, err := broker.Read() + if err != nil { + return + } + calls <- removalCall{method: req.Method, params: req.Params} + reply := &rpc.Message{JSONRPC: "2.0", ID: req.ID, Result: json.RawMessage(`{}`)} + if req.Method == fail { + reply = &rpc.Message{JSONRPC: "2.0", ID: req.ID, + Error: &rpc.RPCError{Code: -32000, Message: "refused"}} + } + _ = broker.Write(reply) + } + }() + return client, calls +} + +// confirmRemoval arms and confirms removing the selected node, then feeds each +// reply back into the view until it reports an outcome. +func confirmRemoval(t *testing.T, v *nodesView) { + t.Helper() + require.Nil(t, v.removeSelected(), "removal was dispatched without confirmation") + cmd := v.handleKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("y")}) + for cmd != nil { + msg := cmd() + if _, done := msg.(nodeActionMsg); done { + v.Update(msg) + return + } + cmd = v.Update(msg) + } + require.FailNow(t, "the removal ended without reporting an outcome") +} + +// wantCall checks the next request the broker received carries field = value. +func wantCall(t *testing.T, calls <-chan removalCall, method, field, value string) { + t.Helper() + select { + case c := <-calls: + require.Equal(t, method, c.method) + var params map[string]string + require.NoError(t, json.Unmarshal(c.params, ¶ms), "decode %s params", method) + assert.Equal(t, value, params[field], "%s %s", method, field) + case <-time.After(2 * time.Second): + require.FailNow(t, "broker never received "+method) + } +} + +// peerAddedByAddress is a nodes view with one member that was first added by +// address, selected. +func peerAddedByAddress(client *rpc.Client) *nodesView { + v := newNodesView(client) + v.feeds.discovered = []availableNode{{ + HostUUID: "peer", Name: "peer", IPAddress: "10.0.0.2", Trusted: true, + }} + v.feeds.manual = []manualNode{{ID: "manual:10.0.0.2", Address: "10.0.0.2", HostUUID: "peer"}} + v.rebuild() + v.selectedKey = "peer" + return v +} + +// TestRemoveMember_ForgetsItsManualEntry checks un-pairing a peer that was added +// by address forgets that address too. The broker keeps manual entries on disk +// and re-adds them when it starts, so one left behind brought the peer back. +func TestRemoveMember_ForgetsItsManualEntry(t *testing.T) { + client, calls := removalBroker(t, "") + v := peerAddedByAddress(client) + + confirmRemoval(t, v) + + wantCall(t, calls, "nodes:remove", "nodeUuid", "peer") + wantCall(t, calls, "node/remove", "id", "manual:10.0.0.2") + assert.Contains(t, v.status.render(), "remove peer ok", "the removal is reported") +} + +// TestRemoveMember_ForgetsEveryManualEntry checks a peer added twice, by +// hostname and by IP, has both entries forgotten when it is un-paired. +func TestRemoveMember_ForgetsEveryManualEntry(t *testing.T) { + client, calls := removalBroker(t, "") + v := peerAddedByAddress(client) + v.feeds.manual = append(v.feeds.manual, + manualNode{ID: "manual:peer.lan", Address: "peer.lan", HostUUID: "peer"}) + v.rebuild() + + confirmRemoval(t, v) + + wantCall(t, calls, "nodes:remove", "nodeUuid", "peer") + wantCall(t, calls, "node/remove", "id", "manual:10.0.0.2") + wantCall(t, calls, "node/remove", "id", "manual:peer.lan") + assert.Contains(t, v.status.render(), "remove peer ok", "the removal is reported") +} + +// TestRemoveMember_KeepsTheManualEntryWhenUnpairingFails checks a peer the cluster manager +// would not un-pair keeps the address it was added by: it is still a member, +// and that address is how it is reached. +func TestRemoveMember_KeepsTheManualEntryWhenUnpairingFails(t *testing.T) { + client, calls := removalBroker(t, "nodes:remove") + v := peerAddedByAddress(client) + + confirmRemoval(t, v) + + wantCall(t, calls, "nodes:remove", "nodeUuid", "peer") + assert.Empty(t, calls, "a failed un-pairing must not go on to forget the manual entry") + assert.Contains(t, v.status.render(), "remove peer failed", "the failure is reported") +} diff --git a/services/nvpair-tui/ui/nodesmodel.go b/services/nvpair-tui/ui/nodesmodel.go index 8419e18b7..a999ae6fe 100644 --- a/services/nvpair-tui/ui/nodesmodel.go +++ b/services/nvpair-tui/ui/nodesmodel.go @@ -113,9 +113,10 @@ type nodeRow struct { membership nodeMembership self bool - // manualID is the handle node/remove needs. Empty unless the entry was - // added by hand. - manualID string + // manualIDs are the handles node/remove needs, one per manual entry that + // merged into this row: the same machine can be added twice, once by + // hostname and once by IP. Empty unless the node was added by hand. + manualIDs []string // nodeInfoTLS is a hand-added node that serves node-info over TLS. That // endpoint needs the backend's cluster trust, which this client does not // hold, so it is never polled from here: probedTelemetry, the manual-node @@ -213,7 +214,7 @@ func mergeNodes(in nodeFeeds) []nodeRow { row.name = m.Name row.address = m.Address } - row.manualID = m.ID + row.manualIDs = append(row.manualIDs, m.ID) if row.name == "" { row.name = m.Address } diff --git a/services/nvpair-tui/ui/nodesmodel_test.go b/services/nvpair-tui/ui/nodesmodel_test.go index ed29cd330..6304584ef 100644 --- a/services/nvpair-tui/ui/nodesmodel_test.go +++ b/services/nvpair-tui/ui/nodesmodel_test.go @@ -152,7 +152,7 @@ func TestMerge_DeduplicatesAcrossFeeds(t *testing.T) { require.Len(t, rows, 1, "one machine must produce one row") row := rows[0] - assert.Equal(t, "m1", row.manualID, "manual handle must survive the merge") + assert.Equal(t, []string{"m1"}, row.manualIDs, "manual handle must survive the merge") assert.Equal(t, nodeMembership(membershipMember), row.membership) assert.Equal(t, 2, row.modelCount()) } @@ -191,7 +191,7 @@ func TestMerge_UsesEveryFactTheManualWorkerReports(t *testing.T) { manual: []manualNode{{ID: "m1", Address: "gpu-box.lan", HostUUID: "u1", NodeInfoUp: true}}, }) require.Len(t, rows, 1, "the manual entry must be joined by its UUID") - assert.Equal(t, "m1", rows[0].manualID, "the joined row must retain its manual handle") + assert.Equal(t, []string{"m1"}, rows[0].manualIDs, "the joined row must retain its manual handle") // A hand-added node reached on a non-default node-info port, over TLS. rows = mergeNodes(nodeFeeds{ @@ -209,6 +209,21 @@ func TestMerge_UsesEveryFactTheManualWorkerReports(t *testing.T) { } } +// TestMerge_KeepsEveryManualEntryForOneMachine checks a machine added twice, by +// hostname and by IP, keeps both entries. Removing it must forget both, or the +// one left behind brings it back when the broker next starts. +func TestMerge_KeepsEveryManualEntryForOneMachine(t *testing.T) { + rows := mergeNodes(nodeFeeds{ + discovered: []availableNode{{HostUUID: "u1", Name: "gpu-box", IPAddress: "10.0.0.5", Port: 14318}}, + manual: []manualNode{ + {ID: "m1", Address: "gpu-box.lan", HostUUID: "u1", NodeInfoUp: true}, + {ID: "m2", Address: "10.0.0.5", HostUUID: "u1", NodeInfoUp: true}, + }, + }) + require.Len(t, rows, 1, "both entries must join the one machine") + assert.Equal(t, []string{"m1", "m2"}, rows[0].manualIDs) +} + // TestAReusedAddressDoesNotMergeTwoMachines checks an address is not taken for // an identity. A manual entry whose probe read one UUID must not fold into a // discovered row with a different UUID at the same address — that is another @@ -222,14 +237,14 @@ func TestAReusedAddress_DoesNotMergeTwoMachines(t *testing.T) { manual: []manualNode{{ID: "m1", Name: "a", Address: "10.0.0.5", HostUUID: "machine-a", NodeInfoUp: true}}, }) require.Len(t, rows, 2, "two machines at one address must remain distinct") - assert.Empty(t, findRow(t, rows, "b").manualID, "machine b must not take the manual entry for machine a") + assert.Empty(t, findRow(t, rows, "b").manualIDs, "machine b must not take the manual entry for machine a") rows = mergeNodes(nodeFeeds{ discovered: discovered, manual: []manualNode{{ID: "m1", Address: "10.0.0.5"}}, }) require.Len(t, rows, 1, "an unidentified entry must join the machine at its address") - assert.Equal(t, "m1", rows[0].manualID) + assert.Equal(t, []string{"m1"}, rows[0].manualIDs) } // TestTLSNodeInfoIsNotPolledInPlainText checks the detail screen shows the