feat(uuid): add partition table uuid support #635

cospotato · 2021-08-30T09:27:56Z

Pull Request template

Why is this PR required? What issue does it fix?: #621

What this PR does?: Add partition table uuid support

Does this PR require any upgrade changes?: No

If the changes in this PR are manually verified, list down the scenarios covered::

on CentOS 7 host: 🆗
on CentOS 8 host: 🆗
on Ubuntu 16.04 host: 🆗

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

Fixes [proposal] use DiskGUID instead of PartitionUUID #621
PR Title follows the convention of <type>(<scope>): <subject>
Has the change log section been updated?
Commit has unit tests
Commit has integration tests
(Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
(Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

codecov-commenter · 2021-08-30T09:55:10Z

Codecov Report

Merging #635 (83b0750) into develop (1fcc2b6) will decrease coverage by 0.89%.
The diff coverage is 7.46%.

@@             Coverage Diff             @@
##           develop     #635      +/-   ##
===========================================
- Coverage    46.58%   45.69%   -0.90%     
===========================================
  Files           78       79       +1     
  Lines         3834     3889      +55     
===========================================
- Hits          1786     1777       -9     
- Misses        1888     1953      +65     
+ Partials       160      159       -1

Impacted Files	Coverage Δ
cmd/ndm_daemonset/probe/addhandler.go	`69.43% <0.00%> (-4.97%)`	⬇️
cmd/ndm_daemonset/probe/blkidprobe.go	`0.00% <0.00%> (ø)`
cmd/ndm_daemonset/probe/probe.go	`100.00% <ø> (ø)`
pkg/features/features.go	`100.00% <ø> (ø)`
pkg/partition/partition.go	`30.26% <0.00%> (-11.56%)`	⬇️
cmd/ndm_daemonset/probe/eventhandler.go	`28.94% <14.28%> (-0.39%)`	⬇️
cmd/ndm_daemonset/probe/uuid.go	`94.73% <57.14%> (-5.27%)`	⬇️
cmd/ndm_daemonset/probe/udevprobe.go	`47.43% <0.00%> (-1.59%)`	⬇️

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 1fcc2b6...83b0750. Read the comment docs.

deploy/helm/charts/values.yaml

pkg/blkid/blkid.go

cmd/ndm_daemonset/probe/uuid.go

akhilerm · 2021-09-14T05:07:00Z

pkg/features/features.go

+	// partition table uuid instead of create partition described in
+	// https://github.com/openebs/node-disk-manager/issues/621 .
+	// This feature must enabled with GPTBasedUUID.
+	PartitionTableUUID Feature = "PartitionTableUUID"


Can you add one check, to make sure this feature gets enabled only with GPTBasedUUID. The check can be added here, so that such dependent features can be checked in future also.

I think you missed this comment. PTAL.

@akhilerm Sorry, the here link previously point to the pkg/partition/partition.go file, so i add the feature check in that files. What the here point to exactly ?

oops. sorry for that, the check should be in the feature pkg here. So everything related to features are handled there.

Also, the check needed is, if PartitionTableUUID feature gate is given , we should make sure that GPTBasedUUID is also provided. The best place to make that check is in the feature pkg.

OK, I get that. I will fix it!

akhilerm

@cospotato Given some comments.

Also you need to update the node-disk-manager.yaml, and run make manifests so that the updated ndm-operator.yaml is generated.

cospotato · 2021-09-14T06:00:52Z

Hello @akhilerm ,comments resolved.

akhilerm · 2021-09-17T08:22:33Z

cmd/ndm_daemonset/probe/blkidprobe.go

+limitations under the License.
+*/
+
+package probe


@cospotato Do we need a new probe for fetching this details? Can this be included in some existing probes? WDYT?

@akhilerm I think it's need this new probe to get partition table UUID by blkid. Because only the udevprobe can provide partition table UUID in the existing probes. But on CentOS 7, the host udev did not provide it. But we use the cache from host udev. So we cannot get partition table UUID with udev on CentOS 7 host.

cmd/ndm_daemonset/probe/blkidprobe.go

akhilerm · 2021-09-17T08:26:17Z

cmd/ndm_daemonset/probe/uuid.go

+	case features.FeatureGates.IsEnabled(features.PartitionTableUUID) && len(bd.PartitionInfo.PartitionTableType) > 0:
+		if len(bd.PartitionInfo.PartitionTableUUID) == 0 {
+			klog.Errorf("device(%s) has a partition table, but can not get partition table uuid", bd.DevPath)
+			break


What will happen in the following case:

The break gets executed here, which means the uuid wont be generated.

NDM goes onto create a partition on the disk, but it will fail since a partition table already exists

This will trigger a rescan and causes an infinite loop

Can we have some checks for the above case also, possibly in the eventhandler or addhandler?

The GPT partition table without UUID is very weird. But This may be happened. Or can we ignore this disk ?

akhilerm · 2021-09-17T08:29:15Z

pkg/blkid/blkid.go

@@ -1,3 +1,6 @@
+//go:build (linux && ignore) || cgo


What is this build tag used for?

Sorry, this and the below line changed because of my local environment is go 1.17.1 . The gofmt cause the change. The help described this: go help

To keep a file from being considered for the build: //go:build ignore (any other unsatisfied word will work as well, but "ignore" is conventional.) To build a file only when using cgo, and only on Linux and OS X: //go:build cgo && (linux || darwin)

It just a mark make us can set the file ignore when compiling.

akhilerm · 2021-09-17T08:30:35Z

pkg/blkid/blkid.go

@@ -1,3 +1,6 @@
+//go:build (linux && ignore) || cgo
+// +build linux,ignore cgo


What does the ignore tag do here?

pkg/partition/partition.go

akhilerm

Commented on a few more changes.

akhilerm · 2021-09-17T12:08:43Z

pkg/features/features.go

+
 // IsEnabled returns true if the feature is enabled
 func (fg featureFlag) IsEnabled(f Feature) bool {
+	if !fg.checkRequired(f) {


This need not be checked everytime, since we set the flag to default value initially.

z0marlin

@cospotato left a few comments.

z0marlin · 2021-09-17T18:28:02Z

cmd/ndm_daemonset/probe/blkidprobe.go

+func (bp *blkidProbe) FillBlockDeviceDetails(bd *blockdevice.BlockDevice) {
+	di := &blkid.DeviceIdentifier{DevPath: bd.DevPath}
+
+	bd.FSInfo.FileSystem = di.GetOnDiskFileSystem()


right now udevprobe and mountprobe fill the filesystem information for the device. Is this required here too?

I think this can check the bd.FSInfo.FileSystem length if zero before fill it.

@cospotato can you please add this check (check if FS is already filled by prior probes)

pkg/features/features.go

z0marlin · 2021-09-18T11:56:33Z

pkg/features/features.go

+		// disable the feature if the requirement does not meet expectations
+		if !checkRequirement(fg, k) {
+			fg[k] = false
+			v = false
+
+			klog.Warningf("Feature gate: %s required: \"%v\" dose not meet expectation, set to disable", k, requirementString(k))
+		}


while using a map works in this scenario, it would break for cases when there are more complex requirements in which case we would have to do a graph traversal of sorts to determine if the requirements are satisfied. Although, I don't think feature gates will reach that complexity, do we want to consider those cases in this code? if not, then can we have some comments explaining the limitations for future reference. @akhilerm @cospotato wdyt?

an example that would break when using the above logic:
A requires B
B requires C
C is set to false by the user while B is set to true.
In this case, the correct setting would be A, B, C set to false. But in the code, it will depend on the order of traversal of the map and we may have a wrong setting.

Maybe we will never reach that complexity. There are two solution in this case:

Calculate the requirement graph in initiation phase

The More easier solution: calculate the requirement in IsEnabled method

I prefer the first solution. But it will introduce complexity. @z0marlin @akhilerm WDYT ?

Maybe we can create a new issue to implement this feature.

I also prefer the first solution. Will raise an issue to enhance the feature flag mechanism. I guess for now we will avoid those checks?

Refer issue #651 for dependent features.

pkg/features/util.go

akhilerm · 2021-09-29T06:07:23Z

@cospotato Can you resolve the conflicts so that CI can be run

cospotato · 2021-09-29T06:08:18Z

@cospotato Can you resolve the conflicts so that CI can be run

OK, I will fix it.

cospotato · 2021-09-29T07:32:10Z

@akhilerm Can you show the the Better Code Hub report of this branch. I will try to refactor it. I tried to analysis in my repo. But it show lots of bad code smell which do not contained in this commit.

akhilerm · 2021-10-25T06:47:28Z

@cospotato I think the BCH can be ignored for now as the results are from old code, which we may need to refactor.

akhilerm · 2021-11-18T06:58:52Z

@cospotato The changes for dependent feature gates have been merged, Can you rebase those changes into this PR so that we can merge this.

cospotato · 2021-11-18T07:01:14Z

@cospotato The changes for dependent feature gates have been merged, Can you rebase those changes into this PR so that we can merge this.

@akhilerm OK. I will rebase it. Thank you.

akhilerm

Changes looks good. Thanks @cospotato

Can we exclude the helm chart section from the PR, so that we can update it as part of the release? The operator.yaml is fine, since its not versioned.

cospotato · 2021-11-18T08:34:36Z

@akhilerm Dose you means exclude the changes in deploy/helm ?

akhilerm · 2021-11-18T08:35:09Z

@akhilerm Dose you means exclude the changes in deploy/helm ?

Yes

akhilerm

LGTM

z0marlin

LGTM. One small change and we are good to go.

z0marlin · 2021-11-18T11:42:07Z

cmd/ndm_daemonset/probe/blkidprobe.go

+func (bp *blkidProbe) FillBlockDeviceDetails(bd *blockdevice.BlockDevice) {
+	di := &blkid.DeviceIdentifier{DevPath: bd.DevPath}
+
+	bd.FSInfo.FileSystem = di.GetOnDiskFileSystem()


@cospotato can you please add this check (check if FS is already filled by prior probes)

cospotato · 2021-11-19T02:43:22Z

@z0marlin Thank you. Fixed

z0marlin

Thanks for working on this :)

akhilerm

LGTM

cospotato force-pushed the master branch from a516982 to ca9ae23 Compare August 30, 2021 09:51

cospotato force-pushed the master branch 2 times, most recently from 27508d1 to 2f61c8e Compare August 31, 2021 08:07

cospotato marked this pull request as ready for review September 1, 2021 02:41

cospotato mentioned this pull request Sep 5, 2021

[proposal] use DiskGUID instead of PartitionUUID #621

Closed

akhilerm requested review from akhilerm, kmova and z0marlin September 14, 2021 04:48

akhilerm reviewed Sep 14, 2021

View reviewed changes

deploy/helm/charts/values.yaml Outdated Show resolved Hide resolved

akhilerm reviewed Sep 14, 2021

View reviewed changes

pkg/blkid/blkid.go Show resolved Hide resolved

akhilerm reviewed Sep 14, 2021

View reviewed changes

cmd/ndm_daemonset/probe/uuid.go Outdated Show resolved Hide resolved

akhilerm reviewed Sep 14, 2021

View reviewed changes

akhilerm suggested changes Sep 14, 2021

View reviewed changes

cospotato force-pushed the master branch from 2f61c8e to ca01e0a Compare September 14, 2021 05:59

akhilerm reviewed Sep 17, 2021

View reviewed changes

cmd/ndm_daemonset/probe/blkidprobe.go Outdated Show resolved Hide resolved

akhilerm reviewed Sep 17, 2021

View reviewed changes

pkg/partition/partition.go Outdated Show resolved Hide resolved

akhilerm suggested changes Sep 17, 2021

View reviewed changes

akhilerm reviewed Sep 17, 2021

View reviewed changes

z0marlin reviewed Sep 18, 2021

View reviewed changes

cospotato force-pushed the master branch 2 times, most recently from edd089b to 2313e9c Compare September 22, 2021 03:33

cospotato closed this Sep 29, 2021

cospotato force-pushed the master branch from 2313e9c to 9fbcfa8 Compare September 29, 2021 07:01

cospotato reopened this Sep 29, 2021

cospotato closed this Sep 29, 2021

cospotato force-pushed the master branch from edd089b to 9fe3968 Compare September 29, 2021 07:10

cospotato reopened this Sep 29, 2021

cospotato force-pushed the master branch 2 times, most recently from c58a061 to 9d4703a Compare November 18, 2021 07:39

cospotato requested review from akhilerm and z0marlin November 18, 2021 07:48

akhilerm previously approved these changes Nov 18, 2021

View reviewed changes

cospotato dismissed akhilerm’s stale review via 039bc90 November 18, 2021 08:36

cospotato force-pushed the master branch from 9d4703a to 039bc90 Compare November 18, 2021 08:36

cospotato requested a review from akhilerm November 18, 2021 08:46

akhilerm previously approved these changes Nov 18, 2021

View reviewed changes

z0marlin reviewed Nov 18, 2021

View reviewed changes

feat(uuid): add partition table uuid support

83b0750

cospotato dismissed akhilerm’s stale review via 83b0750 November 19, 2021 02:42

cospotato force-pushed the master branch from 039bc90 to 83b0750 Compare November 19, 2021 02:42

z0marlin approved these changes Nov 19, 2021

View reviewed changes

akhilerm approved these changes Nov 19, 2021

View reviewed changes

akhilerm merged commit 8e36a0d into openebs-archive:develop Nov 19, 2021

akhilerm mentioned this pull request Dec 24, 2021

chore(changelog): changelog for v1.8.0 #665

Merged

		@@ -1,3 +1,6 @@
		//go:build (linux && ignore) \|\| cgo
		// +build linux,ignore cgo

feat(uuid): add partition table uuid support #635

feat(uuid): add partition table uuid support #635

Uh oh!

Conversation

cospotato commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request template

Uh oh!

codecov-commenter commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akhilerm left a comment

Choose a reason for hiding this comment

Uh oh!

cospotato commented Sep 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akhilerm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

z0marlin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cospotato Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

akhilerm commented Sep 29, 2021

Uh oh!

cospotato commented Sep 29, 2021

Uh oh!

cospotato commented Sep 29, 2021

Uh oh!

cospotato commented Aug 30, 2021 •

edited

Loading

codecov-commenter commented Aug 30, 2021 •

edited

Loading

cospotato Sep 22, 2021 •

edited

Loading