-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
panic in featuregate if a requested feature is unknown #84865
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,8 +32,10 @@ import ( | |
"github.com/Azure/go-autorest/autorest/azure" | ||
|
||
v1 "k8s.io/api/core/v1" | ||
utilruntime "k8s.io/apimachinery/pkg/util/runtime" | ||
"k8s.io/apimachinery/pkg/util/sets" | ||
"k8s.io/apimachinery/pkg/util/wait" | ||
utilfeature "k8s.io/apiserver/pkg/util/feature" | ||
"k8s.io/client-go/informers" | ||
clientset "k8s.io/client-go/kubernetes" | ||
"k8s.io/client-go/kubernetes/scheme" | ||
|
@@ -202,6 +204,8 @@ type Cloud struct { | |
metadata *InstanceMetadataService | ||
vmSet VMSet | ||
|
||
// ipv6DualStack allows overriding for unit testing. It's normally initialized from featuregates | ||
ipv6DualStackEnabled bool | ||
// Lock for access to node caches, includes nodeZones, nodeResourceGroups, and unmanagedNodes. | ||
nodeCachesLock sync.Mutex | ||
// nodeZones is a mapping from Zone to a sets.String of Node's names in the Zone | ||
|
@@ -271,6 +275,19 @@ func NewCloud(configReader io.Reader) (cloudprovider.Interface, error) { | |
unmanagedNodes: sets.NewString(), | ||
routeCIDRs: map[string]string{}, | ||
} | ||
func() { | ||
// this allows the code ot launch without featuregates defined. It is currently required for unit tests where the | ||
// featuregates are not registered. This is effectively coding by side effect and an explicit register should | ||
// be eventually created instead. | ||
defer func() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you split the NewCloud function so that test code doesn't have to execute this, do you still need to catch the panic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
And let it straight die instead? That's not consistent with existing behavior. I'm honestly unsure if this ever worked properly when it really ran. Are you sure you want to change behavior? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yup, that's the goal of the change, isn't it? :) |
||
if r := recover(); r != nil { | ||
utilruntime.HandleError(fmt.Errorf("%v", r)) | ||
} | ||
}() | ||
|
||
az.ipv6DualStackEnabled = utilfeature.DefaultFeatureGate.Enabled(IPv6DualStack) | ||
}() | ||
|
||
err = az.InitializeCloudFromConfig(config, false) | ||
if err != nil { | ||
return nil, err | ||
|
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.
open a follow up issue for cloud-controller-manager/providers to resolve this?