feat(controller): Refactor reconcile loop and add unit tests #180

ballista01 · 2025-07-16T19:17:11Z

Summary

Refactors EtcdClusterReconciler.Reconcile into focused phases to reduce cyclomatic complexity and make behavior easier to reason about.
Adds a reconcileState container so data gathered in earlier phases can be reused without repeated lookups.
Reworks the controller unit tests to target each phase individually with fake clients instead of the previous envtest-based flow.

Controller changes (`internal/controller/etcdcluster_controller.go`)

Split the main reconcile loop into:
- fetchAndValidateState
- bootstrapStatefulSet
- performHealthChecks
- reconcileClusterState
Each returns early with a ctrl.Result when it needs a requeue or encounters an error.
Introduced reconcileState (cluster, StatefulSet, member list, member health) to pass context between phases.
fetchAndValidateState:
- Sets a default image registry.
- Prepares TLS credentials.
- Verifies StatefulSet ownership.
- Signals requeues when dependent resources aren’t yet ready.
bootstrapStatefulSet:
- Unifies StatefulSet creation/initial scaling and the headless service check.
- Uses ctrl.Result.IsZero() to decide whether to continue.
performHealthChecks and reconcileClusterState:
- Now operate on memberHealth.
- Added comments clarifying learner promotion, scale-out/-in, and health gating.

Tests (`internal/controller/etcdcluster_controller_test.go`)

Replaced the previous integration-style test with table-driven unit tests using controller-runtime’s fake client.
Added TestFetchAndValidateState and TestBootstrapStatefulSet, switching to require for hard preconditions and asserting on StatefulSet/Service/ConfigMap outcomes.
Expanded coverage for bootstrap scenarios:
- Initial creation.
- 0→1 promotion.
- No-op when resources exist.
Verified cluster state remains untouched when nothing changes.

k8s-ci-robot · 2025-07-16T19:17:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ballista01
Once this PR has been reviewed and has the lgtm label, please assign justinsb for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

k8s-ci-robot · 2025-07-16T19:17:21Z

Hi @ballista01. 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.

jberkus · 2025-07-22T18:51:44Z

/ok-to-test

- split the monolithic Reconcile into fetch/sync/health/cluster phases backed by a shared reconcileState - ensure TLS setup, STS ownership checks, and primitive bootstrapping are driven through dedicated helpers - refresh controller unit tests to exercise the new helpers and drop obsolete scaffolding Signed-off-by: Wenxue Zhao <[email protected]>

ivanvc · 2025-09-30T05:34:34Z

I was reading the code. But the changes look a little bit lengthy. Do you think that it would be possible to break down this pull request?

ahrtr

The refactoring makes sense to me. Just raised several minor comments.

Thanks.

cc @ivanvc @ArkaSaha30 @abdurrehman107 @hakman @frederiko PTAL when you get free cycle, thx

internal/controller/etcdcluster_controller.go

ivanvc

Thanks for the pull request, @ballista01. I left a couple of comments.

I also noticed that you removed some comments from lengthy functions. I think it makes sense to comment these functions, as it is easy to get lost with all the conditions they have. Thanks again :)

ivanvc · 2025-10-03T05:25:20Z

internal/controller/etcdcluster_controller.go

+	cluster        *ecv1alpha1.EtcdCluster      // cluster custom resource currently being reconciled
+	sts            *appsv1.StatefulSet          // associated StatefulSet for the cluster
+	memberListResp *clientv3.MemberListResponse // member list fetched from the etcd cluster
+	healthInfos    []etcdutils.EpHealth         // health information for each etcd member


Reading this property name out loud makes me wonder if we could rename it to: memberHealth

I also recovered and updated some comments (mainly in reconcileClusterState). Let me know if we need more!

ivanvc · 2025-10-03T05:55:52Z

internal/controller/etcdcluster_controller_test.go

