Skip to content

Commit 2224a38

Browse files
yushan8yushan8
committed
repomanager: derive worker root from clone path, drop config knob entirely
Removes ServiceConfig.WorkerRootPath()/the settable worker_root_path field along with repomanager.Params.WorkerRootPath — none of it is documented in config/README.md, and RepoManager already owns RepoManagerClonePath, so it can derive its own .workers subdirectory (<repo_manager_clone_path>/.workers/<repo>/worker-{1..N}/) instead of being handed a second, independently-configurable path for the same tree.
1 parent 24895e4 commit 2224a38

6 files changed

Lines changed: 14 additions & 68 deletions

File tree

‎config/config_test.go‎

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -70,43 +70,6 @@ repository:
7070
}
7171
}
7272

73-
func TestParse_ServiceDefaults(t *testing.T) {
74-
tests := []struct {
75-
name string
76-
give string
77-
wantWorkerRootPath string
78-
}{
79-
{
80-
name: "worker_root_path defaults when unset",
81-
give: _baseServiceYAML + `
82-
repository:
83-
- remote: "r1"
84-
`,
85-
wantWorkerRootPath: filepath.Join("/tmp/x", ".workers"),
86-
},
87-
{
88-
name: "worker_root_path explicit value preserved",
89-
give: `
90-
service:
91-
workspaces_root_path: "/tmp/x"
92-
worker_root_path: "/tmp/custom-workers"
93-
max_worker_pool_size: 1
94-
repository:
95-
- remote: "r1"
96-
`,
97-
wantWorkerRootPath: "/tmp/custom-workers",
98-
},
99-
}
100-
101-
for _, tt := range tests {
102-
t.Run(tt.name, func(t *testing.T) {
103-
cfg, err := Parse(writeConfig(t, tt.give))
104-
require.NoError(t, err)
105-
assert.Equal(t, tt.wantWorkerRootPath, cfg.Service.WorkerRootPath)
106-
})
107-
}
108-
}
109-
11073
func TestParse_RepositoryDefaults(t *testing.T) {
11174
tests := []struct {
11275
name string

‎config/service_config.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,10 +26,6 @@ type ServiceConfig struct {
2626
// worker checkouts.
2727
WorkspacesRootPath string `yaml:"workspaces_root_path"`
2828
Streaming ChunkConfig `yaml:"streaming"` // streaming chunk sizes; zero values fall back to package defaults
29-
30-
// WorkerRootPath is the root directory for worker workspace checkouts,
31-
// set by Parse to <workspaces_root_path>/.workers. Not settable via YAML.
32-
WorkerRootPath string `yaml:"-"`
3329
}
3430

3531
// ChunkConfig controls the number of entries per gRPC stream message.

‎core/repomanager/repo_manager.go‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ type RepoManager interface {
3636
type repoManager struct {
3737
git git.Interface
3838
repoManagerClonePath string
39-
workerRootPath string
4039
logger *zap.SugaredLogger
4140
poolSize int
4241

@@ -77,7 +76,6 @@ type Params struct {
7776
Git git.Interface
7877
Logger *zap.SugaredLogger
7978
RepoManagerClonePath string
80-
WorkerRootPath string
8179
PoolSize int
8280
}
8381

@@ -93,7 +91,6 @@ func NewRepoManager(appCtx context.Context, p Params) (RepoManager, error) {
9391
return &repoManager{
9492
git: p.Git,
9593
repoManagerClonePath: p.RepoManagerClonePath,
96-
workerRootPath: p.WorkerRootPath,
9794
logger: p.Logger,
9895
poolSize: p.PoolSize,
9996
pools: make(map[string]*workerPool),
@@ -115,7 +112,7 @@ func (r *repoManager) poolFor(repo string) *workerPool {
115112

116113
// Pre-allocate fixed worker slots. Existing directories from a previous
117114
// run are detected and reused without re-cloning.
118-
workersDir := filepath.Join(r.workerRootPath, repo)
115+
workersDir := filepath.Join(r.repoManagerClonePath, ".workers", repo)
119116
for i := 1; i <= r.poolSize; i++ {
120117
dir := filepath.Join(workersDir, fmt.Sprintf("worker-%d", i))
121118
slot := &workerSlot{dir: dir}

‎core/repomanager/repo_manager_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ func TestLease_ClonesOriginAndCreatesWorker(t *testing.T) {
5959
g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil)
6060
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil)
6161

62-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
62+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
6363
ws, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
6464
require.NoError(t, err)
6565
assert.Equal(t, workerDir, ws.Path())
@@ -81,7 +81,7 @@ func TestLease_SkipsOriginClone_WhenExists(t *testing.T) {
8181
// Only worker clone expected
8282
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil)
8383

84-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
84+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
8585
ws, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
8686
require.NoError(t, err)
8787
assert.Equal(t, workerDir, ws.Path())
@@ -102,7 +102,7 @@ func TestLease_ReusesWorker_AfterRelease(t *testing.T) {
102102
g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil)
103103
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil)
104104

105-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
105+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
106106
ctx := context.Background()
107107

108108
ws1, err := rm.Lease(ctx, entity.BuildDescription{Remote: remote})
@@ -131,7 +131,7 @@ func TestLease_CreatesMultipleWorkers(t *testing.T) {
131131
g.EXPECT().Clone(gomock.Any(), originDir, dir, "--local", "-c", "gc.auto=0").Return(nil)
132132
}
133133

134-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 2})
134+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 2})
135135
ctx := context.Background()
136136

137137
ws1, err := rm.Lease(ctx, entity.BuildDescription{Remote: remote})
@@ -157,7 +157,7 @@ func TestLease_BlocksUntilReturn(t *testing.T) {
157157
g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil)
158158
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil)
159159

160-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
160+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
161161
ctx := context.Background()
162162

163163
ws1, err := rm.Lease(ctx, entity.BuildDescription{Remote: remote})
@@ -203,7 +203,7 @@ func TestLease_CtxCanceled(t *testing.T) {
203203
g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil)
204204
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil)
205205

