From 04ed903d146120e641a0cc0dc23e2de8d15d0309 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 --- 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 ba8f6269f841..382cec592ff9 100644 --- a/cmd/controller/controller.go +++ b/cmd/controller/controller.go @@ -373,8 +373,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 6fbb393f9124..a3cd30327b74 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -52,26 +52,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 *config.CfgVars, kubeClientFactory kubeutil.ClientFactoryInterface, leaderElector leaderelector.Interface, concurrencyLevel int) *ExtensionsController { +func NewExtensionsController(s manifestsSaver, k0sVars *config.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 e49166c25121e869f2bf9147b989552ffd90184f 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 --- pkg/component/controller/extensions_controller.go | 6 ++---- pkg/helm/helm.go | 7 +++++++ 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/pkg/component/controller/extensions_controller.go b/pkg/component/controller/extensions_controller.go index a3cd30327b74..5b64d083bdf5 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -354,10 +354,8 @@ func (ec *ExtensionsController) Start(ctx context.Context) error { Metrics: metricsserver.Options{ BindAddress: "0", }, - Logger: logrusr.New(ec.L), - Controller: ctrlconfig.Controller{ - GroupKindConcurrency: map[string]int{gk.String(): 10}, - }, + Logger: logrusr.New(ec.L), + Controller: ctrlconfig.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 8d751d257295..c73730bb5ad8 100644 --- a/pkg/helm/helm.go +++ b/pkg/helm/helm.go @@ -41,6 +41,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 @@ -206,6 +207,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 { @@ -253,6 +256,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 { @@ -308,6 +313,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 62f0a7e8b6749d3e1937cbf23b878014e97bc181 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 --- .../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 5b64d083bdf5..6bf57603e36e 100644 --- a/pkg/component/controller/extensions_controller.go +++ b/pkg/component/controller/extensions_controller.go @@ -224,7 +224,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 @@ -252,7 +252,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, @@ -265,7 +266,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 c73730bb5ad8..59e273a4fd14 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" @@ -209,7 +210,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) @@ -249,7 +250,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) } @@ -258,7 +259,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) @@ -292,7 +293,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) } @@ -315,12 +316,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) }