Decouple backup ready condition for snapshot leases and remove support for deprecated fields in etcd status #820

anveshreddy18 · 2024-06-26T16:32:29Z

How to categorize this PR?

/area usability monitoring
/kind enhancement cleanup

What this PR does / why we need it:

This PR splits the existing BackupReady condition which talks about both full and delta snapshot health as a single condition into two new conditions FullSnapshotBackupReady and DeltaSnapshotBackupReady, one for each type of snapshot i.e Full and Delta respectively. This helps in better understanding of status of each backup type thus helping debug issues and analyze more effectively. The existing BackupReady condition will still be present, which aggregates both the individual snapshot conditions into one.

This PR also removes the support for few fields in etcd status which are in deprecated state for enough time. The fields that are removed are ServiceName, ClusterSize, UpdatedReplicas and LabelSelector. These fields were essentially taken from statefulset and not necessarily needed to be present under etcd.status hence they were deprecated and this PR stops the support.

This PR partially fixes this issue for streamlining etcd status

Which issue(s) this PR fixes:

Special notes for your reviewer:

Release note:

The existing `BackupReady` condition in `etcd.status` is split into 2 new specific conditions `FullSnapshotBackupReady` & `DeltaSnapshotBackupReady` to better understand the health of each type of snapshot backup

Support for fields `ServiceName`, `ClusterSize`,  `UpdatedReplicas` and `LabelSelector`  in `etcd.status`  are removed after they've been deprecated for 4 minor releases

anveshreddy18 · 2024-06-26T17:30:03Z

/retest

anveshreddy18 · 2024-06-27T04:52:20Z

/retest

seshachalam-yv · 2024-07-09T09:01:33Z

/assign

seshachalam-yv

Please remove Cluster Size and Backup Ready.

etcd-druid/api/v1alpha1/etcd.go

Line 53 in d0f3163

    
           // +kubebuilder:printcolumn:name="Cluster Size",type=integer,JSONPath=`.spec.replicas`,priority=1

etcd-druid/api/v1alpha1/etcd.go

Line 51 in d0f3163

    
           // +kubebuilder:printcolumn:name="Backup Ready",type=string,JSONPath=`.status.conditions[?(@.type=="BackupReady")].status`

Add FullSnapshotBackupReady and DeltaSnapshotBackupReady as part of print columns

anveshreddy18 · 2024-07-10T06:30:27Z

@seshachalam-yv Nice find, thanks. Done

go.sum

test/e2e/etcd_backup_test.go

…cated fields in etcd.status

anveshreddy18 · 2024-08-27T08:07:51Z

/test pull-etcd-druid-e2e-kind

unmarshall

I think we need to pair program this. Current code needs a LOT of changes.

unmarshall · 2024-08-27T11:15:51Z

api/v1alpha1/etcd.go

 // +kubebuilder:printcolumn:name="Current Replicas",type=integer,JSONPath=`.status.currentReplicas`,priority=1
 // +kubebuilder:printcolumn:name="Ready Replicas",type=integer,JSONPath=`.status.readyReplicas`,priority=1
+// +kubebuilder:printcolumn:name="Backup Ready",type=string,JSONPath=`.status.conditions[?(@.type=="BackupReady")].status`
+// +kubebuilder:printcolumn:name="Full Backup Ready",type=string,JSONPath=`.status.conditions[?(@.type=="FullSnapshotBackupReady")].status`
+// +kubebuilder:printcolumn:name="Delta Backup Ready",type=string,JSONPath=`.status.conditions[?(@.type=="DeltaSnapshotBackupReady")].status`


Should we call them Delta Backup Ready or Delta Snapshot Backup Ready? Similarly for full snapshot.

unmarshall · 2024-08-27T11:19:44Z

docs/concepts/controllers.md

@@ -79,6 +79,8 @@ Status fields related to the etcd cluster itself, such as `Members`, `PeerUrlTLS
 - `AllMembersReady`: indicates readiness of all members of the etcd cluster.
 - `Ready`: indicates overall readiness of the etcd cluster in serving traffic.
 - `BackupReady`: indicates health of the etcd backups, i.e., whether etcd backups are being taken regularly as per schedule. This condition is applicable only when backups are enabled for the etcd cluster.
+- `FullSnapshotBackupReady`: indicates health of the full snapshot backups, i.e whether full snapshot backups are being taken regularly as per schedule. This condition is applicable only when backups are enabled for the etcd cluster.


in which situation will this be set to false? If one full snapshot is missed or more than one missed?

unmarshall · 2024-08-27T11:20:17Z

docs/concepts/controllers.md

@@ -79,6 +79,8 @@ Status fields related to the etcd cluster itself, such as `Members`, `PeerUrlTLS
 - `AllMembersReady`: indicates readiness of all members of the etcd cluster.
 - `Ready`: indicates overall readiness of the etcd cluster in serving traffic.
 - `BackupReady`: indicates health of the etcd backups, i.e., whether etcd backups are being taken regularly as per schedule. This condition is applicable only when backups are enabled for the etcd cluster.
