Create member, delta/full-snapshot `Leases` #262

timuthy · 2021-11-25T07:22:22Z

How to categorize this PR?

/area control-plane
/kind enhancement

What this PR does / why we need it:
This PR adds member, full snapshot and delta snapshot Leases which are maintained by etcd-backup-restore.

Which issue(s) this PR fixes:
Fixes part of #158
Fixes part of #278

Special notes for your reviewer:
A "component" concept was introduced in this PR which in the long run can replace the chart that we use and instead build the K8s resources within the code to achieve a better test-ability and more control how the client is used to reconcile the objects (e.g. used verb UPDATE vs PATCH) or how merging objects will be implemented. The same approach was taken in gardener/gardener.

Please see this comment for more information.

Release note:

Etcd-Druid now creates member `Lease` objects which enables the heartbeat functionality for etcd members. Along the way a new flag `--etcd-member-unknown-threshold` was introduced. It determines the duration after which a etcd member's state is considered `unknown` when the member `Lease` is not renewed.

timuthy · 2021-11-25T13:24:41Z

/invite @stoyanr

gardener-robot · 2021-11-28T03:28:38Z

@stoyanr You have pull request review open invite, please check

abdasgupta

Thanks Tim for a well-written PR. I just have few minor suggestions.

abdasgupta · 2021-12-01T17:22:34Z

api/v1alpha1/etcd_types.go

