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.

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

[Proposal] API Design for EtcdCluster resource #62

Closed
sergeyshevch opened this issue Mar 22, 2024 · 19 comments
Closed

[Proposal] API Design for EtcdCluster resource #62

sergeyshevch opened this issue Mar 22, 2024 · 19 comments
Milestone

Comments

@sergeyshevch
Copy link
Member

sergeyshevch commented Mar 22, 2024

Here is a design proposal for EtcdCluster resource that will cover more cases of real usage. Requesting for comments
Inspired by https://docs.victoriametrics.com/operator/api/#vmstorage

Cover scope of #61

---
-apiVersion: etcd.aenix.io/v1alpha1
+apiVersion: etcd.aenix.io/v1alpha2      
 kind: EtcdCluster
 metadata:
   name: test
   namespace: default
 spec:
   image: "quay.io/coreos/etcd:v3.5.12"
   replicas: 3
+  imagePullSecrets:  # core.v1.LocalObjectReference Ready k8s type
   - name: myregistrykey
+  serviceAccountName: default
+  podMetadata:
+    labels:
+      env: prod
+    annotations:
+      example.com/annotation: "true"
+  resources: # core.v1.ResourceRequirements Ready k8s type
+    requests:
+      cpu: 100m
+      memory: 100Mi
+    limits:
+      cpu: 200m
+      memory: 200Mi
+  affinity: {} # core.v1.Affinity Ready k8s type
+  nodeSelector: {} # map[string]string
+  tolerations: [] # core.v1.Toleration Ready k8s type
+  securityContext: {} # core.v1.PodSecurityContext Ready k8s type
+  priorityClassName: "low"
+  topologySpreadConstraints: [] # core.v1.TopologySpreadConstraint Ready k8s type
+  terminationGracePeriodSeconds: 30 # int64
+  schedulerName: "default-scheduler"
+  runtimeClassName: "legacy"
+  extraArgs: # map[string]string
+    arg1: "value1"
+    arg2: "value2"
+  extraEnvs: # []core.v1.EnvVar Ready k8s type
+    - name: MY_ENV
+      value: "my-value"
+  serviceSpec:
+    metadata:
+      labels:
+        env: prod
+      annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type
+  podDisruptionBudget:
+    maxUnavailable: 1 # intstr.IntOrString
+    minAvailable: 2
+    selectorLabels: # If not set, the operator will use the labels from the EtcdCluster
+      env: prod
+  readinessGates: [] # core.v1.PodReadinessGate Ready k8s type
+  storage:
+    volumeClaimTemplate:
+      metadata:
+        labels:
+          env: prod
+        annotations:
+          example.com/annotation: "true"
+      spec: # core.v1.PersistentVolumeClaimSpec Ready k8s type
+        storageClassName: gp3
+        accessModes: [ "ReadWriteOnce" ]
+        resources:
+          requests:
+            storage: 10Gi
+    emptyDir: {} # core.v1.EmptyDirVolumeSource Ready k8s type
-  storage:
-    persistence: true # default: true, immutable
-    storageClass: local-path
-    size: 10Gi
 status:
   conditions:
   - lastProbeTime: null
   lastTransitionTime: "2024-03-06T18:39:45Z"
  	status: "True"
  	type: Ready
@sergeyshevch sergeyshevch self-assigned this Mar 22, 2024
@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

+  nodeSelector: {} # map[string]string
+  tolerations: [] # core.v1.Toleration Ready k8s type
+  securityContext: {} # core.v1.PodSecurityContext Ready k8s type
+  priorityClassName: "low"
+  topologySpreadConstraints: [] # core.v1.TopologySpreadConstraint Ready k8s type
+  terminationGracePeriodSeconds: 30 # int64
+  schedulerName: "default-scheduler"
+  runtimeClassName: "legacy"
+  extraArgs: # map[string]string
+    arg1: "value1"
+    arg2: "value2"

I'd prefer this to move to some additional key. Not to overwhelm the base spec key. Like podSpec: or something similar

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

+  storage:
+    volumeClaimTemplate:
+      metadata:
+        labels:
+          env: prod
+        annotations:
+          example.com/annotation: "true"
+      spec: # core.v1.PersistentVolumeClaimSpec Ready k8s type
+        storageClassName: gp3
+        accessModes: [ "ReadWriteOnce" ]
+        resources:
+          requests:
+            storage: 10Gi
+    emptyDir: {} # core.v1.EmptyDirVolumeSource Ready k8s type

need to discuss if we want to enable all possible types of volumes, or restrict them to some safe subset. Because in fact core.v1.PersistentVolumeClaimSpec is very complex for basic use.

+  podDisruptionBudget:
+    maxUnavailable: 1 # intstr.IntOrString
+    minAvailable: 2
+    selectorLabels: # If not set, the operator will use the labels from the EtcdCluster
+      env: prod

we need binary flag: deploy pdb or not. Not just to have the values in the spec, I think. Or podDisruptionBudget: nil would be enough for not deploying it?

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

+  serviceSpec:
+    metadata:
+      labels:
+        env: prod
+      annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type

