diff --git a/cmd/mesh-control/acts.go b/cmd/mesh-control/acts.go index 1a83690..0586ceb 100644 --- a/cmd/mesh-control/acts.go +++ b/cmd/mesh-control/acts.go @@ -3,6 +3,8 @@ package main import ( "context" "fmt" + + "github.com/novox/mesh-control/internal/catalogue" ) // The things the mesh can be asked to do, separated from how it was asked. @@ -25,10 +27,22 @@ func assign(ctx context.Context, open *stores, node, module string) (string, err return "", err } said := fmt.Sprintf("%s is assigned %s", node, module) - if _, _, err := planFor(ctx, open, node); err != nil { + plan, _, err := planFor(ctx, open, node) + if err != nil { // Kept, and still refused. Both halves are the answer. return said, err } + // Kept, and cannot be hosted here. Said at once rather than discovered at push: a module whose + // capability the machine lacks is on the wrong machine, and the assignment records what a person + // meant while this line says it will not run until it moves. The rest of the node still pushes. + for _, u := range plan.Unhostable { + if u.Module != module { + continue + } + for _, c := range u.Missing { + said += "\n but " + catalogue.WrongMachine(u.Module, c, node) + } + } return said + fmt.Sprintf("\n run `push %s` to send it", node), nil } diff --git a/cmd/mesh-control/plan.go b/cmd/mesh-control/plan.go index e41a1d6..b93dd16 100644 --- a/cmd/mesh-control/plan.go +++ b/cmd/mesh-control/plan.go @@ -611,6 +611,13 @@ func planCommand(ctx context.Context, args []string) error { for _, m := range plan.Modules { fmt.Printf(" %-20s %s\n", m.Module, plan.Because[m.Module]) } + // What was assigned here and cannot run here. Said with the rest rather than as a refusal: it is + // one module on the wrong machine, the others still run, and the remedy is to move this one. + for _, u := range plan.Unhostable { + for _, c := range u.Missing { + fmt.Printf(" %-20s not applied — %s\n", u.Module, catalogue.WrongMachine(u.Module, c, args[0])) + } + } for _, c := range plan.Claims { fmt.Printf(" holds %s, one per %s\n", c.Claim, c.Scope) } diff --git a/cmd/mesh-control/push.go b/cmd/mesh-control/push.go index fa26e0a..5bab629 100644 --- a/cmd/mesh-control/push.go +++ b/cmd/mesh-control/push.go @@ -13,10 +13,24 @@ import ( "time" "github.com/novox/mesh-control/internal/broker" + "github.com/novox/mesh-control/internal/catalogue" "github.com/novox/mesh-control/internal/inventory" "github.com/novox/mesh-control/internal/link" ) +// reportUnhostable says which of a node's assigned modules the machine cannot run, once per push. +// +// A module whose declared capability has no detector on the machine is on the wrong machine. It is +// kept out of what the node is sent — the healthy modules beside it still converge — and named here +// so it is neither silently dropped nor a reason the whole node fails to push. +func reportUnhostable(node string, plan catalogue.Resolution) { + for _, u := range plan.Unhostable { + for _, c := range u.Missing { + fmt.Printf("%s not applied — %s\n", node, catalogue.WrongMachine(u.Module, c, node)) + } + } +} + // sending it, and holding the link that carries it. // // Split out of main.go, which had reached 2,769 lines because appending was always the @@ -262,6 +276,10 @@ func pushCommand(ctx context.Context, args []string) error { refusals = append(refusals, fmt.Sprintf("%s:\n%v", n.Name, err)) continue } + // A module assigned here that this machine cannot host is said and left out, not fatal: the + // healthy modules beside it are still resolved and sent. Reported so it is not silently + // dropped — the remedy is to move it, and until then the rest of the node converges. + reportUnhostable(n.Name, plan) // The private network is in here with everything else. It used to be composed separately // and prepended, which meant every machine with an address was on it and no machine could // be kept off. It is a module now, so it arrives the way a module does. @@ -335,6 +353,7 @@ func sendTo(ctx context.Context, open *stores, names []string) error { refusals = append(refusals, fmt.Sprintf("%s:\n%v", name, err)) continue } + reportUnhostable(name, plan) resources, err := declarationWith(ctx, open, name, plan, settings, gens, Allocating) if err != nil { refusals = append(refusals, fmt.Sprintf("%s:\n%v", name, err)) diff --git a/internal/catalogue/resolve.go b/internal/catalogue/resolve.go index ef0ab9d..ef8d99e 100644 --- a/internal/catalogue/resolve.go +++ b/internal/catalogue/resolve.go @@ -113,6 +113,21 @@ type Resolution struct { // because it is where a credential will have to be handed back once there is a mechanism for // that, and because "what does this machine depend on that is not on it" has no other answer. Needs []Needed + // Unhostable is what a person assigned to this machine that this machine cannot run — a module + // whose declared capability has no detector here. Kept out of Modules rather than refusing the + // whole set: a module put on the wrong machine is that one module's problem, and the healthy + // modules beside it still resolve, declare, and converge. Reported so it is neither silently + // dropped nor fatal to the rest. A module that is *required* by something running here is a + // different case — that set is incoherent and is refused (see checkCapabilities). + Unhostable []Unhostable +} + +// Unhostable is one directly-assigned module the machine cannot run. +type Unhostable struct { + // Module is the assigned module the machine cannot host. + Module string + // Missing is the capabilities it declares that this machine does not have. + Missing []string } // Needed is one thing this node's set takes from elsewhere in the mesh. @@ -212,16 +227,32 @@ func Resolve(catalogue map[string]Manifest, assigned []string, node Node, world // how this read when first used. satisfied := map[string]bool{} - // Everything a person assigned goes in first. Those are choices already made, and a - // requirement one of them answers is not a choice to put back to anybody. - queue := append([]string{}, assigned...) + // Everything a person assigned goes in first, except what this machine cannot run. Those are + // choices already made, and a requirement one of them answers is not a choice to put back to + // anybody. + // + // **A module the machine cannot host is left out here rather than refusing the whole set.** + // Assigning fail2ban to a machine whose profile reports no firewall is one module on the wrong + // machine (novox/hq ADR 0009's sibling case): the person meant something, the remedy is theirs, + // and it is a fact about this one module — not a reason the healthy modules beside it should + // fail to resolve and converge. It is reported as un-applied on the resolution, so it is neither + // silently dropped nor fatal to the rest. The distinction is who wanted it: a module *required* + // by something running here that cannot be hosted makes the set itself incoherent, and that is + // still refused, by checkCapabilities, where it stays in the closure. + var unhostable []Unhostable + var queue []string for _, a := range assigned { - because[a] = "assigned" if m, known := catalogue[a]; known { + if missing := missingCapabilities(m, node); len(missing) > 0 { + unhostable = append(unhostable, Unhostable{Module: a, Missing: missing}) + continue + } for _, o := range m.Offers() { satisfied[o] = true } } + because[a] = "assigned" + queue = append(queue, a) } // What has already been complained about. A requirement can be wanted by several modules at @@ -473,7 +504,8 @@ func Resolve(catalogue map[string]Manifest, assigned []string, node Node, world } } - resolution := Resolution{Node: node.Name, At: node.At, Because: because, Needs: needs} + resolution := Resolution{Node: node.Name, At: node.At, Because: because, Needs: needs, + Unhostable: unhostable} for _, n := range providersFirst(order, catalogue) { resolution.Modules = append(resolution.Modules, catalogue[n]) } @@ -519,19 +551,42 @@ func isModule(catalogue map[string]Manifest, want string) bool { return ok } -// checkCapabilities refuses a module the machine cannot run. +// missingCapabilities is the capabilities a module declares that this machine does not have. // -// Said as a fact about the machine rather than about the module, because that is what it is and -// because nothing can be installed to change it. +// Only the absent ones: a capability the machine reports having is nothing to say. Empty means the +// machine can host the module as far as its capabilities go. +func missingCapabilities(m Manifest, node Node) []string { + var missing []string + for _, c := range m.Capabilities { + if !node.Capabilities[c] { + missing = append(missing, c) + } + } + return missing +} + +// WrongMachine is why a module cannot run here, said as a fact about the machine. +// +// Said the same whether a module was refused because something running here requires it or reported +// as un-applied because a person assigned it directly: the reason is identical, and nothing can be +// installed to change it. +func WrongMachine(module, capability, node string) string { + return fmt.Sprintf( + "%s needs the capability %q and %s does not have it — this is the wrong machine, "+ + "not a missing module", module, capability, node) +} + +// checkCapabilities refuses a module the machine cannot run that something running here requires. +// +// A directly-assigned module the machine cannot host is left out of the closure before this runs +// and reported as un-applied instead (see Resolve); what reaches here is a module still in the +// closure because something requires it, and that set is genuinely incoherent — the requiring +// module cannot have its requirement met on this machine. func checkCapabilities(modules []Manifest, node Node) []string { var problems []string for _, m := range modules { - for _, c := range m.Capabilities { - if !node.Capabilities[c] { - problems = append(problems, fmt.Sprintf( - "%s needs the capability %q and %s does not have it — this is the wrong "+ - "machine, not a missing module", m.Module, c, node.Name)) - } + for _, c := range missingCapabilities(m, node) { + problems = append(problems, WrongMachine(m.Module, c, node.Name)) } } return problems diff --git a/internal/catalogue/resolve_test.go b/internal/catalogue/resolve_test.go index 5288036..80c4e69 100644 --- a/internal/catalogue/resolve_test.go +++ b/internal/catalogue/resolve_test.go @@ -131,20 +131,85 @@ func TestAThirdModuleNeedsNoChangeToTheOthers(t *testing.T) { } } -func TestAMissingCapabilityIsSaidToBeTheWrongMachine(t *testing.T) { - // The remedy differs from a missing module and the message has to say which. Nothing can be - // installed to give a server a seat. +func TestADirectlyAssignedModuleTheMachineCannotHostIsReportedNotRefused(t *testing.T) { + // A person put a display server on a seatless machine. The remedy differs from a missing module + // and the message has to say which — nothing can be installed to give a server a seat. But it is + // one module on the wrong machine, not a reason the whole node fails to resolve: it is kept out + // of what the node runs and reported as un-applied, so the healthy modules beside it still + // converge. server := Node{Name: "server", Capabilities: map[string]bool{"container-runtime": true}} - _, err := Resolve(shelf(mod("xorg", nil, nil, []string{"seat"}, Claim{Name: "the-seat"})), []string{"xorg"}, server, World{}) + got, err := Resolve(shelf(mod("xorg", nil, nil, []string{"seat"}, Claim{Name: "the-seat"})), []string{"xorg"}, server, World{}) + + if err != nil { + t.Fatalf("one un-hostable assignment refused the whole node: %v", err) + } + if len(got.Modules) != 0 { + t.Errorf("xorg was declared on a machine that cannot run it: %v", names(got)) + } + if len(got.Unhostable) != 1 || got.Unhostable[0].Module != "xorg" { + t.Fatalf("xorg was not reported as un-applied: %+v", got.Unhostable) + } + if len(got.Unhostable[0].Missing) != 1 || got.Unhostable[0].Missing[0] != "seat" { + t.Errorf("the missing capability was not named: %+v", got.Unhostable[0]) + } + if !strings.Contains(WrongMachine("xorg", "seat", "server"), "wrong machine") { + t.Errorf("the report reads like a missing module") + } +} + +func TestAnUnhostableModuleSomethingRequiresRefusesTheNode(t *testing.T) { + // The other side of the distinction. A module the machine cannot host that is *required* by + // something running here makes the set incoherent: the requiring module cannot have its + // requirement met on this machine, so it is refused rather than quietly declared without it. + server := Node{Name: "server", Capabilities: map[string]bool{"container-runtime": true}} + _, err := Resolve(shelf( + mod("desktop", nil, []string{"display-server"}, nil), + mod("xorg", []string{"display-server"}, nil, []string{"seat"}), + ), []string{"desktop"}, server, World{}) if err == nil { - t.Fatal("a display server was assigned to a machine with no seat") + t.Fatal("a node requiring a module the machine cannot host was resolved") } if !strings.Contains(err.Error(), "wrong machine") { t.Errorf("the refusal reads like a missing module: %v", err) } } +func TestOneUnhostableAssignmentDoesNotTakeDownTheHealthyModules(t *testing.T) { + // The bug the whole-mesh dry-run found: fail2ban declares a capability the host lacks, and its + // one un-hostable assignment refused the entire node's resolution — so a push refused to send + // the healthy modules beside it too. The healthy modules must still resolve and converge; the + // un-hostable one is reported as un-applied, neither silently dropped nor fatal to the rest. + server := Node{Name: "server", Capabilities: map[string]bool{"container-runtime": true}} + got, err := Resolve(shelf( + mod("web", nil, nil, nil), + mod("cache", nil, nil, nil), + mod("cron", nil, nil, nil), + // The one on the wrong machine: it needs a firewall the machine's profile does not report. + mod("fail2ban", nil, nil, []string{"firewall"}), + ), []string{"web", "cache", "cron", "fail2ban"}, server, World{}) + + if err != nil { + t.Fatalf("one un-hostable assignment refused the whole node: %v", err) + } + // The healthy three resolved. + if got := names(got); len(got) != 3 { + t.Fatalf("the healthy modules did not all resolve: %v", got) + } + for _, m := range got.Modules { + if m.Module == "fail2ban" { + t.Error("fail2ban was declared on a machine that cannot run it") + } + } + // The un-hostable one is reported, not silently dropped. + if len(got.Unhostable) != 1 || got.Unhostable[0].Module != "fail2ban" { + t.Fatalf("fail2ban was not reported as un-applied: %+v", got.Unhostable) + } + if len(got.Unhostable[0].Missing) != 1 || got.Unhostable[0].Missing[0] != "firewall" { + t.Errorf("the missing capability was not named: %+v", got.Unhostable[0]) + } +} + func TestAMeshWideClaimIsHeldByOneNode(t *testing.T) { // The hub, said as a claim rather than hard-coded. Another node already holds it, so this one // cannot. @@ -242,12 +307,17 @@ func TestWhatAServiceReflectsIsQualifiedToo(t *testing.T) { } func TestEveryReasonIsGivenAtOnce(t *testing.T) { - // Somebody resolving these fixes them in one pass or in four. - server := Node{Name: "server", Capabilities: map[string]bool{}} + // Somebody resolving these fixes them in one pass or in three. Every reason that genuinely + // refuses the set is collected, rather than only the first: a claim two modules both take, a + // requirement nothing answers, and a requirement several answer without anybody choosing. _, err := Resolve(shelf( - mod("xorg", nil, nil, []string{"seat"}, Claim{Name: "the-seat"}), - mod("wayland", nil, nil, []string{"seat"}, Claim{Name: "the-seat"}), - ), []string{"xorg", "wayland"}, server, World{}) + mod("xorg", nil, nil, nil, Claim{Name: "the-seat"}), + mod("wayland", nil, nil, nil, Claim{Name: "the-seat"}), + mod("i3", nil, []string{"compositor"}, nil), + mod("editor", nil, []string{"shell"}, nil), + mod("bash", []string{"shell"}, nil, nil), + mod("zsh", []string{"shell"}, nil, nil), + ), []string{"xorg", "wayland", "i3", "editor"}, workstation(), World{}) if err == nil { t.Fatal("expected refusals")