-
Notifications
You must be signed in to change notification settings - Fork 807
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
Add expand support #271
Add expand support #271
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bertinatto The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
d56f532
to
2f0293e
Compare
The sanity error is already fixed in csi-test: kubernetes-csi/csi-test@455ebbf |
Pull Request Test Coverage Report for Build 671
💛 - Coveralls |
2a62c49
to
46f3cf9
Compare
/retest |
/test pull-aws-ebs-csi-driver-e2e-single-az |
2e53325
to
d3f67f5
Compare
d3f67f5
to
1f54793
Compare
/test pull-aws-ebs-csi-driver-e2e-single-az |
/test pull-aws-ebs-csi-driver-e2e-multi-az |
ea49e7d
to
f8b7f8f
Compare
f8b7f8f
to
85a39ec
Compare
I was hoping to leave the manifest out for now to avoid exposing the feature before it's ready (see the PR's description). However, I understand it might be hard to test it, so I just added it. |
Good point. How about we add the resizer into the helm chat and make it configurable (like what I did for volume scheduling. ) The advantage is we can keep the manifest to only expose the stable features and use helm chart to expose alpha/unstable features. The drawback is we need to careful to keep both in sync |
85a39ec
to
2ea44a9
Compare
Exec: mount.NewOsExec(), | ||
}) | ||
|
||
// TODO: lock per volume ID to have some idempotency |
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.
@leakingtapan: I opened #307 to discuss this. I think the idempotency of the node service should be fixed in a separate PR.
Sounds good to me. Would it be acceptable to leave this out of this PR, though? Unfortunately I have a limited bandwidth at the moment, so I won't be able to tinker with helm soon. |
I can do the helm part. Could you rebase the PR before the merge? |
Should I remove the manifest commit then?
I think it's already rebased. |
Let's remove the manifest and squash the commits into one. |
2ea44a9
to
72cf94b
Compare
72cf94b
to
dda637f
Compare
Done |
/lgtm |
Is this a bug fix or adding new feature?
/kind feature
What is this PR about? / Why do we need it?
This PR adds partial support for volume expanding.
It doesn't add the resizer to the manifest file on purpose, so we can keep working on this without exposing the feature to people that try out the driver.
Fixes #146.