+- `FullSnapshotBackupReady`: indicates health of the full snapshot backups, i.e whether full snapshot backups are being taken regularly as per schedule. This condition is applicable only when backups are enabled for the etcd cluster.
+- `DeltaSnapshotBackupReady`: indicates health of the delta snapshot backups, i.e whether delta snapshot backups are being taken regularly as per schedule. This condition is applicable only when backups are enabled for the etcd cluster.


What is the criteria for this value to be false?

unmarshall · 2024-08-27T11:20:59Z

docs/development/getting-started-locally.md

@@ -122,7 +122,7 @@ kubectl apply -f config/samples/druid_v1alpha1_etcd.yaml

 ### Verify the Etcd cluster

-To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the cluster size, readiness status of its members, and various other attributes.
+To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the current & ready replicas, readiness status of its members, Full and Delta backup status and various other attributes.


Suggested change

To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the current & ready replicas, readiness status of its members, Full and Delta backup status and various other attributes.

To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the current & ready replicas, readiness status of its members, Full Snapshot and Delta Snapshot backup status and various other attributes.

unmarshall · 2024-08-27T11:26:12Z

internal/health/condition/check_backup_ready.go

+	}
+
+	var FullSnapshotBackupReadyCheckResult, DeltaSnapshotBackupReadyCheckResult Result = nil, nil
+	for _, result := range b.results {


by using the loop variable result you are shadowing the variable result defined at the start of Check method. Please avoid shadowing.

unmarshall · 2024-08-27T11:56:08Z

internal/health/condition/check_backup_ready.go

 	)
-	fullSnapErr = a.cl.Get(ctx, types.NamespacedName{Name: getFullSnapLeaseName(&etcd), Namespace: etcd.ObjectMeta.Namespace}, fullSnapLease)
-	incrSnapErr = a.cl.Get(ctx, types.NamespacedName{Name: getDeltaSnapLeaseName(&etcd), Namespace: etcd.ObjectMeta.Namespace}, deltaSnapLease)
+	fullSnapErr = f.cl.Get(ctx, types.NamespacedName{Name: getFullSnapLeaseName(&etcd), Namespace: etcd.ObjectMeta.Namespace}, fullSnapLease)


if there is an error getting the full snapshot lease then is the error being ignored immediately but handled much later? If there is an error you should exit soon with error instead of continuing with the computations.

unmarshall · 2024-08-27T12:02:31Z

internal/health/condition/check_backup_ready.go

