Propose the spec change for CRD #3

swapnilgm · 2019-10-01T05:54:53Z

Signed-off-by: Swapnil Mhamane [email protected]

** What this PR does? **
I have updated the PR.

Updated the CRD spec.
Changed api version from v1 to v1alpha1
Changed the domain from sapcloud.io -> gardener.cloud

** Comments for reviewers **
⚠️ As there are some spec field changes from variable to pointer. The controller logic doesn't have associated changes like nil checks. This PR is made ready enough to unblock @georgekuruvillak
and work ahead with CRD spec. Controller logic needs to be updated accordingly.

I'm not sure about introducing all those fields all the fields from start in status. We should introduce fields as and when we are sure that we require it. But since i'm not yet completely gone through the controller logic, and considering other task priorities, may i'll propose the status changes in separate PR later on. Till then @georgekuruvillak it would be nice if you could keep it to minimum.

georgekuruvillak · 2019-10-01T06:24:31Z

config/crd/bases/druid.gardener.cloud_etcds.yaml

@@ -398,7 +406,7 @@ spec:
                  #default: 10485760
                deltaSnapshotPeriod:
                  type: string
-                  #default: 60s 
+                  default: 60s


defaults are in alpha and will work from 1.16

Thanks for info. I just kind of cherry picked from old PR. Will go though it again. Also, i will try to change the CRD version from v1 to v1alpha1

georgekuruvillak · 2019-10-01T06:27:09Z

api/v1/etcd_types.go

 	// +optional
-	Password string `json:"password,omitempty"`
-	// +required
-	InitialClusterToken string `json:"initialClusterToken"`


Should we remove InitialCluster details? It may come in handy when we go multi instance. Or do we add it then?

Yes. Precisely we are not yet sure about the mutli node setup. We can make if configurable better later than now.

amshuman-kr

I have made some suggestions. Could you PTAL?

api/v1/etcd_types.go

amshuman-kr · 2019-10-01T08:45:26Z

api/v1/etcd_types.go

+	Replicas        *int32                      `json:"replicas,omitempty"`
+	ReadyReplicas   *int32                      `json:"readyReplicas,omitempty"`
+	Ready           *bool                       `json:"ready,omitempty"`
+	UpdatedReplicas *int32                      `json:"updatedReplicas,omitempty"`


We will need a LabelSelector field in the Status if we want to implement Scale sub-resource, which we should.

LabeSelector is optional. So, didn't introduced here.

It is optional but the default behaviour if it is not supplied is quite useless. So, might be good to have it.

If there is no value under the LabelSelectorPath in the custom resource, the status selector value in the /scale subresource will default to the empty string.

Also,

It must be set to work with HPA.

Though we don't want to use HPA with our CRD but you never know which other component working with the Scale subresource expects a proper value here.

api/v1alpha1/groupversion_info.go

swapnilgm · 2019-10-01T10:08:55Z

@georgekuruvillak @amshuman-kr This is kind of cherry pick PR from old PR. I'll notify you once it is ready for review. Please don't waste you time on reviewing it now. Said that thanks for the suggestions provided till now.

ggaurav10 · 2019-10-01T10:22:48Z

config/crd/bases/druid.gardener.cloud_etcds.yaml

+      # statusReplicasPath defines the JSONPath inside of a custom resource that corresponds to Scale.Status.Replicas.
+      statusReplicasPath: .status.replicas
+      # labelSelectorPath defines the JSONPath inside of a custom resource that corresponds to Scale.Status.Selector.
+      # labelSelectorPath: .status.labelSelector


Why is this commented? This should be required, right?

Nope. its optional.

As Amshu suggested will uncomment it and add filed in status.

👍 Thanks.

swapnilgm · 2019-10-01T11:44:46Z

@amshuman-kr @georgekuruvillak This PR is ready for review. Please go though the description for scope of the PR.

amshuman-kr

LGTM except for the missing pod label selector in the status.

amshuman-kr · 2019-10-01T22:35:18Z

README.md

    kubectl.kubernetes.io/last-applied-configuration: |
