WIP: recheck a take's filesystem, follow only the module's branch, count users in an access change, hold the identity until its data is taken (hq ADR 0325 review)

cmd/mesh-controller/asker.go

@@ -892,7 +892,7 @@ func (a *asker) Decided(ctx context.Context, body []byte) error {
 		// A shared folder taken on the operator's warrant (novox/hq ADR 0325 §8): the act is composed again from the
 		// record and held to what the option bound before the engine is asked.
 		outcome, acted = a.decideTake(ctx, *r, act, w)
-		if acted == nil {
+		if acted == nil && !strings.HasPrefix(outcome, "nothing") {
 			outcome = "done: " + outcome
 		}
 	case r.Take != nil:

cmd/mesh-controller/gate.go

@@ -814,11 +814,18 @@ func movesChangingAccesses(ctx context.Context, inv *inventory.Inventory, g *inv
 			out[j.module] = true
 			continue
 		}
-		out[j.module] = !sameAccesses(before.Accesses, target.Accesses)
+		out[j.module] = !sameAccessUse(before, target)
 	}
 	return out
 }
 
+// sameAccessUse says a move leaves a module's accesses as they were, users included (the review of e1c2b744): the
+// same accesses at the same use, and the same containers declaring identity and mounting each — a container that
+// starts or stops running as the module's identity changes who uses the folder.
+func sameAccessUse(a, b catalogue.Manifest) bool {
+	return sameAccesses(a.Accesses, b.Accesses) && catalogue.AccessUsersShape(a) == catalogue.AccessUsersShape(b)
+}
+
 // sameAccesses says two manifests declare the same accesses, at the same use, in any order.
 func sameAccesses(a, b []catalogue.Access) bool {
 	key := func(as []catalogue.Access) string {

cmd/mesh-controller/kept_folders.go

@@ -235,6 +235,24 @@ func (a *asker) decideTake(ctx context.Context, r asked, act conditions.Action,
 	if a.take == nil {
 		return "", errors.New("this controller cannot take a folder on a warrant")
 	}
+	// **The folder must still wait on the filesystem the operator was asked about** (the review of e1c2b744): a take
+	// is about no condition, so nothing else rechecks it. A folder that moved to another filesystem, or no longer
+	// waits for the operator's word, is not taken: the ask is spent, and one for the filesystem it is on now is asked.
+	if a.folders != nil {
+		waiting, err := a.folders(ctx)
+		if err != nil {
+			return "", fmt.Errorf("whether the folder still waits on %s could not be read, so nothing is taken: %w",
+				t.Node, err)
+		}
+		still := false
+		for _, now := range waiting {
+			still = still || now.key() == t.key()
+		}
+		if !still {
+			return "nothing: the folder no longer waits to be kept on the filesystem the operator was asked about " +
+				"(it moved, or was taken or released meanwhile); asked again where it waits now", nil
+		}
+	}
 	by := fmt.Sprintf("approved by %s via %s at %s (ask %s)", byWords(w), viaWords(w),
 		w.At.Local().Format("2006-01-02 15:04"), r.ID)
 	return a.take(ctx, t, r.ID, by)

cmd/mesh-controller/kept_folders_test.go

@@ -404,8 +404,8 @@ func TestAFolderThatMovedIsAskedAboutAgain(t *testing.T) {
 	}
 }
 
-// The mesh's entries left while a disk was away (kind kept-entries), stated before the send, do not hold a delivery
-// that did not bring them; stated since, they do — and they may name an access no longer declared.
+// The mesh's entries left while a disk was away (kind kept-entries) do not hold a delivery whose move leaves the
+// module's accesses as they were — they may name an access no longer declared; one that changes them is held.
 func TestEntriesLeftWhileADiskWasAwayDoNotHoldADeliveryThatDidNotBringThem(t *testing.T) {
 	now := time.Now()
 	since := now.Add(-time.Minute)
@@ -413,15 +413,19 @@ func TestEntriesLeftWhileADiskWasAwayDoNotHoldADeliveryThatDidNotBringThem(t *te
 		Target: moviesAt, State: link.StateUnhealthy, Reason: "the mesh's entries could not be taken out: its disk is away",
 		Since: since.Add(-time.Hour)}
 	f := gateFacts{now: now, waits: accessFacts(), groupsAdded: map[string]bool{"radarr": false},
-		health: map[string]inventory.NodeHealth{"ace": {Node: "ace", HeardAt: now,
-			Resources: []inventory.ResourceHealth{left}}}}
+		accessesChanged: map[string]bool{"radarr": false}, health: map[string]inventory.NodeHealth{"ace": {Node: "ace",
+			HeardAt: now, Resources: []inventory.ResourceHealth{left}}}}
 	if h, why := moduleHealthWord("radarr", "ace", since, f); h != healthPerson || !strings.Contains(why, "access-gone~entries") {
 		t.Fatalf("entries left before the send read %v %q; want carried in a passing verdict", h, why)
 	}
 	left.Since = now
 	f.health["ace"] = inventory.NodeHealth{Node: "ace", HeardAt: now, Resources: []inventory.ResourceHealth{left}}
+	if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthPerson {
+		t.Fatalf("entries stated since the send by a move that leaves accesses as they were read %v", h)
+	}
+	f.accessesChanged = nil // a move not known changes them
 	if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthNotYet {
-		t.Fatalf("entries left by this send read %v; want judged unhealthy", h)
+		t.Fatalf("entries of a move not known read %v; want judged unhealthy", h)
 	}
 }
 
@@ -494,3 +498,66 @@ func TestTheEnginesAccessWaitsAndEntriesAreKept(t *testing.T) {
 		t.Fatalf("the listing reads:\n%s", all)
 	}
 }