+	}
+}
+
+func (d *deltaSnapshotBackupReadyCheck) Check(ctx context.Context, etcd druidv1alpha1.Etcd) Result {


there is a LOT of code repetition and this code needs to be rewritten.

unmarshall · 2024-08-27T12:04:38Z

internal/health/status/check.go

@@ -36,7 +36,8 @@ var (
 	ConditionChecks = []ConditionCheckFn{
 		condition.AllMembersReadyCheck,
 		condition.ReadyCheck,
-		condition.BackupReadyCheck,
+		condition.FullSnapshotBackupReadyCheck,


you have removed BackupReadyCheck here but you have also changed and kept the implementation of BackupReady in check_backup_ready.go. Any reason for that?

unmarshall · 2024-08-27T12:05:12Z

internal/health/status/check.go

@@ -90,10 +91,12 @@ func (c *Checker) executeConditionChecks(ctx context.Context, etcd *druidv1alpha
 		wg.Wait()
 	})()

-	results := make([]condition.Result, 0, len(ConditionChecks))
+	results := make([]condition.Result, 0, len(ConditionChecks)+1)


why is this conditions checks + 1?

unmarshall · 2024-08-27T12:05:54Z

internal/health/status/check.go

 	for r := range resultCh {
 		results = append(results, r)
 	}
+	backupReadyChecker := condition.BackupReadyCheck(c.cl, results)


i really do not understand this. You removed it from ConditionChecks but introduce this here? why?

gardener-prow · 2024-11-14T09:59:43Z

@anveshreddy18: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-etcd-druid-check-renovate-config	`b0b9725`	link	true	`/test pull-etcd-druid-check-renovate-config`
pull-etcd-druid-integration	`b0b9725`	link	true	`/test pull-etcd-druid-integration`
pull-etcd-druid-unit	`b0b9725`	link	true	`/test pull-etcd-druid-unit`
pull-etcd-druid-verify-image-build	`b0b9725`	link	true	`/test pull-etcd-druid-verify-image-build`

Full PR test history. Your PR dashboard. Command help for this repository.
Please help us cut down on flakes by linking this test failure to an open flake report or filing a new flake report if you can't find an existing one. Also see our testing guideline for how to avoid and hunt flakes.

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. I understand the commands that are listed here.

anveshreddy18 requested a review from a team as a code owner June 26, 2024 16:32

gardener-robot added needs/review Needs review size/xl Size of pull request is huge (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else labels Jun 26, 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 26, 2024

gardener-robot-ci-2 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jun 26, 2024

gardener-robot added area/monitoring Monitoring (including availability monitoring and alerting) related area/usability Usability related kind/cleanup Something that is not needed anymore and can be cleaned up kind/enhancement Enhancement, improvement, extension labels Jun 26, 2024

gardener deleted a comment from gardener-robot Jun 26, 2024

anveshreddy18 self-assigned this Jun 26, 2024

anveshreddy18 force-pushed the decouple_snapshot_leases branch from 715260b to d0f3163 Compare June 26, 2024 16:43

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 26, 2024

gardener-robot-ci-1 removed 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 26, 2024

anveshreddy18 mentioned this pull request Jun 27, 2024

Add a new configurable field fullSnapshotLeaseUpdateInterval in spec.backup section of Etcd CR #764

Closed

gardener-robot assigned seshachalam-yv Jul 9, 2024

seshachalam-yv requested changes Jul 9, 2024

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Jul 9, 2024

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 10, 2024

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 18, 2024

seshachalam-yv reviewed Jul 23, 2024

View reviewed changes

go.sum Outdated Show resolved Hide resolved

seshachalam-yv requested changes Jul 23, 2024

View reviewed changes

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

seshachalam-yv removed their assignment Aug 19, 2024

gardener-robot added the needs/rebase Needs git rebase label Aug 21, 2024

gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 21, 2024

gardener deleted a comment from gardener-prow bot Aug 21, 2024

anveshreddy18 force-pushed the decouple_snapshot_leases branch from c696b9c to 4e2d378 Compare August 21, 2024 06:37

gardener-robot-ci-2 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 Aug 21, 2024

gardener-robot-ci-3 removed 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 Aug 21, 2024

anveshreddy18 force-pushed the decouple_snapshot_leases branch from 4e2d378 to 98b46de Compare August 21, 2024 06:40

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 Aug 21, 2024

gardener deleted a comment from gardener-robot Aug 21, 2024

gardener-robot-ci-3 removed 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 Aug 21, 2024

gardener-robot-ci-1 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 Aug 21, 2024

gardener-robot-ci-2 removed 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 Aug 21, 2024

Decouple full & delta snapshot backup ready conditions & Remove depre…

b0b9725

…cated fields in etcd.status

anveshreddy18 force-pushed the decouple_snapshot_leases branch from 9036d63 to b0b9725 Compare August 27, 2024 07:55

gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Aug 27, 2024

unmarshall requested changes Aug 27, 2024

View reviewed changes

anveshreddy18 removed their assignment Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple backup ready condition for snapshot leases and remove support for deprecated fields in etcd status #820

Decouple backup ready condition for snapshot leases and remove support for deprecated fields in etcd status #820

anveshreddy18 commented Jun 26, 2024 •

edited

Loading

anveshreddy18 commented Jun 26, 2024

anveshreddy18 commented Jun 27, 2024

seshachalam-yv commented Jul 9, 2024

seshachalam-yv left a comment

anveshreddy18 commented Jul 10, 2024

anveshreddy18 commented Aug 27, 2024

unmarshall left a comment

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

unmarshall Aug 27, 2024

gardener-prow bot commented Nov 14, 2024

	To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the current & ready replicas, readiness status of its members, Full and Delta backup status and various other attributes.
	To obtain information regarding the newly instantiated etcd cluster, perform the following step, which gives details such as the current & ready replicas, readiness status of its members, Full Snapshot and Delta Snapshot backup status and various other attributes.

Decouple backup ready condition for snapshot leases and remove support for deprecated fields in etcd status #820

Are you sure you want to change the base?

Decouple backup ready condition for snapshot leases and remove support for deprecated fields in etcd status #820

Conversation

anveshreddy18 commented Jun 26, 2024 • edited Loading

anveshreddy18 commented Jun 26, 2024

anveshreddy18 commented Jun 27, 2024

seshachalam-yv commented Jul 9, 2024

seshachalam-yv left a comment

Choose a reason for hiding this comment

anveshreddy18 commented Jul 10, 2024

anveshreddy18 commented Aug 27, 2024

unmarshall 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gardener-prow bot commented Nov 14, 2024

anveshreddy18 commented Jun 26, 2024 •

edited

Loading