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.

Sign up for GitHub

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

Create a cloned OpenEBS (jiva) Volume from a snapshot #283

Merged
merged 3 commits into from
Mar 28, 2018
Merged

Create a cloned OpenEBS (jiva) Volume from a snapshot #283

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7303d72
Add Snapshot clone API
prateekpandey14 Mar 21, 2018
a27bfc9
sync with master
prateekpandey14 Mar 27, 2018
c71528d
Enahance volume API to support volume clone
prateekpandey14 Mar 27, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions cmd/maya-apiserver/app/server/snapshot_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ func (s *HTTPServer) snapshotSpecificRequest(resp http.ResponseWriter, req *http
return s.snapshotCreate(resp, req)
case strings.Contains(path, "/revert/"):
return s.snapshotRevert(resp, req)
case strings.Contains(path, "/clone/"):
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it will be good if we explain somewhere in the comments or commit or PR on:

  • Snapshot
  • Clone
  • Promote
    This helps contributors as well as docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workflow is changed now, API no longer exists.

return s.snapshotClone(resp, req)
case strings.Contains(path, "/list"):
volName := strings.TrimPrefix(path, "/list/")
return s.snapshotList(resp, req, volName)
Expand Down Expand Up @@ -167,6 +169,17 @@ func (s *HTTPServer) snapshotList(resp http.ResponseWriter, req *http.Request, v

}

// SnapshotClone is http handler for restore a snapshot to a persistent volume
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more explanation might be nice. Some questions that arise here are:

  • Are the PV already available?
  • Are the snapshots already available?
  • Is this possible if all are in the same namespace, different namespaces, different clusters?

You need not put these info here. But this should be answered somewhere.
Was there a design doc that tries to talk about these things?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workflow is changed now, API no longer exists.

func (s *HTTPServer) snapshotClone(resp http.ResponseWriter, req *http.Request) (interface{}, error) {

clone, err := s.volumeAdd(resp, req)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is adding a volume. Why are we calling it as a clone.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let us not reuse the methods here & inject if else conditionals throughout the code.
We will be better off if we go for well defined/dedicated structures that handle clone & snapshot respectively.

if err != nil {
return nil, err
}

return clone, nil
}

/*func (s *HTTPServer) getControllerIP(resp http.ResponseWriter, req *http.Request, snap.Spec.Volname string) (string, err) {
voldetails, err := s.vsmRead(resp, req, snap.Spec.VolumeName)
if err != nil {
Expand Down
11 changes: 11 additions & 0 deletions types/v1/labels.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,14 @@ const (
// This is replaced at runtime
JivaClusterIPHolder JivaAnnotations = "__CLUSTER_IP__"

// JivaCloneIPHolder is used as a placeholder for sync controller IP address
// which will be used as cloneIP
//
// NOTE:
// This is replaced at runtime
JivaCloneIPHolder JivaAnnotations = "__CLONE_IP__"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we calling these as annotations? I guess this is an old type that we are re-using. But lets make it a point to avoid once we feel that this design/code is not good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, these are coming from the Volume struct, just using this to pass as replica args .


JivaReplicaTypeHolder JivaAnnotations = "clone"
// JivaStorageSizeHolder is used as a placeholder for persistent volume's
// storage capacity
//
Expand Down Expand Up @@ -611,6 +619,9 @@ var (

// JivaReplicaArgs is the set of arguments provided to JivaReplicaCmd
JivaReplicaArgs = []string{"replica", "--frontendIP", string(JivaClusterIPHolder), "--size", string(JivaStorageSizeHolder), string(JivaPersistentMountPathDef)}

// JivaCloneReplicaArgs is the set of arguments provided to JivaReplicaCmd
JivaCloneReplicaArgs = []string{"replica", "--frontendIP", string(JivaClusterIPHolder), "--cloneIP", string(JivaCloneIPHolder), "--type", string(JivaReplicaTypeHolder), "--size", string(JivaStorageSizeHolder), string(JivaPersistentMountPathDef)}
)

// TODO
Expand Down
3 changes: 3 additions & 0 deletions types/v1/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ type Volume struct {
// - `true` indicates monitoring is required & should use the defaults
Monitor string `json:"monitor,omitempty" protobuf:"bytes,1,opt,name=monitor"`

// CloneIP is the sync controller IP which will be used to sync and rebuild
// the clone replica
CloneIP string `json:"cloneIP,omitempty" protobuf:"bytes,1,opt,name=cloneIP"`
// Specs contains the desired specifications the volume should have.
// +optional
Specs []VolumeSpec `json:"specs,omitempty" protobuf:"bytes,2,rep,name=specs"`
Expand Down
20 changes: 15 additions & 5 deletions types/v1/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,15 +924,25 @@ func MakeOrDefJivaReplicaArgs(vol *Volume, clusterIP string) []string {

//storSize := GetPVPStorageSize(profileMap)
storSize := vol.Capacity
cloneIP := vol.CloneIP

repArgs := make([]string, len(JivaReplicaArgs))
if cloneIP == "" {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe its best to avoid this if else by structuring the code better.
Probably a new struct that deals with clone would have been great.
When the complexity arises with new business logic/features, a well defined struct will be able to handle this better.

for i, rArg := range JivaReplicaArgs {
rArg = strings.Replace(rArg, string(JivaClusterIPHolder), clusterIP, 1)
rArg = strings.Replace(rArg, string(JivaStorageSizeHolder), storSize, 1)
repArgs[i] = rArg
}
} else {
repArgs := make([]string, len(JivaCloneReplicaArgs))
for i, rArg := range JivaCloneReplicaArgs {
rArg = strings.Replace(rArg, string(JivaClusterIPHolder), clusterIP, 1)
rArg = strings.Replace(rArg, string(JivaStorageSizeHolder), storSize, 1)
rArg = strings.Replace(rArg, string(JivaCloneIPHolder), cloneIP, 1)
repArgs[i] = rArg

for i, rArg := range JivaReplicaArgs {
rArg = strings.Replace(rArg, string(JivaClusterIPHolder), clusterIP, 1)
rArg = strings.Replace(rArg, string(JivaStorageSizeHolder), storSize, 1)
repArgs[i] = rArg
}
}

return repArgs
}

Expand Down