chore(snapshot): adding the design doc for mountable snapshots #183
Conversation
design/lvm/mountable_snapshot.md
Outdated
| ## Drawbacks | ||
|
|
||
| - As far as I understand it, normally when creating a PersistentVolumeClaim with a snapshot as datasource it would clone the Snapshot into an actual volume, so we'd be diverging from this behaviour. | ||
| - We could use an annotation to specify the behaviour that we are implementing here so that the clone behaviour can be added at a later time? |
There was a problem hiding this comment.
can you mention the annotation and also how we are going to use that?
There was a problem hiding this comment.
I've added some additional details to the document. Basically we annotate the PVC to indicate whether it needs to clone the snapshot or mount it.
| The changes below are the primary modules that need to be modified, however, there are some additional utility modules that will need to be modified to support these changes. Most likely the following files will need some additions to support snapshots: [kubernetes.go](../../pkg/builder/volbuilder/kubernetes.go) (in order to implement a `Kubeclient.GetSnapshot` function), [lvm_util.go](../../pkg/lvm/lvm_util.go) (in order to create a function that changes the snapshot write access) and [mount.go](../../pkg/lvm/mount.go) (in order to support mounting/unmounting of snapshots). | ||
|
|
||
| - In the [controller.go](../../pkg/driver/controller.go) the code path in the `CreateVolume` function for the `controller` type that occurs when contentSource.GetSnapshot() is not `nil` needs to be implemented. When this path is triggered it should return the correct `volName`, `topology` and `cntx`. | ||
| - In the [agent.go](../../pkg/driver/agent.go) the `NodePublishVolume` function for the `node` type needs to be changed such that it checks whether the `volName` is a snapshot and if so mount the snapshot to the specified location. |
There was a problem hiding this comment.
can you describe how we are going to find which snapshot we have to mount, how do we find that?
There was a problem hiding this comment.
I've added some additional details in the doc as to how this can be done. But it's rather straightforward, the VolumeId (which is should be provided to this function in the request) should be of the format snapshot-<uuid> (or possibly pvc-<original-uuid>@snapshot-<snapshot-uuid>) where the uuid is also the name of the snapshot on the host.
| The changes below are the primary modules that need to be modified, however, there are some additional utility modules that will need to be modified to support these changes. Most likely the following files will need some additions to support snapshots: [kubernetes.go](../../pkg/builder/volbuilder/kubernetes.go) (in order to implement a `Kubeclient.GetSnapshot` function), [lvm_util.go](../../pkg/lvm/lvm_util.go) (in order to create a function that changes the snapshot write access) and [mount.go](../../pkg/lvm/mount.go) (in order to support mounting/unmounting of snapshots). | ||
|
|
||
| - In the [controller.go](../../pkg/driver/controller.go) the code path in the `CreateVolume` function for the `controller` type that occurs when contentSource.GetSnapshot() is not `nil` needs to be implemented. When this path is triggered it should return the correct `volName`, `topology` and `cntx`. | ||
| - In the [agent.go](../../pkg/driver/agent.go) the `NodePublishVolume` function for the `node` type needs to be changed such that it checks whether the `volName` is a snapshot and if so mount the snapshot to the specified location. |
There was a problem hiding this comment.
Why do we need to know whether the volName is a snapshot? We are creating a volume out of the snapshot and then simply we want to mount that newly created volume. Am I interpreting it right or am I missing something?
There was a problem hiding this comment.
I figured we'd need to call different functions so that we end up with an *apis.LVMSnapshot object instead of a *apis.LVMVolume object, but that might not even be needed. I suppose that's mainly an implementation detail and this should simply say that it mount the snapshot.
| In order to implement this only existing code needs to be changed, there is no need for an entirely new control flow. | ||
| The changes below are the primary modules that need to be modified, however, there are some additional utility modules that will need to be modified to support these changes. Most likely the following files will need some additions to support snapshots: [kubernetes.go](../../pkg/builder/volbuilder/kubernetes.go) (in order to implement a `Kubeclient.GetSnapshot` function), [lvm_util.go](../../pkg/lvm/lvm_util.go) (in order to create a function that changes the snapshot write access) and [mount.go](../../pkg/lvm/mount.go) (in order to support mounting/unmounting of snapshots). | ||
|
|
||
| - In the [controller.go](../../pkg/driver/controller.go) the code path in the `CreateVolume` function for the `controller` type that occurs when contentSource.GetSnapshot() is not `nil` needs to be implemented. When this path is triggered it should return the correct `volName`, `topology` and `cntx`. |
There was a problem hiding this comment.
Do we have anything specific in mind on how to implement this?
There was a problem hiding this comment.
As a matter of fact yes, I created a quick PoC a while back to see if this would even be feasible.
The first step is determining the volume name, which can be parsed from the SnapshotId (contentSource.GetSnapshot().GetSnapshotId()). I'd suggest passing snapshot-<uuid> as the volume name, but the entire snapshot_id would also work.
We'd also need to create a function that retrieves the *lvmapi.LVMSnapshot object based on the request (instead of the *lvmapi.LVMVolume object we get after creating a new LVM Volume. The process is quite similar to that of building a regular volume (using the volbuilder.NewBuilder() function). The only information that is not already present is that of the owner, but we can retrieve this by using the existing lvm.GetLVMSnapshot function.
With this information creating the topology and cntx is exactly the same as before only using the *lvmapi.LVMSnapshot instead of the *lvmapi.LVMVolume.
This feels too detailed to be in a design document and more an implementation specific part, but if you disagree I can add it to the design doc.
| metadata: | ||
| name: csi-lvmpvc-snap | ||
| spec: | ||
| storageClassName: openebs-lvmpv |
There was a problem hiding this comment.
Is storage class required to be of local.csi.openebs.io provisioner type? Can we have different storage class consuming the lvm snapshots?
There was a problem hiding this comment.
Since the dataSource is provided separately I don't think it's required. I'm not entirely sure what's involved with creating a new provisioner type, but it would be a good solution to the downside that I mentioned in the design.
wowditi
force-pushed
the
add-mountable-snapshot-design-doc
branch
from
870b703
to
abdb065
Compare
…PublishVolume function
Ab-hishek
changed the title
There was a problem hiding this comment.
Hi @wowditi. This document looks good. Just a minor comment. Since, this is a design document we really don't need to mention the exact files or function names where the changes need to be done. We can simply mention the goals, non-goals, design change/enhancement, any diagrams if needed. Therefore can you make the necessary changes for it.
|
@wowditi Can you please provide an update on the plans for taking this forward? Are you willing to work on this? |
|
Hi @wowditi I run Product Mgmt for the OpenEBS project. We'd really like to work on developing your PR and integrate the design into the new unified product. Are you still interested and willing to support your PR? |
|
Hi, About the design document, the implementation details are indeed very technical currently and I can make those a bit more high level. PS: I should probably mention that I mainly code in python and not Golang, so it might not be the most beautiful code. |
|
I see at least a few things that aren't talked about in this document:
|
|
This hasn't yet progressed based on last set of clarifying questions. Keeping a track of it. |
Pull Request template
Please, go through these steps before you submit a PR.
Why is this PR required? What issue does it fix?:
What this PR does?: It adds a design for a new feature
Does this PR require any upgrade changes?: No
If the changes in this PR are manually verified, list down the scenarios covered::
Any additional information for your reviewer? :
In the drawbacks section I posed the question of whether we should maybe use annotation to indicate that we are mounting a snapshot instead of creating a new PV as a clone of the snapshot. I'd like a second opinion on whether this is a good idea or what a good alternative could be (for example, we could create a new api-resource although I don't think that would be such a good idea).
Also a PVC requires the Storage to be set, however, it is a little bit useless in this case especially when mounting snapshots. Any suggestions on handling that better would be appreciated.
Checklist:
<type>(<scope>): <subject>