-
Notifications
You must be signed in to change notification settings - Fork 40
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.
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
leader: check the node status for the leader-election (#24) #25
leader: check the node status for the leader-election (#24) #25
Conversation
Pull Request Test Coverage Report for Build 232104749
💛 - Coveralls |
leader/leader.go
Outdated
@@ -167,6 +167,7 @@ func Become(ctx context.Context, lockName string, opts ...Option) error { | |||
log.Info("Leader lock configmap owner reference must be a pod.", "OwnerReference", existingOwners[0]) | |||
default: | |||
leaderPod := &corev1.Pod{} | |||
// leaderNode := &corev1.Node{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not merge code commented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 thanks, I will delete the comment at the new commit.
leader/leader.go
Outdated
@@ -167,6 +167,7 @@ func Become(ctx context.Context, lockName string, opts ...Option) error { | |||
log.Info("Leader lock configmap owner reference must be a pod.", "OwnerReference", existingOwners[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add the logic of the fragments to here as well for we are able to generate the changelog.
c/c @jmrodri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's really no changelog fragment for the operator-lib. The releases are just a signed tag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should have since we should have a way to know what changed in each version/release.
log.Info("Deleting the leader.") | ||
|
||
//Mark the termainating status to the leaderPod and Delete the configmap lock | ||
if err := deleteLeader(ctx, config.Client, leaderPod, existing); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not use the same func across all implementation? Also, why is required to remove the configMap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The reason i added the function(delete Leader) is for the lint-test. golangci-lint limits Cyclomatic complexity in the function. so we have to divide the code into multiple functions.
- if i do not remove the configmap, leader election will not occur imediately when the node where the leader pod is running was 'not ready' status. Because the leader pod was just marked as 'terminating'.(the garbage collector do not delete the configmap until the leader pod is actually deleted)
@@ -209,4 +209,137 @@ var _ = Describe("Leader election", func() { | |||
Expect(pod.TypeMeta.Kind).To(Equal("Pod")) | |||
}) | |||
}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS.: see that is not all changes covered by the tests. its % decreased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@camilamacedo86 i will enhance the coverage in the new commit. Do i need to make a new pull-request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kasonglee I see you already added tests that is just fine. There is no need for a new commit.
/assign jmrodri |
/hold until I can review |
/test test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two minor issue but I will not hold this PR up for it. I can fix those later. So far this looks fine to me.
log.Info("the status of the node where operator pod with leader lock was running has been 'notReady'") | ||
log.Info("Deleting the leader.") | ||
|
||
//Mark the termainating status to the leaderPod and Delete the configmap lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: should have a space after //
: // Mark
Nit: typo termainating
should be terminating
.
func getNode(ctx context.Context, client crclient.Client, nodeName string, node *corev1.Node) error { | ||
key := crclient.ObjectKey{Namespace: "", Name: nodeName} | ||
err := client.Get(ctx, key, node) | ||
if err != nil { | ||
log.Error(err, "Failed to get Node", "Node.Name", nodeName) | ||
return err | ||
} | ||
return nil | ||
} | ||
|
||
func isNotReadyNode(ctx context.Context, client crclient.Client, nodeName string) bool { | ||
leaderNode := &corev1.Node{} | ||
if err := getNode(ctx, client, nodeName, leaderNode); err != nil { | ||
return false | ||
} | ||
for _, condition := range leaderNode.Status.Conditions { | ||
if condition.Type == corev1.NodeReady && condition.Status != corev1.ConditionTrue { | ||
return true | ||
} | ||
} | ||
return false | ||
|
||
} | ||
|
||
func deleteLeader(ctx context.Context, client crclient.Client, leaderPod *corev1.Pod, existing *corev1.ConfigMap) error { | ||
err := client.Delete(ctx, leaderPod) | ||
if err != nil { | ||
log.Error(err, "Leader pod could not be deleted.") | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is all good. Pretty clean and easy to read.
@@ -209,4 +209,137 @@ var _ = Describe("Leader election", func() { | |||
Expect(pod.TypeMeta.Kind).To(Equal("Pod")) | |||
}) | |||
}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kasonglee I see you already added tests that is just fine. There is no need for a new commit.
/hold cancel |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jmrodri 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 |
Description of the change:
node-check for leader re-election #24
Motivation for the change:
#24
Check the condition of the node where the leader pod is running with [Node.Type == "NodeReady" && Node.Status != "ConditionTrue"] and when the node has been failed, delete the leaderPod (only mark the pod with 'terminating' because the node where the pod is running has been failed) and the configmap lock whose OwnerReference is leaderPod).
This approach can bring more reliability