Bug 1508061: Fix panic during openshift controller options initialization#17127
Conversation
mfojtik
added
kind/bug
openshift-ci-robot
added
the
size/S
openshift-merge-robot
added
the
approved
mfojtik
force-pushed
the
fix-initialization-panic
branch
from
a29cfa5 to
dfbd95f
Compare
pkg/cmd/server/start/start_master.go
Outdated
| // Wait for the runEmbeddedKubeControllerManager to mutate the | ||
| // ControllerArguments to prevent getOpenshiftControllerOptions from | ||
| // panicking because of concurrent read/write access. | ||
| <-controllerArgumentsInitialized |
There was a problem hiding this comment.
this smells. Do the openshift controller really have to see the mutated flags? Or was this only an accident?
|
Alternative without chan: mfojtik@4ce220b |
mfojtik
force-pushed
the
fix-initialization-panic
branch
from
dfbd95f to
3b3a5d6
Compare
openshift-ci-robot
added
size/XS
|
@sttts updated. |
|
I pretty much prefer the deep-copy variant over the channel and the |
|
Waiting for tests.... |
|
/retest |
mfojtik
force-pushed
the
fix-initialization-panic
branch
from
3b3a5d6 to
36e4f29
Compare
openshift-ci-robot
added
size/S
| for _, item := range v { | ||
| newVal = append(newVal, item) | ||
| } | ||
| cmdLineArgs[k] = newVal |
There was a problem hiding this comment.
nit: append([]string{}, v...)
mfojtik
force-pushed
the
fix-initialization-panic
branch
from
36e4f29 to
8c7ba88
Compare
openshift-ci-robot
added
size/XS
|
/lgtm |
|
/lgtm |
| func newKubeControllerManager(kubeconfigFile, saPrivateKeyFile, saRootCAFile, podEvictionTimeout, recyclerImage string, dynamicProvisioningEnabled bool, cmdLineArgs map[string][]string) (*controlleroptions.CMServer, []func(), error) { | ||
| if cmdLineArgs == nil { | ||
| cmdLineArgs = map[string][]string{} | ||
| func newKubeControllerManager(kubeconfigFile, saPrivateKeyFile, saRootCAFile, podEvictionTimeout, recyclerImage string, dynamicProvisioningEnabled bool, controllerArgs map[string][]string) (*controlleroptions.CMServer, []func(), error) { |
There was a problem hiding this comment.
same issue exists in newScheduler
There was a problem hiding this comment.
@liggitt i don't think getOpenshiftControllerOptions iterate over m.config.KubernetesMasterConfig.SchedulerArguments so we are probably safe there but yeah, we should deep copy to be sure there as well. I will update this PR
mfojtik
force-pushed
the
fix-initialization-panic
branch
from
8c7ba88 to
9b2f23d
Compare
openshift-ci-robot
added
size/S
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, liggitt, mfojtik, simo5 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
Automatic merge from submit-queue. |
9 participants
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1508061
Basically
newKubeControllerManagermutates the cmdLineArgs in parallel togetOpenshiftControllerOptionswhich is trying to read it.@deads2k @sttts PTAL, i consider this 3.7 blocker.