-
Notifications
You must be signed in to change notification settings - Fork 69
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
feat(REST, CVC): add a support for rest service in CVC Operator #83
Conversation
|
||
switch req.Method { | ||
case "POST": | ||
return backupOp.create() |
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.
Let us log for create request also.
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.
Instead of logging here good to log in create
method.
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.
Then why it is logged for Get
and Delete
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.
I will log it.
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.
looks good 👍
|
||
// constants | ||
const ( | ||
VolumeGrpcListenPort = 7777 |
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.
Please add comment here. Better not to define VolumeGrpcListenPort
value here, instead we can use it from API/some package.
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.
Sure I will move it to openebs/api
repo in subsequent PRs.
case "POST": | ||
return backupOp.create() | ||
case "GET": | ||
klog.Infof("Got backup GET request") |
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.
Better to change level to debug.
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 is good point but I need to check which log level is best i.e for this entier repo we need to fix specific log levels like 1, 2, 4 for different levels verbose. So I will incorporate this in subsequent PRs.
|
||
// If cspiName is not empty then fetch the CStor pool pod using CSPI name | ||
if cspiName == "" { | ||
klog.Errorf("failed to find pool manager empty CSPI is provided") |
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.
klog.Errorf("failed to find pool manager empty CSPI is provided") | |
klog.Errorf("failed to find pool manager, empty CSPI is provided") |
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.
updated
|
||
// If cspiName is not empty then fetch the CStor pool pod using CSPI name | ||
if cspiName == "" { | ||
klog.Errorf("failed to find pool manager empty CSPI is provided") |
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.
klog.Errorf("failed to find pool manager empty CSPI is provided") | |
klog.Errorf("failed to find pool manager, empty CSPI is provided") |
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.
Updated
return "", err | ||
} | ||
|
||
return cstorvolume.Spec.TargetIP, nil |
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.
Is there not a possibility of cstorvolume.Spec.TargetIP
be empty?
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.
No
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.
If it is empty snap creation will fail. So backup will fail
volume string) (*cstorapis.CStorVolumeReplica, error) { | ||
namespace := getOpenEBSNamespace() | ||
listOptions := metav1.ListOptions{ | ||
LabelSelector: "openebs.io/persistent-volume=" + 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.
This label is hardcoded.
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.
replaced with api constant
} | ||
|
||
// getLastBackupSnap will fetch the last successful backup's snapshot name | ||
func (bOps *backupAPIOps) getLastBackupSnap(backUp *openebsapis.CStorBackup) (string, error) { |
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 function is misleading in terms of what it does vs the comments and the function name.
ToDo: Next PR can improve this.
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>
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.
lgtm
This PR adds support for rest service in CVC operator and it adds the following functionality:
Sample Backup resource created by backup endpoint:
Sample Restore resource created by restore endpoint:
Note:
Manual testing:
TODO:
Issues:
Signed-off-by: mittachaitu sai.chaithanya@mayadata.io