feat(pkg/features): added support for feature gate dependencies by jdkramhoft · Pull Request #652 · openebs-archive/node-disk-manager

jdkramhoft · 2021-10-08T13:45:00Z

What this PR does?:
Features can now be added that are dependent on other features.
Attempting to set a feature flag to true if the dependency is not met will result in an error.

Does this PR require any upgrade changes?:
No, I don't think so.

If the changes in this PR are manually verified, list down the scenarios covered::
The unit tests included cover the scenarios:

Setting a feature flag to true when a dependency is met, and when it is unmet.
Setting a feature flag to true when only one of two dependencies are met.
Having multiple feature flags with the same dependencies

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
This supports dependent feature gates for later when UsePartTableUUID will be introduced.

Checklist:

Fixes Enhance feature gate package to add support for dependent feature gates #651
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:

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin

Thanks for the PR @jdkramhoft :) Left a few comments.

z0marlin · 2021-10-08T15:58:04Z

+				missingDependency := !fg[dependency]
+				if missingDependency {
+					fg[f] = false
+					return fmt.Errorf("Feature %s has unmet dependency %s - enable that first", feature, dependency)


returning an error here would cause ndm to exit with failure. Is that what we want? Or do we want to just log, disable the flag and continue?

I have changed behavior to logging, disabling the flags and continuing.

z0marlin · 2021-10-08T16:00:21Z

+		// check if the feature has dependencies
+		dependencies, hasDependencies := featureDependencies[f]
+		// if the feature is being set to true, we need to ensure dependencies are met
+		if hasDependencies && isEnabled {
+			for _, dependency := range dependencies {
+				missingDependency := !fg[dependency]
+				if missingDependency {
+					fg[f] = false
+					return fmt.Errorf("Feature %s has unmet dependency %s - enable that first", feature, dependency)
+				}
+			}
+		}


This entire block should be out of the for loop. This is because the features aren't ordered. It is possible that a dependency for feature A appears in the slice after the feature A. Also, you might have to think of some recursive logic to resolve the dependencies if we want to support a case like A -> B -> C

Oh! Good catch; Right now if someone later disabled a necessary dependency I wouldn't catch it.
I will move it to after the loop and validate the state then, logging and disabling flags.

Seems like I technically should check for cycles and then do a topological sort. Feels a bit heavyweight for only a few support flags. Can I assume there are no cycles of dependencies A -> B -> C -> A ?

yes, @jdkramhoft . cycles wont be there as we itself add the feature gates in the code. What we want to achieve is the user should not be enabling some features while dependent features are disabled.

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

codecov-commenter · 2021-10-12T19:14:23Z

Codecov Report

Merging #652 (d2e2af8) into develop (9fe3968) will increase coverage by 0.79%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #652      +/-   ##
===========================================
+ Coverage    46.09%   46.89%   +0.79%     
===========================================
  Files           78       78              
  Lines         3820     3834      +14     
===========================================
+ Hits          1761     1798      +37     
+ Misses        1901     1882      -19     
+ Partials       158      154       -4

Impacted Files	Coverage Δ
pkg/features/features.go	`100.00% <100.00%> (ø)`
cmd/ndm_daemonset/probe/udevprobe.go	`50.19% <0.00%> (+1.97%)`	⬆️
cmd/ndm_daemonset/probe/addhandler.go	`75.42% <0.00%> (+4.09%)`	⬆️
cmd/ndm_daemonset/probe/eventhandler.go	`37.33% <0.00%> (+7.99%)`	⬆️

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 9fe3968...d2e2af8. Read the comment docs.

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin · 2021-10-19T05:51:44Z

+func ValidateDependencies(feature Feature, flags featureFlag) bool {
+	disabled := !flags[feature]
+	if disabled {
+		return false
+	}
+	dependencies := featureDependencies[feature]
+	for _, dependency := range dependencies {
+		missingDependency := !ValidateDependencies(dependency, flags)
+		if missingDependency {
+			flags[feature] = false
+			klog.Infof("Feature %v was set to false due to missing dependency %v", feature, dependency)
+		}
+	}
+	return flags[feature]
+}


This looks good. one thing you could do is to use another map for storing the computed values of the validated dependencies. This way you can store the results and save computation in case the ValidateDependencies function is called again for the same feature flag.

I have added a map for memoized values.

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

jdkramhoft · 2021-10-24T12:52:05Z

Should be working nicely now - I can't see what Better Code Hub wants.

akhilerm · 2021-10-25T06:43:25Z

+
+// Ensures features are disabled if their dependencies are unmet
+// Returns true if a feature is enabled after validation
+func ValidateDependencies(feature Feature, flags featureFlag, memoizedValues featureFlag) bool {


@z0marlin Can you take a look at this function.

akhilerm · 2021-10-25T06:45:39Z

+		missingDependency := !ValidateDependencies(dependency, flags, memoizedValues)
+		if missingDependency {
+			flags[feature] = false
+			klog.Infof("Feature %v was set to false due to missing dependency %v", feature, dependency)


Can we break here, other wise we may set it to false multiple times for every unmet dependency

akhilerm

given a few comments.

Co-authored-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin

LGTM. Thanks for working on this @jdkramhoft :)

z0marlin · 2021-10-26T16:38:52Z

+
+// Ensures features are disabled if their dependencies are unmet
+// Returns true if a feature is enabled after validation
+func ValidateDependencies(feature Feature, flags featureFlag, memoizedValues featureFlag) bool {


akhilerm

LGTM

added dependency support and test cases

f77403c

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

akhilerm requested review from akhilerm and z0marlin October 8, 2021 13:56

gofmt format

be13ba1

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin reviewed Oct 8, 2021

View reviewed changes

jdkramhoft added 3 commits October 12, 2021 10:35

Added nested feature validation

8cd64ab

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

gofmt formatting

921291c

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

simplified range expression

f6b01c4

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

sonatype-lift Bot reviewed Oct 12, 2021

View reviewed changes

Comment thread pkg/features/features.go Outdated

sonatype-lift S1005 fix

6ed3980

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin reviewed Oct 19, 2021

View reviewed changes

Added memoization of computed values during dependency check

39bd5a4

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

akhilerm reviewed Oct 25, 2021

View reviewed changes

Comment thread pkg/features/features.go Outdated

akhilerm reviewed Oct 25, 2021

View reviewed changes

jdkramhoft and others added 2 commits October 25, 2021 09:49

properly ignoring return value

47e2f0a

Co-authored-by: Akhil Mohan <akhilerm@gmail.com> Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

breaking out of dependency checks after first unmet dependency

d2e2af8

Signed-off-by: Jens Daniel Kramhøft <jdkramhoft@gmail.com>

z0marlin approved these changes Oct 26, 2021

View reviewed changes

z0marlin requested a review from akhilerm November 2, 2021 06:50

akhilerm approved these changes Nov 16, 2021

View reviewed changes

akhilerm merged commit 1fcc2b6 into openebs-archive:develop Nov 16, 2021

akhilerm mentioned this pull request Dec 24, 2021

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

Merged

Conversation

jdkramhoft commented Oct 8, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdkramhoft commented Oct 24, 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!

akhilerm left a comment

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!

akhilerm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-commenter commented Oct 12, 2021 •

edited

Loading