controller: add support for global persistent configuration #364

Rakshith-R · 2023-06-07T10:16:25Z

This commits adds support for configuration options consumed through a optional configmap. This will enable users to create a configmap to supply options that are persisted throughout upgrades.
Current support parameters are "reclaim-space-timeout" and "max-concurrent-reconciles".

Closes: #347

nixpanic · 2023-06-07T11:13:44Z

cmd/manager/main.go

@@ -49,6 +50,7 @@ var (

 const (
 	defaultTimeout = time.Minute * 3
+	configFile     = "/run/config/csi-addons-config.json"


why use a file, and not the Kubernetes API to access the ConfigMap directly? Users can then place keys directly in the ConfigMap, and do not need to worry about weird indirect json syntax.

nixpanic

lgtm, a little more cleaning up is possible now. It would be nice to have a single place to configure default values.

docs/csi-addons-config.md

cmd/manager/main.go

Madhu-1 · 2023-06-12T06:37:40Z

cmd/manager/main.go

@@ -39,6 +39,8 @@ import (
 	controllers "github.com/csi-addons/kubernetes-csi-addons/controllers/csiaddons"
 	replicationController "github.com/csi-addons/kubernetes-csi-addons/controllers/replication.storage"
 	"github.com/csi-addons/kubernetes-csi-addons/internal/connection"
+	"github.com/csi-addons/kubernetes-csi-addons/internal/util"
+	"k8s.io/client-go/kubernetes"


add a new line

Madhu-1 · 2023-06-12T06:41:09Z

deploy/controller/csi-addons-config.yaml

+  "reclaim-space-timeout": "5m"
+  "max-concurrent-reconciles": "50"


match the values for what we have in default , if the user just creates this configmap should not have any impact unless he/she modifies the values

Madhu-1 · 2023-06-12T06:42:23Z

docs/csi-addons-config.md

+| `reclaim-space-timeout`       | `"3m"`          | Timeout for reclaimspace operation            |
+| `max-concurrent-reconciles`   | `"100"`         | Maximum number of concurrent reconciles       |
+
+## Example


let's not duplicate the configmap here as it adds a burden to maintain at 2 places, just add a reference to the configmap here.

Madhu-1 · 2023-06-12T06:47:09Z

internal/util/config.go

+	cm, err := kubeClient.CoreV1().ConfigMaps(cfg.Namespace).
+		Get(context.Background(), csiAddonsConfigMapName, metav1.GetOptions{})


Instead of creating a new context here, create one context in the main and use it.

Pull request has been modified.

Rakshith-R · 2023-06-12T09:36:16Z

@Mergifyio rebase

This commits adds support for configuration options consumed through a optional configmap. This will enable users to create a configmap to supply options that are persisted throughout upgrades. Current support parameters are `"reclaim-space-timeout"` and `"max-concurrent-reconciles"`. Signed-off-by: Rakshith R <[email protected]>

mergify · 2023-06-12T09:36:26Z

rebase

✅ Branch has been successfully rebased

nixpanic · 2023-06-12T09:43:51Z

config/manager/manager_auth_proxy_patch.yaml

@@ -31,6 +31,7 @@ spec:
            memory: 64Mi
      - name: manager
        args:
+        - "--namespace=$(POD_NAMESPACE)"


I think the env section is missing here?

I think the env section is missing here?

its added above in manager.yaml . https://github.com/csi-addons/kubernetes-csi-addons/pull/364/files/fcaf7571fe7ee850410e6f2da6fb853daa0a023f#diff-ce0d597a9e5f20a8fe61c3ba5e6185a6b90cfef2c00bb5b5ef3e97b15b79f480

This yaml patches & overrides the manager container args, so I added the namespace arg here.

nixpanic · 2023-06-12T10:53:55Z

config/manager/manager_auth_proxy_patch.yaml

@@ -31,6 +31,7 @@ spec:
            memory: 64Mi
      - name: manager
        args:
+        - "--namespace=$(POD_NAMESPACE)"


Rakshith-R requested review from nixpanic and Madhu-1 June 7, 2023 10:16

mergify bot requested a review from yati1998 June 7, 2023 10:16

Rakshith-R force-pushed the global-persistent-config branch from 9cc7642 to 926b09b Compare June 7, 2023 10:23

nixpanic reviewed Jun 7, 2023

View reviewed changes

Rakshith-R force-pushed the global-persistent-config branch 3 times, most recently from a64b409 to 0f3737f Compare June 9, 2023 07:09

Rakshith-R requested a review from nixpanic June 9, 2023 07:12

Rakshith-R force-pushed the global-persistent-config branch from 0f3737f to 6348d42 Compare June 9, 2023 07:12

nixpanic reviewed Jun 9, 2023

View reviewed changes

docs/csi-addons-config.md Show resolved Hide resolved

cmd/manager/main.go Outdated Show resolved Hide resolved

Rakshith-R force-pushed the global-persistent-config branch from 6348d42 to 9bf0fcf Compare June 9, 2023 09:47

Rakshith-R requested a review from nixpanic June 9, 2023 09:50

nixpanic previously approved these changes Jun 9, 2023

View reviewed changes

Madhu-1 reviewed Jun 12, 2023

View reviewed changes

Rakshith-R force-pushed the global-persistent-config branch from 9bf0fcf to 1e9e762 Compare June 12, 2023 09:20

Rakshith-R requested review from nixpanic and Madhu-1 June 12, 2023 09:21

Madhu-1 approved these changes Jun 12, 2023

View reviewed changes

Rakshith-R force-pushed the global-persistent-config branch from 1e9e762 to fcaf757 Compare June 12, 2023 09:36

nixpanic reviewed Jun 12, 2023

View reviewed changes

Rakshith-R requested a review from nixpanic June 12, 2023 10:22

nixpanic approved these changes Jun 12, 2023

View reviewed changes

mergify bot merged commit c927efa into csi-addons:main Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: add support for global persistent configuration #364

controller: add support for global persistent configuration #364

Rakshith-R commented Jun 7, 2023 •

edited

Loading

nixpanic Jun 7, 2023

nixpanic left a comment

Madhu-1 Jun 12, 2023

Madhu-1 Jun 12, 2023

Madhu-1 Jun 12, 2023

Madhu-1 Jun 12, 2023

Rakshith-R commented Jun 12, 2023

mergify bot commented Jun 12, 2023

nixpanic Jun 12, 2023

Rakshith-R Jun 12, 2023 •

edited

Loading

nixpanic Jun 12, 2023

nixpanic Jun 12, 2023

		"reclaim-space-timeout": "5m"
		"max-concurrent-reconciles": "50"

		cm, err := kubeClient.CoreV1().ConfigMaps(cfg.Namespace).
		Get(context.Background(), csiAddonsConfigMapName, metav1.GetOptions{})

controller: add support for global persistent configuration #364

controller: add support for global persistent configuration #364

Conversation

Rakshith-R commented Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Jun 12, 2023

mergify bot commented Jun 12, 2023

✅ Branch has been successfully rebased

Choose a reason for hiding this comment

Rakshith-R Jun 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Jun 7, 2023 •

edited

Loading

Rakshith-R Jun 12, 2023 •

edited

Loading