-      {"apiVersion":"druid.sapcloud.io/v1","kind":"Etcd","metadata":{"annotations":{},"labels":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","role":"test"},"name":"test","namespace":"shoot--dev--i308301-1"},"spec":{"annotations":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","networking.gardener.cloud/to-dns":"allowed","networking.gardener.cloud/to-private-networks":"allowed","networking.gardener.cloud/to-public-networks":"allowed","role":"test"},"backup":{"deltaSnapshotMemoryLimit":104857600,"deltaSnapshotPeriod":"300s","etcdConnectionTimeout":"300s","etcdQuotaBytes":8589934592,"fullSnapshotSchedule":"0 */24 * * *","garbageCollectionPeriod":"43200s","garbageCollectionPolicy":"Exponential","imageRepository":"eu.gcr.io/gardener-project/gardener/etcdbrctl","imageVersion":"0.8.0-dev","port":8080,"pullPolicy":"IfNotPresent","resources":{"limits":{"cpu":"500m","memory":"2Gi"},"requests":{"cpu":"23m","memory":"128Mi"}},"snapstoreTempDir":"/var/etcd/data/temp"},"etcd":{"clientPort":2379,"defragmentationSchedule":"0 */24 * * *","enableTLS":false,"imageRepository":"quay.io/coreos/etcd","imageVersion":"v3.3.13","initialClusterState":"new","initialClusterToken":"new","metrics":"basic","pullPolicy":"IfNotPresent","resources":{"limits":{"cpu":"2500m","memory":"4Gi"},"requests":{"cpu":"500m","memory":"1000Mi"}},"serverPort":2380,"storageCapacity":"80Gi","storageClass":"gardener.cloud-fast"},"labels":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","networking.gardener.cloud/to-dns":"allowed","networking.gardener.cloud/to-private-networks":"allowed","networking.gardener.cloud/to-public-networks":"allowed","role":"test"},"pvcRetentionPolicy":"DeleteAll","replicas":1,"storageCapacity":"80Gi","storageClass":"gardener.cloud-fast","store":{"storageContainer":"shoot--dev--i308301-1--b3caa","storageProvider":"S3","storePrefix":"etcd-test","storeSecret":"etcd-backup"},"tlsClientSecret":"etcd-client-tls","tlsServerSecret":"etcd-server-tls"}}
+      {"apiVersion":"druid.gardener.cloud/v1","kind":"Etcd","metadata":{"annotations":{},"labels":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","role":"test"},"name":"test","namespace":"shoot--dev--i308301-1"},"spec":{"annotations":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","networking.gardener.cloud/to-dns":"allowed","networking.gardener.cloud/to-private-networks":"allowed","networking.gardener.cloud/to-public-networks":"allowed","role":"test"},"backup":{"deltaSnapshotMemoryLimit":104857600,"deltaSnapshotPeriod":"300s","etcdConnectionTimeout":"300s","etcdQuotaBytes":8589934592,"fullSnapshotSchedule":"0 */24 * * *","garbageCollectionPeriod":"43200s","garbageCollectionPolicy":"Exponential","imageRepository":"eu.gcr.io/gardener-project/gardener/etcdbrctl","imageVersion":"0.8.0-dev","port":8080,"pullPolicy":"IfNotPresent","resources":{"limits":{"cpu":"500m","memory":"2Gi"},"requests":{"cpu":"23m","memory":"128Mi"}},"snapstoreTempDir":"/var/etcd/data/temp"},"etcd":{"clientPort":2379,"defragmentationSchedule":"0 */24 * * *","enableTLS":false,"imageRepository":"quay.io/coreos/etcd","imageVersion":"v3.3.13","initialClusterState":"new","initialClusterToken":"new","metrics":"basic","pullPolicy":"IfNotPresent","resources":{"limits":{"cpu":"2500m","memory":"4Gi"},"requests":{"cpu":"500m","memory":"1000Mi"}},"serverPort":2380,"storageCapacity":"80Gi","storageClass":"gardener.cloud-fast"},"labels":{"app":"etcd-statefulset","garden.sapcloud.io/role":"controlplane","networking.gardener.cloud/to-dns":"allowed","networking.gardener.cloud/to-private-networks":"allowed","networking.gardener.cloud/to-public-networks":"allowed","role":"test"},"pvcRetentionPolicy":"DeleteAll","replicas":1,"storageCapacity":"80Gi","storageClass":"gardener.cloud-fast","store":{"storageContainer":"shoot--dev--i308301-1--b3caa","storageProvider":"S3","storePrefix":"etcd-test","storeSecret":"etcd-backup"},"tlsClientSecret":"etcd-client-tls","tlsServerSecret":"etcd-server-tls"}}


