⚠️ Remove defaulting for leader election ID #446

alvaroaleman · 2019-05-25T17:56:29Z

With the current defaulting for the leader election ID, there is a clash
as soon as two controller that do not have it explicitly configured run
in the same namespace or have the same namespace configured for leader
election.

This is especially bad since there is no logging about the lock being
held by a different controller, so from a users perspective this looks
like the controller just froze.

Fixes #445

/assign @DirectXMan12
/cc @JoelSpeed

With the current defaulting for the leader election ID, there is a clash as soon as two controller that do not have it explicitly configured run in the same namespace or have the same namespace configured for leader election. This is especially bad since there is no logging about the lock being held by a different controller, so from a users perspective this looks like the controller just froze.

JoelSpeed

Looks good, I do wonder if we should also remove the namespace defaulting too, sounds like it could have the same problem

JoelSpeed · 2019-05-26T17:40:39Z

/lgtm

DirectXMan12 · 2019-06-04T00:04:10Z

/hold

till we resolve the discussion on #445 (I'm happy to be corrected on that issue, just want to finish up that discussion first)

fejta-bot · 2019-09-02T01:01:15Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-10-02T01:44:50Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-11-01T02:28:01Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-11-01T02:28:09Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

DirectXMan12 · 2019-11-05T22:40:57Z

I don't think we meant to close this...

pires

/lgtm

As I already mentioned I'm good with increasing the cognitive load if it means the adopter is cognizant, and sole responsible, of the issue(s) that may arise if resource names collide.

k8s-ci-robot · 2019-11-27T15:16:43Z

@pires: changing LGTM is restricted to collaborators

In response to this:

/lgtm

As I already mentioned I'm good with increasing the cognitive load if it means the adopter is cognizant, and sole responsible, of the issue(s) that may arise if resource names collide.

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.

alvaroaleman · 2019-12-16T13:20:38Z

@DirectXMan12 re #445 (comment): Can we add this change to the breaking-next milestone?

DirectXMan12 · 2020-01-06T22:56:00Z

Added. I think we've got a breaking one coming up due to #749, which I think we need in for supportability reasons.

alexeldeib · 2020-01-21T17:49:00Z

/remove-lifecycle rotten

shawn-hurley · 2020-01-27T21:29:35Z

@DirectXMan12 bump to get this in as #749 merged

vincepri

This should be ready to go.

/hold cancel
/lgtm
/assign @gerred @DirectXMan12

DirectXMan12 · 2020-02-05T21:38:21Z

/approve

gerred · 2020-02-05T21:55:03Z

/approve

k8s-ci-robot · 2020-02-05T21:56:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, DirectXMan12, gerred

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~~ [DirectXMan12,gerred]

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

DirectXMan12 · 2020-02-06T00:00:55Z

pkg/manager/manager_test.go

