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

feat: migrate leaderelection lock to leases #8733

Merged
merged 2 commits into from
Jul 9, 2022
Merged

feat: migrate leaderelection lock to leases #8733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
61c3312
feat: migrate leaderelection lock to leases
tao12345666333 Jun 23, 2022
62a876a
Update RBAC
shafeeqes Jan 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions charts/ingress-nginx/templates/clusterrole.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ rules:
verbs:
- list
- watch
- apiGroups:
- coordination.k8s.io
resources:
- leases
verbs:
- list
- watch
{{- if and .Values.controller.scope.enabled .Values.controller.scope.namespace }}
- apiGroups:
- ""
Expand Down
15 changes: 15 additions & 0 deletions charts/ingress-nginx/templates/controller-role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,21 @@ rules:
- configmaps
verbs:
- create
- apiGroups:
- coordination.k8s.io
resources:
- leases
resourceNames:
- {{ .Values.controller.electionID }}
verbs:
- get
- update
- apiGroups:
- coordination.k8s.io
resources:
- leases
verbs:
- create
- apiGroups:
- ""
resources:
Expand Down
24 changes: 18 additions & 6 deletions internal/ingress/controller/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,24 @@ func setupLeaderElection(config *leaderElectionConfig) {
Host: hostname,
})

lock := resourcelock.ConfigMapLock{
ConfigMapMeta: metav1.ObjectMeta{Namespace: k8s.IngressPodDetails.Namespace, Name: config.ElectionID},
Client: config.Client.CoreV1(),
LockConfig: resourcelock.ResourceLockConfig{
Identity: k8s.IngressPodDetails.Name,
EventRecorder: recorder,
objectMeta := metav1.ObjectMeta{Namespace: k8s.IngressPodDetails.Namespace, Name: config.ElectionID}
resourceLockConfig := resourcelock.ResourceLockConfig{
Identity: k8s.IngressPodDetails.Name,
EventRecorder: recorder,
}

// TODO: If we upgrade client-go to v0.24 then we can only use LeaseLock.
Copy link
Contributor

Choose a reason for hiding this comment

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

When was LeaseLock created? If in v1.21, we can leave in the TODO here that part of the stabilization code will contain only LeaseLock and no ConfigMap anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

When was LeaseLock created?

@rikatz Probably in v1.14.

If in v1.21, we can leave in the TODO here that part of the stabilization code will contain only LeaseLock and no ConfigMap anymore.
the

The original intention of adding MultiLock here is to allow users who have installed ingress-nginx to migrate smoothly.
E.g:

  • The user has already installed ingress-nginx , then when the user installs a new version of ingress-nginx controller containing the code in this PR, we will help it smoothly migrate from configmap to LeaseLock.
    When we release ingress-nginx that only supports LeaseLock in the future, users can not be breaked.

  • If we only consider newly installed users, LeaseLock can be used directly here. (But I don't think it's appropriate)

Copy link
Member Author

Choose a reason for hiding this comment

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

And we may need to release at least two versions.

  • A release is the one that contains this PR code. (for lock migration)

  • One version is the one that contains only LeaseLock code (for full compatibility with Kubernetes v1.24, including the version that upgrades client-go to 1.24)

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. Let's move forward with this, and plan the new release after freeze without the MultiLock

Copy link
Member Author

Choose a reason for hiding this comment

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

@rikatz So, I guess all I need to do is add tests for the v1.24 version of K8s, right?

In our upcoming release, I would like to set ConfigMapLock to Primary to allow smooth migration for existing users.

// MultiLock is used for lock's migration
lock := resourcelock.MultiLock{
Primary: &resourcelock.ConfigMapLock{
ConfigMapMeta: objectMeta,
Client: config.Client.CoreV1(),
LockConfig: resourceLockConfig,
},
Secondary: &resourcelock.LeaseLock{
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is primary instead, will this be used in v1.24 test? So we may know if this is working fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tao12345666333 ping on this. Can we set LeaseLock as Primary and ConfigMapLock as secondary, and see how this behaves?

Also, can we re-add v1.24 tests in CI? :D

Copy link
Member Author

@tao12345666333 tao12345666333 Jul 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Setting ConfigMapLock as Primary here is to allow users to migrate smoothly. ref: ref: #8733 (comment)

I can add tests for v1.24.

LeaseMeta: objectMeta,
Client: config.Client.CoordinationV1(),
LockConfig: resourceLockConfig,
},
}

Expand Down