Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fill filesystem and partition info into Disk CR #197

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
5 participants
@akhilerm
Copy link
Member

akhilerm commented Jan 23, 2019

A new PartitionInfo struct is introduced in DiskInfo to store details of partitions in the disk. This partition data will be added to the Disk CR in etcd which can be used for further purposes like filtering.

This PR is the first step towards solving the filtering of LVM/partititons as mentioned in #74

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 23, 2019

Codecov Report

Merging #197 into master will decrease coverage by 0.42%.
The diff coverage is 51.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   51.07%   50.64%   -0.43%     
==========================================
  Files          43       43              
  Lines        1723     1856     +133     
==========================================
+ Hits          880      940      +60     
- Misses        780      851      +71     
- Partials       63       65       +2
Impacted Files Coverage Δ
pkg/udev/mockdata.go 71.05% <100%> (+0.38%) ⬆️
cmd/probe/udevprobe.go 64.95% <100%> (+4.4%) ⬆️
pkg/udev/common.go 79.36% <28.57%> (-14.52%) ⬇️
cmd/controller/disk.go 90.19% <33.33%> (-4.68%) ⬇️
cmd/probe/eventhandler.go 46.93% <50%> (-0.13%) ⬇️
cmd/probe/seachestprobe.go 0% <0%> (ø) ⬆️
cmd/controller/diskstore.go 87.64% <0%> (+1.22%) ⬆️
cmd/controller/controller.go 23.42% <0%> (+4.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fe7be5...10990f3. Read the comment docs.

@@ -57,6 +57,18 @@ func (c *Controller) CreateDisk(dr apis.Disk) {
}
}