@@ -143,15 +143,16 @@ var _ = Describe("manger.Manager", func() {
 				m, err := New(cfg, Options{
 					LeaderElection:          true,
 					LeaderElectionNamespace: "default",
+					LeaderElectionID:        "test-leader-election-id",


lol, this test should have a different name now. I'll fix it elsewhere.

See kubernetes-sigs/controller-runtime#446

* Update to controller-runtime-0.5.0 * And K8S 1.17 libraries * No need to set --advertise-address This has now been fixed upstream. * Controller-runtime-0.5.0 requires the leaderElectionID to be set See kubernetes-sigs/controller-runtime#446

Reason for upgrade: The new version uses [DynamicRESTMapper](https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/apiutil/dynamicrestmapper.go) as default `RESTMapper` for controller runtime manager. `DynamicRESTMapper` will "reload the delegated `meta.RESTMapper` on a cache miss" (see kubernetes-sigs/controller-runtime#554), which can solve problem that we need to restart HNC after adding a new CRD to create corresponding object reconciler when using controller-runtime v0.2.2 (see details in kubernetes-retired#488). Incompatibility issues addressed in this PR: - Upgrade go version in `go.mod` and `Dockerfile` to 1.13. The [errors.As](https://golang.org/pkg/errors/#As) in [dynamicrestmapper.go](https://github.com/kubernetes-sigs/controller-runtime/blob/bfc982769615817ee15db8a543662652d900d27b/pkg/client/apiutil/dynamicrestmapper.go#L48) requires go 1.13. - A higher version for k8s.io/cli-runtime and k8s.io/client-go are required after upgrading controller-runtime to v0.5.0 - Version changes of other packages in `go.mod` are updated automatically. - serializer.DirectCodecFactory was renamed to [serializer.WithoutConversionCodecFactory](https://godoc.org/k8s.io/apimachinery/pkg/runtime/serializer#WithoutConversionCodecFactory) after k8s.io/apimachinery 1.16 (see [here](kubernetes/apimachinery@ed8af17), and [here](kubernetes/apimachinery@4fac835)) - Default [LeaderElectionID](https://github.com/kubernetes-sigs/controller-runtime/blob/bfc982769615817ee15db8a543662652d900d27b/pkg/leaderelection/leader_election.go#L46) in controller-runtime manager is removed after kubernetes-sigs/controller-runtime#446. - [NewlineReporter](https://godoc.org/sigs.k8s.io/controller-runtime/pkg/envtest/printer) is moved from `sigs.k8s.io/controller-runtime/pkg/envtest/` to `https://godoc.org/sigs.k8s.io/controller-runtime/pkg/envtest/printer` by [this](kubernetes-sigs/controller-runtime@748f55d#diff-42de1d59fbe8f8b90154f01dd01f5748) commit. - In controller-runtime v0.2.2, if a resource does not exist, a controller cannot be created successfully. After update controller-runtime to v0.5.0, a controller can be created without error. However, when the `Reconcile` method is triggered, there will be an error complaining the resource does not exist. Therefore, we will explicitly check if a resource exists before creating the corresponding object reconciler in `createObjectReconciler` in `hnc_config.go` (see details in kubernetes-sigs/controller-runtime#840) Tested: - Unit tests. - Went through [demo script](https://docs.google.com/document/d/1tKQgtMSf0wfT3NOGQx9ExUQ-B8UkkdVZB6m4o3Zqn64/edit#) to make sure HNC behaves as expected on a GKE cluster. - Manually test if the PR solves the restart problem as described in kubernetes-retired#488 with following workflow: - Install HNC - Install a new CRD - Config the new type in `config` singleton Before this PR, corresponding object reconciler for the new type will not be created unless we restart HNC. After the change, corresponding object reconciler can be created and it reconciles objects of the new type as expected without restarting HNC. This partly solve: kubernetes-retired#488

kubernetes-sigs/controller-runtime#446 removed the default and #30 updated kubebuilder to v0.5.0 when this was released.

k8s-ci-robot assigned DirectXMan12 May 25, 2019

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 25, 2019

k8s-ci-robot requested a review from JoelSpeed May 25, 2019 17:56

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 25, 2019

alvaroaleman force-pushed the remove-defaulting-for-leader-election branch from d25b1ee to 687d7fc Compare May 25, 2019 17:57

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 25, 2019

JoelSpeed approved these changes May 26, 2019

View reviewed changes

k8s-ci-robot assigned JoelSpeed May 26, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 26, 2019

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 2, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 2, 2019

k8s-ci-robot closed this Nov 1, 2019

DirectXMan12 reopened this Nov 5, 2019

pires approved these changes Nov 27, 2019

View reviewed changes

DirectXMan12 added this to the breaking-next milestone Jan 6, 2020

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Jan 21, 2020

vincepri approved these changes Feb 5, 2020

View reviewed changes

k8s-ci-robot assigned gerred and vincepri Feb 5, 2020

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2020

k8s-ci-robot merged commit 552b43d into kubernetes-sigs:master Feb 5, 2020

DirectXMan12 reviewed Feb 6, 2020

View reviewed changes

wallrj added a commit to wallrj/etcd-cluster-operator that referenced this pull request Feb 14, 2020

Controller-runtime-0.5.0 requires the leaderElectionID to be set

5f3954b

See kubernetes-sigs/controller-runtime#446

mengqiy mentioned this pull request Feb 27, 2020

✨ default a LeaderElectionID kubernetes-sigs/kubebuilder#1394

Merged

sophieliu15 mentioned this pull request Mar 6, 2020

Upgrade controller-runtime from v0.2.2 to v0.5.0 and its dependencies. kubernetes-retired/multi-tenancy#504

Merged

steved added a commit to dominodatalab/forge that referenced this pull request Jun 26, 2020

add a default leader election ID

d1226b1

kubernetes-sigs/controller-runtime#446 removed the default and #30 updated kubebuilder to v0.5.0 when this was released.

steved mentioned this pull request Jun 26, 2020

add a default leader election ID dominodatalab/forge#31

Merged

steved added a commit to dominodatalab/forge that referenced this pull request Jul 1, 2020

add a default leader election ID (#31)

61a9b7a

kubernetes-sigs/controller-runtime#446 removed the default and #30 updated kubebuilder to v0.5.0 when this was released.

This was referenced Oct 31, 2020

Fixes the default leader election ID kubeflow/kubeflow#5374

Merged

Fixes the leader election ID kubeflow/kubeflow#5385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

⚠️ Remove defaulting for leader election ID #446

⚠️ Remove defaulting for leader election ID #446

alvaroaleman commented May 25, 2019

JoelSpeed left a comment •

edited

Loading

JoelSpeed commented May 26, 2019

DirectXMan12 commented Jun 4, 2019

fejta-bot commented Sep 2, 2019

fejta-bot commented Oct 2, 2019

fejta-bot commented Nov 1, 2019

k8s-ci-robot commented Nov 1, 2019

DirectXMan12 commented Nov 5, 2019

pires left a comment

k8s-ci-robot commented Nov 27, 2019

alvaroaleman commented Dec 16, 2019

DirectXMan12 commented Jan 6, 2020

alexeldeib commented Jan 21, 2020

shawn-hurley commented Jan 27, 2020

vincepri left a comment

DirectXMan12 commented Feb 5, 2020

gerred commented Feb 5, 2020

k8s-ci-robot commented Feb 5, 2020

DirectXMan12 Feb 6, 2020

⚠️ Remove defaulting for leader election ID #446

⚠️ Remove defaulting for leader election ID #446

Conversation

alvaroaleman commented May 25, 2019

JoelSpeed left a comment • edited Loading

Choose a reason for hiding this comment

JoelSpeed commented May 26, 2019

DirectXMan12 commented Jun 4, 2019

fejta-bot commented Sep 2, 2019

fejta-bot commented Oct 2, 2019

fejta-bot commented Nov 1, 2019

k8s-ci-robot commented Nov 1, 2019

DirectXMan12 commented Nov 5, 2019

pires left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 27, 2019

alvaroaleman commented Dec 16, 2019

DirectXMan12 commented Jan 6, 2020

alexeldeib commented Jan 21, 2020

shawn-hurley commented Jan 27, 2020

vincepri left a comment

Choose a reason for hiding this comment

DirectXMan12 commented Feb 5, 2020

gerred commented Feb 5, 2020

k8s-ci-robot commented Feb 5, 2020

DirectXMan12 Feb 6, 2020

Choose a reason for hiding this comment

JoelSpeed left a comment •

edited

Loading