From d9f5e0ade736cd58b484c26933f980d1b28772f0 Mon Sep 17 00:00:00 2001 From: varshaprasad96 Date: Fri, 17 Jul 2020 09:20:21 -0700 Subject: [PATCH 1/3] ansible/helm: Rename max-workers to max-concurrent-reconciles This commit: - Adds --max-concurrent-reconciles flag to helm binary - Rename MaxWorkers to MaxConcurrentReconciles in ansible/helm implementations. --- changelog/fragments/replace-max-workers.yaml | 4 ++ cmd/ansible-operator/main.go | 14 ++--- cmd/helm-operator/main.go | 2 +- pkg/ansible/controller/controller.go | 4 +- pkg/ansible/watches/testdata/valid.yaml.tmpl | 6 +- pkg/ansible/watches/watches.go | 8 +-- pkg/ansible/watches/watches_test.go | 60 ++++++++++---------- pkg/helm/controller/controller.go | 4 +- pkg/helm/flags/flag.go | 12 ++-- 9 files changed, 60 insertions(+), 54 deletions(-) create mode 100644 changelog/fragments/replace-max-workers.yaml diff --git a/changelog/fragments/replace-max-workers.yaml b/changelog/fragments/replace-max-workers.yaml new file mode 100644 index 0000000000..02532a3b78 --- /dev/null +++ b/changelog/fragments/replace-max-workers.yaml @@ -0,0 +1,4 @@ +entries: + - description: > + Add "--max-concurrent-reconciles" flag to helm binary. + kind: "addition" diff --git a/cmd/ansible-operator/main.go b/cmd/ansible-operator/main.go index 61e6200872..2315d54dc8 100644 --- a/cmd/ansible-operator/main.go +++ b/cmd/ansible-operator/main.go @@ -144,13 +144,13 @@ func main() { } ctr := controller.Add(mgr, controller.Options{ - GVK: w.GroupVersionKind, - Runner: runner, - ManageStatus: w.ManageStatus, - AnsibleDebugLogs: getAnsibleDebugLog(), - MaxWorkers: w.MaxWorkers, - ReconcilePeriod: w.ReconcilePeriod, - Selector: w.Selector, + GVK: w.GroupVersionKind, + Runner: runner, + ManageStatus: w.ManageStatus, + AnsibleDebugLogs: getAnsibleDebugLog(), + MaxConcurrentReconciles: w.MaxConcurrentReconciles, + ReconcilePeriod: w.ReconcilePeriod, + Selector: w.Selector, }) if ctr == nil { log.Error(fmt.Errorf("failed to add controller for GVK %v", w.GroupVersionKind.String()), "") diff --git a/cmd/helm-operator/main.go b/cmd/helm-operator/main.go index 65cbb56479..719948b062 100644 --- a/cmd/helm-operator/main.go +++ b/cmd/helm-operator/main.go @@ -132,7 +132,7 @@ func main() { ReconcilePeriod: f.ReconcilePeriod, WatchDependentResources: *w.WatchDependentResources, OverrideValues: w.OverrideValues, - MaxWorkers: f.MaxWorkers, + MaxConcurrentReconciles: f.MaxConcurrentReconciles, }) if err != nil { log.Error(err, "Failed to add manager factory to controller.") diff --git a/pkg/ansible/controller/controller.go b/pkg/ansible/controller/controller.go index 9991fa54f2..d414bc001d 100644 --- a/pkg/ansible/controller/controller.go +++ b/pkg/ansible/controller/controller.go @@ -48,7 +48,7 @@ type Options struct { AnsibleDebugLogs bool WatchDependentResources bool WatchClusterScopedResources bool - MaxWorkers int + MaxConcurrentReconciles int Selector metav1.LabelSelector } @@ -90,7 +90,7 @@ func Add(mgr manager.Manager, options Options) *controller.Controller { c, err := controller.New(fmt.Sprintf("%v-controller", strings.ToLower(options.GVK.Kind)), mgr, controller.Options{ Reconciler: aor, - MaxConcurrentReconciles: options.MaxWorkers, + MaxConcurrentReconciles: options.MaxConcurrentReconciles, }) if err != nil { log.Error(err, "") diff --git a/pkg/ansible/watches/testdata/valid.yaml.tmpl b/pkg/ansible/watches/testdata/valid.yaml.tmpl index 29dc5dfe5e..e3bcfc3a94 100644 --- a/pkg/ansible/watches/testdata/valid.yaml.tmpl +++ b/pkg/ansible/watches/testdata/valid.yaml.tmpl @@ -57,16 +57,16 @@ sentinel: finalizer_running - version: v1alpha1 group: app.example.com - kind: MaxWorkersDefault + kind: MaxConcurrentReconcilesDefault role: {{ .ValidRole }} - version: v1alpha1 group: app.example.com - kind: MaxWorkersIgnored + kind: MaxConcurrentReconcilesIgnored role: {{ .ValidRole }} maxWorkers: 5 - version: v1alpha1 group: app.example.com - kind: MaxWorkersEnv + kind: MaxConcurrentReconcilesEnv role: {{ .ValidRole }} - version: v1alpha1 group: app.example.com diff --git a/pkg/ansible/watches/watches.go b/pkg/ansible/watches/watches.go index 6c7ad86327..1da5003ee8 100644 --- a/pkg/ansible/watches/watches.go +++ b/pkg/ansible/watches/watches.go @@ -56,8 +56,8 @@ type Watch struct { Selector metav1.LabelSelector `yaml:"selector"` // Not configurable via watches.yaml - MaxWorkers int `yaml:"-"` - AnsibleVerbosity int `yaml:"-"` + MaxConcurrentReconciles int `yaml:"-"` + AnsibleVerbosity int `yaml:"-"` } // Finalizer - Expose finalizer to be used by a user. @@ -179,7 +179,7 @@ func (w *Watch) setValuesFromAlias(tmp alias) error { w.Role = tmp.Role w.Vars = tmp.Vars w.MaxRunnerArtifacts = tmp.MaxRunnerArtifacts - w.MaxWorkers = getMaxConcurrentReconciles(gvk, maxConcurrentReconcilesDefault) + w.MaxConcurrentReconciles = getMaxConcurrentReconciles(gvk, maxConcurrentReconcilesDefault) w.ReconcilePeriod = tmp.ReconcilePeriod.Duration w.ManageStatus = *tmp.ManageStatus w.WatchDependentResources = *tmp.WatchDependentResources @@ -306,7 +306,7 @@ func New(gvk schema.GroupVersionKind, role, playbook string, vars map[string]int Role: role, Vars: vars, MaxRunnerArtifacts: maxRunnerArtifactsDefault, - MaxWorkers: maxConcurrentReconcilesDefault, + MaxConcurrentReconciles: maxConcurrentReconcilesDefault, ReconcilePeriod: reconcilePeriodDefault.Duration, ManageStatus: manageStatusDefault, WatchDependentResources: watchDependentResourcesDefault, diff --git a/pkg/ansible/watches/watches_test.go b/pkg/ansible/watches/watches_test.go index 3b5afeddc5..df3385e837 100644 --- a/pkg/ansible/watches/watches_test.go +++ b/pkg/ansible/watches/watches_test.go @@ -65,8 +65,9 @@ func TestNew(t *testing.T) { t.Fatalf("Unexpected maxRunnerArtifacts %v expected %v", watch.MaxRunnerArtifacts, maxRunnerArtifactsDefault) } - if watch.MaxWorkers != maxConcurrentReconcilesDefault { - t.Fatalf("Unexpected maxWorkers %v expected %v", watch.MaxWorkers, maxConcurrentReconcilesDefault) + if watch.MaxConcurrentReconciles != maxConcurrentReconcilesDefault { + t.Fatalf("Unexpected maxConcurrentReconciles %v expected %v", watch.MaxConcurrentReconciles, + maxConcurrentReconcilesDefault) } if watch.ReconcilePeriod != expectedReconcilePeriod { t.Fatalf("Unexpected reconcilePeriod %v expected %v", watch.ReconcilePeriod, @@ -246,31 +247,31 @@ func TestLoad(t *testing.T) { GroupVersionKind: schema.GroupVersionKind{ Version: "v1alpha1", Group: "app.example.com", - Kind: "MaxWorkersDefault", + Kind: "MaxConcurrentReconcilesDefault", }, - Role: validTemplate.ValidRole, - ManageStatus: true, - MaxWorkers: 1, + Role: validTemplate.ValidRole, + ManageStatus: true, + MaxConcurrentReconciles: 1, }, Watch{ GroupVersionKind: schema.GroupVersionKind{ Version: "v1alpha1", Group: "app.example.com", - Kind: "MaxWorkersIgnored", + Kind: "MaxConcurrentReconcilesIgnored", }, - Role: validTemplate.ValidRole, - ManageStatus: true, - MaxWorkers: 1, + Role: validTemplate.ValidRole, + ManageStatus: true, + MaxConcurrentReconciles: 1, }, Watch{ GroupVersionKind: schema.GroupVersionKind{ Version: "v1alpha1", Group: "app.example.com", - Kind: "MaxWorkersEnv", + Kind: "MaxConcurrentReconcilesEnv", }, - Role: validTemplate.ValidRole, - ManageStatus: true, - MaxWorkers: 4, + Role: validTemplate.ValidRole, + ManageStatus: true, + MaxConcurrentReconciles: 4, }, Watch{ GroupVersionKind: schema.GroupVersionKind{ @@ -373,7 +374,7 @@ func TestLoad(t *testing.T) { testCases := []struct { name string path string - maxWorkers int + maxConcurrentReconciles int ansibleVerbosity int expected []Watch shouldError bool @@ -453,7 +454,7 @@ func TestLoad(t *testing.T) { { name: "valid watches file", path: "testdata/valid.yaml", - maxWorkers: 1, + maxConcurrentReconciles: 1, ansibleVerbosity: 2, shouldSetAnsibleCollectionPathEnvVar: true, expected: validWatches, @@ -461,7 +462,7 @@ func TestLoad(t *testing.T) { { name: "should load file successfully with ANSIBLE ROLES PATH ENV VAR set", path: "testdata/valid.yaml", - maxWorkers: 1, + maxConcurrentReconciles: 1, ansibleVerbosity: 2, shouldSetAnsibleRolePathEnvVar: true, shouldSetAnsibleCollectionPathEnvVar: true, @@ -469,8 +470,8 @@ func TestLoad(t *testing.T) { }, } - os.Setenv("WORKER_MAXWORKERSENV_APP_EXAMPLE_COM", "4") - defer os.Unsetenv("WORKER_MAXWORKERSENV_APP_EXAMPLE_COM") + os.Setenv("WORKER_MAXCONCURRENTRECONCILESENV_APP_EXAMPLE_COM", "4") + defer os.Unsetenv("WORKER_MAXCONCURRENTRECONCILESENV_APP_EXAMPLE_COM") os.Setenv("ANSIBLE_VERBOSITY_ANSIBLEVERBOSITYENV_APP_EXAMPLE_COM", "4") defer os.Unsetenv("ANSIBLE_VERBOSITY_ANSIBLEVERBOSITYENV_APP_EXAMPLE_COM") @@ -490,7 +491,7 @@ func TestLoad(t *testing.T) { defer os.Unsetenv("ANSIBLE_COLLECTIONS_PATH") } - watchSlice, err := Load(tc.path, tc.maxWorkers, tc.ansibleVerbosity) + watchSlice, err := Load(tc.path, tc.maxConcurrentReconciles, tc.ansibleVerbosity) if err != nil && !tc.shouldError { t.Fatalf("Error occurred unexpectedly: %v", err) } @@ -546,15 +547,15 @@ func TestLoad(t *testing.T) { gotWatch.Selector, expectedWatch.Selector) } - if expectedWatch.MaxWorkers == 0 { - if gotWatch.MaxWorkers != tc.maxWorkers { - t.Fatalf("Unexpected max workers: %v expected workers: %v", gotWatch.MaxWorkers, - tc.maxWorkers) + if expectedWatch.MaxConcurrentReconciles == 0 { + if gotWatch.MaxConcurrentReconciles != tc.maxConcurrentReconciles { + t.Fatalf("Unexpected max workers: %v expected workers: %v", gotWatch.MaxConcurrentReconciles, + tc.maxConcurrentReconciles) } } else { - if gotWatch.MaxWorkers != expectedWatch.MaxWorkers { - t.Fatalf("Unexpected max workers: %v expected workers: %v", gotWatch.MaxWorkers, - expectedWatch.MaxWorkers) + if gotWatch.MaxConcurrentReconciles != expectedWatch.MaxConcurrentReconciles { + t.Fatalf("Unexpected max workers: %v expected workers: %v", gotWatch.MaxConcurrentReconciles, + expectedWatch.MaxConcurrentReconciles) } } } @@ -562,7 +563,7 @@ func TestLoad(t *testing.T) { } } -func TestMaxWorkers(t *testing.T) { +func TestMaxConcurrentReconciles(t *testing.T) { testCases := []struct { name string gvk schema.GroupVersionKind @@ -654,7 +655,8 @@ func TestMaxWorkers(t *testing.T) { } workers := getMaxConcurrentReconciles(tc.gvk, tc.defValue) if tc.expectedValue != workers { - t.Fatalf("Unexpected MaxWorkers: %v expected MaxWorkers: %v", workers, tc.expectedValue) + t.Fatalf("Unexpected MaxConcurrentReconciles: %v expected MaxConcurrentReconciles: %v", + workers, tc.expectedValue) } }) } diff --git a/pkg/helm/controller/controller.go b/pkg/helm/controller/controller.go index c6d551840d..0fc13c4cfe 100644 --- a/pkg/helm/controller/controller.go +++ b/pkg/helm/controller/controller.go @@ -49,7 +49,7 @@ type WatchOptions struct { ReconcilePeriod time.Duration WatchDependentResources bool OverrideValues map[string]string - MaxWorkers int + MaxConcurrentReconciles int } // Add creates a new helm operator controller and adds it to the manager @@ -71,7 +71,7 @@ func Add(mgr manager.Manager, options WatchOptions) error { c, err := controller.New(controllerName, mgr, controller.Options{ Reconciler: r, - MaxConcurrentReconciles: options.MaxWorkers, + MaxConcurrentReconciles: options.MaxConcurrentReconciles, }) if err != nil { return err diff --git a/pkg/helm/flags/flag.go b/pkg/helm/flags/flag.go index 8ded6d94d8..6ede9e9385 100644 --- a/pkg/helm/flags/flag.go +++ b/pkg/helm/flags/flag.go @@ -27,11 +27,11 @@ import ( type Flags struct { ReconcilePeriod time.Duration WatchesFile string - MaxWorkers int MetricsAddress string EnableLeaderElection bool LeaderElectionID string LeaderElectionNamespace string + MaxConcurrentReconciles int } // AddTo - Add the helm operator flags to the the flagset @@ -47,11 +47,6 @@ func (f *Flags) AddTo(flagSet *pflag.FlagSet) { "./watches.yaml", "Path to the watches file to use", ) - flagSet.IntVar(&f.MaxWorkers, - "max-workers", - runtime.NumCPU(), - "Maximum number of workers to use", - ) flagSet.StringVar(&f.MetricsAddress, "metrics-addr", ":8080", @@ -72,4 +67,9 @@ func (f *Flags) AddTo(flagSet *pflag.FlagSet) { "", "Namespace in which to create the leader election configmap for holding the leader lock (required if running locally with leader election enabled).", ) + flagSet.IntVar(&f.MaxConcurrentReconciles, + "max-concurrent-reconciles", + runtime.NumCPU(), + "Maximum number of concurrent reconciles for controllers.", + ) } From 60c9ca2a1f59be8642a51ac14daa3f275bf5ffc1 Mon Sep 17 00:00:00 2001 From: varshaprasad96 Date: Fri, 17 Jul 2020 16:58:29 -0700 Subject: [PATCH 2/3] Modify changelog fragment --- changelog/fragments/replace-max-workers.yaml | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/changelog/fragments/replace-max-workers.yaml b/changelog/fragments/replace-max-workers.yaml index 02532a3b78..c5b1f595bb 100644 --- a/changelog/fragments/replace-max-workers.yaml +++ b/changelog/fragments/replace-max-workers.yaml @@ -1,4 +1,12 @@ entries: - description: > - Add "--max-concurrent-reconciles" flag to helm binary. - kind: "addition" + **(Valid only for Ansible/Helm-based Operators)**. The flag `--max-workers` is no longer supported. + Use `--max-concurrent-reconciles` instead. + kind: "removal" + breaking: true + migration: + header: Flag `max-workers` is no longer supported. + body: | + The flag `max-workers` is no longer supported with ansible/helm binaries. It has been + renamed to to `--max-concurrent-reconciles`. + From 86251246aeca0607add3f573c837c0e94ddc559e Mon Sep 17 00:00:00 2001 From: varshaprasad96 Date: Sun, 19 Jul 2020 20:48:10 -0700 Subject: [PATCH 3/3] update relevant helm doc --- .../{replace-max-workers.yaml => rename-max-workers.yaml} | 6 +++--- .../building-operators/helm/reference/advanced_features.md | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) rename changelog/fragments/{replace-max-workers.yaml => rename-max-workers.yaml} (63%) diff --git a/changelog/fragments/replace-max-workers.yaml b/changelog/fragments/rename-max-workers.yaml similarity index 63% rename from changelog/fragments/replace-max-workers.yaml rename to changelog/fragments/rename-max-workers.yaml index c5b1f595bb..fbdf852ddb 100644 --- a/changelog/fragments/replace-max-workers.yaml +++ b/changelog/fragments/rename-max-workers.yaml @@ -2,11 +2,11 @@ entries: - description: > **(Valid only for Ansible/Helm-based Operators)**. The flag `--max-workers` is no longer supported. Use `--max-concurrent-reconciles` instead. - kind: "removal" + kind: "change" breaking: true migration: header: Flag `max-workers` is no longer supported. body: | - The flag `max-workers` is no longer supported with ansible/helm binaries. It has been - renamed to to `--max-concurrent-reconciles`. + The flag `--max-workers` is no longer supported with ansible/helm binaries. It has been + renamed to `--max-concurrent-reconciles`. diff --git a/website/content/en/docs/building-operators/helm/reference/advanced_features.md b/website/content/en/docs/building-operators/helm/reference/advanced_features.md index 05d263bfbb..c7bca88a64 100644 --- a/website/content/en/docs/building-operators/helm/reference/advanced_features.md +++ b/website/content/en/docs/building-operators/helm/reference/advanced_features.md @@ -101,6 +101,11 @@ $ cat config/manager/manager.yaml ... ``` +While running locally, this flag can also be added to the helm binary. For example, running `helm-operator` binary with the above mentioned flag would give us a similar result: +``` +helm-operator --max-concurrent-reconciles=10 +``` + **NOTE**: If you're using the default scaffolding, it is necessary to also apply this change to the `config/default/manager_auth_proxy_patch.yaml` file. This file is a `kustomize` patch to the operator deployment that configures [kube-rbac-proxy][kube-rbac-proxy] to require authorization for accessing your operator metrics. When `kustomize` applies this patch, it overrides the args defined in `config/manager/manager.yaml` ## Use `helm upgrade --force` for deployment