Enhance Etcd configMap to provide consistency and controllability #771

anveshreddy18 · 2024-02-27T11:26:33Z

How to categorize this PR?

/area quality
/kind cleanup
/kind enhancement

What this PR does / why we need it:

This PR is to cleanup the Etcd ConfigMap to :

Rename the configMap with the prefix of etcd.Name instead of just having etcd. This is for consistency with other resources that gets created as part of Etcd Reconciliation, like leases, role, rolebinding, etc. This also involves cleanup logic for the existing configMap with older name.
Make snapshot-count configurable with the flag snapshotCount from spec.etcd section of etcd CR thus the operator is able to control the no.of transactions before committing a snapshot to disk if they want to.
Use proper URLs for initial-advertise-peer-urls and advertise-client-urls properties of the configMap and thereby fixing [Enhancement] Simplify etcd configuration and use proper URL instead of using @ as separator #476

Which issue(s) this PR fixes:
Fixes #717

Special notes for your reviewer:

Release note:

Enhances Etcd configuration by organizing ConfigMap naming convention, enabling snapshot-count configuration, and rectifying URL issues for improved functionality and consistency

anveshreddy18 · 2024-02-27T11:42:41Z

/hold until backup-restore PR #715 is merged.

unmarshall

A lot of these changes we will have to be re-done once the #728 is opened up for review. This is a bit unfortunate. What i would suggest is either to include these changes in the refactor PR OR do it after that gets merged.

What happens for configmaps that are still having the old naming convention for all existing etcd-clusters created for shoots which are either active or hibernated?

unmarshall · 2024-03-01T07:12:30Z

api/v1alpha1/types_etcd.go

@@ -443,7 +446,7 @@ func (e *Etcd) GetServiceAccountName() string {

 // GetConfigmapName returns the name of the configmap for the Etcd.
 func (e *Etcd) GetConfigmapName() string {
-	return fmt.Sprintf("etcd-bootstrap-%s", string(e.UID[:6]))
+	return fmt.Sprintf("%s-bootstrap-%s", e.Name, (e.UID[:6]))


Maybe we should just name this as <etcd-name>-config-<etcd UID suffix>. There is no need of including bootstrap as part of the name IMHO.

The reason I left it like that is because the closed PR tried to do the same and since this is a revival of it, I went with what they proposed in it. But yeah that is good, I'll update it.

unmarshall · 2024-03-01T07:15:26Z

api/v1alpha1/types_etcd.go

@@ -208,6 +208,9 @@ type EtcdConfig struct {
 	// Quota defines the etcd DB quota.
 	// +optional
 	Quota *resource.Quantity `json:"quota,omitempty"`
+	// SnapshotCount defines the number of committed transactions to trigger a snapshot to disk
+	// +optional
+	SnapshotCount *int `json:"snapshotCount,omitempty"`


once we include this as a configurable parameter, we will also have to check what happens when this parameter is updated since this is now going to be part of an etcd spec.

The description can simply be: configures the number of applied Raft entries to hold in-memory before compaction and also provide a link to https://etcd.io/docs/v3.4/op-guide/maintenance/#raft-log-retention

we will also have to check what happens when this parameter is updated since this is now going to be part of an etcd spec.

Sorry, I didn't get this. If the param is updated, then in the next reconciliation it will be reflected in the configMap. Is your question about how etcd container reacts to this change? I've tried this locally and it didn't affect the cluster in anyway. Pls correct me if this is about something else.

The description can simply be: configures the number of applied Raft entries to hold in-memory before compaction

Sure, this is more readable. I'll update.

unmarshall · 2024-03-01T07:19:58Z

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

@@ -563,6 +563,10 @@ spec:
                  serverPort:
                    format: int32
                    type: integer
+                  snapshotCount:
+                    description: SnapshotCount defines the number of committed transactions


adjust the description.

anveshreddy18 · 2024-03-01T09:51:26Z

@unmarshall thanks for the review.

either to include these changes in the refactor PR OR do it after that gets merged

That's fine, if that PR decides to incorporate these, then I'll close this. Else, we can merge this after that.

What happens for configmaps that are still having the old naming convention for all existing etcd-clusters created for shoots which are either active or hibernated?

For that, some temporary code has been added here to delete the old configmaps during the reconciliation. That can be removed once all the configmaps of every shoot namespace are deleted, probably after a few releases.

anveshreddy18 · 2024-06-25T07:16:51Z

This PR was closed automatically by Github due to diff becoming null at some point while rebasing the PR on #777. I didn't think about reopening it and instead created a new PR after rebasing. The new PR is #812, for reference.

anveshreddy18 requested a review from a team as a code owner February 27, 2024 11:26

anveshreddy18 mentioned this pull request Feb 27, 2024

Modify URL parsing for advertise-urls used by etcd sidecar gardener/etcd-backup-restore#715

Open

gardener-robot added the reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies label Feb 27, 2024

anveshreddy18 self-assigned this Feb 27, 2024

unmarshall reviewed Mar 1, 2024

View reviewed changes

shreyas-s-rao added this to the v0.24.0 milestone Mar 26, 2024

anveshreddy18 closed this Jun 24, 2024

anveshreddy18 force-pushed the changes/configMap branch from f247507 to fec5c09 Compare June 24, 2024 10:51

gardener-robot added status/closed Issue is closed (either delivered or triaged) size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Jun 24, 2024

gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jun 24, 2024

gardener-robot removed the size/m Size of pull request is medium (see gardener-robot robot/bots/size.py) label Jun 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhance Etcd configMap to provide consistency and controllability #771

Enhance Etcd configMap to provide consistency and controllability #771

anveshreddy18 commented Feb 27, 2024 •

edited

Loading

anveshreddy18 commented Feb 27, 2024

unmarshall left a comment

unmarshall Mar 1, 2024

anveshreddy18 Mar 1, 2024

unmarshall Mar 1, 2024

anveshreddy18 Mar 1, 2024

unmarshall Mar 1, 2024

anveshreddy18 commented Mar 1, 2024

anveshreddy18 commented Jun 25, 2024 •

edited

Loading

Enhance Etcd configMap to provide consistency and controllability #771

Enhance Etcd configMap to provide consistency and controllability #771

Conversation

anveshreddy18 commented Feb 27, 2024 • edited Loading

anveshreddy18 commented Feb 27, 2024

unmarshall left a comment

Choose a reason for hiding this comment

unmarshall Mar 1, 2024

Choose a reason for hiding this comment

anveshreddy18 Mar 1, 2024

Choose a reason for hiding this comment

unmarshall Mar 1, 2024

Choose a reason for hiding this comment

anveshreddy18 Mar 1, 2024

Choose a reason for hiding this comment

unmarshall Mar 1, 2024

Choose a reason for hiding this comment

anveshreddy18 commented Mar 1, 2024

anveshreddy18 commented Jun 25, 2024 • edited Loading

anveshreddy18 commented Feb 27, 2024 •

edited

Loading

anveshreddy18 commented Jun 25, 2024 •

edited

Loading