Change leadership mechanism #182

RamLavi · 2020-06-09T17:48:58Z

Currently our backup-active implementation make it so that only 1 kubemacpool pod is in ready state. This causes a problem with the deployment status to be not ready.
In order to fix this, we need to change the leadership election to occur using the controller-runtime api, and not externally as implemented now.

What this PR does / why we need it:

Special notes for your reviewer:

Release note:

NONE

ormergi

Looks good this is big changes!
Would you please fix typos and warp commits title and body it cut of on github.

I see that we keep using retryOnConflict when interacting with kube-api, can you please elaborate why we need it?

pkg/manager/manager.go

pkg/manager/leaderelection.go

pkg/manager/manager.go

ormergi · 2020-06-09T18:28:33Z

pkg/manager/leaderelection.go

@@ -13,13 +14,18 @@ func (k *KubeMacPoolManager) waitToStartLeading() error {
 }

 func (k *KubeMacPoolManager) markPodAsLeader() error {


Can you change this function name to something like labelLeaderElectedPod?

changed to AddLeaderLabelToElectedPod. is that ok?

pkg/manager/leaderelection.go

pkg/pool-manager/virtualmachine_pool.go

tests/tests.go

RamLavi · 2020-06-10T05:34:35Z

/hold

new elected does not acquire old cache

qinqon

Some error wrapping is missing, also we may weant to do the Elected check at pool manager outside, so pool manager is agnostic of leader election, for that we have to make an poolManager Start method public..

pkg/manager/leaderelection.go

pkg/manager/manager.go

pkg/pool-manager/pool.go

qinqon · 2020-06-10T06:56:15Z

tests/tests.go

+		numberOfReadyPods := int32(0)
 		for _, podObject := range podsList.Items {
-			if podObject.Status.Phase != corev1.PodRunning {
-				return false
+			for _, condition := range podObject.Status.Conditions {
+				if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue {
+					numberOfReadyPods += 1
+				}
 			}
 		}
+		if numberOfReadyPods < numOfReplica {
+			return false
+		}


As @ormergi point out, just checking managerDeployment.Status.ReadyReplicas != numOfReplica is enough since it's more or less the same as this check.

@RamLavi this is not resolved

sorry missed it

pkg/manager/leaderelection.go

Currently we use an external leader election where only the elected pod actually runs the controller-runtime manager. We want to change this behavior so that all pods run the controller-runtime manager, thus are shown as ready. To do that we remove the od code of the leader election. Signed-off-by: Ram Lavi <ralavi@redhat.com>

…elected leader pod This also means we get can remove the use of leaderElectionChannel Signed-off-by: Ram Lavi <ralavi@redhat.com>

…er() Signed-off-by: Ram Lavi <ralavi@redhat.com>

in thie commit we move waitToStartLeading() to a separate go routine, so that both pods will start the controller-runtime manager. we then add the leader election inside the controller runtime manager. Signed-off-by: Ram Lavi <ralavi@redhat.com>

since the webhook service operates only on pods with kubemacpool leader label, we cant start the webhooks until the election is complete and a leader as been chosen. in order to do that, we add a new status to the pod, to prevent the pods to be ready and the webhhoks to start until the label is set. Signed-off-by: Ram Lavi <ralavi@redhat.com>

…appropriate rbacs. Signed-off-by: Ram Lavi <ralavi@redhat.com>

Signed-off-by: Ram Lavi <ralavi@redhat.com>

…y will initiate after teh election is complete since we don't want both pods to perform pool-manager related operations, we start the pool-manager routines only after the kubemacpool leader is selected. Signed-off-by: Ram Lavi <ralavi@redhat.com>

…oyment replicas, as well as making the needed changes in test to wait for both kubemacpool instead of only to the leader. Signed-off-by: Ram Lavi <ralavi@redhat.com>

RamLavi · 2020-06-10T11:39:41Z

/unhold

fixed issues and also added a test for it in other PR #183

pkg/manager/manager.go

pkg/names/names.go

pkg/pool-manager/pool.go

qinqon · 2020-06-10T12:14:52Z

tests/tests.go

+		numberOfReadyPods := int32(0)
 		for _, podObject := range podsList.Items {
-			if podObject.Status.Phase != corev1.PodRunning {
-				return false
+			for _, condition := range podObject.Status.Conditions {
+				if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue {
+					numberOfReadyPods += 1
+				}
 			}
 		}
+		if numberOfReadyPods < numOfReplica {
+			return false
+		}


@RamLavi this is not resolved

pkg/manager/leaderelection.go

qinqon

Last nit.

pkg/pool-manager/pool.go

pkg/manager/leaderelection.go

Signed-off-by: Ram Lavi <ralavi@redhat.com>

qinqon · 2020-06-10T13:26:42Z

/lgtm
/approved

qinqon · 2020-06-10T13:27:11Z

/approve

kubevirt-bot · 2020-06-10T13:27:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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~~ [qinqon]

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

RamLavi · 2020-06-16T06:30:28Z

/cherry-pick release-v0.14.0

kubevirt-bot · 2020-06-16T06:30:37Z

@RamLavi: #182 failed to apply on top of branch "release-v0.14.0":

Applying: add mgr to KubeMacPoolManager and use mgr.Elected() to mark only the elected leader pod This also means we get can remove the use of leaderElectionChannel
Applying: add retryOnConflict to better handle future conflict in markPodAsLeader()
Applying: move leader election into the controller-runtime manager.
Applying: adding a new readinessGate status to kubemacpool pods.
Applying: adding the new readinessGate status to the manifest. also adding the appropriate rbacs.
Applying: changing sync script to wait for deployment ready
Applying: move pool-manager starting routines to waitToStartLeading so that they will initiate after teh election is complete
error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/pool-manager/pool_test.go
M	pkg/pool-manager/virtualmachine_pool.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/pool-manager/virtualmachine_pool.go
Auto-merging pkg/pool-manager/pool_test.go
CONFLICT (content): Merge conflict in pkg/pool-manager/pool_test.go
Patch failed at 0008 move pool-manager starting routines to waitToStartLeading so that they will initiate after teh election is complete

In response to this:

/cherry-pick release-v0.14.0

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.

kubevirt-bot added release-note-none dco-signoff: yes labels Jun 9, 2020

kubevirt-bot requested review from ormergi and qinqon June 9, 2020 17:49

kubevirt-bot added the size/L label Jun 9, 2020

ormergi reviewed Jun 9, 2020

View reviewed changes

kubevirt-bot added the do-not-merge/hold label Jun 10, 2020

qinqon requested changes Jun 10, 2020

View reviewed changes

kubevirt-bot assigned qinqon Jun 10, 2020

RamLavi force-pushed the change_leadership_mechanism branch from 44aca12 to 537cb74 Compare June 10, 2020 09:02

qinqon reviewed Jun 10, 2020

View reviewed changes

pkg/manager/leaderelection.go Show resolved Hide resolved

RamLavi added 9 commits June 10, 2020 13:27

add mgr to KubeMacPoolManager and use mgr.Elected() to mark only the …

e506ebc

…elected leader pod This also means we get can remove the use of leaderElectionChannel Signed-off-by: Ram Lavi <ralavi@redhat.com>

add retryOnConflict to better handle future conflict in markPodAsLead…

10f0ef4

…er() Signed-off-by: Ram Lavi <ralavi@redhat.com>

adding the new readinessGate status to the manifest. also adding the …

8b7cca4

…appropriate rbacs. Signed-off-by: Ram Lavi <ralavi@redhat.com>

changing sync script to wait for deployment ready

28bb17b

Signed-off-by: Ram Lavi <ralavi@redhat.com>

change restartKubemacpoolManagerPods in tests to reboot pods via depl…

7ee59eb

…oyment replicas, as well as making the needed changes in test to wait for both kubemacpool instead of only to the leader. Signed-off-by: Ram Lavi <ralavi@redhat.com>

RamLavi force-pushed the change_leadership_mechanism branch from 537cb74 to a445b80 Compare June 10, 2020 11:37

kubevirt-bot removed the do-not-merge/hold label Jun 10, 2020

RamLavi force-pushed the change_leadership_mechanism branch from a445b80 to e4b432a Compare June 10, 2020 11:56

qinqon requested changes Jun 10, 2020

View reviewed changes

RamLavi force-pushed the change_leadership_mechanism branch from e4b432a to 0325252 Compare June 10, 2020 12:33

qinqon requested changes Jun 10, 2020

View reviewed changes

pkg/pool-manager/pool.go Outdated Show resolved Hide resolved

pkg/manager/leaderelection.go Outdated Show resolved Hide resolved

fix code reviews

a3708fe

Signed-off-by: Ram Lavi <ralavi@redhat.com>

RamLavi force-pushed the change_leadership_mechanism branch from 0325252 to a3708fe Compare June 10, 2020 13:09

kubevirt-bot added the lgtm label Jun 10, 2020

kubevirt-bot added the approved label Jun 10, 2020

kubevirt-bot merged commit c1a31b3 into k8snetworkplumbingwg:master Jun 10, 2020

RamLavi mentioned this pull request Jun 16, 2020

[Release v0.14.0] change leadership mechanism #195

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change leadership mechanism #182

Change leadership mechanism #182

RamLavi commented Jun 9, 2020

ormergi left a comment •

edited

ormergi Jun 9, 2020

RamLavi Jun 10, 2020

ormergi Jun 10, 2020

RamLavi commented Jun 10, 2020

qinqon left a comment

qinqon Jun 10, 2020

qinqon Jun 10, 2020

RamLavi Jun 10, 2020

RamLavi commented Jun 10, 2020

qinqon Jun 10, 2020

qinqon left a comment

qinqon commented Jun 10, 2020

qinqon commented Jun 10, 2020

kubevirt-bot commented Jun 10, 2020

RamLavi commented Jun 16, 2020

kubevirt-bot commented Jun 16, 2020

		@@ -13,13 +14,18 @@ func (k *KubeMacPoolManager) waitToStartLeading() error {
		}

		func (k *KubeMacPoolManager) markPodAsLeader() error {

Change leadership mechanism #182

Change leadership mechanism #182

Conversation

RamLavi commented Jun 9, 2020

ormergi left a comment • edited

Choose a reason for hiding this comment

ormergi Jun 9, 2020

Choose a reason for hiding this comment

RamLavi Jun 10, 2020

Choose a reason for hiding this comment

ormergi Jun 10, 2020

Choose a reason for hiding this comment

RamLavi commented Jun 10, 2020

qinqon left a comment

Choose a reason for hiding this comment

qinqon Jun 10, 2020

Choose a reason for hiding this comment

qinqon Jun 10, 2020

Choose a reason for hiding this comment

RamLavi Jun 10, 2020

Choose a reason for hiding this comment

RamLavi commented Jun 10, 2020

qinqon Jun 10, 2020

Choose a reason for hiding this comment

qinqon left a comment

Choose a reason for hiding this comment

qinqon commented Jun 10, 2020

qinqon commented Jun 10, 2020

kubevirt-bot commented Jun 10, 2020

RamLavi commented Jun 16, 2020

kubevirt-bot commented Jun 16, 2020

ormergi left a comment •

edited