From 2bd85ddb98fd8669046626be0e3f91a86c4c8629 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Luis=20de=20Sousa-Valadas=20Casta=C3=B1o?= Date: Mon, 23 Oct 2023 16:09:41 +0200 Subject: [PATCH 1/3] Remove unused concurrencyLevel in helm controller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This field isn't honored at all and we want to remove concurrent reconciliation anyway so entirely remove the field. Signed-off-by: Juan Luis de Sousa-Valadas Castaño (cherry picked from commit 04ed903d146120e641a0cc0dc23e2de8d15d0309) (cherry picked from commit 293475cbf0c7a30dc7c2d3a933eafd100e8b7062) --- cmd/controller/controller.go | 2 -- .../controller/extensions_controller.go | 24 +++++++++---------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/cmd/controller/controller.go b/cmd/controller/controller.go index bcb687a03498..f53b2cae2b8d 100644 --- a/cmd/controller/controller.go +++ b/cmd/controller/controller.go @@ -355,8 +355,6 @@ func (c *command) start(ctx context.Context) error { c.K0sVars, adminClientFactory, leaderElector, - // Hardcode until the config loading is fixed - 10, )) } diff --git a/pkg/component/controller/extensions_controller.go b/pkg/component/controller/extensions_controller.go index c6baa5c4a54f..7942d57a0f05 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -49,26 +49,24 @@ import ( // Helm watch for Chart crd type ExtensionsController struct { - concurrencyLevel int - saver manifestsSaver - L *logrus.Entry - helm *helm.Commands - kubeConfig string - leaderElector leaderelector.Interface + saver manifestsSaver + L *logrus.Entry + helm *helm.Commands + kubeConfig string + leaderElector leaderelector.Interface } var _ manager.Component = (*ExtensionsController)(nil) var _ manager.Reconciler = (*ExtensionsController)(nil) // NewExtensionsController builds new HelmAddons -func NewExtensionsController(s manifestsSaver, k0sVars constant.CfgVars, kubeClientFactory kubeutil.ClientFactoryInterface, leaderElector leaderelector.Interface, concurrencyLevel int) *ExtensionsController { +func NewExtensionsController(s manifestsSaver, k0sVars constant.CfgVars, kubeClientFactory kubeutil.ClientFactoryInterface, leaderElector leaderelector.Interface) *ExtensionsController { return &ExtensionsController{ - concurrencyLevel: concurrencyLevel, - saver: s, - L: logrus.WithFields(logrus.Fields{"component": "extensions_controller"}), - helm: helm.NewCommands(k0sVars), - kubeConfig: k0sVars.AdminKubeConfigPath, - leaderElector: leaderElector, + saver: s, + L: logrus.WithFields(logrus.Fields{"component": "extensions_controller"}), + helm: helm.NewCommands(k0sVars), + kubeConfig: k0sVars.AdminKubeConfigPath, + leaderElector: leaderElector, } } From 1592c368bf6718fdc285dee3e8a6567625a74a96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Luis=20de=20Sousa-Valadas=20Casta=C3=B1o?= Date: Mon, 23 Oct 2023 17:18:49 +0200 Subject: [PATCH 2/3] Make extensions controller thread-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Helm is unable to reconcile multiple charts at the same time because /var/lib/k0s/helmhome/cache/ is shared between charts and helm cache was not designed to be safe to be used concurrently. Signed-off-by: Juan Luis de Sousa-Valadas Castaño (cherry picked from commit e49166c25121e869f2bf9147b989552ffd90184f) (cherry picked from commit b0de7236768183786272330de76a9753a9ff2f4a) --- pkg/component/controller/extensions_controller.go | 4 +--- pkg/helm/helm.go | 7 +++++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/component/controller/extensions_controller.go b/pkg/component/controller/extensions_controller.go index 7942d57a0f05..3cefbc8fe7b4 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -350,9 +350,7 @@ func (ec *ExtensionsController) Start(ctx context.Context) error { mgr, err := ctrlManager.New(clientConfig, ctrlManager.Options{ MetricsBindAddress: "0", Logger: logrusr.New(ec.L), - Controller: config.Controller{ - GroupKindConcurrency: map[string]int{gk.String(): 10}, - }, + Controller: config.Controller{}, }) if err != nil { return fmt.Errorf("can't build controller-runtime controller for helm extensions: %w", err) diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go index 6f26d44533da..dfb1a1a7d6f9 100644 --- a/pkg/helm/helm.go +++ b/pkg/helm/helm.go @@ -40,6 +40,7 @@ import ( ) // Commands run different helm command in the same way as CLI tool +// This struct isn't thread-safe. Check on a per function basis. type Commands struct { repoFile string helmCacheDir string @@ -205,6 +206,8 @@ func (hc *Commands) isInstallable(chart *chart.Chart) bool { return true } +// InstallChart installs a helm chart +// InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe func (hc *Commands) InstallChart(chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { cfg, err := hc.getActionCfg(namespace) if err != nil { @@ -252,6 +255,8 @@ func (hc *Commands) InstallChart(chartName string, version string, releaseName s return chartRelease, nil } +// UpgradeChart upgrades a helm chart. +// InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe func (hc *Commands) UpgradeChart(chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { cfg, err := hc.getActionCfg(namespace) if err != nil { @@ -307,6 +312,8 @@ func (hc *Commands) ListReleases(namespace string) ([]*release.Release, error) { return helmAction.Run() } +// UninstallRelease uninstalls a release. +// InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe func (hc *Commands) UninstallRelease(releaseName string, namespace string) error { cfg, err := hc.getActionCfg(namespace) if err != nil { From fd2c80e2f58b84d9143bd6736709e273ac947dd2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Juan=20Luis=20de=20Sousa-Valadas=20Casta=C3=B1o?= Date: Thu, 26 Oct 2023 14:04:08 +0200 Subject: [PATCH 3/3] Implement timeouts in helm reconcile loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Up to now there was a context passed but it was never cancelled. Signed-off-by: Juan Luis de Sousa-Valadas Castaño (cherry picked from commit 62f0a7e8b6749d3e1937cbf23b878014e97bc181) (cherry picked from commit 15c1c93b3cab64a563af678618b8ddc6a6881395) --- .../controller/extensions_controller.go | 8 +++++--- pkg/helm/helm.go | 16 +++++++++++----- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/pkg/component/controller/extensions_controller.go b/pkg/component/controller/extensions_controller.go index 3cefbc8fe7b4..977073137327 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -221,7 +221,7 @@ func (cr *ChartReconciler) Reconcile(ctx context.Context, req reconcile.Request) return reconcile.Result{}, nil } func (cr *ChartReconciler) uninstall(ctx context.Context, chart v1beta1.Chart) error { - if err := cr.helm.UninstallRelease(chart.Status.ReleaseName, chart.Status.Namespace); err != nil { + if err := cr.helm.UninstallRelease(ctx, chart.Status.ReleaseName, chart.Status.Namespace); err != nil { return fmt.Errorf("can't uninstall release `%s/%s`: %w", chart.Status.Namespace, chart.Status.ReleaseName, err) } return nil @@ -249,7 +249,8 @@ func (cr *ChartReconciler) updateOrInstallChart(ctx context.Context, chart v1bet if chart.Status.ReleaseName == "" { // new chartRelease cr.L.Tracef("Start update or install %s", chart.Spec.ChartName) - chartRelease, err = cr.helm.InstallChart(chart.Spec.ChartName, + chartRelease, err = cr.helm.InstallChart(ctx, + chart.Spec.ChartName, chart.Spec.Version, chart.Spec.ReleaseName, chart.Spec.Namespace, @@ -262,7 +263,8 @@ func (cr *ChartReconciler) updateOrInstallChart(ctx context.Context, chart v1bet } else { if cr.chartNeedsUpgrade(chart) { // update - chartRelease, err = cr.helm.UpgradeChart(chart.Spec.ChartName, + chartRelease, err = cr.helm.UpgradeChart(ctx, + chart.Spec.ChartName, chart.Spec.Version, chart.Status.ReleaseName, chart.Status.Namespace, diff --git a/pkg/helm/helm.go b/pkg/helm/helm.go index dfb1a1a7d6f9..3dab809a2157 100644 --- a/pkg/helm/helm.go +++ b/pkg/helm/helm.go @@ -17,6 +17,7 @@ limitations under the License. package helm import ( + "context" "fmt" "os" "path/filepath" @@ -208,7 +209,7 @@ func (hc *Commands) isInstallable(chart *chart.Chart) bool { // InstallChart installs a helm chart // InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe -func (hc *Commands) InstallChart(chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { +func (hc *Commands) InstallChart(ctx context.Context, chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { cfg, err := hc.getActionCfg(namespace) if err != nil { return nil, fmt.Errorf("can't create action configuration: %v", err) @@ -248,7 +249,7 @@ func (hc *Commands) InstallChart(chartName string, version string, releaseName s if err != nil { return nil, fmt.Errorf("can't reload loadedChart `%s`: %v", chartDir, err) } - chartRelease, err := install.Run(loadedChart, values) + chartRelease, err := install.RunWithContext(ctx, loadedChart, values) if err != nil { return nil, fmt.Errorf("can't install loadedChart `%s`: %v", loadedChart.Name(), err) } @@ -257,7 +258,7 @@ func (hc *Commands) InstallChart(chartName string, version string, releaseName s // UpgradeChart upgrades a helm chart. // InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe -func (hc *Commands) UpgradeChart(chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { +func (hc *Commands) UpgradeChart(ctx context.Context, chartName string, version string, releaseName string, namespace string, values map[string]interface{}, timeout time.Duration) (*release.Release, error) { cfg, err := hc.getActionCfg(namespace) if err != nil { return nil, fmt.Errorf("can't create action configuration: %v", err) @@ -291,7 +292,7 @@ func (hc *Commands) UpgradeChart(chartName string, version string, releaseName s return nil, fmt.Errorf("can't reload loadedChart `%s`: %v", chartDir, err) } - chartRelease, err := upgrade.Run(releaseName, loadedChart, values) + chartRelease, err := upgrade.RunWithContext(ctx, releaseName, loadedChart, values) if err != nil { return nil, fmt.Errorf("can't upgrade loadedChart `%s`: %v", loadedChart.Metadata.Name, err) } @@ -314,12 +315,17 @@ func (hc *Commands) ListReleases(namespace string) ([]*release.Release, error) { // UninstallRelease uninstalls a release. // InstallChart, UpgradeChart and UninstallRelease(releaseName are *NOT* thread-safe -func (hc *Commands) UninstallRelease(releaseName string, namespace string) error { +func (hc *Commands) UninstallRelease(ctx context.Context, releaseName string, namespace string) error { cfg, err := hc.getActionCfg(namespace) if err != nil { return fmt.Errorf("can't create helmAction configuration: %v", err) } helmAction := action.NewUninstall(cfg) + deadline, ok := ctx.Deadline() + if ok { + helmAction.Timeout = time.Until(deadline) + } + if _, err := helmAction.Run(releaseName); err != nil { return fmt.Errorf("can't uninstall release `%s`: %v", releaseName, err) }