-
Notifications
You must be signed in to change notification settings - Fork 373
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 Volume Health Support to CSI #415
Add Volume Health Support to CSI #415
Conversation
276c4ec
to
277cefe
Compare
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.
Several nits and renames suggested.
Also, if a plugin supports VOLUME_HEALTH, is there any reason why the list-volumes call can't return health for each of the volumes it reports?
Many CSI drivers don't support ListVolumes because it is expensive to make such a call. We could add a volume_id as a filter in ListVolumes but that means drivers have to implement ListVolumes in order to get information from a specific volume. During a previous review meeting, @saad-ali has suggested it is better to have a separate GetVolume RPC. |
I think my suggestion may have been misinterpreted. I wasn't suggesting the
elimination of GetVolume. Instead, I was suggesting that, for those plugins
that already implement list-volumes, maybe they would also be interested in
reporting health information that way.
…On Tue, Feb 11, 2020 at 11:56 AM Xing Yang ***@***.***> wrote:
Also, if a plugin supports VOLUME_HEALTH, is there any reason why the
list-volumes call can't return health for each of the volumes it reports?
Many CSI drivers don't support ListVolumes because it is expensive to make
such a call. We could add a volume_id as a filter in ListVolumes but that
means drivers have to implement ListVolumes in order to get information
from a specific volume. During a previous review meeting, @saad-ali
<https://github.com/saad-ali> has suggested it is better to have a
separate GetVolume RPC.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#415?email_source=notifications&email_token=AAR5KLGYEZYTMCUFWGSAU4TRCLKD5A5CNFSM4KSVIF22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELNF4AQ#issuecomment-584736258>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAR5KLFTVDTGRS4VA3GQ6XLRCLKD5ANCNFSM4KSVIF2Q>
.
--
James DeFelice
585.241.9488 (voice)
650.649.6071 (fax)
|
Sure, I can add volume health in ListVolumes. |
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.
Addressed review comments. Thanks.
cc @bswartz |
Addressed Ben's comments. Thanks. |
dcd82db
to
da542dd
Compare
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 have reviewed these changes and I'm satisfied.
a72e2a4
to
5177b70
Compare
@julian-hj, @msau42 addressed your your comments. PTAL. Thanks. |
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.
👍
It looks like some, not all of the Alpha API support is being added in this PR. While the work committed here isn't misaligned with #365, it is incomplete w/ respect to what was proposed. Suggestion: cherry-pick the following commits from #365 such that Alpha API support is added comprehensively instead of piecemeal, which will make it more clear for others looking to adopt. Or, maybe it would be cleaner to fast-track the above, suggested commits in a separate PR, merge that, and then rebase this one on top? I'm flexible here, just looking for a clear commit stream. |
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 should have marked my last review as "request changes" - sorry for the confusion.
@jdef Addressed most of your comments. I'll rebase this on the alpha api PR and address your remaining comment regarding the alpha feature. |
dca1151
to
0e5a1ff
Compare
Rebased. |
Hi @jdef, I addressed all your comments. Please take a look again. Thanks. |
Hi @saad-ali, I updated the PR based on what we concluded in the review meeting. Please take a look. Thanks. |
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.
some nits, but overall, lgtm
All comments are addressed. Please take a look again. |
@saad-ali Addressed your comments. PTAL. |
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.
Thanks @xing-yang
/lgtm
/approve
@jdef @jieyu @julian-hj all good? @xing-yang Please squash commits and I can help merge. |
Thanks @saad-ali. Squashed commits into 1. |
Thanks @jdef. Addressed your comments. |
LGTM |
@xing-yang please squash one more time and I'll merge |
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
Thanks. Merging. |
This PR adds volume health support to CSI spec.
Fixes: #410