search

container-storage-interface / spec

Container Storage Interface (CSI) Specification.
Apache License 2.0
1.34k stars 373 forks source link

Add Volume Health Support to CSI #415

Closed xing-yang closed 4 years ago

xing-yang commented 4 years ago

This PR adds volume health support to CSI spec.

Fixes: #410

xing-yang commented 4 years ago

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.

jdef commented 4 years ago

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 notifications@github.com 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 https://github.com/container-storage-interface/spec/pull/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)

xing-yang commented 4 years ago

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.

Sure, I can add volume health in ListVolumes.

msau42 commented 4 years ago

cc @bswartz

xing-yang commented 4 years ago

Addressed Ben's comments. Thanks.

xing-yang commented 4 years ago

@jdef I have marked the new method, message, and field experimental based on your PR (https://github.com/container-storage-interface/spec/pull/365). Can you please take a look? https://github.com/container-storage-interface/spec/pull/415/commits/5c862b269809e1bdbf64d81261e3eace5ce78ecf

xing-yang commented 4 years ago

@julian-hj, @msau42 addressed your your comments. PTAL. Thanks.

jdef commented 4 years ago

@jdef I have marked the new method, message, and field experimental based on your PR (#365). Can you please take a look? 5c862b2

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.

xing-yang commented 4 years ago

@jdef I submitted the alpha api support PR separately here: https://github.com/container-storage-interface/spec/pull/417. Will rebase this PR once that one is merged.

xing-yang commented 4 years ago

@jdef Addressed most of your comments. I'll rebase this on the alpha api PR and address your remaining comment regarding the alpha feature.

xing-yang commented 4 years ago

Rebased.

xing-yang commented 4 years ago

Hi @jdef, I addressed all your comments. Please take a look again. Thanks.

xing-yang commented 4 years ago

Removed AlphaFeature bucket from the other PR and rebased this PR. Also squashed all commits into one.

xing-yang commented 4 years ago

@jieyu Addressed your comments.

jdef commented 4 years ago

LGTM

On Wed, Mar 4, 2020 at 2:41 PM Jie Yu notifications@github.com wrote:

@jieyu approved this pull request.

LGTM

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/container-storage-interface/spec/pull/415?email_source=notifications&email_token=AAR5KLDUV5Q776YIGRLYAWDRF2VGJA5CNFSM4KSVIF22YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCX7YQHA#pullrequestreview-369068060, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAR5KLFK5HYNWX46RH3H2NDRF2VGJANCNFSM4KSVIF2Q .

-- James DeFelice 585.241.9488 (voice) 650.649.6071 (fax)

xing-yang commented 4 years ago

Hi @saad-ali, can you please take a look?

xing-yang commented 4 years ago

The Alpha API PR is merged. Just rebased this PR to master.

xing-yang commented 4 years ago

Hi @saad-ali, I updated the PR based on what we concluded in the review meeting. Please take a look. Thanks.

xing-yang commented 4 years ago

All comments are addressed. Please take a look again.

xing-yang commented 4 years ago

@saad-ali Addressed your comments. PTAL.

saad-ali commented 4 years ago

@jdef @jieyu @julian-hj all good?

@xing-yang Please squash commits and I can help merge.

xing-yang commented 4 years ago

Thanks @saad-ali. Squashed commits into 1.

xing-yang commented 4 years ago

Thanks @jdef. Addressed your comments.

jdef commented 4 years ago

LGTM

saad-ali commented 4 years ago

@xing-yang please squash one more time and I'll merge

xing-yang commented 4 years ago

Squashed commits. Thanks @saad-ali and @jieyu.

saad-ali commented 4 years ago

Thanks. Merging.