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

Jump to bottom

Feat(controller): Fix inconsistency between etcd cluster and statefulset #53

Merged
merged 1 commit into from
Jan 28, 2025
Merged

Feat(controller): Fix inconsistency between etcd cluster and statefulset #53

merged 1 commit into from
Jan 28, 2025

Conversation

soma00333
Copy link
Contributor

@soma00333 soma00333 commented Jan 25, 2025 •
edited
Loading

Overview

This pull request enhances the reconciliation logic in the Reconcile method of the EtcdClusterReconciler to better manage the number of replicas in the StatefulSet. The changes ensure that the number of replicas in the StatefulSet matches the number of etcd members in the cluster.

Change

  • Improved logic for handling mismatches between the number of StatefulSet replicas and etcd cluster members:
    • Added logic to increase the StatefulSet replicas when a new etcd member is added but the corresponding Pod hasn't been created yet.
    • Added logic to decrease the StatefulSet replicas when an etcd member is removed but the corresponding Pod is still running.
  • .golangci.yml: Added gocyclo linter with a minimum complexity threshold of 35 to pass the Lint CI

OutOfScope

  • Introduce gofail
  • Add e2e tests

Related Material

@k8s-ci-robot
Copy link

Hi @soma00333. Thanks for your PR.

I'm waiting for a etcd-io 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-sigs/prow repository.

@soma00333 soma00333 force-pushed the enhance-reconcile-logic branch from 13a76bf to 3a0f051 Compare January 25, 2025 01:11
@soma00333 soma00333 marked this pull request as ready for review January 25, 2025 01:17
This was referenced Jan 25, 2025
Closed
Open
@ahrtr
Copy link
Member

ahrtr commented Jan 25, 2025

/ok-to-test

@soma00333
Copy link
Contributor Author

/retest

@soma00333
Copy link
Contributor Author

cf. https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/etcd-io_etcd-operator/53/pull-etcd-operator-lint/1883067012654043136

level=error msg="Timeout exceeded: try increasing it by passing --timeout option"

We may need to fix the timeout config

@soma00333
Copy link
Contributor Author

Okey, retest worked

soma00333
soma00333 commented Jan 25, 2025
View reviewed changes
.golangci.yml
@@ -47,3 +47,5 @@ linters-settings:
revive:
rules:
- name: comment-spacings
gocyclo:
min-complexity: 35
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default 30

Copy link
Member

Choose a reason for hiding this comment

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

We should consider minimizing the complexity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right. The lint workflow fails without this now.
I’d like to tackle this as future work.

ahrtr
ahrtr reviewed Jan 28, 2025
View reviewed changes
internal/controller/etcdcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/etcdcluster_controller.go Outdated Show resolved Hide resolved
internal/controller/etcdcluster_controller.go Outdated Show resolved Hide resolved
@soma00333 soma00333 force-pushed the enhance-reconcile-logic branch from 3a0f051 to cf38149 Compare January 28, 2025 11:06
ahrtr
ahrtr approved these changes Jan 28, 2025
View reviewed changes
.golangci.yml
@@ -47,3 +47,5 @@ linters-settings:
revive:
rules:
- name: comment-spacings
gocyclo:
min-complexity: 35
Copy link
Member

Choose a reason for hiding this comment

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

We should consider minimizing the complexity.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, soma00333

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr
Copy link
Member

ahrtr commented Jan 28, 2025

Thank you!

@ahrtr ahrtr merged commit 073a14a into etcd-io:main Jan 28, 2025
6 checks passed
@soma00333
Copy link
Contributor Author

I appreciate your quick response

@soma00333 soma00333 deleted the enhance-reconcile-logic branch January 28, 2025 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

Assignees
No one assigned
Labels
approved ok-to-test size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@soma00333 @k8s-ci-robot @ahrtr