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 a property to Mount trait that creates then mounts a PVC #3230

Closed
wants to merge 1 commit into from

Conversation

haanhvu
Copy link
Contributor

@haanhvu haanhvu commented Apr 22, 2022

Release Note

NONE

fixes #2994

@squakez please review

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.

Good work. I've added some comments on how to continue. Moreover, once the implementation is over, we should have an example and possibly an e2e test to validate the new functionality.

pkg/trait/mount.go Outdated Show resolved Hide resolved
pkg/util/resource/config.go Outdated Show resolved Hide resolved
@haanhvu haanhvu force-pushed the issue2994 branch 2 times, most recently from 9218666 to 36c7053 Compare April 29, 2022 06:40
@squakez
Copy link
Contributor

squakez commented Apr 29, 2022

The latest changes have hardly broken the build. Please, have a look.

@haanhvu
Copy link
Contributor Author

haanhvu commented Apr 29, 2022

The latest changes have hardly broken the build. Please, have a look.

Yeah the errors are mostly about go syntax and kubernetes api. I'll take a look shortly.

@haanhvu haanhvu force-pushed the issue2994 branch 3 times, most recently from 7e29988 to 76684c0 Compare April 29, 2022 16:34
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.

Good work, thanks. I guess we may improve the parsing with a regular expression and more precise error messages, but let's keep it as a future enhancement.

@haanhvu haanhvu force-pushed the issue2994 branch 3 times, most recently from 01c88bc to de042ad Compare May 13, 2022 06:39
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.

It looks good now. Please, remind to we should have an example and possibly an e2e test to validate the new functionality before merging.

@haanhvu haanhvu force-pushed the issue2994 branch 2 times, most recently from b4d7d90 to 80354e6 Compare May 14, 2022 07:33
@haanhvu
Copy link
Contributor Author

haanhvu commented May 17, 2022

@squakez when creating the example I found out several things:

  1. accessModes was missed in the pvc definition. So I added a ReadWriteOnce accessModes in the pvc definition. In fact another option would be letting users specify the accessModes in the trait property. What do you prefer?

  2. The pvc couldn't be created due to this error: Cannot reconcile Integration routes: error executing post actions: error during apply resource: /pvc-example1: persistentvolumeclaims "pvc-example1" is forbidden: User "system:serviceaccount:default:camel-k-operator" cannot patch resource "persistentvolumeclaims" in API group "" at the cluster scope.

This can be easily fixed with the kubectl create clusterrolebinding command. However, is this strange that the operator cannot patch pvc by default? Because from what I see in https://github.com/apache/camel-k/blob/main/config/rbac/operator-role.yaml#L70&L80 and https://github.com/apache/camel-k/blob/main/config/rbac/patch-role-to-clusterrole.yaml the operator can patch pvc at the cluster scope?

@squakez
Copy link
Contributor

squakez commented May 17, 2022

About 1) I think it would be nice to let the user provide the info, so we might add to the trait parse. As for 2) I think you're missing in the creation of the PersistentVolumeClaim struct to define the namespace, so probably it tries to install at cluster scope, for which the namespaced operator has no privileges. Try to amend that and see how it goes.

@squakez squakez added the status/wip Work in progress label Jun 20, 2022
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale due to 90 days of inactivity.
It will be closed if no further activity occurs within 15 days.
If you think that’s incorrect or the issue should never stale, please simply write any comment.
Thanks for your contributions!

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

Successfully merging this pull request may close these issues.

RFE to allow dynamic creation of Persistent Volumes from storage classes
2 participants