controllers: refactor code and start informers first, fix template/render race #482

runcom · 2019-02-23T13:02:52Z

This patch does various things, all related:

start the informers for controllers before running them to follow what
https://github.com/kubernetes/sample-controller/blob/master/main.go#L64-L71
does and to avoid races already spotted in unit tests
(pkg/controller: start informers before populating store in tests #457)
move the clients builder code from cmd/common to a new package just for that
under the new internal/ folder so nobody but us can use that
move common controllers code under pkg/controller/common to be reused
create an e2e only clientset, avoiding us to type client version
everytime (eg client.CoreV1()..). This is a need cause if in the future
the api change, we don't play grep for a week replacing old apis...
fix template/render controllers race

Closes: #338
Closes: #385

Signed-off-by: Antonio Murdaca [email protected]

runcom · 2019-02-23T20:11:45Z

/retest

cgwalters · 2019-03-04T19:09:49Z

I think with stuff like this it'd help me a lot to have a "reference operator" codebase. There's the kube sample controller but our code is different enough I'm not sure if it's doing the same thing.

runcom · 2019-03-06T09:51:35Z

I think with stuff like this it'd help me a lot to have a "reference operator" codebase. There's the kube sample controller but our code is different enough I'm not sure if it's doing the same thing.

cool, and from that example operator it looks like informers are indeed started before the controller itself https://github.com/kubernetes/sample-controller/blob/master/main.go#L64-L71 - I've rebased this as well

/assign @LorbusChris

runcom · 2019-03-17T14:39:48Z

/retest

runcom · 2019-03-17T19:06:32Z

This has been updated (re-read the summary in the first comment) and does fix #338 as well (by waiting on the template controller 🎉 ):

I0317 16:27:21.495296       1 node_controller.go:383] Pool master is unconfigured, pausing 5s for renderer to initialize
I0317 16:27:21.495462       1 node_controller.go:383] Pool worker is unconfigured, pausing 5s for renderer to initialize
I0317 16:27:21.495537       1 render_controller.go:380] Error syncing machineconfigpool master: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.495584       1 render_controller.go:380] Error syncing machineconfigpool worker: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.501140       1 render_controller.go:380] Error syncing machineconfigpool master: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.501180       1 render_controller.go:380] Error syncing machineconfigpool worker: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.511368       1 render_controller.go:380] Error syncing machineconfigpool master: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.511407       1 render_controller.go:380] Error syncing machineconfigpool worker: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.531622       1 render_controller.go:380] Error syncing machineconfigpool master: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.531647       1 render_controller.go:380] Error syncing machineconfigpool worker: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.571897       1 render_controller.go:380] Error syncing machineconfigpool worker: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.571897       1 render_controller.go:380] Error syncing machineconfigpool master: ControllerConfig has not completed: completed(false) running(false) failing(true)
I0317 16:27:21.671461       1 render_controller.go:500] Generated machineconfig worker-b058168e9ec42a1704343e84bb2dc7eb from 4 configs: [{MachineConfig  00-worker  machineconfiguration.openshift.io/v1  } {MachineConfig  00-worker-ssh  machineconfiguration.openshift.io/v1  } {MachineConfig  01-worker-container-runtime  machineconfiguration.openshift.io/v1  } {MachineConfig  01-worker-kubelet  machineconfiguration.openshift.io/v1  }]
I0317 16:27:21.684235       1 render_controller.go:500] Generated machineconfig master-34908aca2b6075843cc77cd59449a0df from 4 configs: [{MachineConfig  00-master  machineconfiguration.openshift.io/v1  } {MachineConfig  00-master-ssh  machineconfiguration.openshift.io/v1  } {MachineConfig  01-master-container-runtime  machineconfiguration.openshift.io/v1  } {MachineConfig  01-master-kubelet  machineconfiguration.openshift.io/v1  }]

runcom · 2019-03-17T19:08:50Z

@cgwalters @kikisdeliveryservice @LorbusChris ptal

This patch does various things, all related: - start the informers for controllers before running them to follow what https://github.com/kubernetes/sample-controller/blob/master/main.go#L64-L71 does and to avoid races already spotted in unit tests (openshift#457) - move the clients builder code from cmd/common to a new package just for that under the new internal/ folder so nobody but us can use that - move common controllers code under pkg/controller/common to be reused - create an e2e only clientset, avoiding us to type client version everytime (eg client.CoreV1()..). This is a need cause if in the future the api change, we don't play grep for a week replacing old apis... Signed-off-by: Antonio Murdaca <[email protected]>

The template controller is responsible for generating the initial MCs but the render controller can kick in before the template is done. Add a sync mechanism by looking at the controller config status as that's the souce of truth to understand if a sync is done if the controller config changes and the template controller runs again. Signed-off-by: Antonio Murdaca <[email protected]>

Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2019-03-18T07:15:42Z

/retest

kikisdeliveryservice · 2019-03-18T18:07:54Z

LGTM! Really like some of the changes here, will leave it to @cgwalters / @LorbusChris to give it one more set of 👀

cgwalters

/lgtm

cgwalters · 2019-03-18T18:18:18Z

pkg/controller/render/render_controller.go

+	}
+
+	if cur.Generation != cur.Status.ObservedGeneration {
+		return fmt.Errorf("status for ControllerConfig %s is being reported for %d, expecting it for %d", cc[0].GetName(), cur.Status.ObservedGeneration, cur.Generation)


Feels like this isn't an error but eh.

openshift-ci-robot · 2019-03-18T18:18:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-03-18T19:00:08Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-18T21:01:04Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-03-18T23:02:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot requested review from abhinavdahiya and cgwalters February 23, 2019 13:02

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 23, 2019

runcom force-pushed the start-informeres-first branch from 3194d28 to 21563df Compare February 24, 2019 22:55

openshift-ci-robot assigned LorbusChris Mar 6, 2019

runcom force-pushed the start-informeres-first branch 2 times, most recently from a12e170 to 4b7aaf4 Compare March 6, 2019 12:05

runcom force-pushed the start-informeres-first branch 2 times, most recently from 08f1f4a to 33edd4d Compare March 17, 2019 13:37

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 17, 2019

runcom changed the title ~~controller_context: start informers first~~ controllers: refactor code and start informers first Mar 17, 2019

runcom force-pushed the start-informeres-first branch from 33edd4d to bd3e2e8 Compare March 17, 2019 15:11

This was referenced Mar 17, 2019

controller: Wait for template and kubelet before starting renderer #385

Closed

fix initial race between template and render sub-controllers #338

Closed

runcom force-pushed the start-informeres-first branch 3 times, most recently from 17df599 to d65d8a5 Compare March 17, 2019 16:08

runcom changed the title ~~controllers: refactor code and start informers first~~ controllers: refactor code and start informers first, fix template/render race Mar 17, 2019

runcom force-pushed the start-informeres-first branch from c3bb896 to a2ad129 Compare March 17, 2019 22:33

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 17, 2019

runcom added 2 commits March 18, 2019 00:59

move common api method to the apis pkg

1e381d2

Signed-off-by: Antonio Murdaca <[email protected]>

runcom force-pushed the start-informeres-first branch from a2ad129 to 1e381d2 Compare March 17, 2019 23:59

cgwalters reviewed Mar 18, 2019

View reviewed changes

openshift-ci-robot assigned cgwalters Mar 18, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 18, 2019

openshift-merge-robot merged commit 782e8e7 into openshift:master Mar 19, 2019

controllers: refactor code and start informers first, fix template/render race #482

controllers: refactor code and start informers first, fix template/render race #482

Uh oh!

Conversation

runcom commented Feb 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Feb 23, 2019

Uh oh!

cgwalters commented Mar 4, 2019

Uh oh!

runcom commented Mar 6, 2019

Uh oh!

runcom commented Mar 17, 2019

Uh oh!

runcom commented Mar 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Mar 17, 2019

Uh oh!

runcom commented Mar 18, 2019

Uh oh!

kikisdeliveryservice commented Mar 18, 2019

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters Mar 18, 2019

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Mar 18, 2019

Uh oh!

openshift-bot commented Mar 18, 2019

Uh oh!

openshift-bot commented Mar 18, 2019

Uh oh!

openshift-bot commented Mar 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

runcom commented Feb 23, 2019 •

edited

Loading

runcom commented Mar 17, 2019 •

edited

Loading