Controller: Make Leader Election TTL configurable. #11142

msfidelis · 2024-03-19T22:21:39Z

What this PR does / why we need it:

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #11141

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
[] I have added unit and/or e2e tests to cover my changes.
[] All new and existing tests passed.

k8s-ci-robot · 2024-03-19T22:21:48Z

Hi @msfidelis. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-03-19T22:21:58Z

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name	Link
🔨 Latest commit	`76fe216`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66055f11795fcb00095cb0da
😎 Deploy Preview	https://deploy-preview-11142--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

rikatz · 2024-03-24T19:29:38Z

/ok-to-test

rikatz · 2024-03-24T21:27:56Z

hum, @Gacko I'm not sure if this is part of the new helm CI. Do you know if we use previously compiled binaries for the test? PR seems fine to me, but still during helm CI it is not recognizing the new flag

Gacko · 2024-03-24T23:17:11Z

/hold

Gacko

Chart CI is failing because it's (probably) referencing the image version set in the values.yaml, which is v1.10.0 at the moment of writing this. This controller version doesn't know that flag and therefore the container isn't coming up.

To solve that, we would either need to fix the CI so it always uses the image being built right before the tests start or make this particular flag only being set if not null and default it to null or a negative value in the values.yaml.

Both are not really perfect, but in general I'd like to pretend setting flags on the controller if they are equal to the controller's default value.

docs/user-guide/cli-arguments.md

charts/ingress-nginx/values.yaml

internal/ingress/controller/controller.go

msfidelis · 2024-03-25T01:43:19Z

Chart CI is failing because it's (probably) referencing the image version set in the values.yaml, which is v1.10.0 at the moment of writing this. This controller version doesn't know that flag and therefore the container isn't coming up.

To solve that, we would either need to fix the CI so it always uses the image being built right before the tests start or make this particular flag only being set if not null and default it to null or a negative value in the values.yaml.

Both are not really perfect, but in general I'd like to pretend setting flags on the controller if they are equal to the controller's default value.

I think the Helm CI is no longer sad. I've set the empty value in the values.yaml and assume the default value in the controller. Is that correct?

charts/ingress-nginx/values.yaml

ubergesundheit · 2024-03-25T07:53:38Z

I think the Helm CI is no longer sad. I've set the empty value in the values.yaml and assume the default value in the controller. Is that correct?

You specified the default as empty string, which IMO is not ideal. See my in line comment.

Also can we change ~~the helm value to electionTtl and all other~~ Time To Live occurrences to Time to live?
Edit: Ok TTL seems to be the commonly used abbrebivation.

charts/ingress-nginx/values.yaml

charts/ingress-nginx/templates/_params.tpl

internal/ingress/controller/status.go

internal/ingress/controller/controller.go

pkg/flags/flags.go

Gacko · 2024-03-25T10:52:31Z

/retitle Controller: Make Leader Election TTL configurable.

cpanato · 2024-03-27T10:42:36Z

please squash all commits

Gacko · 2024-03-27T11:12:22Z

/unhold

msfidelis · 2024-03-27T11:55:54Z

please squash all commits

Done @cpanato

/honk

k8s-ci-robot · 2024-03-27T11:55:56Z

@msfidelis:

In response to this:

please squash all commits

Done @cpanato

/honk

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.

cpanato · 2024-03-27T13:22:53Z

docs/user-guide/cli-arguments.md

@@ -22,6 +22,7 @@ They are set in the container spec of the `ingress-nginx-controller` Deployment
 | `--disable-sync-events` | Disables the creation of 'Sync' Event resources, but still logs them |
 | `--dynamic-configuration-retries` | Number of times to retry failed dynamic configuration before failing to sync an ingress. (default 15) |
 | `--election-id`                    | Election id to use for Ingress status updates. (default "ingress-controller-leader") |
+| `--election-ttl`                  | Duration a leader election is valid before it's getting re-elected. (Default: 30s) |


i think here we need to call out the specific way to set this, otherwise they will not add the correct value, something like

Suggested change

| `--election-ttl` | Duration a leader election is valid before it's getting re-elected. (Default: 30s) |

| `--election-ttl` | Duration for a leader election is valid before it's getting re-elected. (Default: 30s). The syntax in X<unit>, with <unit>: s, h, d |

cpanato

my last comment, otherwise lgtm

thanks for working on this

docs/user-guide/cli-arguments.md

cpanato

thank you!
/approve

cpanato · 2024-03-28T12:47:47Z

/kind feature
/priority important-soon
/triage accepted

strongjz · 2024-03-28T12:56:06Z

/lgtm

k8s-ci-robot · 2024-03-28T12:56:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cpanato, Gacko, msfidelis, strongjz

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:

