Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable leader election by default. #1476

Merged
merged 5 commits into from
Jul 13, 2020
Merged

Enable leader election by default. #1476

merged 5 commits into from
Jul 13, 2020

Conversation

mattmoor
Copy link
Member

This consolidates the core of sharedmain around the new leaderelection logic, which will now be enabled by default.

This can now be disabled with --disable-ha or by passing sharedmain.WithHADisabled(ctx) to sharedmain.MainWithConfig.

/assign @pmorie @vagababov @markusthoemmes

@mattmoor
Enable HA by default.
591ba44 
This consolidates the core of sharedmain around the new leaderelection logic, which will now be **enabled by default**.

This can now be disabled with `--disable-ha` or by passing `sharedmain.WithHADisabled(ctx)` to `sharedmain.MainWithConfig`.
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 10, 2020
@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 10, 2020
vagababov
vagababov reviewed Jul 10, 2020
View reviewed changes
injection/sharedmain/main.go Outdated Show resolved Hide resolved
injection/sharedmain/main.go Outdated Show resolved Hide resolved
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 10, 2020
@mattmoor mattmoor mentioned this pull request Jul 10, 2020
Closed
@mattmoor
Copy link
Member Author

/hold

While I stage things

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 10, 2020
@mattmoor mattmoor mentioned this pull request Jul 10, 2020
Merged
@mattmoor
Copy link
Member Author

Downstream serving PR: knative/serving#8602
Downstream eventing PR: knative/eventing#3564

vagababov
vagababov reviewed Jul 11, 2020
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgmt
/hold
if you want to upgrade to issue number.

injection/sharedmain/main.go
controllers, _ := ControllersAndWebhooksFromCtors(ctx, cmw, ctors...)
WatchLoggingConfigOrDie(ctx, cmw, logger, atomicLevel, component)
WatchObservabilityConfigOrDie(ctx, cmw, profilingHandler, logger, component)
// TODO(mattmoor): Remove this once HA is stable.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue rather?

@mattmoor mattmoor mentioned this pull request Jul 11, 2020
Merged
@vagababov
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2020
yanweiguo
yanweiguo reviewed Jul 11, 2020
View reviewed changes
Copy link
Contributor

@yanweiguo yanweiguo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

I guess the title should be Enable leader election by default?

injection/sharedmain/main.go
func WebhookMainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) {
// MainWithConfig runs the generic main flow for controllers and webhooks
// with the given config.
func MainWithConfig(ctx context.Context, component string, cfg *rest.Config, ctors ...injection.ControllerConstructor) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If leader election mode is enabled, before this change:

  • webhook is running on bucket level leader election mode.
  • other controllers are running on pod level leader election mode.

After this change, all controllers are running on bucket level leader election mode. Is my understading right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@mattmoor mattmoor changed the title Enable HA by default. Enable leader election by default. Jul 11, 2020
@mattmoor
Move the configmap watcher startup.
7339b84 
This race was uncovered by the chaos duck on knative/serving!  When we have enabled a feature flag, e.g. multi-container, and the webhook pods are restarted, there is a brief window where the webhook is up and healthy before the configmaps have synchronized and the new webhook pod realizes the feature is enabled.
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2020
@mattmoor
Copy link
Member Author

I pushed one additional change that was uncovered by the chaos testing downstream on serving. With this, I've had at least one clean run of serving (rerunning now) and just pushed it to the eventing PR.

vagababov
vagababov reviewed Jul 11, 2020
View reviewed changes
Copy link
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2020
@vaikas
Copy link
Contributor

vaikas commented Jul 13, 2020

/lgtm
/approve

/hold
Just a hold if you want to address the nit, up to you and remove the hold if you want to address later/never :)

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, vaikas

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2020
@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-pkg-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
leaderelection/config.go 80.0% 78.1% -1.9

@vagababov
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 13, 2020
@mattmoor
Copy link
Member Author

/hold cancel

I am going to land this now, and after lunch I'll start rebasing and landing the downstream PRs.

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 13, 2020
@knative-prow-robot knative-prow-robot merged commit a817277 into knative:master Jul 13, 2020
@mattmoor mattmoor deleted the single-sharedmain branch July 13, 2020 19:53
@Harwayne Harwayne mentioned this pull request Aug 11, 2020
Merged
@nishpa214 nishpa214 mentioned this pull request Sep 2, 2020
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanweiguo yanweiguo yanweiguo left review comments

@vaikas vaikas vaikas left review comments

@vagababov vagababov vagababov left review comments

@markusthoemmes markusthoemmes Awaiting requested review from markusthoemmes

@n3wscott n3wscott Awaiting requested review from n3wscott

Assignees

@pmorie pmorie

@vagababov vagababov

@markusthoemmes markusthoemmes

@vaikas vaikas

@yanweiguo yanweiguo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mattmoor @vagababov @vaikas @knative-prow-robot @knative-metrics-robot @yanweiguo @pmorie @markusthoemmes @googlebot