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

(#5654) Added emptyDir support to mount trait #5709

Merged

Conversation

hernanDatgDev
Copy link
Contributor

Handles: #5654

Currently there is no way to add an emptyDir volume without using a pod template. This is useful but using the pod template will override any volumes mounted when using the mount trait e.g. configmaps, secrets.
This PR adds the trait field mount.empty-dirs which uses the same syntax as the existing mount.volumes trait field. To add an emptyDir volume use:
trait=mount.empty-dirs=my-empty-dir:/var/test
📓 Note that the trait field empty-dirs will be translated to emptyDirs in the integration yaml (if using helm or working with yaml directly)

The example above will add the following to your integration yaml

spec:
  containers:
  - name: integration
    volumeMounts:
    - mountPath: /var/test
      name: my-empty-dir
  volumes:
  - emptyDir: {}
    name: my-empty-dir

Release Note

NONE

@hernanDatgDev
Copy link
Contributor Author

Also I renamed the existing mount trait field mount.volumes to mount.persistent-volume-claims since it is accurate to what the operator is actually mounting, a persistent volume claim. This is to help avoid confusion and make a distinction clear for what type of volume was being mounted. This seemed important since I was adding support for another volume type.

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

LGTM but we need to revert a major API change introduced.

@@ -33,7 +33,9 @@ type MountTrait struct {
// Syntax: [configmap|secret]:name[/key][@path], where name represents the resource name, key optionally represents the resource key to be filtered and path represents the destination path
Resources []string `property:"resources" json:"resources,omitempty"`
// A list of Persistent Volume Claims to be mounted. Syntax: [pvcname:/container/path]
Volumes []string `property:"volumes" json:"volumes,omitempty"`
PersistentVolumeClaims []string `property:"persistent-volume-claims" json:"persistentVolumeClaims,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot change this without a period of deprecation notice. This change affects the API specification, so, it must be reverted. It was an unfortunate choice when we created it as it really was meant to be PersistentVolumeClaims. We may think to change this when moving to a major API upgrade.

Configs: []string{"configmap:my-cm"},
Resources: []string{"secret:my-secret"},
PersistentVolumeClaims: []string{"my-pvc:/over/the/rainbow"},
EmptyDirs: []string{"my-empty-dir:/some/path"},
Copy link
Contributor

Choose a reason for hiding this comment

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

We should create a new test with empty dirs instead. So we test both the default (void) and the presence of the parameter.

@squakez squakez linked an issue Jul 26, 2024 that may be closed by this pull request
@hernanDatgDev hernanDatgDev force-pushed the feature/5654/add-volume-to-mount-trait branch from a9fde33 to 6ea745c Compare July 26, 2024 22:31
@hernanDatgDev
Copy link
Contributor Author

@squakez Made the appropriate changes and updated the test as well. Thanks for the feedback!

@squakez squakez merged commit 535a5a8 into apache:main Jul 29, 2024
9 of 10 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.

Add support for mounting volumes with mount trait
2 participants