-
Notifications
You must be signed in to change notification settings - Fork 174
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
Introducing FeatureGates and global feature config #344
Conversation
cmd/sriovdp/main.go
Outdated
@@ -33,6 +35,8 @@ func flagInit(cp *cliParams) { | |||
"JSON device pool config file location") | |||
flag.StringVar(&cp.resourcePrefix, "resource-prefix", "intel.com", | |||
"resource name prefix used for K8s extended resource") | |||
flag.StringVar(&cp.featureGates, "feature-gates", "", | |||
"resource name prefix used for K8s extended resource") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update cmd description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cmd/sriovdp/main.go
Outdated
@@ -57,6 +61,25 @@ func main() { | |||
glog.Fatalf("Exiting.. one or more invalid configuration(s) given") | |||
return | |||
} | |||
|
|||
// Read global config | |||
if err := config.Config.ReadConfig(cp.configFile); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add an example of what the configMap looks like when featureGates
data entry is added?
It looks like in the ReadConfig function, it is re-using the same cli cmd configFile
which points to the resourceList data entry in configMap, but the umarshalling inside ReadConfig is failing when there is a resourceList data entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the example as a part of manager_test, just for now:
"featureGates": { |
pkg/config/config.go
Outdated
} | ||
|
||
// ReadConfig loads config | ||
func (cfg *config) ReadConfig(configFile string) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a readConfig
in manager.go file to parse the resourceList
data in configMap.
I think perhaps we want to combine both resourceList
and featureGates
to the same readConfig
function and make it a sub-element of resourceManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I intended, was to make this config as independent of the existing 'manager config' as possible as it is still yet to be decided if we would like to use one big global config, or for example separate the device-discovery-related config and feature-related config (which I think is optional by it's own nature).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipatrykx I don't see where each features default "maturity" is located. My understanding is the user can toggle this via CLI or CM but where is the default? Thanks! Never mind, its for future features!
@@ -21,6 +21,8 @@ import ( | |||
"syscall" | |||
|
|||
"github.com/golang/glog" | |||
"github.com/k8snetworkplumbingwg/sriov-network-device-plugin/pkg/config" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Separate imports according to convention [1]
- Native
(space) - Local
(space) - Remote
[1] https://github.com/golang/go/wiki/CodeReviewComments#imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/config/config.go
Outdated
) | ||
|
||
// Config is global config object | ||
var Config *config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a fan of globals unless necessary because when I was reading the 'main' file I had to go searching up the code to see where it was defined and then I see your new package addition is named config and guessed it was there.
I much rather have main call your newConfig
func and then having a GetConfig function (with a check to make sure non-nil config, return error if it is). At least then, folks can clearly see where config is defined and coming from. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed that to use getters. Also, after thinking about this I've decided that NewConfig()
and NewFeatureGate()
will not return the pointer to the config/fg but the usage of getter will be forced, so it should be clearly visible that this object is instantiated inside the package.
|
||
fgMap := make(map[string]bool) | ||
|
||
if err = json.Unmarshal(allCfg["featureGates"], &fgMap); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't you check if "featureGates" key is in the map before indexing? Potential panic if it isn't available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/features/features.go
Outdated
) | ||
|
||
// FeatureGate is feature gate 'manager' to be used | ||
var FeatureGate *featureGate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, not a fan of globals. Can we use a Getter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
pkg/features/features.go
Outdated
|
||
var defaultSriovDpFeatureGates = map[feature]featureSpec{ | ||
AlphaFeature: {Default: false, Maturity: Alpha}, | ||
BetaFeature: {Default: true, Maturity: Beta}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ipatrykx Beta to me seems redundant. I know the difference between Beta and GA in k8 is a feature cannot be turned off by default in GA, where as it can be in Beta. Seems like needless complexity for this software to have Beta.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed beta
pkg/features/features.go
Outdated
) | ||
|
||
// Feature name | ||
type feature string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a blocker, but not a fan of aliasing base type string like this. WDYT? I think it makes it harder to read the code. Same with maturity above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought back then that it will make code more readable, but after thinking about this, changed that to use just string.
pkg/features/features.go
Outdated
return fmt.Errorf("Feature %s is not supported", f) | ||
} | ||
fg.enabled[f] = status | ||
if status == true && fg.knownFeatures[f].Maturity == Deprecated { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant comparison to true. Also, check if key in knownFeatures before indexing map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean that is thousl be just `if status &&'?
As for the key checking, it's done in isFeatureSupported()
which is called earlier in the code.
pkg/features/features.go
Outdated
// SetFromMap sets the enablement status of featuers accordig to a map | ||
func (fg *featureGate) SetFromMap(valuesToSet map[string]bool) error { | ||
for k, v := range valuesToSet { | ||
err := fg.set(feature(k), v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you should wrap your if statement around the set:
if err := fg.set.... err != nil {
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/features/features.go
Outdated
} | ||
|
||
// SetFromString converts config string to map and sets the enablement status of the selected features | ||
// copied form k8s and slightly changed - TBC? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
form - > from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
pkg/features/features.go
Outdated
func (fg *featureGate) SetFromString(value string) error { | ||
featureMap := make(map[string]bool) | ||
for _, s := range strings.Split(value, ",") { | ||
if len(s) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of that, but using flags we will have to add a flag for every feature introduced, so decided that it will be better to do this the same way the k8s handles that (as far as I remember). Would like to hear more input on this from other though.
Hi, @martinkennelly, @zshi-redhat, @adrianchiris is it possible we could move this thing further? Also tagging @Eoghan1232 who was interested. |
a57ff7b
to
f478c93
Compare
Pull Request Test Coverage Report for Build 3345559109Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Hi all! |
Is this PR still relevant or we can close it? |
closing this one |
Regards discussion: #321
This PR introduces FeatureGates mechanism similar to one available in Kubernetes, that could be used by users to disable/enable features provided by SR-IOV DP. Additionally added global config structure that could hold configuration of features introduced in the future.