+			},
+			req: ctrl.Request{NamespacedName: types.NamespacedName{Name: "etcd", Namespace: "default"}},
+			assert: func(t *testing.T, state *reconcileState, res ctrl.Result, err error, ec *ecv1alpha1.EtcdCluster, _ *appsv1.StatefulSet) {
+				if assert.NotNil(t, state) {


What's the value of this if statement? If it fails, either way the test will fail in the end. So, I'm trying to understand why we would want this behavior.

Thanks for the question! testify’s assert helpers only record the failure and carry on, so the original if assert.NotNil(...) blocks were just there to stop us from dereferencing state/state.sts and panicing. That might seem to be confusing, so I’ve switched those checks to require.NotNil(...). Now the tests fail fast when the state is missing, and the following assert.Equal calls simply verify the expected fields.

ivanvc · 2025-10-03T05:56:28Z

internal/controller/etcdcluster_controller_test.go

+				if assert.NotNil(t, state) {
+					assert.Equal(t, ec.Name, state.cluster.Name)
+					if assert.NotNil(t, state.sts) {
+						assert.Equal(t, sts.Name, state.sts.Name)
+					}


Same question here.

Changed the usage to require.NotNil(...)

…n tests Signed-off-by: Wenxue Zhao <[email protected]>

ahrtr · 2025-10-03T19:40:33Z

The change for internal/controller/etcdcluster_controller.go looks clean and better. Thanks. (But do not get much time to look into the test case.)

Let's target to merge this PR before our next WG meeting.

ahrtr · 2025-10-08T16:08:22Z

@ivanvc any further comments on this PR?

k8s-ci-robot added needs-ok-to-test size/XXL labels Jul 16, 2025

ballista01 force-pushed the reconcile-refactor branch from baf8e00 to 59d969f Compare July 16, 2025 19:19

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Jul 22, 2025

ballista01 force-pushed the reconcile-refactor branch from dffb919 to fa7f810 Compare August 5, 2025 19:53

ballista01 mentioned this pull request Sep 29, 2025

Implement Status Reporting for EtcdCluster #136

Open

ballista01 force-pushed the reconcile-refactor branch from fa7f810 to 9250067 Compare September 29, 2025 11:06

k8s-ci-robot added size/XL and removed size/XXL labels Sep 29, 2025

ballista01 force-pushed the reconcile-refactor branch from 9250067 to b43d579 Compare September 29, 2025 11:10

ballista01 changed the title ~~feat(controller, e2e): Refactor reconcile loop and add E2E tests for scaling and promotion~~ feat(controller): Refactor reconcile loop and add unit tests Sep 30, 2025

ballista01 mentioned this pull request Sep 30, 2025

REQUEST: New membership for @ballista01 kubernetes/org#5890

Closed

11 tasks

ahrtr reviewed Oct 1, 2025

View reviewed changes

ivanvc reviewed Oct 3, 2025

View reviewed changes

refactor: clarify bootstrap statefulset flow in controller and tighte…

9354995

…n tests Signed-off-by: Wenxue Zhao <[email protected]>

feat(controller): Refactor reconcile loop and add unit tests #180

Are you sure you want to change the base?

feat(controller): Refactor reconcile loop and add unit tests #180

Conversation

ballista01 commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Controller changes (internal/controller/etcdcluster_controller.go)

Tests (internal/controller/etcdcluster_controller_test.go)

Uh oh!

k8s-ci-robot commented Jul 16, 2025

Uh oh!

k8s-ci-robot commented Jul 16, 2025

Uh oh!

jberkus commented Jul 22, 2025

Uh oh!

ivanvc commented Sep 30, 2025

Uh oh!

ahrtr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ivanvc left a comment

Choose a reason for hiding this comment

Uh oh!

ivanvc Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ballista01 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ballista01 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ivanvc Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ballista01 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ivanvc Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ballista01 Oct 3, 2025

Choose a reason for hiding this comment

Uh oh!

ahrtr commented Oct 3, 2025

Uh oh!

ahrtr commented Oct 8, 2025

Uh oh!

Uh oh!

ballista01 commented Jul 16, 2025 •

edited

Loading

Controller changes (`internal/controller/etcdcluster_controller.go`)

Tests (`internal/controller/etcdcluster_controller_test.go`)