Skip to content

update statefulset to include verbose PVC reference#654

Open
ritzk wants to merge 1 commit intoopensearch-project:mainfrom
ritzk:argo-sync
Open

update statefulset to include verbose PVC reference#654
ritzk wants to merge 1 commit intoopensearch-project:mainfrom
ritzk:argo-sync

Conversation

@ritzk
Copy link
Copy Markdown

@ritzk ritzk commented Feb 21, 2025

Description

The issue is that the verbose reference is added once ArgoCD applies the chart to cluster. This results in ArgoCD thinking there is a diff, when there is none. This can be solved by adding a patch which adds the verbose reference.

Issues Resolved

n/a

Check List

  • Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Similar to bitnami/charts#17140

@DandyDeveloper
Copy link
Copy Markdown
Collaborator

@ritzk Please do DCO Checks and check the linting output.

@ritzk
Copy link
Copy Markdown
Author

ritzk commented Feb 21, 2025

@ritzk Please do DCO Checks and check the linting output.

@DandyDeveloper lint failed due to n/w issues on the action runner.

→ ct lint
Linting charts...

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 opensearch => (version: "2.31.1", path: "charts/opensearch")
------------------------------------------------------------------------------------------------------------------------

Linting chart "opensearch => (version: \"2.31.1\", path: \"charts/opensearch\")"
Checking chart "opensearch => (version: \"2.31.1\", path: \"charts/opensearch\")" for a version bump...
Old chart version: 2.31.0
New chart version: 2.31.1
Chart version ok.
Validating charts/opensearch/Chart.yaml...
Validation success! 👍
Validating maintainers...

Linting chart with values file "charts/opensearch/ci/ci-ingress-class-name-values.yaml"...

==> Linting charts/opensearch
[INFO] Chart.yaml: icon is recommended

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

Linting chart with values file "charts/opensearch/ci/ci-rbac-enabled-values.yaml"...

==> Linting charts/opensearch
[INFO] Chart.yaml: icon is recommended

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

Linting chart with values file "charts/opensearch/ci/ci-values.yaml"...

==> Linting charts/opensearch
[INFO] Chart.yaml: icon is recommended

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

------------------------------------------------------------------------------------------------------------------------
 ✔︎ opensearch => (version: "2.31.1", path: "charts/opensearch")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully

@DandyDeveloper
Copy link
Copy Markdown
Collaborator

@ritzk One thing you will wanna test, can you install the original, then upgrade with your changes. Can you confirm if this "breaks" because the STS isn't immutable? This will cause some real problems because people would effectively need to remove the STS, then apply these updates which could bring multiple nodes down on an upgrade if forced.

@ritzk
Copy link
Copy Markdown
Author

ritzk commented Feb 27, 2025

@ritzk One thing you will wanna test, can you install the original, then upgrade with your changes. Can you confirm if this "breaks" because the STS isn't immutable? This will cause some real problems because people would effectively need to remove the STS, then apply these updates which could bring multiple nodes down on an upgrade if forced.

This is what is written to kubernetes, atleast on 1.24.x & 1.29.x

→ helm install -n opensearch --create-namespace opensearch  opensearch/opensearch
→ kubectl -n opensearch  get statefulsets.apps  opensearch-cluster-master -o jsonpath-as-json='{.spec.volumeClaimTemplates[0]}'|jq .
[
  {
    "apiVersion": "v1",
    "kind": "PersistentVolumeClaim",
    "metadata": {
      "creationTimestamp": null,
      "name": "opensearch-cluster-master"
    },
    "spec": {
      "accessModes": [
        "ReadWriteOnce"
      ],
      "resources": {
        "requests": {
          "storage": "8Gi"
        }
      },
      "volumeMode": "Filesystem"
    },
    "status": {
      "phase": "Pending"
    }
  }
]

@DandyDeveloper
Copy link
Copy Markdown
Collaborator

@ritzk Sorry for the delay, I mean we need to test an install from the current chart version, then upgrade to yours. Because the change in spec MAY complain that the StatefulSet resource is immutable and cannot be changed.

This will mean people upgrading will all break and have to forcefully remove and add the statefulset. This could be bad!

Can you check this?

@ritzk
Copy link
Copy Markdown
Author

ritzk commented Mar 8, 2025

@ritzk Sorry for the delay, I mean we need to test an install from the current chart version, then upgrade to yours. Because the change in spec MAY complain that the StatefulSet resource is immutable and cannot be changed.

This will mean people upgrading will all break and have to forcefully remove and add the statefulset. This could be bad!

Can you check this?

@DandyDeveloper Works fine.

install

opensearch ±|main|→ git remote  -v
origin	https://github.com/opensearch-project/helm-charts.git (fetch)
origin	https://github.com/opensearch-project/helm-charts.git (push)

opensearch ±|main|→ helm upgrade --install opensearch -n opensearch --create-namespace .
Release "opensearch" does not exist. Installing it now.
NAME: opensearch
LAST DEPLOYED: Sat Mar  8 14:16:41 2025
NAMESPACE: opensearch
STATUS: deployed
REVISION: 1
TEST SUITE: None
NOTES:
Watch all cluster members come up.

upgrade

opensearch ±|argo-sync ✗|→ git remote  -v
origin	git@github.com:ritzk/opensearch-helm-charts.git (fetch)
origin	git@github.com:ritzk/opensearch-helm-charts.git (push)

opensearch ±|argo-sync ✗|→ helm upgrade --install opensearch -n opensearch --create-namespace . 
Release "opensearch" has been upgraded. Happy Helming!
NAME: opensearch
LAST DEPLOYED: Sat Mar  8 14:24:46 2025
NAMESPACE: opensearch
STATUS: deployed
REVISION: 2
TEST SUITE: None
NOTES:
Watch all cluster members come up.
  $ kubectl get pods --namespace=opensearch -l app.kubernetes.io/component=opensearch-cluster-master -w

Signed-off-by: Ritesh Khadgaray <khadgaray@gmail.com>
@ritzk ritzk changed the title update statefulset to inlcude verbose PVC reference update statefulset to include verbose PVC reference Mar 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants