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

Always include volumes in statefulsets #5081

Merged
merged 1 commit into from Dec 6, 2023
Merged

Conversation

dzsibi
Copy link
Contributor

@dzsibi dzsibi commented Dec 6, 2023

What problem are we solving?

When both master.data.type and master.logs.type is persistentVolumeClaim, deployment of the Helm chart fails:

create Pod seaweedfs-master-0 in StatefulSet seaweedfs-master failed error: Pod "seaweedfs-master-0" is invalid: spec.containers[0].volumeMounts[1].name: Not found: "master-config"

This is due to a condition that omits the entire volumes section in this case:

https://github.com/seaweedfs/seaweedfs/blob/d5d9fbb8aa5e86f2b866e25f58bf36a7a989478d/k8s/charts/seaweedfs/templates/master-statefulset.yaml#L217C20-L217C20

The issue was introduced by #4797, as it did not take into account this condition.

How are we solving the problem?

I removed the conditions entirely, including the ones for volume-statefulset. They also interfere with a bunch of other options (extraVolumes, enableSecurity).

How is the PR tested?

For the default configuration (using hostPath volumes), deployment works before and after my changes. For a configuration that uses PVCs instead, deployments fail before and work after.

An example values.yaml to trigger the bug:

seaweedfs:
  master:
    data:
      type: persistentVolumeClaim
    logs:
      type: persistentVolumeClaim

Checks

I am unsure if these are relevant for this PR.

  • I have added unit tests if possible.
  • I will add related wiki document changes and link to this PR after merging.

@chrislusf
Copy link
Collaborator

Thanks! Merging.

cc @aviau let me know if you have any concern.

@chrislusf chrislusf merged commit 43a6b60 into seaweedfs:master Dec 6, 2023
5 checks passed
@dzsibi
Copy link
Contributor Author

dzsibi commented Dec 6, 2023

Hmm, I didn't know it's going to auto-publish immediately, I think it may have overwritten the previous version since I did not include a version bump in the PR.

@jessebot
Copy link
Contributor

jessebot commented Dec 14, 2023

@dzsibi yes, can you please submit a PR to bump the helm chart version?

@chrislusf merge this after the helm chart is bumped to prevent this in the future: #5106 If you don't want till after, you'll have to force merge, because the lint might complain that nothing has changed for the version bump.

@jessebot
Copy link
Contributor

nevermind, #5108 should take care of this :)

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.

None yet

3 participants