From 595d2a47471fbae07d3e80cbbe146df968ead521 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 26 Oct 2026 08:37:09 +0000 Subject: [PATCH 1/2] refactor(git): key a worker's checkout by its GitBranch UID Local clones move from /tmp/gitops-reverser-workers/{provider UID}/{branch-component} to /tmp/gitops-reverser-workers/{GitBranch UID}. A GitBranch pins its branch, the claim gives a repository branch to one GitBranch, and EnsureWorker keeps one live worker per GitBranch, so its UID is a complete name for the checkout. The branch name no longer appears in the path, so branchPathComponent goes. EnsureWorker reads the UID from the claim holder it already checks. A provider recreated against another URL keeps the GitBranch, so its replacement worker reuses the directory after the old worker has removed the clone in it. Co-Authored-By: Claude Opus 5.5 (1M context) --- docs/architecture.md | 24 ++-- .../controller/git_branch_helpers_test.go | 2 +- internal/git/branch_worker.go | 82 ++++------- internal/git/claim_helpers_test.go | 8 +- internal/git/helpers_test.go | 4 + internal/git/worker_manager.go | 22 +-- internal/git/worker_manager_test.go | 127 ++++++++---------- internal/watch/git_branch_helpers_test.go | 2 +- 8 files changed, 118 insertions(+), 153 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 618993b1..33464fc2 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1560,19 +1560,17 @@ pushed at once, cooldown or not, because the apply waits for them. ### Local clones and conflict retry -Local clones live under `/tmp/gitops-reverser-workers/{provider UID}/{branch-component}`. The first -segment is the `GitProvider`'s `metadata.uid`: globally unique, and minted fresh by the API server for -every object, so it says which **incarnation** of the provider this is. `spec.url` is immutable and a -repoint is therefore a recreate, which means a replacement worker cannot inherit, share, or delete the -directory its predecessor was using. Two providers naming one repository do not share a checkout -either. A worker built with no provider behind it (the CLI, and tests) has no UID to be unique by and -falls back to a digest of its remote. - -`{branch-component}` is the branch rendered as **one** path component -([`branchPathComponent`](../internal/git/branch_worker.go)): a readable prefix for whoever is looking at -the directory, plus a digest of the raw name. Branch names contain slashes, and joining one in would make -`release/v1`'s directory a child of `release`'s, which is data loss the moment a retired worker deletes -its own tree. [PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the +Local clones live under `/tmp/gitops-reverser-workers/{GitBranch UID}`: one directory per +`GitBranch`, named by its `metadata.uid`. A `GitBranch` pins its branch (`spec.branch` is immutable), +the branch claim gives a repository branch to one `GitBranch` at a time, and a UID is minted fresh for +every object, so no two live workers share a directory and a `GitBranch` recreated under its name starts +in a new one. The branch name is not part of the path, so its slashes cannot nest one checkout inside +another. The repository is the one thing the UID does not pin: a `GitProvider` recreated against +another URL keeps the `GitBranch`, so its replacement worker reuses the directory, after the old worker +has removed the clone in it. A worker never trusts a checkout it finds on disk: its first cycle repoints +`origin`, fetches and resets. + +[PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the remote diverged it smart fetches the latest tip, hard resets the local clone, replays the retained pending writes against the fresh tip (refreshing commit hashes), and retries up to the attempt limit. This is valid because every pending write is rebuilt from sanitized API state; nothing depends on locally edited files. diff --git a/internal/controller/git_branch_helpers_test.go b/internal/controller/git_branch_helpers_test.go index 6db7fb63..21c89708 100644 --- a/internal/controller/git_branch_helpers_test.go +++ b/internal/controller/git_branch_helpers_test.go @@ -24,7 +24,7 @@ var shopRepo1Main = k8stypes.NamespacedName{Namespace: "shop", Name: gitBranchNa func claimWorkerForTest(m *git.WorkerManager, repo git.RepoIdentity) git.WorkerRequest { m.Claims().Observe(git.BranchKeyFor(repo.URL, "main"), git.BranchClaimant{ GitBranch: shopRepo1Main, - UID: k8stypes.UID(shopRepo1Main.String()), + UID: "uid-shop-repo1-main", Created: time.Unix(0, 0), }, true) return git.WorkerRequest{ diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 4843d5f1..811a3545 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -70,10 +70,12 @@ var ( // It can serve multiple GitTargets that write to different paths in the same branch. // This design ensures serialized commits per branch, preventing merge conflicts. type BranchWorker struct { - // Identity (immutable after creation). GitBranch is the claim holder this worker writes for; - // GitProviderRef and GitProviderNamespace name that GitBranch's GitProvider, whose credential - // and signing key it uses. + // Identity (immutable after creation). GitBranch is the claim holder this worker writes for, and + // GitBranchUID its metadata.uid, which names the checkout; GitProviderRef and + // GitProviderNamespace name that GitBranch's GitProvider, whose credential and signing key it + // uses. GitBranch types.NamespacedName + GitBranchUID types.UID GitProviderRef string GitProviderNamespace string Branch string @@ -459,61 +461,32 @@ func NewBranchWorker( //nolint:gochecknoglobals // a test seam, like pushAtomicFn above var workerStateRoot = filepath.Join("/tmp", "gitops-reverser-workers") -// branchPathComponent renders a branch name as ONE directory component. +// repoRootPath is where this worker keeps its checkout: one directory, named by the UID of the +// GitBranch it writes for. // -// Git branch names contain slashes — `release/v1` is ordinary — and joining one into a path makes -// its worker's directory a CHILD of the `release` worker's. That is harmless while nothing deletes -// anything, and it is a data-loss bug the moment something does: retiring `release` would take the -// live `release/v1` worker's checkout with it. +// The GitBranch pins the branch (spec.branch is immutable), the claim gives a repository branch to +// one GitBranch at a time, and EnsureWorker keeps at most one live worker per GitBranch, so no two +// live workers share a directory. A UID is minted fresh for every object, so a GitBranch deleted +// and recreated under its name starts in a new directory. And because the branch is not part of +// the path, a branch name's slashes cannot nest one worker's checkout inside another's. // -// The readable prefix is for whoever is looking at the directory; the hash of the raw name is what -// makes it unambiguous, including for the names sanitising would otherwise collapse together -// (`release/v1` and `release-v1`). -func branchPathComponent(branch string) string { - sum := sha256.Sum256([]byte(branch)) - digest := hex.EncodeToString(sum[:8]) - - readable := make([]rune, 0, len(branch)) - for _, r := range branch { - switch { - case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9', r == '-', r == '_': - readable = append(readable, r) - default: - readable = append(readable, '-') - } - } - if len(readable) > branchComponentReadableMax { - readable = readable[:branchComponentReadableMax] - } - return string(readable) + "-" + digest -} - -// branchComponentReadableMax caps the readable half so a long branch name cannot push the path -// past what a filesystem accepts; the digest that follows is what carries the identity. -const branchComponentReadableMax = 40 - -// repoRootPath is where this worker keeps its checkout: the GitProvider's UID, then the branch. -// -// The UID is the whole of the first component, and it replaces three things that were there -// before: the provider's namespace, its name, and a digest of the remote URL. A UID is globally -// unique and is minted fresh by the API server for every object, so it says everything those -// three said and one thing they could not — WHICH INCARNATION of the provider this is. A -// repointed provider is a recreated one, so its worker cannot inherit, share, or delete the -// directory its predecessor was using, and no digest has to stand in for that. +// The repository is the one thing the UID does not pin: the GitProvider is named, not owned, and +// can be recreated against another URL. EnsureWorker removes the old worker's checkout before the +// replacement starts in the same directory, and a checkout left behind by a restart is never +// trusted: a new worker's first cycle repoints origin, fetches and resets. func (w *BranchWorker) repoRootPath() string { - return filepath.Join(workerStateRoot, w.stateComponent(), branchPathComponent(w.Branch)) + return filepath.Join(workerStateRoot, w.stateComponent()) } // stateComponent names this worker's state directory. // -// A worker built with no GitProvider behind it — the CLI, and tests — has no UID to be unique by, -// and every one of them would otherwise share one directory per branch. Those workers fall back to -// a digest of the remote they were built for, which is the only identity they have. +// A worker built with no GitBranch behind it — tests — has no UID to be unique by. It falls back +// to a digest of its remote and branch, which is the only identity it has. func (w *BranchWorker) stateComponent() string { - if w.repo.ProviderUID != "" { - return string(w.repo.ProviderUID) + if w.GitBranchUID != "" { + return string(w.GitBranchUID) } - sum := sha256.Sum256([]byte(strings.TrimSpace(w.repo.URL))) + sum := sha256.Sum256([]byte(strings.TrimSpace(w.repo.URL) + "\x00" + w.Branch)) return "url-" + hex.EncodeToString(sum[:16]) } @@ -529,18 +502,17 @@ func (w *BranchWorker) stateComponent() string { // have that happen to it; the WorkerManager replaces it instead. func (w *BranchWorker) repoPath() string { return w.repoRootPath() } -// removeLocalState deletes everything this worker kept on disk: the clone of every remote it -// worked with, under its own (provider UID, branch) directory. +// removeLocalState deletes everything this worker kept on disk: its GitBranch's directory. // // It is called only when a worker is being retired because nothing needs it any more, never on // shutdown — a restart wants the clones it left behind, and re-cloning every branch on every // restart is exactly the cost the on-disk cache exists to avoid. // -// The identity guard is not defensive programming. repoRootPath joins the UID and the branch into -// a fixed prefix, so a worker with neither — which tests build — resolves to the ROOT of every -// worker's state, and deleting that would take out the live workers next to it. +// Only a worker with a GitBranch UID reclaims anything. That is every worker the WorkerManager +// starts; one a test builds by hand has a directory derived from whatever identity it was given, +// down to none at all, and deleting a directory nobody can name is how a live worker's goes too. func (w *BranchWorker) removeLocalState() { - if w.repo.IsZero() || w.Branch == "" { + if w.GitBranchUID == "" { w.Log.V(1).Info("Not removing worker state: the worker has no complete identity") return } diff --git a/internal/git/claim_helpers_test.go b/internal/git/claim_helpers_test.go index bac3f0de..8255fd32 100644 --- a/internal/git/claim_helpers_test.go +++ b/internal/git/claim_helpers_test.go @@ -21,7 +21,7 @@ func claimForTest(m *WorkerManager, provider, namespace, branch string, repo Rep gitBranch := testGitBranch(provider, namespace, branch) m.Claims().Observe(BranchKeyFor(repo.URL, branch), BranchClaimant{ GitBranch: gitBranch, - UID: k8stypes.UID(gitBranch.String()), + UID: testGitBranchUID(gitBranch), Created: time.Unix(0, 0), }, true) return WorkerRequest{ @@ -32,5 +32,11 @@ func claimForTest(m *WorkerManager, provider, namespace, branch string, repo Rep } } +// testGitBranchUID is the UID claimForTest gives gitBranch. Like a real one, it holds no slash: it +// names the worker's checkout directory. +func testGitBranchUID(gitBranch k8stypes.NamespacedName) k8stypes.UID { + return k8stypes.UID("uid-" + gitBranch.Namespace + "-" + gitBranch.Name) +} + // testGitBranchName is the name of testGitBranch(provider, namespace, branch). func testGitBranchName(provider, branch string) string { return provider + "-" + branch } diff --git a/internal/git/helpers_test.go b/internal/git/helpers_test.go index aafc077e..59cfd40e 100644 --- a/internal/git/helpers_test.go +++ b/internal/git/helpers_test.go @@ -237,6 +237,10 @@ func newTestBranchWorker( nil, BranchWorkerLimits{}, ) + // The GitBranch's UID names the checkout, as it does for every worker the WorkerManager starts, + // so it differs wherever the provider UID above does, and per branch too. + branchDigest := sha256.Sum256([]byte(string(providerUID) + "\x00" + branch)) + worker.GitBranchUID = k8stypes.UID("uid-gitbranch-" + hex.EncodeToString(branchDigest[:8])) worker.ctx = context.Background() return worker, nil } diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index ab7f066a..37b9b4df 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -414,7 +414,8 @@ func (m *WorkerManager) EnsureWorker(_ context.Context, req WorkerRequest) error if key == (BranchKey{}) { return fmt.Errorf("GitProvider %s names no repository", req.Provider) } - if err := m.refuseUnclaimed(key, req.GitBranch); err != nil { + holder, err := m.refuseUnclaimed(key, req.GitBranch) + if err != nil { return err } @@ -459,27 +460,31 @@ func (m *WorkerManager) EnsureWorker(_ context.Context, req WorkerRequest) error defer m.mu.Unlock() if _, live := m.workers[key]; !live { - return m.startWorkerLocked(key, req) + return m.startWorkerLocked(key, req, holder.UID) } return nil } // refuseUnclaimed is EnsureWorker's claim check: only the GitBranch holding key gets its worker. -func (m *WorkerManager) refuseUnclaimed(key BranchKey, gitBranch k8stypes.NamespacedName) error { +// It returns that holder. +func (m *WorkerManager) refuseUnclaimed(key BranchKey, gitBranch k8stypes.NamespacedName) (BranchClaimant, error) { holder, held := m.Claims().Holder(key) switch { case !held: - return fmt.Errorf("%w: %s, and no GitBranch holds it yet", ErrBranchOwnedElsewhere, key) + return BranchClaimant{}, fmt.Errorf("%w: %s, and no GitBranch holds it yet", ErrBranchOwnedElsewhere, key) case holder.GitBranch != gitBranch: - return fmt.Errorf("%w: %s, held by GitBranch %s", ErrBranchOwnedElsewhere, key, holder.GitBranch) + return BranchClaimant{}, fmt.Errorf("%w: %s, held by GitBranch %s", + ErrBranchOwnedElsewhere, key, holder.GitBranch) } - return nil + return holder, nil } // startWorkerLocked creates and starts the worker for key, writing through provider, and records -// it. Called with lifecycleMu and mu held. -func (m *WorkerManager) startWorkerLocked(key BranchKey, req WorkerRequest) error { +// it. gitBranchUID is the claim holder's UID, read from the claim rather than the request because +// the claim is what EnsureWorker just checked; it names the worker's checkout. Called with +// lifecycleMu and mu held. +func (m *WorkerManager) startWorkerLocked(key BranchKey, req WorkerRequest, gitBranchUID k8stypes.UID) error { provider, repo, parentBranch := req.Provider, req.Repo, req.ParentBranch providerName, providerNamespace, branch := provider.Name, provider.Namespace, key.Branch m.Log.Info("Creating new branch worker", "key", key.String(), "repository", repo.String()) @@ -497,6 +502,7 @@ func (m *WorkerManager) startWorkerLocked(key BranchKey, req WorkerRequest) erro // goroutine Start spawns, so setting it here (under m.mu, before that goroutine // exists) is race-free. worker.GitBranch = req.GitBranch + worker.GitBranchUID = gitBranchUID worker.SetParentBranch(parentBranch) worker.mapper = m.mapper worker.clusterMapper = m.clusterMapper diff --git a/internal/git/worker_manager_test.go b/internal/git/worker_manager_test.go index 62597e95..9557f94b 100644 --- a/internal/git/worker_manager_test.go +++ b/internal/git/worker_manager_test.go @@ -7,7 +7,6 @@ import ( "errors" "os" "path/filepath" - "strings" "testing" "time" @@ -448,37 +447,21 @@ func TestReconcileWorkers_RemovesTheRetiredWorkersCheckout(t *testing.T) { assert.True(t, os.IsNotExist(err), "the retired worker's checkout must go with it") } -// TestRemoveLocalState_RefusesAWorkerWithNoIdentity. repoRootPath joins the identity and the -// branch into a fixed prefix, so a worker with neither resolves to the ROOT of every worker's -// state. Deleting that would take out the live workers beside it, which is a far worse outcome -// than the leak this reclaims. +// TestRemoveLocalState_RefusesAWorkerWithNoIdentity. Only a worker the WorkerManager started has a +// GitBranch UID, and only its directory is known to be its own. Deleting anything else risks the +// live workers beside it, which is a far worse outcome than the leak this reclaims. func TestRemoveLocalState_RefusesAWorkerWithNoIdentity(t *testing.T) { root := withTemporaryWorkerStateRoot(t) require.NoError(t, os.MkdirAll(filepath.Join(root, "someone-elses-state"), 0o750)) (&BranchWorker{Log: logr.Discard()}).removeLocalState() + (&BranchWorker{Log: logr.Discard(), repo: RepoIdentity{URL: "https://example.invalid/repo.git"}, Branch: "main"}). + removeLocalState() - _, err := os.Stat(filepath.Join(root, "someone-elses-state")) - assert.NoError(t, err, "a worker with no identity must never delete the shared root") -} - -// TestBranchPathComponent_KeepsOneBranchOutOfAnothersDirectory. Git branch names contain slashes, -// so joining one into a path made `release/v1`'s worker a child of `release`'s — and once a -// retired worker deletes its directory, retiring `release` would take the live worker's checkout -// with it. -func TestBranchPathComponent_KeepsOneBranchOutOfAnothersDirectory(t *testing.T) { - withTemporaryWorkerStateRoot(t) - provider := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"} - parent := &BranchWorker{repo: provider, Branch: "release"} - child := &BranchWorker{repo: provider, Branch: "release/v1"} - - rel, err := filepath.Rel(parent.repoRootPath(), child.repoRootPath()) + entries, err := os.ReadDir(root) require.NoError(t, err) - assert.True(t, strings.HasPrefix(rel, ".."), "release/v1 must not be reachable from inside release") - - // And names that sanitising alone would collapse stay apart. - lookalike := &BranchWorker{repo: provider, Branch: "release-v1"} - assert.NotEqual(t, child.repoRootPath(), lookalike.repoRootPath()) + require.Len(t, entries, 1, "a worker with no GitBranch UID must never delete anything") + assert.Equal(t, "someone-elses-state", entries[0].Name()) } // withTemporaryWorkerStateRoot points every worker's on-disk state at this test's own directory, @@ -493,23 +476,31 @@ func withTemporaryWorkerStateRoot(t *testing.T) string { return root } -// TestRepoRootPath_IsKeyedByTheProviderIncarnation. The UID is the whole of the first path -// component, which is what makes a repointed provider's worker unable to inherit, share or delete -// the directory its predecessor was using: a repoint is a recreate, and a recreate mints a UID. -func TestRepoRootPath_IsKeyedByTheProviderIncarnation(t *testing.T) { - withTemporaryWorkerStateRoot(t) - sameURL := "https://example.invalid/repo.git" - before := &BranchWorker{repo: RepoIdentity{ProviderUID: "uid-1", URL: sameURL}, Branch: "main"} - after := &BranchWorker{repo: RepoIdentity{ProviderUID: "uid-2", URL: sameURL}, Branch: "main"} - - assert.NotEqual(t, before.repoRootPath(), after.repoRootPath(), - "the same URL under a recreated provider is a different checkout") - - // A worker with no provider behind it — the CLI, and tests — has only its remote to be unique - // by, and two of them must still not share a directory. - cliA := &BranchWorker{repo: RepoIdentity{URL: "https://example.invalid/a.git"}, Branch: "main"} - cliB := &BranchWorker{repo: RepoIdentity{URL: "https://example.invalid/b.git"}, Branch: "main"} - assert.NotEqual(t, cliA.repoRootPath(), cliB.repoRootPath()) +// TestRepoRootPath_IsOneDirectoryPerGitBranch. The GitBranch's UID is the whole path below the +// root, so the branch name, slashes included, never shapes it: `release/v1`'s checkout cannot sit +// inside `release`'s, where retiring `release` would take it. +func TestRepoRootPath_IsOneDirectoryPerGitBranch(t *testing.T) { + root := withTemporaryWorkerStateRoot(t) + repo := RepoIdentity{ProviderUID: "uid-provider", URL: "https://example.invalid/repo.git"} + parent := &BranchWorker{GitBranchUID: "uid-release", repo: repo, Branch: "release"} + child := &BranchWorker{GitBranchUID: "uid-release-v1", repo: repo, Branch: "release/v1"} + + assert.Equal(t, filepath.Join(root, "uid-release"), parent.repoRootPath()) + assert.Equal(t, filepath.Join(root, "uid-release-v1"), child.repoRootPath()) + + // A worker with no GitBranch behind it, which only tests build, has its remote and branch to be + // unique by, and two of them must still not share a directory. + tests := map[string]*BranchWorker{ + "another remote": {repo: RepoIdentity{URL: "https://example.invalid/other.git"}, Branch: "main"}, + "another branch": {repo: RepoIdentity{URL: "https://example.invalid/repo.git"}, Branch: "release"}, + } + base := &BranchWorker{repo: RepoIdentity{URL: "https://example.invalid/repo.git"}, Branch: "main"} + for name, other := range tests { + t.Run(name, func(t *testing.T) { + assert.NotEqual(t, base.repoRootPath(), other.repoRootPath()) + assert.Equal(t, root, filepath.Dir(other.repoRootPath()), "still one directory below the root") + }) + } } // TestWorkerLifecycle_BuildsAndReclaimsItsOwnState walks one worker's whole life on disk, because @@ -527,15 +518,15 @@ func TestWorkerLifecycle_BuildsAndReclaimsItsOwnState(t *testing.T) { require.NoError(t, manager.EnsureWorker(ctx, claimForTest(manager, "repo1", "gitops-system", "main", repo))) worker, ok := manager.WorkerForGitBranch(testGitBranch("repo1", "gitops-system", "main")) require.True(t, ok) - require.True(t, strings.HasPrefix(worker.repoPath(), root+string(filepath.Separator)), - "precondition: the worker keeps its state under the root this test owns") + own := filepath.Join(root, string(testGitBranchUID(testGitBranch("repo1", "gitops-system", "main")))) + require.Equal(t, own, worker.repoPath(), + "precondition: the worker keeps its state under the root this test owns, named by its GitBranch") - // Three directories under that root: this worker's, another provider's, and — the one that - // matters most — a SIBLING BRANCH of the same provider. Branch siblings share the UID - // component, so a removal one level too high takes every branch the provider mirrors. + // Three directories under that root: this worker's, and those of two other GitBranches — one + // writing another branch of the same repository, one writing the same branch name elsewhere. require.NoError(t, os.MkdirAll(worker.repoPath(), 0o750)) - sibling := filepath.Join(root, string(repo.ProviderUID), branchPathComponent("release")) - neighbour := filepath.Join(root, "uid-2", branchPathComponent("main")) + sibling := filepath.Join(root, "uid-gitops-system-repo1-release") + neighbour := filepath.Join(root, "uid-other-repo2-main") for _, dir := range []string{sibling, neighbour} { require.NoError(t, os.MkdirAll(dir, 0o750)) require.NoError(t, os.WriteFile(filepath.Join(dir, "HEAD"), []byte("ref: refs/heads/main\n"), 0o600)) @@ -547,37 +538,30 @@ func TestWorkerLifecycle_BuildsAndReclaimsItsOwnState(t *testing.T) { _, err := os.Stat(worker.repoPath()) assert.True(t, os.IsNotExist(err), "a retired worker takes its checkout with it") _, err = os.Stat(filepath.Join(sibling, "HEAD")) - require.NoError(t, err, "a sibling BRANCH of the same provider is not this worker's to delete") + require.NoError(t, err, "another GitBranch on the same repository is not this worker's to delete") _, err = os.Stat(filepath.Join(neighbour, "HEAD")) - require.NoError(t, err, "and neither is another provider's") + require.NoError(t, err, "and neither is any other GitBranch's") _, err = os.Stat(root) require.NoError(t, err, "least of all the root every worker's state lives under") _, stillThere := manager.WorkerForGitBranch(testGitBranch("repo1", "gitops-system", "main")) assert.False(t, stillThere) } -// TestWorkerLifecycle_AReplacementDoesNotReclaimItsPredecessorsCheckout is the same rule across a -// repoint. The two workers are different incarnations of one (provider, branch), so they hold -// different directories, and stopping the old one must not touch the new one's — which is exactly -// what the old layout, keyed by provider NAME with a URL digest inside it, could not promise for a -// provider recreated against the same repository. -func TestWorkerLifecycle_AReplacementDoesNotReclaimItsPredecessorsCheckout(t *testing.T) { +// TestWorkerLifecycle_AReplacementStartsFromAnEmptyCheckout is the same rule across a repoint. +// The replacement writes for the same GitBranch, so it works in the same directory, and the clone +// there is about the repository the provider used to name. The old worker takes it with it before +// the replacement starts, so nothing of the old repository is left to plan against. +func TestWorkerLifecycle_AReplacementStartsFromAnEmptyCheckout(t *testing.T) { withTemporaryWorkerStateRoot(t) manager, ctx := startedManager(t) - sameURL := "https://example.invalid/repo.git" - before := RepoIdentity{ProviderUID: "uid-1", URL: sameURL} - after := RepoIdentity{ProviderUID: "uid-2", URL: sameURL} + before := RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/first.git"} + after := RepoIdentity{ProviderUID: "uid-2", URL: "https://example.invalid/second.git"} require.NoError(t, manager.EnsureWorker(ctx, claimForTest(manager, "repo1", "gitops-system", "main", before))) old, ok := manager.WorkerForGitBranch(testGitBranch("repo1", "gitops-system", "main")) require.True(t, ok) require.NoError(t, os.MkdirAll(old.repoPath(), 0o750)) - - // The replacement's directory, and a sentinel in it, BEFORE the replacement happens: a - // directory created afterwards would prove nothing about what the removal deleted. - successor := filepath.Join(workerStateRoot, string(after.ProviderUID), branchPathComponent("main")) - require.NoError(t, os.MkdirAll(successor, 0o750)) - sentinel := filepath.Join(successor, "HEAD") + sentinel := filepath.Join(old.repoPath(), "HEAD") require.NoError(t, os.WriteFile(sentinel, []byte("ref: refs/heads/main\n"), 0o600)) require.NoError(t, manager.EnsureWorker(ctx, claimForTest(manager, "repo1", "gitops-system", "main", after))) @@ -585,14 +569,9 @@ func TestWorkerLifecycle_AReplacementDoesNotReclaimItsPredecessorsCheckout(t *te require.True(t, ok) require.NotSame(t, old, replacement) - require.Equal(t, successor, replacement.repoPath(), - "the replacement's checkout is the one the sentinel was placed in") - require.NotEqual(t, old.repoPath(), replacement.repoPath(), - "one repository under two provider incarnations is two checkouts") - _, err := os.Stat(old.repoPath()) - assert.True(t, os.IsNotExist(err), "the retired worker took its own checkout with it") - _, err = os.Stat(sentinel) - require.NoError(t, err, "and left the replacement's alone") + assert.Equal(t, old.repoPath(), replacement.repoPath(), "one GitBranch is one directory") + _, err := os.Stat(sentinel) + assert.True(t, os.IsNotExist(err), "the retired worker took the old repository's checkout with it") assert.True(t, manager.ReplacementPending(BranchKeyFor(after.URL, "main")), "and the targets on the branch still wait to be rebuilt in the new one") } diff --git a/internal/watch/git_branch_helpers_test.go b/internal/watch/git_branch_helpers_test.go index 348d0228..e437eec2 100644 --- a/internal/watch/git_branch_helpers_test.go +++ b/internal/watch/git_branch_helpers_test.go @@ -26,7 +26,7 @@ func claimWorkerForTest( gitBranch := gitBranchForTest(provider, namespace, branch) m.Claims().Observe(git.BranchKeyFor(repo.URL, branch), git.BranchClaimant{ GitBranch: gitBranch, - UID: k8stypes.UID(gitBranch.String()), + UID: k8stypes.UID("uid-" + gitBranch.Namespace + "-" + gitBranch.Name), Created: time.Unix(0, 0), }, true) return git.WorkerRequest{ From 0461828c34f3acf1abb1f2df27a01d4bab1ca304 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Mon, 26 Oct 2026 09:11:42 +0000 Subject: [PATCH 2/2] feat(git): reclaim checkouts no GitBranch names A retiring worker removes its own checkout, but a GitBranch deleted while no worker ran for it, across a restart that kept the disk, left its directory behind. The worker sweep now also deletes every directory under the state root that no live worker uses and no existing GitBranch names. It runs under lifecycleMu, so no worker starts while it reads, and a GitBranch List that fails deletes nothing. Checkouts from the previous {provider UID}/{branch} layout go the same way. Because the sweep deletes whatever its own cluster does not name, a test binary that runs a WorkerManager must not share the default root: SetWorkerStateRoot gives internal/git and internal/controller their own from TestMain. The chart README shows how to keep checkouts across pod replacement by mounting a PersistentVolumeClaim through the existing volumes/volumeMounts values. Co-Authored-By: Claude Opus 5.5 (1M context) --- charts/gitops-reverser/README.md | 21 ++++++++ docs/architecture.md | 8 ++++ internal/controller/main_test.go | 24 ++++++++++ internal/git/branch_worker.go | 7 +++ internal/git/git_operations_test.go | 11 ++++- internal/git/worker_manager.go | 52 +++++++++++++++++++- internal/git/worker_manager_test.go | 74 +++++++++++++++++++++++++++-- 7 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 internal/controller/main_test.go diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 89cb01f4..c9c610ce 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -485,6 +485,27 @@ resources: memory: 1Gi ``` +### Keeping Git checkouts across pod restarts + +The controller keeps one Git checkout per `GitBranch` under `/tmp/gitops-reverser-workers`. By +default that is the pod's `emptyDir`: it survives a container restart, and a new pod clones every +branch again. To keep the checkouts across pod replacement too, mount a volume there: + +```yaml +volumes: + - name: git-checkouts + persistentVolumeClaim: + claimName: gitops-reverser-git-checkouts +volumeMounts: + - name: git-checkouts + mountPath: /tmp/gitops-reverser-workers +``` + +Create the `PersistentVolumeClaim` yourself, in the release namespace. `ReadWriteOnce` is enough: the +chart runs one replica, and the default `deploymentStrategy` stops the old pod before it starts the +new one. A checkout found on disk is a cache only. The controller fetches and resets it before +its first write, and deletes every checkout whose `GitBranch` no longer exists. + ## Release Strategy New versions are automatically released via GitHub Actions: diff --git a/docs/architecture.md b/docs/architecture.md index 33464fc2..779876c0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1570,6 +1570,14 @@ another URL keeps the `GitBranch`, so its replacement worker reuses the director has removed the clone in it. A worker never trusts a checkout it finds on disk: its first cycle repoints `origin`, fetches and resets. +A retiring worker removes its own directory. The worker sweep, which runs every minute and after +every `GitTarget` delete, also removes every directory that no live worker uses and no existing +`GitBranch` names. That covers a `GitBranch` deleted while no worker ran for it, which happens across +a restart that keeps the disk. The root is an `emptyDir` by default, so the checkouts survive a +container restart but not a new pod. The +[chart README](../charts/gitops-reverser/README.md#keeping-git-checkouts-across-pod-restarts) shows how to +mount a volume there instead. + [PushAtomic](../internal/git/git_atomic_push.go) checks the remote ref before pushing. If the remote diverged it smart fetches the latest tip, hard resets the local clone, replays the retained pending writes against the fresh tip (refreshing commit hashes), and retries up to the attempt limit. This is valid diff --git a/internal/controller/main_test.go b/internal/controller/main_test.go new file mode 100644 index 00000000..182e861d --- /dev/null +++ b/internal/controller/main_test.go @@ -0,0 +1,24 @@ +// SPDX-License-Identifier: Apache-2.0 + +package controller + +import ( + "os" + "testing" + + "github.com/ConfigButler/gitops-reverser/internal/git" +) + +// TestMain gives this binary its own worker state root: the WorkerManagers these tests start sweep +// every directory under it that their GitBranches do not name (git.SetWorkerStateRoot). +func TestMain(m *testing.M) { + root, err := os.MkdirTemp("", "gitops-reverser-workers-") + if err != nil { + panic(err) + } + git.SetWorkerStateRoot(root) + + code := m.Run() + _ = os.RemoveAll(root) + os.Exit(code) +} diff --git a/internal/git/branch_worker.go b/internal/git/branch_worker.go index 811a3545..e8f0ea65 100644 --- a/internal/git/branch_worker.go +++ b/internal/git/branch_worker.go @@ -461,6 +461,13 @@ func NewBranchWorker( //nolint:gochecknoglobals // a test seam, like pushAtomicFn above var workerStateRoot = filepath.Join("/tmp", "gitops-reverser-workers") +// SetWorkerStateRoot points every worker's on-disk state at dir, for the whole process. The +// operator never calls it. A test binary that runs a WorkerManager calls it from TestMain, before +// any worker exists: the manager's sweep deletes every directory under the root that its own +// GitBranches do not name, so under the shared default it would delete the checkouts of every +// other process using that root. +func SetWorkerStateRoot(dir string) { workerStateRoot = dir } + // repoRootPath is where this worker keeps its checkout: one directory, named by the UID of the // GitBranch it writes for. // diff --git a/internal/git/git_operations_test.go b/internal/git/git_operations_test.go index f6d030af..283cdcd5 100644 --- a/internal/git/git_operations_test.go +++ b/internal/git/git_operations_test.go @@ -48,12 +48,21 @@ func skipIfPublicNetworkUnavailable(t *testing.T, err error) { // TestMain initializes the controller-runtime logger before running tests. // This prevents "log.SetLogger(...) was never called" warnings when code uses log.FromContext(). +// It also gives this binary its own worker state root (SetWorkerStateRoot). func TestMain(m *testing.M) { // Initialize controller-runtime logger for all tests logf.SetLogger(zap.New(zap.UseDevMode(true))) + root, err := os.MkdirTemp("", "gitops-reverser-workers-") + if err != nil { + panic(err) + } + SetWorkerStateRoot(root) + // Run all tests - os.Exit(m.Run()) + code := m.Run() + _ = os.RemoveAll(root) + os.Exit(code) } func TestCheckRepo_ConnectivityAndMetadata(t *testing.T) { diff --git a/internal/git/worker_manager.go b/internal/git/worker_manager.go index 37b9b4df..848ef12b 100644 --- a/internal/git/worker_manager.go +++ b/internal/git/worker_manager.go @@ -6,6 +6,9 @@ import ( "context" "errors" "fmt" + "io/fs" + "os" + "path/filepath" "sync" "time" @@ -737,7 +740,8 @@ func (m *WorkerManager) CommitRequestOwner(namespace, name, uid string) (*Branch //nolint:gochecknoglobals // a test seam, like pushAtomicFn var afterOrphanSelection func() -// ReconcileWorkers stops every worker no live GitTarget still needs. +// ReconcileWorkers stops every worker no live GitTarget still needs, then deletes the checkouts +// no GitBranch names (reclaimOrphanedCheckoutsLocked). // // It is the ONLY thing that removes a worker before shutdown, and it is driven from the GitTarget // reconcile's deleted-object path: that reconcile is what the delete watch event produces, so the @@ -809,6 +813,50 @@ func (m *WorkerManager) ReconcileWorkers(ctx context.Context) error { "neededWorkers", len(needed), "stopped", len(orphans)) + return m.reclaimOrphanedCheckoutsLocked(ctx) +} + +// reclaimOrphanedCheckoutsLocked deletes every directory under the worker state root that no +// GitBranch names: one whose GitBranch was deleted while no worker ran for it, which no retiring +// worker was there to remove. That happens across a restart that keeps the disk, such as a +// container restart inside its pod, and for a GitBranch deleted after its last GitTarget. +// +// A directory is kept while a live worker uses it or a GitBranch with its UID exists, so a +// GitBranch whose worker has not started yet keeps its checkout for when it does. Called with +// lifecycleMu held, so no worker starts while the directories are read: one that starts later +// creates its directory after the sweep. It fails safe like the worker sweep: a List that errors +// deletes nothing. +func (m *WorkerManager) reclaimOrphanedCheckoutsLocked(ctx context.Context) error { + var branches configv1alpha3.GitBranchList + if err := m.Client.List(ctx, &branches); err != nil { + return fmt.Errorf("failed to list GitBranches: %w", err) + } + kept := make(map[string]bool, len(branches.Items)) + for i := range branches.Items { + kept[string(branches.Items[i].UID)] = true + } + for _, worker := range m.liveWorkers() { + kept[worker.stateComponent()] = true + } + + entries, err := os.ReadDir(workerStateRoot) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return fmt.Errorf("failed to read the worker state root: %w", err) + } + for _, entry := range entries { + if kept[entry.Name()] { + continue + } + path := filepath.Join(workerStateRoot, entry.Name()) + if err := os.RemoveAll(path); err != nil { + m.Log.Error(err, "Could not remove an orphaned checkout", "path", path) + continue + } + m.Log.Info("Removed an orphaned checkout: no GitBranch names it", "path", path) + } return nil } @@ -873,7 +921,7 @@ func (m *WorkerManager) sweepPeriodically(ctx context.Context) { if err := m.ReconcileWorkers(ctx); err != nil { // Failing safe: a List that errors stops nothing, so the only cost is that the // sweep is late. - m.Log.V(1).Info("Periodic worker sweep could not read the GitTargets", + m.Log.V(1).Info("Periodic worker sweep could not read the cluster", "error", err.Error()) } } diff --git a/internal/git/worker_manager_test.go b/internal/git/worker_manager_test.go index 9557f94b..24efa7b1 100644 --- a/internal/git/worker_manager_test.go +++ b/internal/git/worker_manager_test.go @@ -14,6 +14,7 @@ import ( "github.com/go-logr/logr" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" k8stypes "k8s.io/apimachinery/pkg/types" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -524,11 +525,11 @@ func TestWorkerLifecycle_BuildsAndReclaimsItsOwnState(t *testing.T) { // Three directories under that root: this worker's, and those of two other GitBranches — one // writing another branch of the same repository, one writing the same branch name elsewhere. + // Both GitBranches exist, so the sweep of orphaned checkouts keeps their directories too. require.NoError(t, os.MkdirAll(worker.repoPath(), 0o750)) - sibling := filepath.Join(root, "uid-gitops-system-repo1-release") - neighbour := filepath.Join(root, "uid-other-repo2-main") + sibling := createGitBranchCheckout(ctx, t, manager, "gitops-system", "repo1-release") + neighbour := createGitBranchCheckout(ctx, t, manager, "other", "repo2-main") for _, dir := range []string{sibling, neighbour} { - require.NoError(t, os.MkdirAll(dir, 0o750)) require.NoError(t, os.WriteFile(filepath.Join(dir, "HEAD"), []byte("ref: refs/heads/main\n"), 0o600)) } @@ -547,6 +548,73 @@ func TestWorkerLifecycle_BuildsAndReclaimsItsOwnState(t *testing.T) { assert.False(t, stillThere) } +// createGitBranchCheckout creates a GitBranch and the directory its worker would keep, and returns +// that directory. +func createGitBranchCheckout(ctx context.Context, t *testing.T, m *WorkerManager, namespace, name string) string { + t.Helper() + uid := k8stypes.UID("uid-" + namespace + "-" + name) + require.NoError(t, m.Client.Create(ctx, &configv1alpha3.GitBranch{ + ObjectMeta: metav1.ObjectMeta{Namespace: namespace, Name: name, UID: uid}, + })) + dir := filepath.Join(workerStateRoot, string(uid)) + require.NoError(t, os.MkdirAll(dir, 0o750)) + return dir +} + +// TestReconcileWorkers_ReclaimsCheckoutsNoGitBranchNames. A retiring worker removes its own +// checkout, but a GitBranch deleted while no worker ran for it, across a restart that kept the +// disk, has nobody to do that. The sweep removes every directory no GitBranch names, and keeps +// the checkout of a GitBranch whose worker has not started yet, and that of any live worker. +func TestReconcileWorkers_ReclaimsCheckoutsNoGitBranchNames(t *testing.T) { + manager, ctx := startedManager(t) + root := workerStateRoot + + waiting := createGitBranchCheckout(ctx, t, manager, "team-a", "waiting") + require.NoError(t, manager.EnsureWorker(ctx, claimForTest(manager, "repo1", "team-b", "main", + RepoIdentity{ProviderUID: "uid-1", URL: "https://example.invalid/repo.git"}))) + live, ok := manager.WorkerForGitBranch(testGitBranch("repo1", "team-b", "main")) + require.True(t, ok) + require.NoError(t, os.MkdirAll(live.repoPath(), 0o750)) + deleted := filepath.Join(root, "uid-of-a-deleted-gitbranch") + previousLayout := filepath.Join(root, "uid-of-a-provider", "main-0123456789abcdef") + for _, dir := range []string{deleted, previousLayout} { + require.NoError(t, os.MkdirAll(dir, 0o750)) + } + + manager.lifecycleMu.Lock() + err := manager.reclaimOrphanedCheckoutsLocked(ctx) + manager.lifecycleMu.Unlock() + require.NoError(t, err) + + for _, kept := range []string{waiting, live.repoPath()} { + _, err := os.Stat(kept) + require.NoError(t, err, "%s belongs to a GitBranch or a live worker", kept) + } + for _, gone := range []string{deleted, filepath.Dir(previousLayout)} { + _, err := os.Stat(gone) + assert.True(t, os.IsNotExist(err), "%s is named by no GitBranch", gone) + } +} + +// TestReconcileWorkers_ReclaimsNothingWhenTheGitBranchesCannotBeRead. Reading "I could not list +// the GitBranches" as "there are none" would delete every checkout the operator keeps. +func TestReconcileWorkers_ReclaimsNothingWhenTheGitBranchesCannotBeRead(t *testing.T) { + root := withTemporaryWorkerStateRoot(t) + orphan := filepath.Join(root, "uid-of-a-deleted-gitbranch") + require.NoError(t, os.MkdirAll(orphan, 0o750)) + k8sClient := fake.NewClientBuilder().WithScheme(setupScheme()).WithInterceptorFuncs(interceptor.Funcs{ + List: func(context.Context, client.WithWatch, client.ObjectList, ...client.ListOption) error { + return errors.New("the API server is unreachable") + }, + }).Build() + manager := NewWorkerManager(k8sClient, logr.Discard(), BranchWorkerLimits{}, types.SensitiveResourcePolicy{}) + + require.Error(t, manager.reclaimOrphanedCheckoutsLocked(context.Background())) + + _, err := os.Stat(orphan) + require.NoError(t, err, "a failed List deletes nothing") +} + // TestWorkerLifecycle_AReplacementStartsFromAnEmptyCheckout is the same rule across a repoint. // The replacement writes for the same GitBranch, so it works in the same directory, and the clone // there is about the repository the provider used to name. The old worker takes it with it before