From 0b3d50444ec8d36dc257a2ed0c4fbb94fc08ea51 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Mon, 11 May 2026 19:15:13 -0700 Subject: [PATCH] feat(s3/lifecycle): drop Worker Count knob from admin config form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "Worker Count" admin field controlled in-process pipeline goroutines across the 16-shard space — per-worker tuning, not a cluster-wide scope concern. Operators looking at the form alongside Cluster Delete Rate reasonably misread it as the number of workers in the cluster. Drop the form field and DefaultValues entry. cfg.Workers is now hardcoded to shardPipelineGoroutines (=1) inside ParseConfig; the rest of the plumbing through dailyrun.Config.Workers stays so a future need can re-introduce it as a worker-local knob (or just bump the constant). handler_test.go pins that "workers" must NOT appear in the form so the removal doesn't silently regress. --- weed/worker/tasks/s3_lifecycle/config.go | 12 +++++---- weed/worker/tasks/s3_lifecycle/config_test.go | 12 +++------ weed/worker/tasks/s3_lifecycle/handler.go | 12 +-------- .../worker/tasks/s3_lifecycle/handler_test.go | 26 ++++++------------- 4 files changed, 19 insertions(+), 43 deletions(-) diff --git a/weed/worker/tasks/s3_lifecycle/config.go b/weed/worker/tasks/s3_lifecycle/config.go index 104934def..ed3ec9f8c 100644 --- a/weed/worker/tasks/s3_lifecycle/config.go +++ b/weed/worker/tasks/s3_lifecycle/config.go @@ -9,7 +9,12 @@ import ( const ( jobType = "s3_lifecycle" - defaultWorkers = 1 + // shardPipelineGoroutines is the in-process fan-out across the + // 16-shard space. Kept as a hardcoded internal default — formerly + // an admin form field, removed because it's a per-worker tuning + // knob, not a cluster-coordination concern. + shardPipelineGoroutines = 1 + defaultDispatchTickMinutes = int64(1) defaultCheckpointTickSeconds = int64(30) defaultRefreshIntervalMinutes = int64(5) @@ -45,7 +50,7 @@ type Config struct { // admin+worker config values. Missing fields fall back to defaults. func ParseConfig(adminValues, workerValues map[string]*plugin_pb.ConfigValue) Config { cfg := Config{ - Workers: int(readInt64(adminValues, "workers", defaultWorkers)), + Workers: shardPipelineGoroutines, DispatchTick: time.Duration(readInt64(workerValues, "dispatch_tick_minutes", defaultDispatchTickMinutes)) * time.Minute, CheckpointTick: time.Duration(readInt64(workerValues, "checkpoint_tick_seconds", defaultCheckpointTickSeconds)) * time.Second, RefreshInterval: time.Duration(readInt64(workerValues, "refresh_interval_minutes", defaultRefreshIntervalMinutes)) * time.Minute, @@ -53,9 +58,6 @@ func ParseConfig(adminValues, workerValues map[string]*plugin_pb.ConfigValue) Co MaxRuntime: time.Duration(readInt64(workerValues, "max_runtime_minutes", defaultMaxRuntimeMinutes)) * time.Minute, Algorithm: readString(adminValues, "algorithm", defaultAlgorithm), } - if cfg.Workers <= 0 { - cfg.Workers = defaultWorkers - } if cfg.DispatchTick <= 0 { cfg.DispatchTick = time.Duration(defaultDispatchTickMinutes) * time.Minute } diff --git a/weed/worker/tasks/s3_lifecycle/config_test.go b/weed/worker/tasks/s3_lifecycle/config_test.go index aab98a477..dc7b91c7d 100644 --- a/weed/worker/tasks/s3_lifecycle/config_test.go +++ b/weed/worker/tasks/s3_lifecycle/config_test.go @@ -9,8 +9,8 @@ import ( func TestParseConfigDefaults(t *testing.T) { cfg := ParseConfig(nil, nil) - if cfg.Workers != defaultWorkers { - t.Errorf("Workers default=%d, want %d", cfg.Workers, defaultWorkers) + if cfg.Workers != shardPipelineGoroutines { + t.Errorf("Workers default=%d, want %d", cfg.Workers, shardPipelineGoroutines) } if cfg.DispatchTick != 1*time.Minute { t.Errorf("DispatchTick default=%v, want 1m", cfg.DispatchTick) @@ -58,9 +58,6 @@ func TestParseConfig_AlgorithmUnknownValueFallsBackToDefault(t *testing.T) { } func TestParseConfigOverrides(t *testing.T) { - admin := map[string]*plugin_pb.ConfigValue{ - "workers": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 4}}, - } worker := map[string]*plugin_pb.ConfigValue{ "dispatch_tick_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 2}}, "checkpoint_tick_seconds": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 15}}, @@ -68,10 +65,7 @@ func TestParseConfigOverrides(t *testing.T) { "bootstrap_interval_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 120}}, "max_runtime_minutes": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 120}}, } - cfg := ParseConfig(admin, worker) - if cfg.Workers != 4 { - t.Errorf("Workers=%d, want 4", cfg.Workers) - } + cfg := ParseConfig(nil, worker) if cfg.DispatchTick != 2*time.Minute { t.Errorf("DispatchTick=%v, want 2m", cfg.DispatchTick) } diff --git a/weed/worker/tasks/s3_lifecycle/handler.go b/weed/worker/tasks/s3_lifecycle/handler.go index a3c1997b8..98c2404fd 100644 --- a/weed/worker/tasks/s3_lifecycle/handler.go +++ b/weed/worker/tasks/s3_lifecycle/handler.go @@ -74,17 +74,8 @@ func (h *Handler) Descriptor() *plugin_pb.JobTypeDescriptor { { SectionId: "scope", Title: "Scope", - Description: "How many pipeline goroutines split the 16-shard space.", + Description: "Cluster-wide algorithm choice and delete-throughput cap.", Fields: []*plugin_pb.ConfigField{ - { - Name: "workers", - Label: "Worker Count", - Description: "Number of pipeline goroutines per executing worker. Each owns a contiguous slice of [0, 16) shards. Default 1 = one goroutine handles all 16 shards.", - FieldType: plugin_pb.ConfigFieldType_CONFIG_FIELD_TYPE_INT64, - Widget: plugin_pb.ConfigWidget_CONFIG_WIDGET_NUMBER, - MinValue: &plugin_pb.ConfigValue{Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 1}}, - MaxValue: &plugin_pb.ConfigValue{Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 16}}, - }, { Name: "algorithm", Label: "Algorithm", @@ -116,7 +107,6 @@ func (h *Handler) Descriptor() *plugin_pb.JobTypeDescriptor { }, }, DefaultValues: map[string]*plugin_pb.ConfigValue{ - "workers": {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: defaultWorkers}}, "algorithm": {Kind: &plugin_pb.ConfigValue_StringValue{StringValue: defaultAlgorithm}}, ClusterDeletesPerSecondAdminKey: {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 0}}, ClusterDeletesBurstAdminKey: {Kind: &plugin_pb.ConfigValue_Int64Value{Int64Value: 0}}, diff --git a/weed/worker/tasks/s3_lifecycle/handler_test.go b/weed/worker/tasks/s3_lifecycle/handler_test.go index c4529e1f4..9b326b636 100644 --- a/weed/worker/tasks/s3_lifecycle/handler_test.go +++ b/weed/worker/tasks/s3_lifecycle/handler_test.go @@ -284,31 +284,21 @@ func TestDescriptor_BasicShape(t *testing.T) { assert.Greater(t, d.DescriptorVersion, uint32(0), "descriptor version must be positive (admins use it for compat)") } -func TestDescriptor_AdminConfigFormHasWorkersField(t *testing.T) { - // Workers is the only admin-side knob today; if it disappears, the - // admin form would render empty and operators couldn't tune - // concurrency. +func TestDescriptor_AdminConfigFormHasNoWorkersField(t *testing.T) { + // "workers" used to be an admin form field controlling in-process + // pipeline goroutines. It's per-worker tuning, not a cluster-wide + // scope concern, so it was removed from the form. ParseConfig hard- + // codes cfg.Workers from shardPipelineGoroutines instead. h := NewHandler(nil) d := h.Descriptor() require.NotNil(t, d.AdminConfigForm) - assert.Equal(t, "s3-lifecycle-admin", d.AdminConfigForm.FormId) - - // Walk every section's fields and find "workers". - var found bool for _, sec := range d.AdminConfigForm.Sections { for _, f := range sec.Fields { - if f.Name == "workers" { - found = true - assert.Equal(t, plugin_pb.ConfigFieldType_CONFIG_FIELD_TYPE_INT64, f.FieldType) - } + assert.NotEqual(t, "workers", f.Name, "admin form must NOT expose the in-memory pipeline-goroutine knob") } } - assert.True(t, found, "admin form must expose 'workers' field") - - // Default value matches the constant used by ParseConfig. - dv, ok := d.AdminConfigForm.DefaultValues["workers"] - require.True(t, ok, "workers must have a default in AdminConfigForm") - assert.Equal(t, int64(defaultWorkers), dv.GetInt64Value()) + _, hasDefault := d.AdminConfigForm.DefaultValues["workers"] + assert.False(t, hasDefault, "DefaultValues must NOT include 'workers' (form field removed)") } func TestDescriptor_WorkerConfigFormCadenceDefaultsMatchParseConfig(t *testing.T) {