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

feat(helm): add gcEnable configuration to chart for PR #167 #177

Closed
wants to merge 3 commits into from

Conversation

njuptlzf
Copy link
Contributor

@njuptlzf njuptlzf commented Dec 7, 2023

Pull Request

Why is this PR required? What issue does it fix?:
for PR #167

What this PR does?:
add gcEnable configuration to helm chart.

Does this PR require any upgrade changes?:

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

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

nfsProvisioner:
  # Provide a switch to turn off the function of clearing stale pvc to avoid
  # garbage collecting an NFS backend PVC if the NFS PVC is deleted.
  enableGarbageCollection: "true"

@njuptlzf
Copy link
Contributor Author

njuptlzf commented Dec 7, 2023

@niladrih ptal. :)

@njuptlzf
Copy link
Contributor Author

njuptlzf commented Dec 7, 2023

I found that CI reported an error when executing ct lint --config ct.yml

Run ct lint --config ct.yml
... ...
Error: failed linting charts: failed processing charts
Error: unknown flag: --timeout
------------------------------------------------------------------------------------------------------------------------
 ✖︎ nfs-provisioner => (version: "0.10.3", path: "deploy/helm/charts") > failed waiting for process: exit status 1
------------------------------------------------------------------------------------------------------------------------
failed linting charts: failed processing charts
Error: Process completed with exit code 1.

It's strange that --timeout is not supported, this is a capability that has been supported since 2021 and is still supported in helm v3.13.2, even if CI is using helm v3.12.1.

It doesn't happen locally.

root@k3s-1:/home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner# helm version
version.BuildInfo{Version:"v3.13.2", GitCommit:"2a2fb3b98829f1e0be6fb18af2f6599e0f4e8243", GitTreeState:"clean", GoVersion:"go1.20.10"}

root@k3s-1:/home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner# ct version
Version:         v3.10.0
Git commit:      0cb17e5aa89e2d6cf49cb4e7f09b602af58adfbb
Date:            2023-10-31T14:27:21Z
License:         Apache 2.0

root@k3s-1:/home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner# export CT_CONFIG_DIR=/home/helm/ct-v3.10.0/etc

root@k3s-1:/home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner# ct lint --config ct.yml
Linting charts...

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 nfs-provisioner => (version: "0.10.3", path: "deploy/helm/charts")
------------------------------------------------------------------------------------------------------------------------

Linting chart "nfs-provisioner => (version: \"0.10.3\", path: \"deploy/helm/charts\")"
Checking chart "nfs-provisioner => (version: \"0.10.3\", path: \"deploy/helm/charts\")" for a version bump...
Old chart version: 0.10.2
New chart version: 0.10.3
Chart version ok.
Validating /home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner/deploy/helm/charts/Chart.yaml...
Validation success! 👍
Validating maintainers...
==> Linting deploy/helm/charts

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ nfs-provisioner => (version: "0.10.3", path: "deploy/helm/charts")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully
root@k3s-1:/home/go_project/src/github.com/njuptlzf/dynamic-nfs-provisioner# 

@niladrih, I'm still troubleshooting the differences between CI and local, do you have any ideas? :(

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.

Left few comments @njuptlzf

Comment on lines +108 to +109
- name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
value: "{{ .Values.nfsProvisioner.enableGarbageCollection }}"
Copy link
Member

Choose a reason for hiding this comment

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

nitpick:

Suggested change
- name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
value: "{{ .Values.nfsProvisioner.enableGarbageCollection }}"
{{ if .Values.nfsProvisioner.enableGarbageCollection }}
- name: OPENEBS_IO_NFS_SERVER_GARBAGE_COLLECTION_ENABLED
value: {{ quote .Values.nfsProvisioner.enableGarbageCollection }}
{{- end }}

And on the values.yaml, the option wouldn't need quotes around it anymore

enableGarbageCollection: true

@@ -131,6 +131,7 @@ helm install openebs-nfs openebs-nfs/nfs-provisioner --namespace openebs --creat
| `nfsProvisioner.nfsServerNodeAffinity` | NFS Server node affinity rules | `""` |
| `nfsProvisioner.nfsBackendPvcTimeout` | Timeout for backend PVC binding in seconds | `"60"` |
| `nfsProvisioner.nfsHookConfigMap` | Existing Configmap name to load hook configuration | `""` |
| `nfsProvisioner.enableGarbageCollection` | Enable garbage collection | `true` |
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| `nfsProvisioner.enableGarbageCollection` | Enable garbage collection | `true` |
| `nfsProvisioner.enableGarbageCollection` | Enable garbage collection for the backend PVC | `true` |

@niladrih
Copy link
Member

niladrih commented Dec 8, 2023

@niladrih, I'm still troubleshooting the differences between CI and local, do you have any ideas? :(

This is because of this bug helm/chart-testing-action#135 (comment). It was fixed in 2.6.1 of the action, I'll raise a PR to fix this. I'll pull in your commit and add it my suggested change. I think this will be a release PR as well :)

@niladrih
Copy link
Member

niladrih commented Dec 8, 2023

Closing this, as these changes are included in #178

@niladrih niladrih closed this Dec 8, 2023
@njuptlzf njuptlzf deleted the gc_enable_helm branch December 8, 2023 11:04
@njuptlzf
Copy link
Contributor Author

njuptlzf commented Dec 8, 2023

Left few comments @njuptlzf

Thanks, this will benefit my habits a lot

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.

2 participants