Skip to content

[ALPHA] Add volume health proposal#604

Open
gnufied wants to merge 4 commits into
container-storage-interface:masterfrom
gnufied:implement-volume-health
Open

[ALPHA] Add volume health proposal#604
gnufied wants to merge 4 commits into
container-storage-interface:masterfrom
gnufied:implement-volume-health

Conversation

@gnufied

@gnufied gnufied commented May 21, 2026

Copy link
Copy Markdown
Contributor

@gnufied gnufied changed the title {WIP} Add volume health proposal Add volume health proposal May 26, 2026
@gnufied
gnufied force-pushed the implement-volume-health branch 3 times, most recently from 8789044 to 456214c Compare May 28, 2026 21:06
Comment thread spec.md Outdated
Comment thread spec.md
@nixpanic

nixpanic commented Jun 4, 2026 via email

Copy link
Copy Markdown
Contributor

@nixpanic nixpanic left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The current proposal looks good to me, and it should be workable with the CSI-drivers for Ceph.

Comment thread spec.md Outdated
Comment thread spec.md Outdated
Comment thread spec.md Outdated
Comment thread spec.md
Comment thread spec.md Outdated
Comment thread spec.md
Comment thread spec.md
@gnufied gnufied changed the title Add volume health proposal [ALPHA] Add volume health proposal Jul 15, 2026
Comment thread csi.proto Outdated
// being accessible from that node.
INACCESSIBLE = 2;

// Data loss is known or strongly suspected on underlying volume.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Permanent data loss is known or suspected.....

This is to distinguish from situations where data was lost but self-healing will eventually repair the problem.

We may also want to add text here indicating that we recommend discarding this volume and rebuilding from other (higher level) replicas if possible, because this is supposed to be one of the actionable health states and we don't want there to be any confusion about what its intended use is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Permanent data loss is known or suspected.....
+1

We may also want to add text here indicating that we recommend discarding this volume and rebuilding from other (higher level) replicas if possible, because this is supposed to be one of the actionable health states and we don't want there to be any confusion about what its intended use is.

So far we have avoided being too prescriptive on remediation (notice inaccessible doesn't say do not schedule workload that uses this volume on this node). I think that returned health condition is basically a suggestion to the CO and the workload (managed via some kind of operator), at the end of day - it is impossible to enforce this one way or the other from CSI perspective, so being very prescriptive IMO does not add much value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree we want to leave a little wiggle room, but too much wiggle room makes it impossible for SPs to implement the feature. An SP will worry that reporting a health condition could result in unknown COs doing unknown inappropriate things in response to that health condition, and could choose to just never report anything other than degraded for fear of some unintented outcome. If we constrain what the CO can/should do in response to any given condition, it will give the SP authors confidence.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Okay I have updated the wording to following:

// Permanent data loss is known or strongly suspected on the
// underlying volume. The CO MAY use this signal to inform
// users to trigger recovery workflows such as restoring from
// a snapshot or replica.

Comment thread csi.proto Outdated
Comment thread csi.proto Outdated
Comment thread csi.proto Outdated
Comment thread csi.proto Outdated
Comment thread csi.proto
gnufied added 2 commits July 16, 2026 11:40
Make ControllerGetVolumeHealth required if LIST_VOLUME_HEALTH is implemented
@gnufied
gnufied force-pushed the implement-volume-health branch from 85d1755 to 72e2e56 Compare July 16, 2026 16:06
@gnufied

gnufied commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@bswartz addressed the review comments PTAL

@torredil

Copy link
Copy Markdown

/lgtm

@bswartz bswartz left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM Thanks for addressing all my comments @gnufied

@xing-yang

Copy link
Copy Markdown
Contributor

lgtm

Comment thread spec.md
Comment thread spec.md
Comment thread spec.md
Comment thread spec.md
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.

5 participants