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

[CSI] OFFLINE volume expansion support #353

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

sushanthakumar
Copy link
Collaborator

What this PR does / why we need it:
This change addresses the offline volume expansion support from side

When the volume demand (pvc) is edited to request more volume csi containers will trigger csi block plugin to provision volume expansion.
Some key highlights

Node and controller capabilities are extended to acknowledge offline volume support
csi resizer sidecar container will be started during the deployment to receive expand volume related notification
Controller Expand Volume expand function is added to trigger the volume expansion at opensds/backend side
Node Expand Volume function is added to expand the filesystem already mounted on node

Which issue this PR fixes (optional, in fixes #(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
This PR addresses part(offline) of the issue: #209

Special notes for your reviewer:

Any other PR(s) this PR is dependant on: opensds/opensds#1174

Test steps:

Create a pod which uses pvc of size x gb
Delete just the pod where pvc is still there
Edit the pvc with size more than initial size
Create the pod again with the same pvc
Verify that pv/pvc/opensds volumes all are expanded
Verify that the volume mounted on node has expanded size

Release note:

@@ -21,7 +22,7 @@ metadata:
name: csi-pvc-opensdsplugin-block
spec:
accessModes:
- ReadWriteMany
- ReadWriteOnce
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the reason for this change?

Copy link
Collaborator Author

@sushanthakumar sushanthakumar Feb 11, 2020

Choose a reason for hiding this comment

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

This is an example deployment file and ReadWriteOnce is generally used accessMode. So updated the example file, no other reason for change

//Output looks like : [4:0:0:1] disk IET VIRTUAL-DISK 0001 /dev/sdb
// Need to parse and get host identifier (4 in above case)
glog.V(5).Infof("end to node expand volume, lsscsi: %v", output)
hostId := strings.Split(output, " ")[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was just thinking will this is be better:
i := strings.Index(output, "[")
fmt.Printf("%c", output[i+1])
Or else, simple regex

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved

glog.V(5).Infof("start to controller expand volume")
defer glog.V(5).Info("end to controller expand volume")

return nil, status.Error(codes.Unimplemented, "")
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to keep this unimplemented?


defer glog.V(5).Info("end to node expand volume")
return nil, status.Error(codes.Unimplemented, "")
}
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason to keep this unimplemented?

})

if err != nil {
msg := fmt.Sprintf("failed to extend volume: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

this is similar to error msg in line 241, probably we can re-prahse it "failed to get stable status after extendVolume . "
(Suggestion)

Copy link
Member

@asifdxtreme asifdxtreme left a comment

Choose a reason for hiding this comment

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

LGTM

@asifdxtreme
Copy link
Member

@sushanthakumar @kumarashit Can we merge this PR?

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.

3 participants