-
Notifications
You must be signed in to change notification settings - Fork 379
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
Added the feature to use secrets of ListSnapshots #252
Changes from 1 commit
3b76de4
4017329
2045ef1
0a1579d
dd390a3
8e9680a
7cbcf6a
3c2baf4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -252,10 +252,33 @@ func (ctrl *csiSnapshotSideCarController) checkandUpdateContentStatusOperation(c | |
var readyToUse = false | ||
var driverName string | ||
var snapshotID string | ||
var snapshotterListCredentials map[string]string | ||
|
||
if content.Spec.Source.SnapshotHandle != nil { | ||
klog.V(5).Infof("checkandUpdateContentStatusOperation: call GetSnapshotStatus for snapshot which is pre-bound to content [%s]", content.Name) | ||
readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content) | ||
|
||
if content.Spec.VolumeSnapshotClassName != nil { | ||
class, err := ctrl.getSnapshotClass(*content.Spec.VolumeSnapshotClassName) | ||
if err != nil { | ||
klog.Errorf("failed to getSnapshotClass %s", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/failed/Failed |
||
return nil, fmt.Errorf("cannot get snapshot class for snapshot content %s", err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add snapshot class name here too. |
||
} | ||
|
||
snapshotterListSecretRef, err := utils.GetSecretReference(utils.SnapshotterListSecretParams, class.Parameters, content.GetObjectMeta().GetName(), nil) | ||
if err != nil { | ||
klog.Errorf("Failed to get secret reference for snapshot %s: %s", content.Name, err.Error()) | ||
return nil, fmt.Errorf("cannot get secret reference for snapshot content %#v", content.Name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. print err here as well. |
||
} | ||
|
||
snapshotterListCredentials, err = utils.GetCredentials(ctrl.client, snapshotterListSecretRef) | ||
if err != nil { | ||
// Continue with deletion, as the secret may have already been deleted. | ||
klog.Errorf("Failed to credentials for snapshot %s: %s", content.Name, err.Error()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing "get": |
||
return nil, fmt.Errorf("cannot get credentials for snapshot content %#v", content.Name) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's good to have the "err" here as well. You can format a message and have it used in both logging and the return message. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @xing-yang Sorry,
This means should I create a variable that store formatted error message, and hand over the variable to klog.Errorf argument and return value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. So that means the message needs to start with lower case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Um...Sorry I can't understand yet. // Continue with deletion, as the secret may have already been deleted.
klog.Errorf("Failed to credentials for snapshot %s: %s", content.Name, err.Error())
return nil, fmt.Errorf("failed to credentials for snapshot %s: %s", content.Name, err.Error()) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that is fine too. s/to credentials/to get credentials There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you. I understand. |
||
} | ||
} | ||
|
||
readyToUse, creationTime, size, err = ctrl.handler.GetSnapshotStatus(content, snapshotterListCredentials) | ||
if err != nil { | ||
klog.Errorf("checkandUpdateContentStatusOperation: failed to call get snapshot status to check whether snapshot is ready to use %q", err) | ||
return nil, err | ||
|
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.
/Delete/List