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

Add support for adding PVC volumes as Additional Volumes #852

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ikorchynskyi
Copy link

@ikorchynskyi ikorchynskyi commented Jul 3, 2024

Description

This PR adds the capability to add PVC volumes as AdditionalVolumes in the OpenSearch cluster

Issues Resolved

Closes #484
Closes #525

Check List

  • Commits are signed per the DCO using --signoff
  • Unittest added for the new/changed functionality and all unit tests are successful
  • Customer-visible features documented
  • No linter warnings (make lint)

If CRDs are changed:

  • CRD YAMLs updated (make manifests) and also copied into the helm chart
  • Changes to CRDs documented

Please refer to the PR guidelines before submitting this pull request.

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.

@prudhvigodithi
Copy link
Member

Hey @ikorchynskyi thanks for your contribution.
With this change is there a chance the Additional Volumes mount path would overlap with the defaults, it could be with the secrets, configmaps or with the default volumes created, if so what is the behavior and error?
Also I assume users should be aware of this and have the AdditionalVolumes mount to the seperate volume path that is not been used by the configmaps or secrets right?
Thank you
@getsaurabh02 @swoehrl-mw

@ikorchynskyi
Copy link
Author

With this change is there a chance the Additional Volumes mount path would overlap with the defaults, it could be with the secrets, configmaps or with the default volumes created, if so what is the behavior and error?

Hi @prudhvigodithi, I tried to make it as similar as possible to the existing CSI volume configuration, and from what I may see - there is no such checks at all. Is there any check to ensure that configmaps or secrets do not overwrite each other?

Also I assume users should be aware of this and have the AdditionalVolumes mount to the seperate volume path that is not been used by the configmaps or secrets right?

In fact, my use case is quite the opposite. I needed this to make S3 bucket mounts possible via aws-ebs-csi-driver. Unfortunately, it does not allow ephemeral volumes, and the mount was needed to pass synonym files. Thus, in my case this volume is mounted to the /usr/share/opensearch/config/external folder - as synonyms should be under the config directory.

Copy link
Collaborator

@swoehrl-mw swoehrl-mw left a comment

Choose a reason for hiding this comment

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

Hi @ikorchynskyi. Thank you for your contribution.

  • Code change looks good
  • Please document the new fields in the docs/designs/crd.md reference and in the userguide
  • See my inline comment
  • Afterwards make sure CRD manifests are generated and copied into the helm chart

@@ -296,6 +296,8 @@ type AdditionalVolume struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
// CSI object to use to populate the volume
CSI *corev1.CSIVolumeSource `json:"csi,omitempty"`
// Persistent object to use to populate the volume
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// Persistent object to use to populate the volume
// PersistentVolumeClaim object to use to populate the volume

@sugaf1204
Copy link

Hi @ikorchynskyi

I would like to have this nice feature as soon as possible, how is the progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

Support additionalVolumes types other than Secret or ConfigMap
4 participants