not enough as business logic supposes to have several type of services. Basic one - headless, and probably addiitonal one for clients? Should we give user option to choose if he wants the second? Or just deploy them both every time?

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

+  extraArgs: # map[string]string
+    arg1: "value1"
+    arg2: "value2"
+  extraEnvs: # []core.v1.EnvVar Ready k8s type
+    - name: MY_ENV
+      value: "my-value"

Should we have here some additional validation? Let's say that we pass --cluster-initial-state here which is already appended to spec by operator itself? It is where the abstraction leaks.

@sircthulhu
Copy link
Member

As of PDB spec, I think user of etcd cluster does not need such complex spec. Operator can configure maxUnavailable according to the number of replicas in cluster, only labels and annotations can be useful

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

serviceAccountName: default

then you need to also disable / or enable creation of custom service account...

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

Also I want to have a good hierarchy:

---
-apiVersion: etcd.aenix.io/v1alpha1
+apiVersion: etcd.aenix.io/v1alpha2      
 kind: EtcdCluster
 metadata:
   name: test
   namespace: default
 spec:
   image: "quay.io/coreos/etcd:v3.5.12"
   replicas: 3
+  podSpec:
+    imagePullSecrets:  # core.v1.LocalObjectReference Ready k8s type
     - name: myregistrykey
+    serviceAccountName: default
+    podMetadata:
+      labels:
+       env: prod
+     annotations:
+       example.com/annotation: "true"
+    resources: # core.v1.ResourceRequirements Ready k8s type
+      requests:
+        cpu: 100m
+        memory: 100Mi
+      limits:
+        cpu: 200m
+        memory: 200Mi
+    affinity: {} # core.v1.Affinity Ready k8s type
+    nodeSelector: {} # map[string]string
+    tolerations: [] # core.v1.Toleration Ready k8s type
+    securityContext: {} # core.v1.PodSecurityContext Ready k8s type
+    priorityClassName: "low"
+    topologySpreadConstraints: [] # core.v1.TopologySpreadConstraint Ready k8s type
+    terminationGracePeriodSeconds: 30 # int64
+    schedulerName: "default-scheduler"
+    runtimeClassName: "legacy"     
+  service-headless:
+    serviceSpec:
+      metadata:
+        labels:
+          env: prod
+        annotations:
+          example.com/annotation: "true"
+      spec: # core.v1.ServiceSpec Ready k8s type
+  service-main:
+    serviceSpec:
+      metadata:
+        labels:
+          env: prod
+        annotations:
+          example.com/annotation: "true"
+      spec: # core.v1.ServiceSpec Ready k8s type
+  storage:
+    volumeClaimTemplate:
+      metadata:
+        labels:
+          env: prod
+        annotations:
+          example.com/annotation: "true"
+      spec: # core.v1.PersistentVolumeClaimSpec Ready k8s type
+        storageClassName: gp3
+        accessModes: [ "ReadWriteOnce" ]
+        resources:
+          requests:
+            storage: 10Gi
+    emptyDir: {} # core.v1.EmptyDirVolumeSource Ready k8s type

something like this. Then in the editor I will be able to easily collapse / expand particular blocks with the technical details. I want to emphasize that I think that it is really great idea to separate the main business logic parameters (like number of replicas) from the deep technical things like service labels

@sergeyshevch
Copy link
Member Author

Updated spec addressing all comments

---
apiVersion: etcd.aenix.io/v1alpha1
kind: EtcdCluster
metadata:
 name: test
 namespace: default
spec:
  image: "quay.io/coreos/etcd:v3.5.12"
  replicas: 3
+ podSpec:
+   imagePullPolicy: "Always" # core.v1.PullPolicy Ready k8s type
+   imagePullSecrets:  # core.v1.LocalObjectReference Ready k8s type
+   - name: myregistrykey
+   podMetadata:
+     labels:
+       env: prod
+     annotations:
+       example.com/annotation: "true"
+   resources: # core.v1.ResourceRequirements Ready k8s type
+     requests:
+       cpu: 100m
+       memory: 100Mi
+    limits:
+       cpu: 200m
+       memory: 200Mi
+   affinity: {} # core.v1.Affinity Ready k8s type
+   nodeSelector: {} # map[string]string
+   tolerations: [] # core.v1.Toleration Ready k8s type
+   securityContext: {} # core.v1.PodSecurityContext Ready k8s type
+   priorityClassName: "low"
+   topologySpreadConstraints: [] # core.v1.TopologySpreadConstraint Ready k8s type
+   terminationGracePeriodSeconds: 30 # int64
+   schedulerName: "default-scheduler"
+   runtimeClassName: "legacy"
+   extraArgs: # map[string]string
+     arg1: "value1"
+     arg2: "value2"
+   extraEnvs: # []core.v1.EnvVar Ready k8s type
+     - name: MY_ENV
+       value: "my-value"
+ serviceAccountSpec: # TBD. How to represent it? Do we need ability to specify existing service account?
+   create: true
+   metadata:
+     labels:
+       env: prod
+     annotations:
+       example.com/annotation: "true"
+ serviceSpec:
+   client:
+     metadata:
+       labels:
+         env: prod
+       annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type
+   headless:
+     metadata:
+       labels:
+         env: prod
+       annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type
+ podDisruptionBudget:
+   maxUnavailable: 1 # intstr.IntOrString
+   minAvailable: 2
+   selectorLabels: # If not set, the operator will use the labels from the EtcdCluster
+     env: prod
+ readinessGates: [] # core.v1.PodReadinessGate Ready k8s type
+ storage:       # Discussed separately
+   volumeClaimTemplate:
+     metadata:
+       labels:
+         env: prod
+       annotations:
+         example.com/annotation: "true"
+     spec: # core.v1.PersistentVolumeClaimSpec Ready k8s type
+       storageClassName: gp3
+       accessModes: [ "ReadWriteOnce" ]
+       resources:
+         requests:
+           storage: 10Gi
+   emptyDir: {} # core.v1.EmptyDirVolumeSource Ready k8s type
- storage:
-   persistence: true # default: true, immutable
-   storageClass: local-path
-   size: 10Gi
status:
  conditions:
  - lastProbeTime: null
    lastTransitionTime: "2024-03-06T18:39:45Z"
    status: "True"
    type: Ready