206-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
206+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
207207

208208
ws1, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
209209
require.NoError(t, err)
@@ -225,7 +225,7 @@ func TestLease_OriginCloneFails(t *testing.T) {
225225
remote := "git@github.com:org/repo"
226226
g.EXPECT().Clone(gomock.Any(), remote, filepath.Join(root, "org/repo"), "-c", "gc.auto=0").Return(assert.AnError)
227227

228-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
228+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
229229
_, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
230230
require.Error(t, err)
231231
assert.Contains(t, err.Error(), "clone origin")
@@ -244,7 +244,7 @@ func TestLease_WorkerCloneFails(t *testing.T) {
244244
g.EXPECT().Clone(gomock.Any(), remote, originDir, "-c", "gc.auto=0").Return(nil)
245245
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(assert.AnError)
246246

247-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
247+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
248248
_, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
249249
require.Error(t, err)
250250
assert.Contains(t, err.Error(), "create worker")
@@ -263,7 +263,7 @@ func TestLease_DiscoversExistingWorker(t *testing.T) {
263263
require.NoError(t, os.MkdirAll(filepath.Join(root, ".workers", "org/repo", "worker-1", ".git"), 0o755))
264264

265265
// No Clone calls — everything already exists
266-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
266+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
267267
ws, err := rm.Lease(context.Background(), entity.BuildDescription{Remote: remote})
268268
require.NoError(t, err)
269269
assert.Contains(t, ws.Path(), "worker-1")
@@ -289,7 +289,7 @@ func TestLease_DifferentRepos_IndependentPools(t *testing.T) {
289289
g.EXPECT().Clone(gomock.Any(), remote2, origin2, "-c", "gc.auto=0").Return(nil)
290290
g.EXPECT().Clone(gomock.Any(), origin2, worker2, "--local", "-c", "gc.auto=0").Return(nil)
291291

292-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
292+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
293293
ctx := context.Background()
294294

295295
// Both repos can be leased concurrently even with pool size 1
@@ -322,7 +322,7 @@ func TestLease_WorkerCloneFails_SlotReturnedToPool(t *testing.T) {
322322
g.EXPECT().Clone(gomock.Any(), originDir, workerDir, "--local", "-c", "gc.auto=0").Return(nil),
323323
)
324324

325-
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, WorkerRootPath: filepath.Join(root, ".workers"), PoolSize: 1})
325+
rm := newTestRepoManager(t, context.Background(), Params{Git: g, Logger: zap.NewNop().Sugar(), RepoManagerClonePath: root, PoolSize: 1})
326326
ctx := context.Background()
327327

328328
// First attempt fails

‎example/main.go‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,21 +72,15 @@ func run() error {
7272

7373
// Repo manager and orchestrator
7474
repoManagerClonePath := cfg.Service.WorkspacesRootPath
75-
workerRootPath := cfg.Service.WorkerRootPath
7675
if err := os.MkdirAll(repoManagerClonePath, 0o755); err != nil {
7776
return fmt.Errorf("failed to create repo manager clone path: %w", err)
7877
}
7978
defer os.RemoveAll(repoManagerClonePath)
80-
if err := os.MkdirAll(workerRootPath, 0o755); err != nil {
81-
return fmt.Errorf("failed to create worker root path: %w", err)
82-
}
83-
defer os.RemoveAll(workerRootPath)
8479

8580
rm, err := repomanager.NewRepoManager(appCtx, repomanager.Params{
8681
Git: git.New(repoManagerClonePath, logger),
8782
Logger: logger,
8883
RepoManagerClonePath: repoManagerClonePath,
89-
WorkerRootPath: workerRootPath,
9084
PoolSize: cfg.Service.MaxWorkerPoolSize,
9185
})
9286
if err != nil {

‎integration/integration_test.go‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ func repoRemote(t *testing.T) string {
6060
return remote
6161
}
6262

63-
func writeConfig(t *testing.T, dir, remote, clonePath, workerPath string) string {
63+
func writeConfig(t *testing.T, dir, remote, clonePath string) string {
6464
t.Helper()
6565

6666
tmpl, err := template.ParseFiles(configTemplateFile)
@@ -74,12 +74,10 @@ func writeConfig(t *testing.T, dir, remote, clonePath, workerPath string) string
7474
err = tmpl.Execute(f, struct {
7575
Remote string
7676
ClonePath string
77-
WorkerPath string
7877
BazelCommand string
7978
}{
8079
Remote: remote,
8180
ClonePath: clonePath,
82-
WorkerPath: workerPath,
8381
BazelCommand: filepath.Join(remote, "tools", "bazel"),
8482
})
8583
require.NoError(t, err, "failed to render config template")
@@ -92,9 +90,8 @@ func startServer(t *testing.T, remote string) string {
9290

9391
configDir := t.TempDir()
9492
clonePath := t.TempDir()
95-
workerPath := t.TempDir()
9693

97-
configPath := writeConfig(t, configDir, remote, clonePath, workerPath)
94+
configPath := writeConfig(t, configDir, remote, clonePath)
9895

9996
zl := zaptest.NewLogger(t)
10097
logger := zl.Sugar()
@@ -108,7 +105,6 @@ func startServer(t *testing.T, remote string) string {
108105
Git: git.New(clonePath, logger),
109106
Logger: logger,
110107
RepoManagerClonePath: clonePath,
111-
WorkerRootPath: workerPath,
112108
PoolSize: 2,
113109
})
114110
require.NoError(t, err, "failed to create repo manager")

0 commit comments

Comments
 (0)