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.

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

fix(cleanup): add cleanup of volumes from node to monitor func #142

Merged
merged 4 commits into from
Jan 11, 2021

Conversation

payes
Copy link
Contributor

@payes payes commented Jan 9, 2021

This PR addresses the following:

  1. Handle clean up errors on node restarts
  2. Monitor and remove stale cstorVolumeAttachment objects. To handle cases when kubelet doesn't send unstage requests on node reboots.
  3. Avoid patching nodeID to CVC object every time the volume is staged. This may cause issues when the application pod is moved to a new node. Updating CVC requires the admission server to be reachable.
  4. Improve logging
    Signed-off-by: Payes Anand [email protected]

Signed-off-by: Payes Anand <[email protected]>
@codecov-io
Copy link

codecov-io commented Jan 10, 2021

Codecov Report

Merging #142 (89cf4af) into master (adaaa44) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #142   +/-   ##
=======================================
  Coverage   42.47%   42.47%           
=======================================
  Files          12       12           
  Lines         412      412           
=======================================
  Hits          175      175           
  Misses        224      224           
  Partials       13       13           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update adaaa44...89cf4af. Read the comment docs.

@prateekpandey14 prateekpandey14 changed the title fix(cleanup): add cleanup of volumes from node to monittor func fix(cleanup): add cleanup of volumes from node to monitor func Jan 10, 2021
@@ -359,6 +364,9 @@ func (ns *node) NodeUnpublishVolume(
}
vol, err := utils.GetCStorVolumeAttachment(volumeID + "-" + utils.NodeIDENV)
if err != nil {
if k8serror.IsNotFound(err) {
return &csi.NodeUnpublishVolumeResponse{}, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have log here saying cstorvolumeattachement not found

logrus.Infof("CstorVolumeAttachment %s was deleted or cannot be found: %s", volumeID, err.Error())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, I will add this log. Besides I will also add logs while creating and deleting CVAs

@@ -250,8 +251,12 @@ func (ns *node) NodeUnstageVolume(
defer removeVolumeFromTransitionList(volumeID)

if vol, err = utils.GetCStorVolumeAttachment(volumeID + "-" + utils.NodeIDENV); err != nil {
if k8serror.IsNotFound(err) {
return &csi.NodeUnstageVolumeResponse{}, nil
Copy link
Contributor

@prateekpandey14 prateekpandey14 Jan 10, 2021

Choose a reason for hiding this comment

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

can we have log here before retrun saying cstorvolumeattachement not found

logrus.Infof("CstorVolumeAttachment %s was deleted or cannot be found: %s", volumeID, err.Error())

Signed-off-by: Payes Anand <[email protected]>
return
count := 0
for {
if csivolList, err = GetVolListForNode(); err == nil {
Copy link
Contributor

@shubham14bajpai shubham14bajpai Jan 10, 2021

Choose a reason for hiding this comment

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

If this error out more than 5 times then the list will be empty and the error will not be logged as well. Can we log the error outside the for loop in that case?

Copy link
Contributor Author

@payes payes Jan 10, 2021

Choose a reason for hiding this comment

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

If it errors 5 times, we are returning an error to the caller function. If this func returns an error during startup, we will fatal out else we ignore the error. Will add one extra log if it errors 5 times.

Signed-off-by: Payes Anand <[email protected]>
Copy link
Contributor

@shubham14bajpai shubham14bajpai left a comment

Choose a reason for hiding this comment

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

Lgtm

Copy link
Contributor

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

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

Lgtm

@prateekpandey14 prateekpandey14 merged commit 313ddbc into openebs-archive:master Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants