Convert to controller runtime runnables #4202

sunjayBhatia · 2021-11-30T23:18:43Z

controller runtime manages controllers and other runnables (previously
goroutines added to the server workgroup)
leader election now uses a combination of configmap and lease objects,
in a future release we can move to just leases
controllers and other components are currently not dependent on leader election
other components (status writers) are set to be dependent on leader
election
adds to the leaderelection e2e tests to check on leaderelection
configmap in addition to new lease resource and events

- controller runtime manages controllers and other runnables (previously goroutines added to the server workgroup) - leader election now uses a combination of configmap and lease objects, in a future release we can move to just leases - controllers and other components are currently not dependent on leader election - other components (status writers) are set to be dependent on leader election - adds to the leaderelection e2e tests to check on leaderelection configmap in addition to new lease resource and events Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2021-11-30T23:19:18Z

overrides #4157

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

codecov · 2021-11-30T23:27:07Z

Codecov Report

Merging #4202 (975532e) into main (7bf2309) will increase coverage by 0.07%.
The diff coverage is 28.34%.

@@            Coverage Diff             @@
##             main    #4202      +/-   ##
==========================================
+ Coverage   76.52%   76.59%   +0.07%     
==========================================
  Files         111      114       +3     
  Lines        9869     9999     +130     
==========================================
+ Hits         7552     7659     +107     
+ Misses       2150     2149       -1     
- Partials      167      191      +24

Impacted Files	Coverage Δ
cmd/contour/serve.go	`12.01% <1.56%> (-0.49%)`	⬇️
internal/k8s/status.go	`22.64% <10.52%> (+4.45%)`	⬆️
internal/controller/mocks/manager.go	`12.90% <12.90%> (ø)`
internal/controller/gatewayclass.go	`15.65% <33.33%> (+15.65%)`	⬆️
internal/controller/gateway.go	`12.86% <37.50%> (+12.86%)`	⬆️
internal/controller/httproute.go	`26.92% <42.85%> (+26.92%)`	⬆️
internal/controller/tlsroute.go	`26.92% <42.85%> (+26.92%)`	⬆️
cmd/contour/ingressstatus.go	`40.00% <50.00%> (+5.62%)`	⬆️
internal/debug/debug.go	`9.09% <50.00%> (+9.09%)`	⬆️
internal/contour/metrics.go	`68.96% <69.23%> (+0.33%)`	⬆️
... and 11 more

sunjayBhatia · 2021-11-30T23:34:19Z

internal/controller/controller_test.go

+	for name, test := range tests {
+		t.Run(name, func(t *testing.T) {
+			mockManager := &mocks.Manager{}
+


curious what people think about these tests, they ensure the controller is registered and that it does not require leader election, using a generated mock manager (see mock package and go generate command above)

sunjayBhatia · 2021-11-30T23:48:03Z

internal/contour/handler_test.go

+	"github.com/stretchr/testify/require"
+)
+
+func TestEventHandlerNotRequireLeaderElection(t *testing.T) {


curious what people think of these tests for the various components, settled on this after spiking through another option which i wonder what people think of

new object as thin wrapper around controller-runtime manager

mainly for testing, can test by mocking controller-runtime manager (see controller tests for example)

instead of manager.Add(Runnable) method, have something like manager.Add(Runnable, requireLeaderElection bool)

decided against this for a few reasons, mainly bc it ended up being a v thin wrapper (need to expose a lot of things from the base controller-runtime manager) and puts more logic in the main package, rather than closer to the components themselves which should have the knowledge about whether they need leader election or not

skriss

Handful of comments, but overall this is looking nice!

examples/contour/02-role-contour.yaml

hack/actions/install-kubernetes-toolchain.sh

internal/controller/controller.go

internal/debug/debug.go

internal/httpsvc/http.go

cmd/contour/serve.go

internal/featuretests/v3/featuretests.go

…aderelection Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

examples/contour/02-role-contour.yaml

internal/controller/controller.go

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

stevesloka

/lgtm with @skriss's comments. Nice work @sunjayBhatia!

internal/controller/gateway.go

…aderelection Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

skriss · 2021-12-03T15:51:25Z

@sunjayBhatia I was playing with this locally and had a couple issues:

proxies created after my local Contour acquires leadership do not seem to trigger a DAG rebuild/get status
the process is not shutting down cleanly on SIGTERM

I imagine the first is related to the E2E failures. I haven't had a chance to dig into these any further yet, just wanted to put the reports here. I'll look some more later this AM.

sunjayBhatia · 2021-12-03T16:01:52Z

@sunjayBhatia I was playing with this locally and had a couple issues:

proxies created after my local Contour acquires leadership do not seem to trigger a DAG rebuild/get status

the process is not shutting down cleanly on SIGTERM

I imagine the first is related to the E2E failures. I haven't had a chance to dig into these any further yet, just wanted to put the reports here. I'll look some more later this AM.

yep, same issues here, e2e tests do some capturing of the issues but a little worried about other components besides just the main status updater (e.g load balancer status updater), needs a little more work

sunjayBhatia · 2021-12-03T16:15:46Z

Contour server failed" error="failed waiting for all runnables to end within grace period of 30s: context deadline exceeded on shutdown, could adjust to not use graceful shutdown as well

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2021-12-03T16:56:36Z

Contour server failed" error="failed waiting for all runnables to end within grace period of 30s: context deadline exceeded on shutdown, could adjust to not use graceful shutdown as well

i think fixing my silly channels mistake also helped with shutdown issues

skriss · 2021-12-03T17:07:22Z

Yep looks better for me

skriss

One thought on the tests, otherwise I think this looks good, doing some ad-hoc testing locally too

internal/k8s/status_test.go

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

internal/leadership/notifier.go

skriss

👍 local testing all looks good to me, as does the new leader election notification abstraction

internal/leadership/notifier.go

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia · 2021-12-03T21:40:13Z

Tests need a small fix to make them more robust now we’re deleting contour and restarting in incluster suite

Also check new leader pod exists Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

skriss

LGTM, not sure if there were any other outstanding threads here though

sunjayBhatia · 2021-12-06T20:44:45Z

LGTM, not sure if there were any other outstanding threads here though

i think we got everything!

sunjayBhatia requested a review from a team as a code owner November 30, 2021 23:18

sunjayBhatia requested review from tsaarni and stevesloka and removed request for a team November 30, 2021 23:18

sunjayBhatia added the release-note/minor A minor change that needs about a paragraph of explanation in the release notes. label Nov 30, 2021

sunjayBhatia requested review from skriss and youngnick November 30, 2021 23:20

sunjayBhatia added 2 commits November 30, 2021 23:21

Linter

0feb9b2

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

makefile go generate

0bcabad

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia commented Nov 30, 2021

View reviewed changes

sunjayBhatia mentioned this pull request Dec 1, 2021

WIP: Moving to controller-runtime leader election and runnables #4157

Closed

skriss requested changes Dec 1, 2021

View reviewed changes

Merge remote-tracking branch 'origin/main' into controller-runtime-le…

6594d53

…aderelection Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

stevesloka reviewed Dec 1, 2021

View reviewed changes

examples/contour/02-role-contour.yaml Show resolved Hide resolved

internal/controller/controller.go Show resolved Hide resolved

sunjayBhatia added 3 commits December 1, 2021 17:27

Move mockery to tools.go

523d33c

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Fix mockery invocation

6cb14c8

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Comments

dd6f68f

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia mentioned this pull request Dec 1, 2021

Switch to Role for leaderelection resource RBAC #4204

Merged

stevesloka approved these changes Dec 1, 2021

View reviewed changes

skriss mentioned this pull request Dec 1, 2021

drop internal/workgroup package #4206

Closed

sunjayBhatia commented Dec 1, 2021

View reviewed changes

internal/controller/gateway.go Show resolved Hide resolved

Merge remote-tracking branch 'origin/main' into controller-runtime-le…

640b248

…aderelection Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia force-pushed the controller-runtime-leaderelection branch 2 times, most recently from 5dc4c52 to 640b248 Compare December 2, 2021 22:17

Bring back event handler update on leader elect

9525e0a

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

fix some silly mistakes

be75e8a

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

skriss reviewed Dec 3, 2021

View reviewed changes

internal/k8s/status_test.go Show resolved Hide resolved

sunjayBhatia added 2 commits December 3, 2021 19:12

Use new leadership notifier abstraction

602f8f7

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

linter

706dcbb

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

sunjayBhatia commented Dec 3, 2021

View reviewed changes

internal/leadership/notifier.go Show resolved Hide resolved

skriss approved these changes Dec 3, 2021

View reviewed changes

internal/leadership/notifier.go Show resolved Hide resolved

sunjayBhatia added 2 commits December 3, 2021 20:41

delete/recreate contour before each incluster test

4ae9b17

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Changelog

3b56b43

Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

Use eventually for fetching original pod name

975532e

Also check new leader pod exists Signed-off-by: Sunjay Bhatia <sunjayb@vmware.com>

skriss approved these changes Dec 6, 2021

View reviewed changes

sunjayBhatia merged commit 801c194 into projectcontour:main Dec 6, 2021

sunjayBhatia deleted the controller-runtime-leaderelection branch December 6, 2021 20:46

sunjayBhatia mentioned this pull request Dec 6, 2021

Update rbac for changes to leaderelection projectcontour/contour-operator#470

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert to controller runtime runnables #4202

Convert to controller runtime runnables #4202

sunjayBhatia commented Nov 30, 2021

sunjayBhatia commented Nov 30, 2021

codecov bot commented Nov 30, 2021 •

edited

Loading

sunjayBhatia Nov 30, 2021

sunjayBhatia Nov 30, 2021

skriss left a comment

stevesloka left a comment

skriss commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

skriss commented Dec 3, 2021

skriss left a comment

skriss left a comment

sunjayBhatia commented Dec 3, 2021

skriss left a comment

sunjayBhatia commented Dec 6, 2021

Convert to controller runtime runnables #4202

Convert to controller runtime runnables #4202

Conversation

sunjayBhatia commented Nov 30, 2021

sunjayBhatia commented Nov 30, 2021

codecov bot commented Nov 30, 2021 • edited Loading

Codecov Report

sunjayBhatia Nov 30, 2021

Choose a reason for hiding this comment

sunjayBhatia Nov 30, 2021

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

stevesloka left a comment

Choose a reason for hiding this comment

skriss commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

sunjayBhatia commented Dec 3, 2021

skriss commented Dec 3, 2021

skriss left a comment

Choose a reason for hiding this comment

skriss left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Dec 3, 2021

skriss left a comment

Choose a reason for hiding this comment

sunjayBhatia commented Dec 6, 2021

codecov bot commented Nov 30, 2021 •

edited

Loading