Skip to content

BUGFIX: cainjector leaderelection defaults are missing #6816

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
Merged
merged 2 commits into from
Mar 6, 2024
Merged

BUGFIX: cainjector leaderelection defaults are missing #6816

merged 2 commits into from
Mar 6, 2024

Conversation

inteon
Copy link
Member

@inteon inteon commented Mar 5, 2024 •
edited
Loading

In #6345, I forgot to add the defaults for the cainjector leaderelection options.

The original defaults can be found here: https://github.com/cert-manager/cert-manager/blob/ba6268135d0f2ebea5f8eb29a57934f1acae15e2/internal/cmd/util/defaults.go.

Fixes #6808

$ _bin/server/cainjector-linux-amd64 --help
...
      --leader-elect                                        If true, cainjector will perform leader election between instances to ensure no more than one instance of cainjector operates at a time (default true)
      --leader-election-lease-duration duration             The duration that non-leader candidates will wait after observing a leadership renewal until attempting to acquire leadership of a led but unrenewed leader slot. This is effectively the maximum duration that a leader can be stopped before it is replaced by another candidate. This is only applicable if leader election is enabled. (default 1m0s)
      --leader-election-namespace string                    Namespace used to perform leader election. Only used if leader election is enabled (default "kube-system")
      --leader-election-renew-deadline duration             The interval between attempts by the acting master to renew a leadership slot before it stops leading. This must be less than or equal to the lease duration. This is only applicable if leader election is enabled. (default 40s)
      --leader-election-retry-period duration               The duration the clients should wait between attempting acquisition and renewal of a leadership. This is only applicable if leader election is enabled. (default 15s)
...

$ k -n cert-manager logs cert-manager-cainjector-7f8bfb6865-d2hkn | grep leader
I0306 09:32:53.829792       1 leaderelection.go:250] attempting to acquire leader lease kube-system/cert-manager-cainjector-leader-election...
I0306 09:32:53.843439       1 leaderelection.go:260] successfully acquired lease kube-system/cert-manager-cainjector-leader-election
I0306 09:32:53.843613       1 recorder.go:104] "cert-manager-cainjector-7f8bfb6865-d2hkn_845b6bd8-82ae-4dbf-a0e9-a2cee4006852 became leader" logger="cert-manager.events" type="Normal" object={"kind":"Lease","namespace":"kube-system","name":"cert-manager-cainjector-leader-election","uid":"7b45984d-5a05-427f-b482-9d9bb9cad161","apiVersion":"coordination.k8s.io/v1","resourceVersion":"795"} reason="LeaderElection"

Kind

/kind bug

Release Note

BUGFIX: cainjector leaderelection flag/ config option defaults are missing

@inteon
BUGFIX: cainjector leaderelection defaults were missing
620d6ff 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@jetstack-bot jetstack-bot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 5, 2024
@inteon
Copy link
Member Author

inteon commented Mar 5, 2024

/cherrypick release-1.14

@jetstack-bot
Copy link
Contributor

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.14 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@inteon inteon requested a review from wallrj March 5, 2024 18:24
@inteon
prevent fuzzer from generating impossible configurations
ad1847c 
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
@inteon
Copy link
Member Author

inteon commented Mar 5, 2024

/retest

wallrj
wallrj requested changes Mar 6, 2024
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

  • Please demonstrate that you've tested this.
  • Show that the command line --help displays these defaults.
  • Consider documenting the defaults in https://cert-manager.io/docs/reference/api-docs/#cainjector.config.cert-manager.io/v1alpha1.LeaderElectionConfig (here or in a future PR)
  • Write an issue with some ideas about how we can prevent the leader election being accidentally disabled in future. Perhaps there should be an E2E test which checks the status of the lease resource? Or perhaps there should be a cmctl check deployment command which we could run in the E2E test setup?

@inteon inteon mentioned this pull request Mar 6, 2024
Closed
@inteon inteon requested a review from wallrj March 6, 2024 09:37
@inteon inteon mentioned this pull request Mar 6, 2024
Closed
wallrj
wallrj approved these changes Mar 6, 2024
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Thanks for updating the PR description and creating those followup issues.

I haven't run this locally, but I'll run the best-practice E2E tests which runs 2 replicas of the cainjector and take a look at the logs of the leader and non-leader.

/test pull-cert-manager-master-e2e-v1-28-bestpractice-install

@jetstack-bot jetstack-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2024
wallrj
wallrj approved these changes Mar 6, 2024
View reviewed changes
Copy link
Member

@wallrj wallrj left a comment

Choose a reason for hiding this comment

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

Ok. Leader election does seem to be working. The leader logs that it got the lease and creates an event.

Leader

2024-03-06T10:03:44.835697696Z stderr F I0306 10:03:44.835555       1 leaderelection.go:250] attempting to acquire leader lease kube-system/cert-manager-cainjector-leader-election...
2024-03-06T10:03:44.846209174Z stderr F I0306 10:03:44.844625       1 leaderelection.go:260] successfully acquired lease kube-system/cert-manager-cainjector-leader-election
...
2024-03-06T10:03:44.846438134Z stderr F I0306 10:03:44.845959       1 recorder.go:104] "cert-manager-cainjector-569fb9bb86-7tn59_39ca634f-499b-4eb8-bb0c-7703731830aa became leader" logger="cert-manager.events" type="Normal" object={"kind":"Lease","namespace":"kube-system","name":"cert-manager-cainjector-leader-election","uid":"0a30f665-0ba4-4aa5-9c8e-eb4c9f7292e4","apiVersion":"coordination.k8s.io/v1","resourceVersion":"1972"} reason="LeaderElection"



2024-03-06T10:03:45.108960288Z stderr F I0306 10:03:45.108857       1 leaderelection.go:250] attempting to acquire leader lease kube-system/cert-manager-cainjector-leader-election...

/lgtm
/approve

@jetstack-bot jetstack-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 6, 2024
@jetstack-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj

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

@jetstack-bot jetstack-bot merged commit 098c072 into cert-manager:master Mar 6, 2024
@jetstack-bot
Copy link
Contributor

@inteon: new pull request created: #6819

In response to this:

/cherrypick release-1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jetstack-bot jetstack-bot mentioned this pull request Mar 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallrj wallrj wallrj approved these changes

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cert-manager 1.14.3 cainjector leader election seems broken
3 participants
@inteon @jetstack-bot @wallrj