CSI translation for in-tree volumes#5
Conversation
Signed-off-by: Deep Debroy <ddebroy@docker.com>
davidz627
left a comment
davidz627
left a comment
There was a problem hiding this comment.
generally looks pretty good to me! did you test it out with a real driver yet?
| } else { | ||
| return &volume.Spec{}, fmt.Errorf("not a valid volume spec") | ||
| // TranslateInTreeVolumeToCSI will create a new PV | ||
| csiPV, err = csilib.TranslateInTreeVolumeToCSI(spec.Volume) |
There was a problem hiding this comment.
many instances of InTreeVolume should be changed to InlineVolume I think
There was a problem hiding this comment.
So I was trying to distinguish TranslateInTreeVolumeToCSI from TranslateInTreePVToCSI as the former handles spec.Volume (associated with inline) while the latter handles Spec.PersistentVolume (associated with PV). I understand the fact that it may not be clear that TranslateInTreeVolumeToCSI is meant to handle inline.
Maybe we can be a bit verbose and call the in-tree in-line translator: TranslateInTreeInlineVolumeToCSI?
There was a problem hiding this comment.
oh I see now, yea lets err on the side of verbosity. It's really long but also this feature is inherently confusing so I think its better we fully qualify names so its marginally easier to understand
| pvName := spec.PersistentVolume.GetName() | ||
| attachID := getAttachmentName(csiSource.VolumeHandle, csiSource.Driver, node) | ||
| var vaSource storage.VolumeAttachmentSource | ||
| if spec.InlineVolume == false { |
| } | ||
|
|
||
| func translateSpec(spec *volume.Spec) (*volume.Spec, error) { | ||
| if spec.PersistentVolume == nil && spec.Volume == nil { |
There was a problem hiding this comment.
nit: I would prefer this error case as the else block in the below if/else statements as it is kind of the 3rd condition there
| PersistentVolumeSource: v1.PersistentVolumeSource{ | ||
| CSI: &v1.CSIPersistentVolumeSource{ | ||
| Driver: GCEPDDriverName, | ||
| // TODO: PDName is not going to work for GCE. We need to get this to a volIDZonalFmt |
There was a problem hiding this comment.
Ah i understand the issue now, the format can be:
volID = fmt.Sprintf(volIDZonalFmt, UnspecifiedValue, UnspecifiedValue, pv.Spec.GCEPersistentDisk.PDName)
And what we can do is actually find the disk on the PD driver side when we encounter UnspecifiedValue.
we already do this here: https://github.com/davidz627/kubernetes/pull/5/files#diff-307e088a66bf6532855c7b623e41af8fR121
There was a problem hiding this comment.
Excellent! So the first two conditions where the labels are used to form the volID mainly an optimization to save the cloud lookup?
There was a problem hiding this comment.
yep! And I just checked the PD CSI Driver code and looks like I predicted this and already implemented it already. kubernetes-sigs/gcp-compute-persistent-disk-csi-driver#129
specifically: https://github.com/kubernetes-sigs/gcp-compute-persistent-disk-csi-driver/pull/129/files#diff-2cae80be86c0ea6e6c4f4b6dfc89c607R67
:D
ddebroy
changed the title
2 participants
A very preliminary version of the CSI translation work for in-tree volumes. This is to make sure my thinking is in the right track.
For GCE PD CSI translation, the ID generation will require access to the labels for the volumes which are not available through labels (as in-tree volumes do not have labels). So we may need to query the cloud to get the labels which may increase time required to provision due to additional cloud API calls.
For the other CSI drivers, it seems the ID generation (based on specific labels) is not necessary and the name appears to be enough.
/cc @davidz627 @msau42 @leakingtapan