// UpdateDiskCR to edit the DiskCR in etcd
func (c *Controller) UpdateDiskCR(oldDr *apis.Disk) error {

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 24, 2019

Collaborator

Better to rename oldDR to newDR to avoid confusion. Can we change function name to a better some meaningful name. There is one more function just below this name "UpdateDisk" which along with it may confuse in developer in future, when to use which .....

pe.Controller.PushDiskResource(oldDr, diskDetails)
// if old DiskCR doesn't exist and parition is found, it is ignored since we don't need info
// of partition if disk as a whole is ignored
if oldDr == nil && len(diskDetails.PartitionData) != 0 {

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 24, 2019

Collaborator

If oldDr is null it implies either Parent disk (sda) is excluded by filter so partition (sda1) should also be filtered out. So what I mean is that we have have passed filter check for a partition it means its parent disk (sda) should be there in etcd. Just check what happens to a disk-partition who's parent disk is filtered disk.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 24, 2019

Author Member

The functionality mentioned is currently done only by path-filter. That is, for an exclude config of /dev/sda, path-filter will exclude /dev/sda,/dev/sda1, /dev/sda2 etc. But for os-disk-filter , it will exclude only /dev/sda. The functionality to exclude all matching devices has to be implmented in the os-disk-filter. Any further changes needed on the os-filter @shovanmaity

pe.Controller.PushDiskResource(oldDr, diskDetails)
}
/// update the list of DiskCRs
diskList, err = pe.Controller.ListDiskResource()

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 24, 2019

Collaborator

I hope diskList is not causing any memory leak, as we have to call ListDiskResource many a time for each update/push to etcd.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Feb 1, 2019

Author Member

diskList is not causing a memory leak. Only pointers that we use in C Go, needs to be explicitly deallocated. The memory that we use here will be automatically garbage collected.

@akhilerm akhilerm changed the title Fill partition info into Disk CR Fill filesystem and partition info into Disk CR Jan 25, 2019

@akhilerm

This comment has been minimized.

Copy link
Member Author

akhilerm commented Jan 25, 2019

The functionality to include FileSystem information into DiskCR has been updated. Now DiskCR will have the filesystem information, even if the disk is formatted without creating partitions.

@akhilerm akhilerm force-pushed the akhilerm:74-detect-used-disks branch from 23117a1 to 3062752 Jan 28, 2019

@@ -45,6 +45,8 @@ type DiskInfo struct {
Compliance string // Compliance is implemented specifications version i.e. SPC-1, SPC-2, etc
DiskType string // DiskType represents the type of disk like Disk, Sparse etc.,
DriveType string // DriveType represents the type of disk like HHD, HDD etc.,
PartitionData []PartitionInfo // Information of the partitions on the disk
FileSystemInfo string // FileSystemInfo stores the filesystem on the disk. Can be none, xfs, ext4 etc

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 28, 2019

Collaborator

Move it above PartitionData.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 28, 2019

Author Member

Changes has been made.

@@ -139,6 +157,9 @@ func (di *DiskInfo) getDiskSpec() apis.DiskSpec {
diskSpec.Path = di.getPath()
diskSpec.Details = di.getDiskDetails()
diskSpec.Capacity = di.getDiskCapacity()
if di.FileSystemInfo != "none" {

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 28, 2019

Collaborator

Better to have constant for "None" and use that wherever required.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 28, 2019

Author Member

Replaced the string for no filesystem with a string constant


udr, err := c.Clientset.OpenebsV1alpha1().Disks().Update(newDr)
if err != nil {
glog.Error("unable to update disk object : ", err)

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 28, 2019

Collaborator

Report disk CR name along with err for better debugging.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 28, 2019

Author Member

Included diskCR name in error logging.

@satbirchhikara
Copy link
Collaborator

satbirchhikara left a comment

Some minor comments.

if oldDr == nil && len(diskDetails.PartitionData) != 0 {
glog.Info("Skipping partition of already excluded disk ", diskDetails.ProbeIdentifiers.Uuid)
continue
} else

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 31, 2019

Collaborator

Since we are continuing from here, "else" is not required.

@@ -146,6 +146,19 @@ func (up *udevProbe) scan() error {
deviceDetails.ProbeIdentifiers.Uuid = uuid
deviceDetails.ProbeIdentifiers.UdevIdentifier = newUdevice.GetSyspath()
diskInfo = append(diskInfo, deviceDetails)
} else if newUdevice.IsParitition() {

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 31, 2019

Collaborator

Fold it in single if block something like this "if (newUdevice.IsDisk() || newUdevice.IsPartition()"
because we are doing same thing in both blocks difference is just getting partition info if this is partition which we can anyway do by checking if this is partiton.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 31, 2019

Author Member

The folding has been done and pushed.

@@ -57,6 +57,18 @@ func (c *Controller) CreateDisk(dr apis.Disk) {
}
}

// UpdateDiskCR to edit the DiskCR in etcd
func (c *Controller) UpdateDiskCR(newDr *apis.Disk) error {

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Jan 31, 2019

Collaborator

This function and UpdateDisk can be folded in to one. As we discussed how to fix it, try to make it that way.

This comment has been minimized.

Copy link
@akhilerm

akhilerm Jan 31, 2019

Author Member

We have completely got rid of the UpdateDiskCR() . The same functionality will now be done by UpdateDisk()

@satbirchhikara
Copy link
Collaborator

satbirchhikara left a comment

Look good to me. Just one comment about memory leak which we need to find answer for.

@@ -87,6 +109,7 @@ func (pe *ProbeEvent) deleteDiskEvent(msg controller.EventMessage) {
// used for initial setup and when any uid mismatch or error occurred.
func (pe *ProbeEvent) initOrErrorEvent() {
udevProbe := newUdevProbe(pe.Controller)
defer udevProbe.free()

This comment has been minimized.

Copy link
@satbirchhikara

satbirchhikara Feb 1, 2019

Collaborator

There is udevEnumerate pointer inside udevProbe which is allocated using "C" memory allocation API, we need to free that as well (I think so). Need something like "defer udevProbe.udevEnumerate.free()"

This comment has been minimized.

Copy link
@akhilerm

akhilerm Feb 1, 2019

Author Member

Yes, there is udevEnumerate pointer inside udevProbe. But the free() of udevProbe struct handles all its memory allocations. Therefore we don't need to explicitly free the allocations.

@satbirchhikara
Copy link
Collaborator

satbirchhikara left a comment

LGTM

@akhilerm akhilerm force-pushed the akhilerm:74-detect-used-disks branch from 7fd19c0 to aedabb0 Feb 1, 2019

@satbirchhikara
Copy link
Collaborator

satbirchhikara left a comment

LGTM

@akhilerm akhilerm force-pushed the akhilerm:74-detect-used-disks branch from aedabb0 to b5be3c8 Feb 4, 2019

@@ -78,6 +81,11 @@ type ProbeIdentifier struct {
SeachestIdentifier string // SeachestIdentifier (devPath) is used to identify disk by seachest.
}

type PartitionInfo struct {
PartitionType string // Partition type like 83, 8e etc.
FileSystem string // Filesystem of partition. can be none, xfs etc.

This comment has been minimized.

Copy link
@AmitKumarDas

AmitKumarDas Feb 4, 2019

Member

Can we have enum to name a particular filesystem

This comment has been minimized.

Copy link
@akhilerm

akhilerm Feb 4, 2019

Author Member

I think you meant using const. But that means I will have to store name of every filesystem right? Is it a good approach?

[US4264] feat(diskCR): add partition and filesystem info
added partition and filesystem info to DiskCR. If partitions are present on
the disk, the partition type and filesystem information will be stored onto
the diskCR. If the whole disk is formatted with a filesystem, that info will
also be included

The change will give the following output when a partitioned disk is described
   ...
       links:
       - /dev/disk/by-path/pci-0000:00:03.0-scsi-0:0:3:0
     partitionDetails:
     - fileSystemType: None
       partitionType: "0x83"
     - fileSystemType: None
       partitionType: "0x8e"
     path: /dev/sdb
   stats:
   ...

If the disk is formatted as a whole with a filesystem
   ...
       links:
       - /dev/disk/by-path/pci-0000:00:03.0-scsi-0:0:5:0
     fileSystem: ext4
     path: /dev/sdb
   stats:
   ...

Signed-off-by: Akhil Mohan <akhil.mohan@mayadata.io>

@akhilerm akhilerm dismissed stale reviews from AmitKumarDas and satbirchhikara via 10990f3 Feb 4, 2019

@akhilerm akhilerm force-pushed the akhilerm:74-detect-used-disks branch from b5be3c8 to 10990f3 Feb 4, 2019

@kmova

kmova approved these changes Feb 4, 2019

Copy link
Member

kmova left a comment

Thanks @akhilerm

@kmova kmova merged commit f972084 into openebs:master Feb 4, 2019

2 of 4 checks passed

codecov/patch 51.02% of diff hit (target 51.07%)
Details
codecov/project 50.64% (-0.43%) compared to 1fe7be5
Details
Better Code Hub ☕ Don’t forget to refactor
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@akhilerm akhilerm deleted the akhilerm:74-detect-used-disks branch Feb 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.