+
+// Review of e1c2b744: a warrant for a folder on one filesystem never takes it on another. When the warrant comes and
+// the folder no longer waits on the filesystem the ask named, nothing is sent, the ask is spent, and the operator is
+// asked again for the filesystem it is on now.
+func TestAWarrantForOneFilesystemIsNotCarriedOutOnAnother(t *testing.T) {
+	placed := "fs-000000000001"
+	r, taken := takeRig(t, func() map[string]map[string][]waitingFolder {
+		return waitingOn("ace", waitingFolder{module: "radarr", access: "movies", resource: "radarr.access-movies",
+			placed: placed})
+	})
+	ctx := context.Background()
+	if err := r.a.reconcile(ctx); err != nil {
+		t.Fatal(err)
+	}
+	a := takeAsks(r)[0]
+	placed = "fs-0000000000ff" // the folder moved before the operator answered
+	answerWith(t, r, warrantOn(a, "let-the-mesh-keep-it", r.now))
+	if len(*taken) != 0 {
+		t.Fatalf("a warrant for %s was carried out on %s: %v", a.Take.Placed, placed, *taken)
+	}
+	if spent := r.store[a.ID]; spent.State == askOpen || !strings.HasPrefix(spent.Acted, "nothing") {
+		t.Fatalf("the ask after the folder moved: %s, %q", spent.State, spent.Acted)
+	}
+	if err := r.a.reconcile(ctx); err != nil {
+		t.Fatal(err)
+	}
+	var again []asked
+	for _, x := range takeAsks(r) {
+		if x.State == askOpen {
+			again = append(again, x)
+		}
+	}
+	if len(again) != 1 || again[0].Take.Placed != placed {
+		t.Fatalf("not asked again for the filesystem the folder is on now: %+v", again)
+	}
+}
+
+// Review of e1c2b744: a move that changes who uses an access — a container of the module that starts declaring
+// identity — changes the access, so an access the engine cannot judge is not passed uncounted.
+func TestAMoveThatChangesAnAccesssUsersChangesTheAccess(t *testing.T) {
+	before := catalogue.Manifest{Module: "radarr", Accesses: radarrManifest.Accesses, Resources: []map[string]any{
+		{"id": "app", "type": "container", "volumes": []any{"${access:movies}:/movies"}}}}
+	after := catalogue.Manifest{Module: "radarr", Accesses: radarrManifest.Accesses, Resources: []map[string]any{
+		{"id": "app", "type": "container", "identity": map[string]any{"user": "run"},
+			"volumes": []any{"${access:movies}:/movies"}}}}
+	if sameAccessUse(before, after) {
+		t.Fatal("a move that adds identity to the container mounting the access left it the same")
+	}
+	if !sameAccessUse(before, before) {
+		t.Fatal("an unchanged manifest reads changed")
+	}
+	// Entries left while a disk was away, stated before the send, of a move that changes the access: counted.
+	now := time.Now()
+	since := now.Add(-time.Minute)
+	left := inventory.ResourceHealth{Module: "radarr", Resource: "radarr.access-movies~entries", Kind: link.KindKeptEntries,
+		Target: moviesAt, State: link.StateUnhealthy, Since: since.Add(-time.Hour)}
+	f := gateFacts{now: now, waits: accessFacts(), groupsAdded: map[string]bool{"radarr": false},
+		accessesChanged: map[string]bool{"radarr": true}, health: map[string]inventory.NodeHealth{"ace": {Node: "ace",
+			HeardAt: now, Resources: []inventory.ResourceHealth{left}}}}
+	if h, _ := moduleHealthWord("radarr", "ace", since, f); h != healthNotYet {
+		t.Fatalf("entries left before the send by a move that changes the access read %v; want counted", h)
+	}
+}

cmd/mesh-controller/module_health.go

@@ -678,12 +678,13 @@ func moduleHealthWord(module, machine string, since time.Time, f gateFacts) (hea
 			continue
 		}
 		// **The mesh's entries left on a folder whose disk was away are a condition this build found** when its move
-		// leaves the module's accesses as they were, or they were stated before this send (novox/hq ADR 0325 §8): they
-		// may name an access no longer declared, they wait for the disk, and a delivery that did not bring them is not
-		// held for them — said in the verdict, as a directory used as found is. Otherwise they are judged unhealthy.
+		// leaves the module's accesses and their users as they were (novox/hq ADR 0325 §8): they may name an access no
+		// longer declared, they wait for the disk, and a delivery that did not bring them is not held for them — said in
+		// the verdict, as a directory used as found is. A move that changes the accesses is held to them, whenever they
+		// were stated (the review of e1c2b744).
 		changed, known := f.accessesChanged[module]
 		changes := !known || changed
-		if r.Kind == link.KindKeptEntries && r.State == link.StateUnhealthy && (!changes || r.Since.Before(since)) {
+		if r.Kind == link.KindKeptEntries && r.State == link.StateUnhealthy && !changes {
 			leftover = append(leftover, r.Resource)
 			continue
 		}

cmd/mesh-controller/plan.go

@@ -977,7 +977,7 @@ func renderingFor(ctx context.Context, open *stores, node string,
 		return catalogue.Rendering{}, inventory.Node{}, err
 	}
 	// Each module's identity and the folders taken here (novox/hq ADR 0325 §3, §8), to an engine that keeps folders.
-	keepsFolders, identities, keptFolders, err := folderFacts(ctx, inv, node, plan, choosing)
+	keepsFolders, identities, keptFolders, readyFolders, err := folderFacts(ctx, inv, node, plan, choosing)
 	if err != nil {
 		return catalogue.Rendering{}, inventory.Node{}, err
 	}
@@ -987,6 +987,7 @@ func renderingFor(ctx context.Context, open *stores, node string,
 		KeepsFolders:  keepsFolders,
 		Identities:    identities,
 		KeptFolders:   keptFolders,
+		ReadyFolders:  readyFolders,
 		BusMembership: memberships[node],
 		Settings:      settings, Generators: gens, Grants: grants, Needed: needed, Foreseen: foreseen, Ports: ports,
 		Certificate: certificate, Authority: authority, Mesh: private, Names: names,
@@ -1035,10 +1036,10 @@ func engineKeepsFolders(ctx context.Context, inv *inventory.Inventory, node stri
 // (Allocating), as a port is: a question about what would be sent reads what was allocated, and a module that has
 // none yet is composed without one. Nothing is read for an engine that does not keep folders.
 func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, plan catalogue.Resolution,
-	choosing Choosing) (bool, map[string]catalogue.ModuleIdentity, map[string]bool, error) {
+	choosing Choosing) (bool, map[string]catalogue.ModuleIdentity, map[string]bool, map[string]bool, error) {
 	keeps, err := engineKeepsFolders(ctx, inv, node)
 	if err != nil || !keeps {
-		return false, nil, nil, err
+		return false, nil, nil, nil, err
 	}
 	identities := map[string]catalogue.ModuleIdentity{}
 	for _, m := range plan.Modules {
@@ -1053,7 +1054,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla
 			id, had, err = inv.IdentityOf(ctx, m.Module)
 		}
 		if err != nil {
-			return false, nil, nil, err
+			return false, nil, nil, nil, err
 		}
 		if had {
 			identities[m.Module] = id
@@ -1061,7 +1062,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla
 	}
 	taken, err := inv.KeptFolders(ctx, node)
 	if err != nil {
-		return false, nil, nil, err
+		return false, nil, nil, nil, err
 	}
 	kept := map[string]bool{}
 	for path := range taken {
@@ -1069,9 +1070,17 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla
 	}
 	// **A folder that moved to another filesystem is not taken there until its new warrant** (ADR 0325 §8): the
 	// machine's newest statement names, without a path, the filesystem each folder waiting for the operator is on now.
+	// And every folder the engine says it keeps (its access healthy): one it found empty, or took itself — what, with
+	// a taken one, lets a container switch to its identity (identityHeld).
+	ready := map[string]bool{}
 	if stated, had, err := inv.HealthOf(ctx, node); err != nil {
-		return false, nil, nil, err
+		return false, nil, nil, nil, err
 	} else if had {
+		for _, r := range stated.Resources {
+			if r.Kind == link.KindAccess && r.State == link.StateHealthy && r.Target != "" {
+				ready[filepath.Clean(r.Target)] = true
+			}
+		}
 		for _, r := range stated.Resources {
 			for _, w := range r.Waits {
 				path := filepath.Clean(r.Target)
@@ -1081,7 +1090,7 @@ func folderFacts(ctx context.Context, inv *inventory.Inventory, node string, pla
 			}
 		}
 	}
-	return true, identities, kept, nil
+	return true, identities, kept, ready, nil
 }
 
 // zonesInTheMesh is every zone a module in the mesh declares, where the mesh placed it (novox/hq ADR