-
Notifications
You must be signed in to change notification settings - Fork 475
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
Exploratory: Enable volumes mounting in functions #595
Exploratory: Enable volumes mounting in functions #595
Conversation
12ae83e
to
52b878d
Compare
Enabling volumes to be mounted in function's file system using annotations Signed-off-by: Martin Dekov <[email protected]>
52b878d
to
c1ff6b8
Compare
Just a thought here. If this was added, would it cover every use-case if only one volume was supported per function? |
@@ -142,12 +142,27 @@ func makeDeploymentSpec(request types.FunctionDeployment, existingSecrets map[st | |||
annotations := buildAnnotations(request) | |||
|
|||
var serviceAccount string | |||
var volumeName string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'd have to handle updates too, if someone wanted to remove a volume that was already attached.
@@ -207,11 +222,27 @@ func makeDeploymentSpec(request types.FunctionDeployment, existingSecrets map[st | |||
SecurityContext: &corev1.SecurityContext{ | |||
ReadOnlyRootFilesystem: &request.ReadOnlyRootFilesystem, | |||
}, | |||
VolumeMounts: []apiv1.VolumeMount{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds a volume even if one isn't configured, so you should set up the slice outside of this piece of code. ideally in a method to create it.
}, | ||
}, | ||
ServiceAccountName: serviceAccount, | ||
RestartPolicy: corev1.RestartPolicyAlways, | ||
DNSPolicy: corev1.DNSClusterFirst, | ||
Volumes: []apiv1.Volume{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as comment above
Added this as POC if we want to merge it in master I can bring this in a better state. |
I will clear the shortcomings. I am aware of those but was not sure whether we want this to enter master. I suppose we do want to integrate the change and I will update the PR and fix all issues you mentioned. For the question whether only one volume is supported per function - yes this is the main idea. We might be able to extend this with wildcard annotation. So the key of the annotation itself suggest the volume name path etc. But I believe this would not be needed, at least I don't need more than one volume mounted. |
I think you should work to fix these basic issues, since you've gone ahead with a public PR, we can at least have it at a quality that anyone who lands here can test it or try it out. Would you be open to fixing up the initial feedback? |
I will close this, but will continue refining it and will keep the change in the branch. |
Enabling volumes to be mounted in function's file
system using annotations
Signed-off-by: Martin Dekov [email protected]
Disclaimer
This PR is exploratory and not solution discussed with the community.
Description
Functions can now mount volumes with this PR
Motivation and Context
I needed the functionality and decided that this can be used by someone inspiration as part of the profiles support: #586
Also was suggested in:
openfaas/faas#1178
and:
openfaas/faas#320
This POC does exactly that.
How Has This Been Tested?
Image in:
martindekov/faas-netes/0.0.2
With function applying the following annotations:
The function's deployment had those fields:
And the volume was mounted successfully.
Types of changes
Checklist:
git commit -s