@sergeyshevch
Copy link
Member Author

Should we have here some additional validation? Let's say that we pass --cluster-initial-state here which is already appended to spec by operator itself? It is where the abstraction leaks.

I don't thins it's necessary. I guess user can make anytinng that he want here

As of PDB spec, I think user of etcd cluster does not need such complex spec. Operator can configure maxUnavailable according to the number of replicas in cluster, only labels and annotations can be useful

I guess it's better to have ability to configure all fields in PDB. For example my cluster has policy that check PDB and require percent value in minAvailable

@sergeyshevch
Copy link
Member Author

#67 Implements storage spec from this proposal. All discussions about storage part can now be addressed here

@gecube
Copy link
Collaborator

gecube commented Mar 22, 2024

@sergeyshevch

I don't thins it's necessary. I guess user can make anytinng that he want here

no way. Anything that does not break etcd cluster.

I guess it's better to have ability to configure all fields in PDB. For example my cluster has policy that check PDB and require percent value in minAvailable

yes, I agree. I never argued with it. I am pointing different thing.

@sergeyshevch
Copy link
Member Author

@gecube Ok than we need to peform additional validation for extraArgs field. Added it to #69

@kvaps kvaps added this to the v0.0.2 milestone Mar 22, 2024
@AlexGluck
Copy link
Collaborator

I think we need image placed under podSpec.

@gecube
Copy link
Collaborator

gecube commented Mar 23, 2024

@AlexGluck I don't think so, because it is much more important thing than affinity, tolerations etc. But let it be.

@sergeyshevch sergeyshevch pinned this issue Mar 24, 2024
@sergeyshevch
Copy link
Member Author

Updated spec after merge of #69. Other spec fields is implemented and removed from this diff

---
apiVersion: etcd.aenix.io/v1alpha1
kind: EtcdCluster
metadata:
 name: test
 namespace: default
spec:
  image: "quay.io/coreos/etcd:v3.5.12"
  replicas: 3
+ serviceAccountSpec: # TBD. How to represent it? Do we need ability to specify existing service account?
+   create: true
+   metadata:
+     labels:
+       env: prod
+     annotations:
+       example.com/annotation: "true"
+ serviceSpec:
+   client:
+     metadata:
+       labels:
+         env: prod
+       annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type
+   headless:
+     metadata:
+       labels:
+         env: prod
+       annotations:
+        example.com/annotation: "true"
+    spec: # core.v1.ServiceSpec Ready k8s type
+ podDisruptionBudget:
+   maxUnavailable: 1 # intstr.IntOrString
+   minAvailable: 2
+   selectorLabels: # If not set, the operator will use the labels from the EtcdCluster
+     env: prod
+ readinessGates: [] # core.v1.PodReadinessGate Ready k8s type

@prometherion
Copy link

I was taking a look at the specification, and noticed this:

// Replicas is the count of etcd instances in cluster.
// +optional
// +kubebuilder:default:=3
// +kubebuilder:validation:Minimum:=0
Replicas *int32 `json:"replicas,omitempty"`

In the context of etcd an even number of instances is non-sense, wondering if we could take advantage of the +kubebuilder:validation:MultipleOf kubebuilder validation marker:

specifies that this field must have a numeric value that's a multiple of this one.

@gecube
Copy link
Collaborator

gecube commented Mar 30, 2024 via email

@prometherion
Copy link

Damn, you're right 😁
I thought we could use that for this kind of even/odd check 🤦🏻

@kvaps
Copy link
Member

kvaps commented Apr 2, 2024

Okay I'm going to close this proposal in favor #109.
Thank you for your design!

@kvaps kvaps closed this as completed Apr 2, 2024
@sergeyshevch sergeyshevch unpinned this issue Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

No branches or pull requests

6 participants