replication: add replication state to GetVolumeReplicationInfo response

[Nikhil-Ladha]

add replication_state to GetVolumeReplicationInfo response.
Fixes: #78

[Nikhil-Ladha]

/cc @Madhu-1 @nixpanic @Rakshith-R

[black-dragon74]

Just curious, as we will have predictable states, can this not be an enum instead?

[Nikhil-Ladha]

We want the spec to be storage agnostic.
We can't have values based on ceph's mirror status values.

[Rakshith-R]

We want the spec to be storage agnostic. We can't have values based on ceph's mirror status values.

How are you going to use this value in csi-addons then?
its marked as required too so it may break compatibility ?
I think this needs more discussion.

[Nikhil-Ladha]

How are you going to use this value in csi-addons then?

We will fetch the state and parse it in ceph-csi and pass the value accordingly to the api.
CSI-Addons doesn't need to validate anything then, and just update the Status.State value based on the api's response.

its marked as required too so it may break compatibility ?

Hmm, yeah we would have to mark it optional for backward compatibility.

[black-dragon74]

LGTM

[Rakshith-R]

More discussion is required before deciding on this particular approach.

#78 (comment)

Needs further discussions..

[nixpanic]

This probably should be optional instead of required. If it is a message, maybe call it status_message instead, and explain that it is user-facing information.

If something is expected to inspect the contents and act on it, like a state, it definitely should be an enum that has values which fit for different storage providers.

[Nikhil-Ladha]

If it is a message, maybe call it status_message instead, and explain that it is user-facing information.

It would be the mirroring status state like stopped , syncing, error replaying and so on. I don't think we would want it be a message instead.

If something is expected to inspect the contents and act on it, like a state, it definitely should be an enum that has values which fit for different storage providers.

Based on the above mentioned value, I am not sure which values to use that make it fit for different storage providers?
Making it generic like primary, secondary, unkown and error could be one way

[Nikhil-Ladha]

And, then the storage provider is responsible for mapping the respective states for them to the csi-addons spec's state?
Like, for us (ceph's pov) stopped to primary, replaying to secondary, error to error and any other state to unkown

[nixpanic]

sounds reasonable, each state should have a description, something along these lines:

0. unknown status was not provided
1. primary this volume is in a primary state and can be used for writing to
2. secondary this volume is actively tracking changes from a primary volume
3. error something went wrong, and replication has stopped

The error state is probably final, and no automated recovery is possible. Maybe a mis-configuration or environment failure.

In case there is an error state that can be recovered automatically (or retried), such a state should be mentioned separately, maybe something like split-brain or out-of-space.

[Madhu-1]

@nixpanic IMO we should keep it as string because it will be hard to map this from storage to the csi-addons and also state is more of enum where we have pre defined states . can we do something like below

// Represents the current replication state as reported by the backend storage system.
//
// This field is optional.
//
// This is a free-form string that conveys the current replication state,
// such as "synchronized", "initializing", "error", "paused", etc.
//
// Storage providers SHOULD use values that make sense within their own ecosystem.
// This field is intended to be displayed to users or for diagnostic purposes,
// and is NOT intended to be used for strict programmatic logic across vendors.
string replication_status = 4;

[nixpanic]

Sure, that is basically what I meant with

... If it is a message, maybe call it status_message instead, and explain that it is user-facing information.

There is no need to add replication_ in the name, as the call is GetVolumeReplicationInfoResponse.
To me, status still sounds like something that can be parsed by non-humans. I therefor prefer status_message so that storage providers can phrase something helpful to guide the humans reading it.

[Madhu-1]

+1 for status_message as we need this to be representation only for users for information not the programmatic

[Madhu-1]

We can have status and a message for the very clear status and the more details message as well

enum Status {
    UNKNOWN = 0;
    HEALTHY = 1;
    DEGRADED = 2;
    ERROR = 3;
  }

 Status status = 1;
 string status_message = 2