Skip to content

Add cluster size validation marker #116

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

Merged
merged 3 commits into from
May 13, 2025
Merged

Add cluster size validation marker #116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee6cc59
Add minimum cluster size validation
frederiko Mar 24, 2025
f1a38d4
Add e2e to check 0 size cluster
frederiko May 9, 2025
4b2835e
Add test for negative cluster creation
frederiko May 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/v1alpha1/etcdcluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ type EtcdClusterSpec struct {
// Important: Run "make" to regenerate code after modifying this file

// Size is the expected size of the etcd cluster.
// +kubebuilder:validation:Minimum=0
Size int `json:"size"`
// Version is the expected version of the etcd container image.
Version string `json:"version"`
Expand Down
3 changes: 2 additions & 1 deletion config/crd/bases/operator.etcd.io_etcdclusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
annotations:
controller-gen.kubebuilder.io/version: v0.17.2
controller-gen.kubebuilder.io/version: v0.17.3
name: etcdclusters.operator.etcd.io
spec:
group: operator.etcd.io
Expand Down Expand Up @@ -48,6 +48,7 @@ spec:
type: array
size:
description: Size is the expected size of the etcd cluster.
minimum: 0
type: integer
storageSpec:
description: StorageSpec is the name of the StorageSpec to use for
Expand Down
8 changes: 7 additions & 1 deletion config/manager/kustomization.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
resources:
- manager.yaml
- manager.yaml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
images:
- name: controller
newName: etcd-operator
newTag: v0.1
3 changes: 2 additions & 1 deletion config/samples/operator_v1alpha1_etcdcluster.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ metadata:
app.kubernetes.io/managed-by: kustomize
name: etcdcluster-sample
spec:
# TODO(user): Add fields here
version: v3.5.20
size: 3
2 changes: 0 additions & 2 deletions internal/controller/etcdcluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,6 @@ func (r *EtcdClusterReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, nil
}

// TODO: Implement finalizer logic here

logger.Info("Reconciling EtcdCluster", "spec", etcdCluster.Spec)

// Get the statefulsets which has the same name as the EtcdCluster resource
Expand Down
108 changes: 108 additions & 0 deletions test/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package e2e

import (
"context"
"os"
"testing"
"time"

Expand All @@ -28,8 +29,115 @@ import (

appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

ecv1alpha1 "go.etcd.io/etcd-operator/api/v1alpha1"
)

var etcdVersion = os.Getenv("ETCD_VERSION")

// TestZeroMemberCluster tests if zero member Etcd Cluster does not create its StatefulSet
// TODO: update this test once https://github.com/etcd-io/etcd-operator/issues/125 is addressed
func TestZeroMemberCluster(t *testing.T) {
feature := features.New("zero-member-cluster")
etcdClusterName := "etcd-cluster-zero"
size := 0
Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@frederiko frederiko May 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

OK, thx


etcdClusterSpec := &ecv1alpha1.EtcdCluster{
TypeMeta: metav1.TypeMeta{
APIVersion: "operator.etcd.io/v1alpha1",
Kind: "EtcdCluster",
},
ObjectMeta: metav1.ObjectMeta{
Name: etcdClusterName,
Namespace: namespace,
},
Spec: ecv1alpha1.EtcdClusterSpec{
Size: size,
Version: etcdVersion,
},
}

feature.Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {

if err := c.Client().Resources().Create(ctx, etcdClusterSpec); err != nil {
t.Fatalf("fail to create Etcd cluster: %s", err)
}

return ctx
})

feature.Assess("statefulSet is not created when etcdCluster.Spec.Size is 0",
func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {

var etcdCluster ecv1alpha1.EtcdCluster
if err := c.Client().Resources().Get(ctx, etcdClusterName, namespace, &etcdCluster); err != nil {
t.Fatalf("unable to fetch Etcd cluster: %s", err)
}

var sts appsv1.StatefulSet
err := c.Client().Resources().Get(ctx, etcdClusterName, namespace, &sts)

if !errors.IsNotFound(err) {
t.Fatalf("statefulSet found when Etcd Cluster size is zero: %s", err)
}

return ctx
},
)

_ = testEnv.Test(t, feature.Feature())

}

// TestNegativeClusterSize tests negative membership cluster creation
func TestNegativeClusterSize(t *testing.T) {
feature := features.New("negative-member-cluster")
etcdClusterName := "etcd-cluster-negative"
size := -1

etcdClusterSpec := &ecv1alpha1.EtcdCluster{
TypeMeta: metav1.TypeMeta{
APIVersion: "operator.etcd.io/v1alpha1",
Kind: "EtcdCluster",
},
ObjectMeta: metav1.ObjectMeta{
Name: etcdClusterName,
Namespace: namespace,
},
Spec: ecv1alpha1.EtcdClusterSpec{
Size: size,
Version: etcdVersion,
},
}

feature.Setup(func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {

err := c.Client().Resources().Create(ctx, etcdClusterSpec)
if !errors.IsInvalid(err) {
t.Fatalf("etcdCluster with negative size failed with unexpected error: %s", err)
}

return ctx
})

feature.Assess("etcdCluster resource should not be created when etcdCluster.Spec.Size is negative",
func(ctx context.Context, t *testing.T, c *envconf.Config) context.Context {

var etcdCluster ecv1alpha1.EtcdCluster
err := c.Client().Resources().Get(ctx, etcdClusterName, namespace, &etcdCluster)
if !errors.IsNotFound(err) {
t.Fatalf("found etcdCluster resource with negative size: %s", err)
}

return ctx
},
)

_ = testEnv.Test(t, feature.Feature())
}

func TestClusterHealthy(t *testing.T) {
feature := features.New("etcd-operator-controller")

Expand Down
Loading