Add cluster size validation marker #116

frederiko · 2025-03-27T02:20:06Z

This PR adds:

minimum cluster size of 0 members
generate CRD
add defaults to config/samples manifest

k8s-ci-robot · 2025-03-27T02:20:15Z

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

frederiko · 2025-03-27T02:20:56Z

/ok-to-test

k8s-ci-robot · 2025-03-27T02:21:11Z

@frederiko: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

ahrtr · 2025-03-27T09:31:42Z

Thanks for the contribution, but I think it makes more sense to align with K8s's behaviour: set the minimum value as 0 instead of 1.

$ kc apply -f sts.yaml 
The StatefulSet "web" is invalid: spec.replicas: Invalid value: -2: must be greater than or equal to 0

internal/controller/etcdcluster_controller.go

api/v1alpha1/etcdcluster_types.go

jberkus · 2025-03-31T15:13:42Z

So, what happens right now if you apply a cluster of size 0 is that we have a cluster object with no pods. Which seems desireable; I can think of a number of circumstances where you might (temporarily) want a cluster with no members.

So, +1 on @ahrtr's proposal

frederiko · 2025-03-31T17:50:59Z

@jberkus @ahrtr Not a problem at all. I just happened to have a different view from user's perspective. If I create a cluster with 0 members and I have no information about what happened, but have the resource persisted, I wouldn't like to have to tap into controller log for extra information. I couldn't also see a use case where an user may need a cluster with members. I will make the adjustments.

ivanvc · 2025-04-02T08:59:14Z

I wouldn't like to have to tap into controller log for extra information.

In my opinion, we should start adding events to the EtcdCluster so the users can understand the current state of the CR.

frederiko · 2025-04-02T16:05:31Z

If that is not desirable, other mechanisms would entail to write an event or update the status. However, more logic would be required.

"If that is not desirable, other mechanisms would entail to write an event or update the status. However, more logic would be required.". I wouldn't oppose to that. :-)

ahrtr · 2025-04-10T10:09:09Z

"If that is not desirable, other mechanisms would entail to write an event or update the status. However, more logic would be required.".

We will need to update/make use of the status regardless. What's the behaviour of some existing controller when the replica is 0, i.e. StatefulSet?

frederiko · 2025-04-15T16:10:36Z

"If that is not desirable, other mechanisms would entail to write an event or update the status. However, more logic would be required.".

We will need to update/make use of the status regardless. What's the behaviour of some existing controller when the replica is 0, i.e. StatefulSet?

tbh, I don't recall a controller that presents this behavior. I would have to check.

ahrtr · 2025-04-15T16:17:15Z

tbh, I don't recall a controller that presents this behavior. I would have to check.

I mean the Kubernetes built-in controller, i.e. StatefulSet. As mentioned above #116 (comment), the minimum replica is 0. Not sure whether the status will have some message or warning to indicate the 0 replica. We will need to do some experiments.

ahrtr · 2025-04-15T16:22:48Z

@frederiko this PR is basing on a very old commit, please rebase this PR. thx

frederiko · 2025-04-26T02:37:37Z

tbh, I don't recall a controller that presents this behavior. I would have to check.

I mean the Kubernetes built-in controller, i.e. StatefulSet. As mentioned above #116 (comment), the minimum replica is 0. Not sure whether the status will have some message or warning to indicate the 0 replica. We will need to do some experiments.

Yes, sts supports that. I meant more in the context of non-k8s controllers.

ahrtr

Overall looks good with two minor comments, thx.

Also it would be better if we have an e2e test to verify the minimum size.

internal/controller/etcdcluster_controller.go

config/manager/kustomization.yaml

ahrtr · 2025-05-05T14:10:24Z

/ok-to-test

ahrtr · 2025-05-05T18:23:39Z

@frederiko have you manually verified this PR and confirmed a negative value (for the size) will raise an error? If yes, I think it's OK to merge it and add e2e test in followup PR; but it's also OK if you want to add the test in this PR. Please let me know your thought.

Signed-off-by: Frederiko Costa <[email protected]>

