Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(helm): Add extra args to zfsController containers and leader election inteligence #492

Merged
merged 1 commit into from
Jan 24, 2024

Conversation

trunet
Copy link
Contributor

@trunet trunet commented Nov 22, 2023

Added zfsController extraArgs variable to pass extra argumenta to containers command line.

Also adding inteligence to enable leader election only when number of replicas is more than one.

Fixes #486

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?: It'll fix a high number of restarts on slow clusters by disabling leader election when replicas < 2

What this PR does?: It'll add option to add extra arguments to zfsController containers and enable leader election only when replicas > 1

Does this PR require any upgrade changes?: No

If the changes in this PR are manually verified, list down the scenarios covered::

  • helm install with default values
  • helm install --set zfsController.replicas=2
  • helm install --set zfsController.replicas=2 --set zfsController.resizer.extraArgs={--leader-election-lease-duration 60}

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

@trunet trunet force-pushed the leader-election-helm-options branch from b1bd898 to 63a0103 Compare November 22, 2023 00:43
@sinhaashish
Copy link
Member

Thanks for contributing @trunet. Can you please share the test result with the newly added command. @pawanpraka1 It would be great if you too can review it.

@trunet
Copy link
Contributor Author

trunet commented Nov 22, 2023

@sinhaashish what do you expect from "test results"? What I did was helm template with the variables I sent and checking the output. If I share, will be a very long comment because each run will be a lot of resources.

@trunet
Copy link
Contributor Author

trunet commented Nov 22, 2023

These are the tests I did. Hope it's enough.

# helm template deploy/helm/charts
...
      containers:
        - name: csi-resizer
          image: "registry.k8s.io/sig-storage/csi-resizer:v1.8.0"
          args:
            - "--v=5"
            - "--csi-address=$(ADDRESS)"
...
        - name: csi-snapshotter
          image: "registry.k8s.io/sig-storage/csi-snapshotter:v6.2.2"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
...
        - name: snapshot-controller
          image: "registry.k8s.io/sig-storage/snapshot-controller:v6.2.2"
          args:
            - "--v=5"
...
        - name: csi-provisioner
          image: "registry.k8s.io/sig-storage/csi-provisioner:v3.5.0"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
            - "--v=5"
            - "--feature-gates=Topology=true"
            - "--strict-topology"
            - "--enable-capacity=true"
            - "--extra-create-metadata=true"
            - "--default-fstype=ext4"
...
# helm template deploy/helm/charts --set zfsController.replicas=2
...
      containers:
        - name: csi-resizer
          image: "registry.k8s.io/sig-storage/csi-resizer:v1.8.0"
          args:
            - "--v=5"
            - "--csi-address=$(ADDRESS)"
            - "--leader-election"
...
        - name: csi-snapshotter
          image: "registry.k8s.io/sig-storage/csi-snapshotter:v6.2.2"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
            - "--leader-election"
...
        - name: snapshot-controller
          image: "registry.k8s.io/sig-storage/snapshot-controller:v6.2.2"
          args:
            - "--v=5"
            - "--leader-election"
          imagePullPolicy: IfNotPresent
        - name: csi-provisioner
          image: "registry.k8s.io/sig-storage/csi-provisioner:v3.5.0"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
            - "--v=5"
            - "--feature-gates=Topology=true"
            - "--strict-topology"
            - "--enable-capacity=true"
            - "--extra-create-metadata=true"
            - "--default-fstype=ext4"
            - "--leader-election"
...
# helm template deploy/helm/charts --set zfsController.replicas=2 --set zfsController.resizer.extraArgs={"--leader-election-lease-duration 60"}
         containers:
        - name: csi-resizer
          image: "registry.k8s.io/sig-storage/csi-resizer:v1.8.0"
          args:
            - "--v=5"
            - "--csi-address=$(ADDRESS)"
            - "--leader-election"
            - "--leader-election-lease-duration 60"
...
        - name: csi-snapshotter
          image: "registry.k8s.io/sig-storage/csi-snapshotter:v6.2.2"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
            - "--leader-election"
...
        - name: snapshot-controller
          image: "registry.k8s.io/sig-storage/snapshot-controller:v6.2.2"
          args:
            - "--v=5"
            - "--leader-election"
          imagePullPolicy: IfNotPresent
        - name: csi-provisioner
          image: "registry.k8s.io/sig-storage/csi-provisioner:v3.5.0"
          imagePullPolicy: IfNotPresent
          args:
            - "--csi-address=$(ADDRESS)"
            - "--v=5"
            - "--feature-gates=Topology=true"
            - "--strict-topology"
            - "--enable-capacity=true"
            - "--extra-create-metadata=true"
            - "--default-fstype=ext4"
            - "--leader-election"
...

@trunet
Copy link
Contributor Author

trunet commented Nov 27, 2023

@sinhaashish and @pawanpraka1 Is there anything else I need to do to have this merged?

@trunet
Copy link
Contributor Author

trunet commented Dec 4, 2023

@sinhaashish @pawanpraka1 could you give a feedback on this?

@trunet trunet force-pushed the leader-election-helm-options branch from 63a0103 to 7e5ebd5 Compare December 4, 2023 23:29
@sinhaashish
Copy link
Member

@trunet Can you please resolve the conflict so that it can be merged.

@trunet
Copy link
Contributor Author

trunet commented Dec 21, 2023

It's resolved, but if you take that long again, any new release will keep causing a new conflict.

@niladrih niladrih requested a review from pawanpraka1 December 21, 2023 13:40
Copy link
Member

@niladrih niladrih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trunet -- Changes look good. However, we require DCO signs on all of your commits. The git pull-based Merge commit doesn't have a signature. You'd want to rebase your branch instead.

git checkout leader-election-helm-options
git fetch upstream develop
git rebase upstream/develop
git push origin leader-election-helm-options

Let me know if you need help with that. I'm going to have to hold off on the merge until this is fixed.

@sinhaashish
Copy link
Member

@trunet can you please do the needful here

…tion inteligence

Added zfsController extraArgs variable to pass extra argumenta to containers command line.

Also adding inteligence to enable leader election only when number of replicas is more than one.

Fixes openebs#486

Signed-off-by: Wagner Sartori Junior <wsartori@wsartori.com>
@trunet trunet force-pushed the leader-election-helm-options branch from 5fa7ed7 to 6410239 Compare January 11, 2024 01:59
@trunet
Copy link
Contributor Author

trunet commented Jan 11, 2024

@sinhaashish I was off-grid, on holidays, just fixed.

@trunet trunet requested a review from niladrih January 11, 2024 02:02
@niladrih niladrih merged commit 15260d6 into openebs:develop Jan 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ZFS Controller leader election arguments in helm charts
4 participants