~~OWNERS~~ [cpanato,strongjz]

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

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/helm Issues or PRs related to helm charts needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 19, 2024

k8s-ci-robot requested review from strongjz and ubergesundheit March 19, 2024 22:21

k8s-ci-robot added the needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Mar 19, 2024

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 19, 2024

msfidelis changed the title ~~feature(leader_election_ttl): initial proposal~~ Proposal: Custom Leader Election TTL Mar 19, 2024

k8s-ci-robot added area/docs size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 19, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 24, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2024

Gacko suggested changes Mar 24, 2024

View reviewed changes

docs/user-guide/cli-arguments.md Outdated Show resolved Hide resolved

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

internal/ingress/controller/controller.go Outdated Show resolved Hide resolved

ubergesundheit reviewed Mar 25, 2024

View reviewed changes

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

Gacko reviewed Mar 25, 2024

View reviewed changes

k8s-ci-robot changed the title ~~Proposal: Custom Leader Election TTL~~ Controller: Make Leader Election TTL configurable. Mar 25, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 25, 2024

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2024

msfidelis requested review from cpanato and Gacko March 27, 2024 10:42

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 27, 2024

feature(leader_ttl): feature to customize ttl to leader be re-elected

ba2040e

msfidelis force-pushed the feature/leader_election_ttl branch from 2b3a559 to ba2040e Compare March 27, 2024 11:54

cpanato reviewed Mar 27, 2024

View reviewed changes

Gacko reviewed Mar 27, 2024

View reviewed changes

docs/user-guide/cli-arguments.md Outdated Show resolved Hide resolved

fix(review): docs

76fe216

cpanato approved these changes Mar 28, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 28, 2024

cpanato added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Mar 28, 2024

strongjz added this to the release-1.11 milestone Mar 28, 2024

k8s-ci-robot assigned strongjz Mar 28, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 28, 2024

k8s-ci-robot merged commit 7c8af49 into kubernetes:main Mar 28, 2024
29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Controller: Make Leader Election TTL configurable. #11142

Controller: Make Leader Election TTL configurable. #11142

msfidelis commented Mar 19, 2024 •

edited

k8s-ci-robot commented Mar 19, 2024

netlify bot commented Mar 19, 2024 •

edited

rikatz commented Mar 24, 2024

rikatz commented Mar 24, 2024

Gacko commented Mar 24, 2024

Gacko left a comment •

edited

msfidelis commented Mar 25, 2024

ubergesundheit commented Mar 25, 2024 •

edited

Gacko commented Mar 25, 2024

cpanato commented Mar 27, 2024

Gacko commented Mar 27, 2024

msfidelis commented Mar 27, 2024

k8s-ci-robot commented Mar 27, 2024

cpanato Mar 27, 2024 •

edited

cpanato left a comment

cpanato left a comment

cpanato commented Mar 28, 2024

strongjz commented Mar 28, 2024

k8s-ci-robot commented Mar 28, 2024

	\| `--election-ttl` \| Duration a leader election is valid before it's getting re-elected. (Default: 30s) \|
	\| `--election-ttl` \| Duration for a leader election is valid before it's getting re-elected. (Default: 30s). The syntax in X<unit>, with <unit>: s, h, d \|

Controller: Make Leader Election TTL configurable. #11142

Controller: Make Leader Election TTL configurable. #11142

Conversation

msfidelis commented Mar 19, 2024 • edited

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

k8s-ci-robot commented Mar 19, 2024

netlify bot commented Mar 19, 2024 • edited

✅ Deploy Preview for kubernetes-ingress-nginx ready!

rikatz commented Mar 24, 2024

rikatz commented Mar 24, 2024

Gacko commented Mar 24, 2024

Gacko left a comment • edited

Choose a reason for hiding this comment

msfidelis commented Mar 25, 2024

ubergesundheit commented Mar 25, 2024 • edited

Gacko commented Mar 25, 2024

cpanato commented Mar 27, 2024

Gacko commented Mar 27, 2024

msfidelis commented Mar 27, 2024

k8s-ci-robot commented Mar 27, 2024

cpanato Mar 27, 2024 • edited

Choose a reason for hiding this comment

cpanato left a comment

Choose a reason for hiding this comment

cpanato left a comment

Choose a reason for hiding this comment

cpanato commented Mar 28, 2024

strongjz commented Mar 28, 2024

k8s-ci-robot commented Mar 28, 2024

msfidelis commented Mar 19, 2024 •

edited

netlify bot commented Mar 19, 2024 •

edited

Gacko left a comment •

edited

ubergesundheit commented Mar 25, 2024 •

edited

cpanato Mar 27, 2024 •

edited