frederiko · 2025-05-12T02:15:23Z

@frederiko have you manually verified this PR and confirmed a negative value (for the size) will raise an error? If yes, I think it's OK to merge it and add e2e test in followup PR; but it's also OK if you want to add the test in this PR. Please let me know your thought.

I have just added a basic test to ensure that, when an Etcdcluster is created with zero members, no statefulset is created. Let me know if you would like to something else.

test/e2e/e2e_test.go

ahrtr · 2025-05-12T08:21:50Z

test/e2e/e2e_test.go

+func TestZeroMemberCluster(t *testing.T) {
+	feature := features.New("zero-member-cluster")
+	etcdClusterName := "etcd-cluster-zero"
+	size := 0


The size of 0 should work, but it doesn't work for now due to a bug (although a minor one) #125.

Since we explicitly require the minimum size is 0, so I think we should have two cases (i.e. table-driven sub tests):

create a EtcdCluster with size of negative value (i.e. -1), it should fail.

create a EtcdCluster with size 0, it should succeed. But it doesn't work for now due to the bug mentioned above, so as we comment it out for now (and link to the issue Operator should support scaling down to 0 ... maybe? #125).

@ahrtr yes, I am aware of the bug. What I tried to address in this PR was the 0 size behavior (aware of the bug). I am going to work on that bug later, on a separate PR, which should make this test to go away (remember my first commit was actually removing that block of code, but I was asked to revert?).

As for the -1 size, do we really need to test it, since the api server will not persist the etcdcluster resource altogether? I can certainly add if this is a concern.

but I was asked to revert?

Sorry, when I raised the comment #116 (comment), we didn't know the bug (#125) yet

I am going to work on that bug later, on a separate PR

Please feel free to deliver a PR for it. Thanks.

As for the -1 size, do we really need to test it, since the api server will not persist the etcdcluster resource altogether? I can certainly add if this is a concern.

The high level idea is that we create or apply an EtcdCluster with -1, we should get an error, which might not be returned by etcd-operator's Reconcile method. I think it'd better to have a test for each feature or restriction.

So, I have added a test for negative cluster size as well. I have kept the 0 size cluster one, and I will address the bug later, if that's fine with you. ltmk your thoughts.

Signed-off-by: Frederiko Costa <[email protected]>

ahrtr

LGTM with a minor comment

Thanks

test/e2e/e2e_test.go

Signed-off-by: Frederiko Costa <[email protected]>

jmhbnz

LGTM - Thanks for your work on this @frederiko

k8s-ci-robot · 2025-05-12T20:08:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, frederiko, jmhbnz

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~~ [ahrtr,jmhbnz]

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 added the needs-ok-to-test label Mar 27, 2025

k8s-ci-robot added the size/S label Mar 27, 2025

frederiko force-pushed the etcd-minimum-size branch from 2ccf9d4 to 4745981 Compare March 27, 2025 02:22

ahrtr reviewed Mar 27, 2025

View reviewed changes

internal/controller/etcdcluster_controller.go Outdated Show resolved Hide resolved

ahrtr reviewed Mar 27, 2025

View reviewed changes

api/v1alpha1/etcdcluster_types.go Outdated Show resolved Hide resolved

jberkus mentioned this pull request Apr 4, 2025

Operator should support scaling down to 0 ... maybe? #125

Open

frederiko force-pushed the etcd-minimum-size branch from 83e5506 to 59e3c3b Compare April 15, 2025 16:16

frederiko changed the title ~~Fail to apply manifest when EtcdCluster is zero~~ Add cluster size validation marker Apr 15, 2025

frederiko requested a review from ahrtr April 15, 2025 16:20

frederiko force-pushed the etcd-minimum-size branch 2 times, most recently from deed49b to e6f3650 Compare April 22, 2025 00:09

ahrtr reviewed Apr 27, 2025

View reviewed changes

internal/controller/etcdcluster_controller.go Show resolved Hide resolved

config/manager/kustomization.yaml Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels May 5, 2025

k8s-ci-robot removed the size/S label May 9, 2025

k8s-ci-robot added the size/M label May 9, 2025

frederiko force-pushed the etcd-minimum-size branch 2 times, most recently from 3e83dd2 to 987e9e3 Compare May 9, 2025 23:48

Add minimum cluster size validation

ee6cc59

Signed-off-by: Frederiko Costa <[email protected]>

frederiko force-pushed the etcd-minimum-size branch from 987e9e3 to 3b36917 Compare May 12, 2025 02:00

ArkaSaha30 reviewed May 12, 2025

View reviewed changes

test/e2e/e2e_test.go Outdated Show resolved Hide resolved

ahrtr reviewed May 12, 2025

View reviewed changes

k8s-ci-robot added size/L and removed size/M labels May 12, 2025

Add e2e to check 0 size cluster

f1a38d4

Signed-off-by: Frederiko Costa <[email protected]>

frederiko force-pushed the etcd-minimum-size branch from 2e25fd8 to 4f267dd Compare May 12, 2025 16:34

ahrtr approved these changes May 12, 2025

View reviewed changes

test/e2e/e2e_test.go Show resolved Hide resolved

k8s-ci-robot added the approved label May 12, 2025

Add test for negative cluster creation

4b2835e

Signed-off-by: Frederiko Costa <[email protected]>

frederiko force-pushed the etcd-minimum-size branch from 4f267dd to 4b2835e Compare May 12, 2025 19:08

ahrtr requested review from ivanvc, hakman and jmhbnz May 12, 2025 19:17

jmhbnz approved these changes May 12, 2025

View reviewed changes

ahrtr merged commit 5d7e01c into etcd-io:main May 13, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cluster size validation marker #116

Add cluster size validation marker #116

frederiko commented Mar 27, 2025 •

edited

Loading

k8s-ci-robot commented Mar 27, 2025

frederiko commented Mar 27, 2025

k8s-ci-robot commented Mar 27, 2025

ahrtr commented Mar 27, 2025

jberkus commented Mar 31, 2025

frederiko commented Mar 31, 2025

ivanvc commented Apr 2, 2025

frederiko commented Apr 2, 2025

ahrtr commented Apr 10, 2025

frederiko commented Apr 15, 2025

ahrtr commented Apr 15, 2025

ahrtr commented Apr 15, 2025

frederiko commented Apr 26, 2025

ahrtr left a comment

ahrtr commented May 5, 2025

ahrtr commented May 5, 2025

frederiko commented May 12, 2025

ahrtr May 12, 2025

frederiko May 12, 2025 •

edited

Loading

ahrtr May 12, 2025

frederiko May 12, 2025

ahrtr May 12, 2025

ahrtr left a comment

jmhbnz left a comment

k8s-ci-robot commented May 12, 2025

Add cluster size validation marker #116

Add cluster size validation marker #116

Conversation

frederiko commented Mar 27, 2025 • edited Loading

k8s-ci-robot commented Mar 27, 2025

frederiko commented Mar 27, 2025

k8s-ci-robot commented Mar 27, 2025

ahrtr commented Mar 27, 2025

jberkus commented Mar 31, 2025

frederiko commented Mar 31, 2025

ivanvc commented Apr 2, 2025

frederiko commented Apr 2, 2025

ahrtr commented Apr 10, 2025

frederiko commented Apr 15, 2025

ahrtr commented Apr 15, 2025

ahrtr commented Apr 15, 2025

frederiko commented Apr 26, 2025

ahrtr left a comment

Choose a reason for hiding this comment

ahrtr commented May 5, 2025

ahrtr commented May 5, 2025

frederiko commented May 12, 2025

ahrtr May 12, 2025

Choose a reason for hiding this comment

frederiko May 12, 2025 • edited Loading

Choose a reason for hiding this comment

ahrtr May 12, 2025

Choose a reason for hiding this comment

frederiko May 12, 2025

Choose a reason for hiding this comment

ahrtr May 12, 2025

Choose a reason for hiding this comment

ahrtr left a comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 12, 2025

frederiko commented Mar 27, 2025 •

edited

Loading

frederiko May 12, 2025 •

edited

Loading