Added specification for refactoring gluster get-state output #6
Conversation
Current output of get-state command from gluster also provides volume options details. This specification talks about changes required for accomodating the same and updating details in Tendrl central store. Signed-off-by: Shubhendu <shtripat@redhat.com>
|
@r0h4n @brainfunked @nthomas-redhat @anmolbabu @nnDarshan please review. |
| Notes: | ||
|
|
||
| * In light of new changes to gluster get-state | ||
| http://review.gluster.org/#/c/15889 |
There was a problem hiding this comment.
Update new review http://review.gluster.org/#/c/15975
|
|
||
| == Work items | ||
|
|
||
| * Introduce a class VolumeOptions in persister module to render volume options |
There was a problem hiding this comment.
Volume options should be part of the same Volume class, No new class is required, please update
There was a problem hiding this comment.
As commented in previous comment
"The volume options names are not statically defined for a volume. The list gets changed based on what features (e.g. snapshot, tiering, quota, aribter, nfs-ganesha) are enabled for the volume. Rather "Options" need to be an attribute for the volume under which a dictionary represents the various options set for the volume."
| Volume1.type: Distribute | ||
| Volume1.transport_type: tcp | ||
| Volume1.status: Started | ||
| Volume1.brickcount: 3 |
There was a problem hiding this comment.
Please ensure all these new attributes and options are reflected in the "Volume" object itself
There was a problem hiding this comment.
Done. Will add all other attributes of volume.
But volume options names are not statically defined for a volume. The list gets changed based on what features (e.g. snapshot, tiering, quota, aribter, nfs-ganesha) are enabled for the volume. Rather "Options" need to be an attribute for the volume under which a dictionary represents the various options set for the volume.
There was a problem hiding this comment.
It is ok if the option visibility changes based on features.
We still need to check for each option and define them in Tendrl definitions and treat them as attributes.
There was a problem hiding this comment.
Based on features enabled or disabled these options may appear/disappear from the list. Not sure why we need to keep static in tendrl only whereas its actually dynamic in gluster CLI output.
There was a problem hiding this comment.
You need to define all these options/attributes via Tendrl definitions because tendrl-api, tendrl-dashboard, tendrl-gluster-integration can discover these.
If not defined in Tendrl definitions, we must not use them in our code. This is clear violation of our definitions driven approach
There was a problem hiding this comment.
So, its like if a feature for the volume is enabled, yes all the required volume options fir sure would be set with some default values as long as I remember :)
I feel a new spec for handling "dict" type in definitions is fine and linking with this. But do we block this spec from merging or we can go ahead and handle "dict" support in definitions as separate item?
There was a problem hiding this comment.
Both specs can continue in parallel, But you would need the "dict" type changes in place to test your spec.
Please document the other spec as well and link it to your spec.
There was a problem hiding this comment.
As discussed volume options attribute could be a json which contains key:value pairs.
Will update this spec with details of changes to be done in definitions as well.
There was a problem hiding this comment.
Regarding the Edit Volume scenario mentioned above by @shtripat, we need to document which specific scenarios will need to be handled as individual flows, and which can be satisfied directly by a generic UpdateVolumeOptions or similar. This has impact on the definitions and needs to be documented in this spec.
There was a problem hiding this comment.
As long as gluster is concerned these options are set through features enable/disable and also can be set/edited individually using CLI.
What I have thought for tendrl is that, provide a edit option for the volume and let the user edit any option which is already set using a single flow say named as UpdateVolumeOptions.
These volume options can be set/edited as well from different work-flows in tendrl UI as well.
Why I suggest this way because some of the options get set through volume creation options whereas some of them are set as specific volume set option CLI.
|
|
||
| * Check if the gluster get-state output is parsed without any error | ||
|
|
||
| * Verify the volume details in central store if it contains the volume options |
There was a problem hiding this comment.
Lets not differentiate betweeen volume attributes and options, they are same and should go into "Volume" object
There was a problem hiding this comment.
As commented earlier
"The volume options names are not statically defined for a volume. The list gets changed based on what features (e.g. snapshot, tiering, quota, aribter, nfs-ganesha) are enabled for the volume. Rather "Options" need to be an attribute for the volume under which a dictionary represents the various options set for the volume."
We actually need to differentiate between normal volume attributes like "id", "name" etc from options. Rather we should have an attribute called "options" and a dict of different available options need to be listed under this attribute.
There was a problem hiding this comment.
Why do we need to differentiate?
There was a problem hiding this comment.
We need to differentiati because they are not volume attributes. Rather "options" is volume attribute.
Also if volume features are enabled dynamically runtime, we dont need to change anything in tendrl for this and autimatically all the keys would be available in tendrl as part of refresh of cluster data.
|
|
||
| == Dependencies | ||
|
|
||
| * Required Gluster 3.9 + patch http://review.gluster.org/#/c/15889 |
There was a problem hiding this comment.
Update new review http://review.gluster.org/#/c/15975
Signed-off-by: Shubhendu <shtripat@redhat.com>
shtripat
force-pushed
the
gluster-get-state
branch
from
54aab65
to
34522e1
Compare
|
@nthomas-redhat @brainfunked plz review |
| central store. | ||
|
|
||
| Also not all the volume attributes are added to Volume object in central store. | ||
| The same should be taken care and added. |
There was a problem hiding this comment.
Should be: "The same should be taken care of and added."
| Volume1.type: Distribute | ||
| Volume1.transport_type: tcp | ||
| Volume1.status: Started | ||
| Volume1.brickcount: 3 |
There was a problem hiding this comment.
Regarding the Edit Volume scenario mentioned above by @shtripat, we need to document which specific scenarios will need to be handled as individual flows, and which can be satisfied directly by a generic UpdateVolumeOptions or similar. This has impact on the definitions and needs to be documented in this spec.
| Last allocated port: 49153 | ||
| ``` | ||
|
|
||
| * General refactoring of code handling the gluster get-state output and add |
There was a problem hiding this comment.
The details of the refactoring need to be specified below.
Signed-off-by: Shubhendu <shtripat@redhat.com>
| @@ -148,6 +148,9 @@ Last allocated port: 49153 | |||
|
|
|||
| * General refactoring of code handling the gluster get-state output and add | |||
| all missing volume attributes to the Volume object | |||
| ** Additional fields available under volume details from get-state output | |||
| like transport_type, snap_count, stripe_count, replica_count, arbiter_count, | |||
| quorum_status, ... to be added to central store | |||
|
|
|||
| * Modify the tendrl definitions specifications for gluster and add all the | |||
| missing attributes to Volume object | |||
There was a problem hiding this comment.
- list down all attributes you are planning to add
- structure json which will updated to etcd
- Any changes to tendrl definitions to be written down here(snip from tendrl definitions)
Signed-off-by: Shubhendu <shtripat@redhat.com>
shtripat
force-pushed
the
gluster-get-state
branch
from
c9e584e
to
5e1e17a
Compare
|
@shtripat , Looks good to me. Only thing I found is the github issue is missing in the spec. You need to file an issue on specifications repo. Also file issues on all the components where the chnages are made. Refer these issues in the refereces section of the documnent. |
Signed-off-by: Shubhendu <shtripat@redhat.com>
|
@nthomas-redhat added issues in references section. please review.. |
Current output of get-state command from gluster also provides
volume options details. This specification talks about changes
required for accomodating the same and updating details in Tendrl
central store.
Signed-off-by: Shubhendu shtripat@redhat.com