@@ -201,6 +201,9 @@ type EtcdConfig struct {
 	// EtcdDefragTimeout defines the timeout duration for etcd defrag call
 	// +optional
 	EtcdDefragTimeout *metav1.Duration `json:"etcdDefragTimeout,omitempty"`
+	// HeartbeatDuration defines the time members send a heartbeat. The default value is 10s.


Suggested change

// HeartbeatDuration defines the time members send a heartbeat. The default value is 10s.

// HeartbeatDuration defines the timeout for members to send heartbeats. The default value is 10s.

Can you explain why timeout? It's actually the duration after which a member renews the member lease.

Okay. Then, instead of time we should use duration

abdasgupta · 2021-12-01T17:24:19Z

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

@@ -293,6 +293,10 @@ spec:
                    description: EtcdDefragTimeout defines the timeout duration for
                      etcd defrag call
                    type: string
+                  heartbeatDuration:
+                    description: HeartbeatDuration defines the time members send a


See above suggestion regarding this

charts/etcd/templates/etcd-statefulset.yaml

abdasgupta · 2021-12-07T09:14:50Z

pkg/component/etcd/lease/values.go

+	"k8s.io/apimachinery/pkg/types"
+)
+
+type Values struct {


Instead of struct, should we make Values as map[string]interface{}? If we make it as struct, we need to pass separate structs to the getMapFromEtcd function for separate components in future. WDYT?

Hm yes, got your point. However, getMapFromEtcd will only be there as long as we have not entirely transitioned all manifests to components. For the transitioning phase it should be fine to do so, wdyt?

The awkward point about map[string]interface{} is the missing type safety for values and that you need to create constants for the keys, so that other components can use them.

Hmm.. Sadly, until getMapFromEtcd is removed completely, parameter list of getMapFromEtcd for each component type

I added another componentetcd.Values struct which aggregates the different sub values. I hope that's fine.

timuthy · 2021-12-08T15:38:11Z

Thanks a lot for your review @abdasgupta. Please see my comments.

aaronfern

Thanks @timuthy for the PR!
I have one suggestion and one minor nitpick

pkg/component/etcd/lease/values_helper.go

pkg/component/etcd/lease/lease_test.go

timuthy · 2021-12-20T13:38:00Z

/ping @abdasgupta

gardener-robot · 2021-12-20T13:38:04Z

@abdasgupta

Message

/ping @abdasgupta

gardener-robot · 2021-12-21T14:11:30Z

@timuthy You need rebase this pull request with latest master branch. Please check.

timuthy · 2021-12-21T14:24:23Z

Thanks a lot for your review @abdasgupta. I addressed your comments in 735d69c, PTAL.

abdasgupta · 2021-12-21T16:40:37Z

/lgtm

* Add lease component * Add HeartbeatDuration configuration * Add label selector for member check * Use etcd-member-unknown-threshold flag instead of LeaseDurationSeconds * Address review feedback * Address review feedback

timuthy requested a review from a team as a code owner November 25, 2021 07:22

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 Nov 25, 2021

gardener-robot added area/control-plane Control plane related kind/enhancement Enhancement, improvement, extension 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 Nov 25, 2021

gardener-robot-ci-1 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 Nov 25, 2021

timuthy mentioned this pull request Nov 25, 2021

Create member, delta/full-snapshot Leases #254

Closed

timuthy force-pushed the feature.lease-component branch from 6687900 to e2870a6 Compare November 25, 2021 09:07

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 Nov 25, 2021

timuthy force-pushed the feature.lease-component branch from e2870a6 to 9b4c140 Compare November 25, 2021 13:24

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 Nov 25, 2021

gardener-robot requested a review from stoyanr November 25, 2021 13:24

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 Nov 25, 2021

timuthy force-pushed the feature.lease-component branch from 9b4c140 to 282cdc9 Compare November 30, 2021 10:30

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 Nov 30, 2021

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 Nov 30, 2021

abdasgupta reviewed Dec 1, 2021

View reviewed changes

abdasgupta suggested changes Dec 7, 2021

View reviewed changes

gardener-robot added the needs/changes Needs (more) changes label Dec 7, 2021

timuthy mentioned this pull request Dec 7, 2021

Simplify reconciliation #208

Closed

timuthy mentioned this pull request Dec 9, 2021

Add service component and peer service #273

Merged

aaronfern requested changes Dec 16, 2021

View reviewed changes

pkg/component/etcd/lease/values_helper.go Outdated Show resolved Hide resolved

pkg/component/etcd/lease/lease_test.go Outdated Show resolved Hide resolved

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 Dec 17, 2021

gardener-robot removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Dec 17, 2021

gardener-robot added needs/rebase Needs git rebase needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Dec 21, 2021

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 Dec 21, 2021

timuthy added 6 commits December 21, 2021 15:13

Add lease component

124698c

Add HeartbeatDuration configuration

ce176c8

Add label selector for member check

8e20270

Use etcd-member-unknown-threshold flag instead of LeaseDurationSeconds

bebee1b

Address review feedback

de34b78

Address review feedback

735d69c

timuthy force-pushed the feature.lease-component branch from 0822067 to 735d69c Compare December 21, 2021 14:23

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 Dec 21, 2021

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 Dec 21, 2021

abdasgupta approved these changes Dec 21, 2021

View reviewed changes

gardener-robot added reviewed/lgtm Has approval for merging and removed needs/rebase Needs git rebase needs/second-opinion Needs second review by someone else labels Dec 21, 2021

abdasgupta mentioned this pull request Dec 21, 2021

Implement "component" concept for all the K8s resources #278

Closed

8 tasks

timuthy merged commit 0469cc9 into gardener:master Dec 22, 2021

timuthy deleted the feature.lease-component branch December 22, 2021 13:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create member, delta/full-snapshot `Leases` #262

Create member, delta/full-snapshot `Leases` #262

timuthy commented Nov 25, 2021 •

edited

Loading

timuthy commented Nov 25, 2021

gardener-robot commented Nov 28, 2021

abdasgupta left a comment

abdasgupta Dec 1, 2021

timuthy Dec 8, 2021

abdasgupta Dec 21, 2021

abdasgupta Dec 1, 2021

abdasgupta Dec 7, 2021

timuthy Dec 7, 2021

abdasgupta Dec 21, 2021

timuthy Dec 21, 2021

timuthy commented Dec 8, 2021

aaronfern left a comment

timuthy commented Dec 20, 2021

gardener-robot commented Dec 20, 2021

gardener-robot commented Dec 21, 2021

timuthy commented Dec 21, 2021

abdasgupta commented Dec 21, 2021

	// HeartbeatDuration defines the time members send a heartbeat. The default value is 10s.
	// HeartbeatDuration defines the timeout for members to send heartbeats. The default value is 10s.

Create member, delta/full-snapshot Leases #262

Create member, delta/full-snapshot Leases #262

Conversation

timuthy commented Nov 25, 2021 • edited Loading

timuthy commented Nov 25, 2021

gardener-robot commented Nov 28, 2021

abdasgupta 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

timuthy commented Dec 8, 2021

aaronfern left a comment

Choose a reason for hiding this comment

timuthy commented Dec 20, 2021

gardener-robot commented Dec 20, 2021

gardener-robot commented Dec 21, 2021

timuthy commented Dec 21, 2021

abdasgupta commented Dec 21, 2021

Create member, delta/full-snapshot `Leases` #262

Create member, delta/full-snapshot `Leases` #262

timuthy commented Nov 25, 2021 •

edited

Loading