Add Volume Health Support to CSI

[xing-yang]

This PR adds volume health support to CSI spec.

Fixes: #410

[jdef]

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?

[xing-yang]

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]

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)

[xing-yang]

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.

[xing-yang]

Addressed review comments. Thanks.

[msau42]

cc @bswartz

[xing-yang]

Addressed Ben's comments. Thanks.

[xing-yang]

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

[bswartz]

I have reviewed these changes and I'm satisfied.

[xing-yang]

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

[julian-hj]

👍

[jdef]

@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.

• 4cf1497
• 4c4253d

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.

[jdef]

I should have marked my last review as "request changes" - sorry for the confusion.

[xing-yang]

@jdef I submitted the alpha api support PR separately here: #417. Will rebase this PR once that one is merged.

[xing-yang]

@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]

Rebased.

[xing-yang]

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

[xing-yang]

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

[jieyu]

some nits, but overall, lgtm

[xing-yang]

All comments are addressed. Please take a look again.

[xing-yang]

@saad-ali Addressed your comments. PTAL.

[saad-ali]

Thanks @xing-yang

/lgtm
/approve

[saad-ali]

@jdef @jieyu @julian-hj all good?

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

[xing-yang]

Thanks @saad-ali. Squashed commits into 1.

[xing-yang]

Thanks @jdef. Addressed your comments.

[jdef]

LGTM

[saad-ali]

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

[jieyu]

lgtm

[xing-yang]

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

[saad-ali]

Thanks. Merging.