Specification: Adding disk details in hardware inventory #21
Conversation
GowthamShanmugam
force-pushed
the
disk
branch
from
0cd5111
to
7f3f2c0
Compare
GowthamShanmugam
changed the title
|
@shtripat @r0h4n @nnDarshan please review it |
| @@ -0,0 +1,113 @@ | |||
| = Specification for adding disk details in node hardware inventory | |||
There was a problem hiding this comment.
Please refer my comments from: #19 (review) . More or less all the suggestions hold good here as well
There was a problem hiding this comment.
i will change it
GowthamShanmugam
force-pushed
the
disk
branch
from
7f3f2c0
to
6b21433
Compare
|
@nnDarshan @shtripat @r0h4n @Tendrl/qe Please review it |
| Currently disk details are missing in node hardware inventory. Add disk details | ||
| in node hardware inventory and disk details should contain device_name, fs_type, | ||
| fs_uuid, model, mount_point, parent, size, name, disk_type, used, ssd, vendor, | ||
| disk_id, node_id. |
There was a problem hiding this comment.
There is an issue of disk names getting remapped when one of the disks are removed from existing inventory. @brainfunked was talking about some disk-id/fsid maintained by kernel can be used to maintain the uniqueness of the disk. This needs to figured out and added to this spec
There was a problem hiding this comment.
I will find it out and update in this specification file
|
|
||
| === Tendrl/node_agent impact | ||
|
|
||
| Modify persistence and manager modules to find and store disks details. |
There was a problem hiding this comment.
Any changes required in the tendrl definitions should be captured here
There was a problem hiding this comment.
i will add it
| node_inventory list. | ||
| * Create a new class in persistence module to keep disk details. | ||
| * Populate the object using node_inventory list and persist the object in etcd. | ||
|
|
There was a problem hiding this comment.
More details should go in here like the command used and output as in the pull request #6
There was a problem hiding this comment.
i will add command and output in this spec file
|
|
||
| == Tendrl API impact: | ||
|
|
||
| Api to get disk details v2/keys/nodes/node_id/Disks |
There was a problem hiding this comment.
Do we really need a separate API to get the disk details of the node?
There was a problem hiding this comment.
I think no need, everything comes from node api. I will remove it
|
|
||
| == Notifications/Monitoring impact | ||
|
|
||
| We might have to monitor the status of the disk. |
There was a problem hiding this comment.
How? If you think this should be addressed as part of another spec, raise github issues and link it here
There was a problem hiding this comment.
I have added a field called "used", it will tell whether a disk is used or not. I have mentioned this field as monitor here.
|
@brainfunked I have checked disk id for formatted and un formatted disk, I can't find out any unique id for both disk. A command ls /dev/disk/by-id is giving id for all disks but it generated based on disk serial number, But this id is not in good format lrwxrwxrwx. 1 root root 9 Dec 9 17:07 ata-QEMU_DVD-ROM_QM00003 -> ../../sr0 I dont think it is usable |
|
@GowthamShanmugam I think using lsblk is better here. Please look at https://github.com/skyrings/skyring/blob/master/backend/salt/python/skyring/saltwrapper.py#L177 |
|
@mkudlej The initial plan was to use lsblk. But the concern we see is, when lsblk is used it doesn't give UUID for a raw disk. UUID is available only for provisioned disks. If you are aware of any tool that can give UUID for all type of disks(raw and used) please let us know. |
|
@GowthamShanmugam suppose if you don't have any such id available, can you document an alternative approach to handle this?We need to make a decision and move on at the earliest. |
|
If we don't have such IDs available then, my suggestion would be to store used and raw disks separately. And use disk-uuid to identify used-disks and use disk-name to identify unused raw disks.something like:
We are mainly concerned about the rename of the disks that are being used, If we use UUID for disks that are being used this issue will be solved. And we are not bothered about the rename of unused disks, any how the disk details will be updated as part of hardware inventory sync. Also, if we go with the above approach, lsblk command would suffice. |
|
@nnDarshan am i modify the specification file as per your comment? |
|
@GowthamShanmugam wait. |
|
@nnDarshan your suggestion looks good to me but the only thing is the etcd dir names dont look good. May be the I would suggest rather apis as below The only issue with this I see is that we would have keep removing entries from free_disks once they are moved to used. |
|
@nnDarshan looks good to me. Please double check
@shtripat etcd path suggested by @nnDarshan (nodes/node-id/disks/used/disk-uuid/ ) looks better to me. |
GowthamShanmugam
force-pushed
the
disk
branch
from
6b21433
to
4435f9c
Compare
|
@nnDarshan , @nthomas-redhat @shtripat please review it |
My suggestion is almost matching with this. The only thing is I am merging words disks + used. so final apis could be like It reduces one level of apis in URL :) |
GowthamShanmugam
force-pushed
the
disk
branch
3 times, most recently
from
cd33aec
to
635fbfe
Compare
|
@nthomas-redhat @r0h4n @shtripat @nnDarshan @brainfunked review please |
|
Implemented in Tendrl/node-agent#78 |
| == Work Items | ||
|
|
||
| * Add Util-linux as a dependency. | ||
| * Create a new class called in persistence module. |
There was a problem hiding this comment.
Create a new class called what?
There was a problem hiding this comment.
New class nothing but Disk in persistence to keep disk details
There was a problem hiding this comment.
The statement is incomplete @GowthamShanmugam , "Create a new class called in persistence module"
shouldnt it be "Create a new class called Disk in persistence module" ?
There was a problem hiding this comment.
Disk details are stored from inventory list to Disk class instance , and store instance in etcd
There was a problem hiding this comment.
Update the document with the class name please.
|
|
||
| == Work Items | ||
|
|
||
| * Add Util-linux as a dependency. |
There was a problem hiding this comment.
Is this available in epel and rhel6/7 and pip?
There was a problem hiding this comment.
for rhel, util-linux, pip i dont know
There was a problem hiding this comment.
i will check
There was a problem hiding this comment.
We need it available in PIP and epel
There was a problem hiding this comment.
for epel it is available
There was a problem hiding this comment.
@r0h4n i am using lsblk command for find disk details.. do we really need this package... It is a default pakcage in linux right?
There was a problem hiding this comment.
for pip it is not available
There was a problem hiding this comment.
lsblk is a basic command in linux right.. it is available in all linux os... do we really need to specify this package?
There was a problem hiding this comment.
This doesn't need to be in pip. We'll just need to execute system commands and parse their output.
| None | ||
|
|
||
| == Other deployer impact | ||
|
|
There was a problem hiding this comment.
There's a new dependency being added, please document impact @nthomas-redhat @TimothyAsir
There was a problem hiding this comment.
..and add the necessary dependencies to the RPM spec file.
There was a problem hiding this comment.
Any updates @nthomas-redhat ?
| == Work Items | ||
|
|
||
| * Add Util-linux as a dependency. | ||
| * Create a new class called in persistence module. |
There was a problem hiding this comment.
The statement is incomplete @GowthamShanmugam , "Create a new class called in persistence module"
shouldnt it be "Create a new class called Disk in persistence module" ?
|
@r0h4n ok i will change it in specification file |
|
|
||
| * Add used disk details in etcd with key nodes/node_id/Used_disks. | ||
| * Add free disk details in etcd with key nodes/node_id/Free_disks. | ||
| * Add two different object in tendrl definitions for used_disk and free_disk. |
There was a problem hiding this comment.
There needs to be one object per disk under the disks directory. This object's key needs to be it's unique id. This unique id must always be present, whether the disk is free or in use. The used and free directories under disks need to simply contain objects that are the same id as the object in disks. The objects under free and used can be empty. The objects under disks should contain all the information about a disk, including it's partitions. We'll get the following structure:
/nodes/<node_id>/disks
|--> <disk_id1> # object with all the details for disk_id1
|--> <disk_id2> # object with all the details for disk_id2
|--> /used
|--|--> <disk_id1> # empty object
|--> /free
|--|--> <disk_id2> #empty object
GowthamShanmugam
force-pushed
the
disk
branch
3 times, most recently
from
faa0756
to
ea6dd10
Compare
|
@nthomas-redhat @shtripat @brainfunked please review this |
|
|
||
| == Proposed change | ||
|
|
||
| * "hwinfo" and "lsblk" to find the disk details. |
There was a problem hiding this comment.
I feel some of the details like classes etc should move to implementation section
|
|
||
| == Implementation | ||
|
|
||
| https://github.com/Tendrl/node_agent/issues/78 |
There was a problem hiding this comment.
Here we should talk about what classes, attributes, function etc to be written
| * Add used disks id in etcd with key nodes/node_id/Disks/Used. | ||
| * Add free disks id in etcd with key nodes/node_id/Disks/Free. | ||
| * Add all disks details in etcd with key nodes/node_id/Disks | ||
| * Add three different object in tendrl definitions for disks, used disks and |
There was a problem hiding this comment.
Not three different objects- one object per disk. I'd mentioned this in my previous comment yesterday, but it seems to have been lost.
There needs to be one object per disk, which contains all the information about the disk. This needs to be in the disks directory. Lower case, not capitalised. The object's key needs to be the id that's guaranteed to always be available.
The objects under used and free directories under disks (again, lower case, not capitalised), need to be empty objects with their key being the id of the disk object under disks.
Add validations to ensure that the same id doesn't fall under both used and free.
There was a problem hiding this comment.
@brainfunked I have tried to say same :) i will change this paragraph
GowthamShanmugam
force-pushed
the
disk
branch
4 times, most recently
from
1337d35
to
2f12e83
Compare
|
@brainfunked review please |
| === Tendrl/node_agent impact | ||
|
|
||
| * Add a new function in pull_hardware_inventory to collect disk details. | ||
| * Create three different classes Disks, FreeDisks, UsedDisks in persistence |
There was a problem hiding this comment.
Disk, UnusedDisk, UsedDisk. Both the latter ones should inherit from Disk.
There was a problem hiding this comment.
@brainfunked i will modify it to inherit both classes
| ``` | ||
| 1. columns = 'NAME,FSTYPE,MOUNTPOINT,UUID,SIZE,ROTA | ||
| lsblk --all --bytes --noheadings --output='%s' --path --raw" % columns | ||
| 2. hwinfo --block --only disk_name |
There was a problem hiding this comment.
Are you not using the output to file functionality from hwinfo?
There was a problem hiding this comment.
no i am not @nthomas-redhat told me no need to store the data in file and parse it directly
GowthamShanmugam
force-pushed
the
disk
branch
2 times, most recently
from
1ae8c60
to
2c8900c
Compare
|
@nthomas-redhat @brainfunked @shtripat review please |
| device_files, device_number, bios_id, geometry_logical, geometry_bios_legacy). | ||
| ``` | ||
| * Create three different classes called Disks, FreeDisks, UsedDisks in persistence | ||
| module and inherit freeDisks and UsedDisks from Disks class. |
There was a problem hiding this comment.
i will change it
GowthamShanmugam
force-pushed
the
disk
branch
from
2c8900c
to
ef7155f
Compare
|
@shtripat done |
|
@anivargi Please check the data model and approve the specification if it meets your requirements. |
spcification-issue: Tendrl#43 Signed-off-by: GowthamShanmugam <gshanmug@redhat.com>
GowthamShanmugam
force-pushed
the
disk
branch
from
ef7155f
to
02051cf
Compare
|
@r0h4n please review |
1 similar comment
|
@r0h4n please review |
| None | ||
|
|
||
| == Other deployer impact | ||
|
|
There was a problem hiding this comment.
Any updates @nthomas-redhat ?
Signed-off-by: GowthamShanmugam gshanmug@redhat.com