Please delete this.

api/v1alpha1/etcd_types.go

georgekuruvillak

Thanks for spending time to refactor. The spec looks nicer. I shall merge and fix any errors that may have resulted as part of the changes.

amshuman-kr

LGTM

Signed-off-by: Swapnil Mhamane <[email protected]>

swapnilgm · 2019-10-14T10:57:10Z

Rebased and removed conflicts.

swapnilgm requested a review from a team as a code owner October 1, 2019 05:54

georgekuruvillak reviewed Oct 1, 2019

View reviewed changes

amshuman-kr suggested changes Oct 1, 2019

View reviewed changes

ggaurav10 suggested changes Oct 1, 2019

View reviewed changes

swapnilgm force-pushed the refator/crd-spec branch from 51a8cb7 to d4df760 Compare October 1, 2019 11:33

swapnilgm changed the title ~~[WIP] Propose the spec change for CRD~~ Propose the spec change for CRD Oct 1, 2019

amshuman-kr suggested changes Oct 1, 2019

View reviewed changes

swapnilgm force-pushed the refator/crd-spec branch 2 times, most recently from 273086f to 0a6c7ed Compare October 3, 2019 14:24

georgekuruvillak approved these changes Oct 9, 2019

View reviewed changes

amshuman-kr approved these changes Oct 14, 2019

View reviewed changes

swapnilgm force-pushed the refator/crd-spec branch from 0a6c7ed to 7393f81 Compare October 14, 2019 10:52

Propose the spec change for CRD

2ff20a8

Signed-off-by: Swapnil Mhamane <[email protected]>

swapnilgm force-pushed the refator/crd-spec branch from 7393f81 to 2ff20a8 Compare October 14, 2019 10:56

georgekuruvillak merged commit bb34d88 into gardener:master Oct 15, 2019

swapnilgm deleted the refator/crd-spec branch October 15, 2019 08:33

unmarshall mentioned this pull request Apr 3, 2024

upgraded go version to 1.22.1 #780

Closed

unmarshall mentioned this pull request Jul 3, 2024

Update golang version to 1.22 and adapt loop var improvements #826

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Propose the spec change for CRD #3

Propose the spec change for CRD #3

swapnilgm commented Oct 1, 2019 •

edited

Loading

georgekuruvillak Oct 1, 2019

swapnilgm Oct 1, 2019

georgekuruvillak Oct 1, 2019

swapnilgm Oct 1, 2019

amshuman-kr left a comment

amshuman-kr Oct 1, 2019

swapnilgm Oct 1, 2019

amshuman-kr Oct 1, 2019

swapnilgm commented Oct 1, 2019 •

edited

Loading

ggaurav10 Oct 1, 2019

swapnilgm Oct 1, 2019

swapnilgm Oct 3, 2019 •

edited

Loading

amshuman-kr Oct 3, 2019

swapnilgm commented Oct 1, 2019

amshuman-kr left a comment

amshuman-kr Oct 1, 2019

georgekuruvillak left a comment

amshuman-kr left a comment

swapnilgm commented Oct 14, 2019

Propose the spec change for CRD #3

Propose the spec change for CRD #3

Conversation

swapnilgm commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapnilgm commented Oct 1, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapnilgm Oct 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swapnilgm commented Oct 1, 2019

amshuman-kr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

georgekuruvillak left a comment

Choose a reason for hiding this comment

amshuman-kr left a comment

Choose a reason for hiding this comment

swapnilgm commented Oct 14, 2019

swapnilgm commented Oct 1, 2019 •

edited

Loading

swapnilgm commented Oct 1, 2019 •

edited

Loading

swapnilgm Oct 3, 2019 •

edited

Loading