Add Volume Storage Adapter for Devfiles #2651
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
maysunfaisal
force-pushed
the
2472-pvc-4-odo
branch
from
244b4e8 to
16b8cc4
Compare
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
maysunfaisal
force-pushed
the
2472-pvc-4-odo
branch
from
16b8cc4 to
515bb3f
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kadel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
| @@ -1,6 +1,16 @@ | |||
| package common | |||
|
|
|||
| // ComponentAdapter defines the functions that platform-specific adapters must implement | |||
| import ( | |||
| corev1 "k8s.io/api/core/v1" | |||
There was a problem hiding this comment.
The common package is meant to be platform neutral so we should avoid platform-specific imports like this kubernetes import in this package. For example, if we had a docker platform adapter then it wouldn't need it.
| type ComponentAdapter interface { | ||
| Start() error | ||
| Initialize() (*corev1.PodTemplateSpec, error) |
There was a problem hiding this comment.
Do we really need an initialize function? Seems like we could remove it if the component adapter uses the storage adapter within its start function.
|
|
||
| // StorageAdapter defines the storage functions that platform-specific adapters must implement | ||
| type StorageAdapter interface { | ||
| Start(*corev1.PodTemplateSpec) error |
There was a problem hiding this comment.
Not sure Start makes much sense in terms of storage. Maybe Create instead, where create could be responsible for creating the PVC?
| } | ||
| } | ||
|
|
||
| // Start creates Kubernetes resources that correspond to the devfile if they don't already exist | ||
| func (k Adapter) Start() error { | ||
|
|
||
| err := k.componentAdapter.Start() | ||
| podTemplateSpec, err := k.componentAdapter.Initialize() |
There was a problem hiding this comment.
Same question as above about whether we really need an initialize function. Seems like the component adapter can make use of the storage adapter? Then in the component adapter's Start function it can call the storage adapter to add the volume mounts to the pod spec.
| componentAliasToVolumes := make(map[string][]common.DockerimageVolume) | ||
| // Only components with aliases are considered because without an alias commands cannot reference them | ||
| for _, comp := range devfileObj.Data.GetAliasedComponents() { | ||
| if comp.Type == common.DevfileComponentTypeDockerimage { |
There was a problem hiding this comment.
There are a few places in the code where we're calling GetAliasedComponents() and then filtering by type for DevfileComponentTypeDockerimage. I think we should create a common function like GetSupportedComponents() on the odo level (ie. not on the parser level) that will call GetAliasedComponents() and then filter. The benefit would be if we decide to support other types then we would only need to update that single function.
| @@ -44,6 +44,21 @@ func GetContainers(devfileObj devfile.DevfileObj) []corev1.Container { | |||
| return containers | |||
| } | |||
|
|
|||
| // GetVolumes iterates through the components in the devfile and returns a map of component alias to the devfile volumes | |||
| func GetVolumes(devfileObj devfile.DevfileObj) map[string][]common.DockerimageVolume { | |||
There was a problem hiding this comment.
Should the return type be something more generic than common.DockerimageVolume? When we support Kubernetes or other component types might be weird for them to have to convert to use the Docker image the type. I'm on the fence about this though because I think if we do create a generic type it will likely have the same properties.
pkg/kclient/volumes.go
Outdated
| @@ -13,6 +15,9 @@ import ( | |||
| const ( | |||
| PersistentVolumeClaimKind = "PersistentVolumeClaim" | |||
| PersistentVolumeClaimAPIVersion = "v1" | |||
|
|
|||
| // The length of the string to be generated for names of resources | |||
| nameLength = 5 | |||
There was a problem hiding this comment.
I'm assuming we'd want to use this constant through kclient so makes sense to move it to kclient.go?
pkg/kclient/volumes.go
Outdated
| func AddVolumeMountToPodTemplateSpec(podTemplateSpec *corev1.PodTemplateSpec, volumeName, pvc string, containerMountPathsMap map[string][]string) error { | ||
| // AddVolumeMountToPodTemplateSpec adds the Volume Mounts in componentAliasToMountPaths to the podTemplateSpec containers for a given pvc and volumeName | ||
| // componentAliasToMountPaths is a map of a container alias to an array of its Mount Paths | ||
| func AddVolumeMountToPodTemplateSpec(podTemplateSpec *corev1.PodTemplateSpec, volumeName, pvc string, componentAliasToMountPaths map[string][]string) error { |
There was a problem hiding this comment.
pvc doesn't appear to be used in this function
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
maysunfaisal
force-pushed
the
2472-pvc-4-odo
branch
from
23e9c33 to
d40d889
Compare
|
@rajivnathan PR has been updated with your feedback, thanks for the help with the interfaces! Tests after feedback refactor commit |
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
maysunfaisal
force-pushed
the
2472-pvc-4-odo
branch
from
d40d889 to
e22cd68
Compare
Codecov Report
@@ Coverage Diff @@
## master #2651 +/- ##
==========================================
+ Coverage 43.65% 43.87% +0.22%
==========================================
Files 81 85 +4
Lines 7498 7663 +165
==========================================
+ Hits 3273 3362 +89
- Misses 3910 3971 +61
- Partials 315 330 +15
Continue to review full report at Codecov.
|
| var components []common.DevfileComponent | ||
| // Only components with aliases are considered because without an alias commands cannot reference them | ||
| for _, comp := range data.GetAliasedComponents() { | ||
| if comp.Type == common.DevfileComponentTypeDockerimage { |
There was a problem hiding this comment.
We should probably add a comment explaining that we're only handling components of Dockerimage type to begin with because most of the devfiles in the che devfile registry use it.
|
|
||
| // Add PVC and Volume Mounts to the podTemplateSpec | ||
| err = kclient.AddPVCAndVolumeMount(podTemplateSpec, volumeNameToPVC, componentAliasToVolumes) | ||
| a.VolumeNameToPVCName, err = CreateComponentStorage(&a.Client, volumes, componentName) |
There was a problem hiding this comment.
We can use a.ComponentName directly and remove the line above
|
@rajivnathan I have removed the storage adapter test because it's Create just calls function |
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
There was a problem hiding this comment.
One more thing, the ownerReferences should be set to the deployment so that when the deployment is deleted the pvc will be deleted as well. That is what odo already does in the non-devfile case.
eg.
apiVersion: v1
kind: PersistentVolumeClaim
metadata:
annotations:
pv.kubernetes.io/bind-completed: "yes"
pv.kubernetes.io/bound-by-controller: "yes"
creationTimestamp: 2020-03-05T16:02:20Z
finalizers:
- kubernetes.io/pvc-protection
labels:
app: app
app.kubernetes.io/instance: nodejs-nodejs-ex-qnxf
app.kubernetes.io/managed-by: odo
app.kubernetes.io/managed-by-version: v1.1.0
app.kubernetes.io/name: nodejs
app.kubernetes.io/part-of: app
app.openshift.io/runtime-version: latest
name: nodejs-nodejs-ex-qnxf-app-s2idata
namespace: odo
ownerReferences:
- apiVersion: apps.openshift.io/v1
kind: DeploymentConfig
name: nodejs-nodejs-ex-qnxf-app
uid: b195a66b-5efa-11ea-ab06-00000a1f025e
resourceVersion: "60867020"
selfLink: /api/v1/namespaces/odo/persistentvolumeclaims/nodejs-nodejs-ex-qnxf-app-s2idata
uid: b19d2b22-5efa-11ea-ab06-00000a1f025e
spec:
accessModes:
- ReadWriteOnce
resources:
requests:
storage: 1Gi
volumeName: volume6
status:
accessModes:
- ReadWriteOnce
capacity:
storage: 8Gi
phase: Bound
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
There was a problem hiding this comment.
Couple minor comments but also when I tried it the first time I hit an error.
odo push -v 3
• Push devfile component spring-devfile ...
I0305 18:08:51.091152 73206 utils.go:17] Found component dockerimage with alias tools
I0305 18:08:51.091267 73206 utils.go:17] Found component dockerimage with alias tools
I0305 18:08:51.091279 73206 utils.go:26] Checking for PVC with name m2 and label component=spring-devfile,storage-name=m2
I0305 18:08:51.133687 73206 utils.go:37] Creating a PVC with name m2 and label component=spring-devfile,storage-name=m2
I0305 18:08:51.133723 73206 utils.go:77] Creating a PVC with name m2-spring-devfile-yfyz
I0305 18:08:51.142243 73206 adapter.go:77] Creating deployment spring-devfile
I0305 18:08:51.142261 73206 adapter.go:78] The component name is spring-devfile
I0305 18:08:51.158319 73206 adapter.go:93] Successfully created component spring-devfile
I0305 18:08:51.184479 73206 volumes.go:135] Updating owner references for pvc m2-spring-devfile-yfyz
✗ Failed to start component with name spring-devfile.
Error: Failed to create the component: Operation cannot be fulfilled on persistentvolumeclaims "m2-spring-devfile-yfyz": the object has been modified; please apply your changes to the latest version and try again
Looks like it is hit or miss. I think we should create the volume after the deployment and just set the owner reference when we're creating the volume instead of trying to update it after the fact?
pkg/kclient/kclient.go
Outdated
| // GenerateOwnerReference genertes an ownerReference from the deployment which can then be set as | ||
| // owner for various Kubernetes objects and ensure that when the owner object is deleted from the | ||
| // cluster, all other objects are automatically removed by Kubernetes garbage collector | ||
| func GenerateOwnerReference(deployment *appsv1.Deployment) metav1.OwnerReference { |
There was a problem hiding this comment.
I think this function should be moved to the generators.go file?
pkg/kclient/kclient.go
Outdated
| func GenerateOwnerReference(deployment *appsv1.Deployment) metav1.OwnerReference { | ||
|
|
||
| ownerReference := metav1.OwnerReference{ | ||
| APIVersion: "extensions/v1beta1", |
There was a problem hiding this comment.
Think it's probably safer to get the APIVersion from the deployment directly. ie. deployment.APIVersion That way in the future if we update our deployment spec generation code to use a newer version we wouldn't need to update this code as well. It will just work.
There was a problem hiding this comment.
To add on to what Rajiv said and why we might not want to reference it directly: we should not be using or referencing Deployments in the extensions/v1beta1 API group.
Starting with Kube 1.16, it’s been removed from the Kube API and cant be referenced.
| package storage | ||
|
|
||
| // HelperAdapter defines functions that kubernetes storage adapters may implement | ||
| type HelperAdapter interface { |
There was a problem hiding this comment.
Should this be called something other than HelperAdapter? I know we can't conflict with StorageAdapter, but I think HelperAdapter is confusing here.
There was a problem hiding this comment.
being removed with the latest commit already
| } | ||
|
|
||
| randomChars := util.GenerateRandomString(4) | ||
| namespaceKubernetesObject, err := util.NamespaceOpenShiftObject(name, componentName) |
There was a problem hiding this comment.
Nit: I know that functionally it's the same and that you didn't write the function, but given that the code we're running here is Kube generic, could we rename the utils function to NamespaceKubernetesObject?
There was a problem hiding this comment.
wanted to, but a lot of code in odo1 is using it. Still change the function name?
There was a problem hiding this comment.
If it's just a rename, it should be safe to do in my opinion. @kadel what are your thoughts?
| volNames := [...]string{"vol1", "vol2", "vol3"} | ||
| volSize := "5Gi" | ||
|
|
||
| tests := []struct { |
There was a problem hiding this comment.
Could you add some more test cases for:
- Volune names that might cause issues (0 length, over kubernetes maximum, etc)
- Different numbers of volumes (0, and 1)
- Different sizes of volumes, and making sure the functions error out with invalid values
There was a problem hiding this comment.
let me see what I can do with these
| ) | ||
|
|
||
| // GetSupportedComponents iterates through the components in the devfile and returns a list of odo supported components | ||
| func GetSupportedComponents(data versions.DevfileData) []common.DevfileComponent { |
There was a problem hiding this comment.
Could you add a unit test for GetSupportedComponents?
There was a problem hiding this comment.
makes sense, let me see
| @@ -42,26 +45,63 @@ func (a Adapter) Start() (err error) { | |||
|
|
|||
| objectMeta := kclient.CreateObjectMeta(componentName, a.Client.Namespace, labels, nil) | |||
There was a problem hiding this comment.
We're probably unlikely to hit this issue with component names, but to be safe, we should probably trim and sanitize componentName with either utils.DNS1123Name() or utils.NamespaceOpenShiftObject before passing it into CreateObjectMeta.
pkg/kclient/volumes.go
Outdated
| // generateVolumeNameFromPVC generates a random volume name based on the name | ||
| // of the given PVC | ||
| func generateVolumeNameFromPVC(pvc string) string { | ||
| return fmt.Sprintf("%v-%v-volume", pvc, util.GenerateRandomString(nameLength)) |
There was a problem hiding this comment.
Nit: Any reason why we wouldn't just want to call utils.NamespacedOpenShiftObject here?
There was a problem hiding this comment.
I think volume name is not limited to 63 limit since its not a kube resource(need to confirm tho).
There was a problem hiding this comment.
You're right, it might be 253
There was a problem hiding this comment.
actually, does not allow my vol name to be more than 63, updated
[spec.template.spec.volumes[0].name: Invalid value: "m3-abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxy-tmnm-ccdde-volume": must be no more than 63 characters
|
/retest |
Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>
|
@rajivnathan @johnmcollier I've pushed a new commit, pls review at your convenience, thx! |
@rajivnathan @johnmcollier, after you review this you can just do |
|
Looks good to me now. @johnmcollier please add the |
openshift-ci-robot
added
the
lgtm
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
rm3l
added
the
estimated-size/XL (40-60)
Signed-off-by: Maysun J Faisal maysun.j.faisal@ibm.com
What type of PR is this?
/kind feature
What does does this PR do / why we need it:
This PR implements the Storage Adapter for Devfiles. It reads the parsed Devfile data, creates the storage and adds them to the component deployment.
For Acceptance Criteria, pls have a look at the issue post #2472 (comment)
List of changes:
-- PVC to the pod template spec
-- Volume Mounts to the pod template spec's containers
-- Adds a default storage of
1Gi, see issue Discussion on Devfile volume size support #2511 on how this can be improvedWhich issue(s) this PR fixes:
Fixes #2472
How to test changes / Special notes to the reviewer:
All pkg adapter tests (includes new
TestStorageAdapter,TestStorageCreateand updatedTestComponentAdapter):All the kclient pkg tests:
To test it out manually:
odo preference set experimental trueodo push(Pls remember to check if your devfile has volumes) for example:kubectl get po,deploy,pvcand confirm the pvc createdkubectl describe po <component pod>and confirm the volume mounts for the containersIntegration tests to be covered by #2673