-
Notifications
You must be signed in to change notification settings - Fork 50
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
Create member, delta/full-snapshot Leases
#254
Conversation
8ac8416
to
83258d9
Compare
83258d9
to
560a431
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general comment, I am a bit intimidated by the Etcd
component that is introduced in this PR. Yes, it's fairly small now, but it already makes a commitment to refactoring the Etcd
controller in a similar fashion in the future, which is a rather huge refactoring. Until this done, the unfinished Etcd
component will stick out badly and beg for attention.
Also, the way components are used here is markedly different from the way they are used in g/g. Here, the Etcd
component is basically taking over part of the Reconcile
. In g/g, components are responsible for deploying / destroying / waiting upon specific objects such as extensions, dns resources, k8s resources such as deployments and services, etc. that have to be deployed / destroyed as part of the very complex shoot reconciliation.
I would propose a somewhat different approach that is equally good (or better) in terms of modularity / testability but avoids making such commitments, and is also much more similar to the way components are used in g/g. Instead of an Etcd
component, we could introduce a Lease
component to actually create / update / delete an individual lease, and perhaps a MemberLease
to capture member lease specifics, as well as helper functions such as DeployOrDestroyLease
and DeployOrDestroyMemberLeases
with the logic currently in the respective Etcd
methods. The controller will then call these functions (in the future, perhaps as part of a flow, as in the g/g shoot controller).
pkg/component/etcd/etcd.go
Outdated
// GetDeltaSnapshotLeaseName returns the `Lease` name for delta snapshots. | ||
GetDeltaSnapshotLeaseName() string | ||
// GetDeltaSnapshotLeaseName returns the `Lease` name for full snapshots. | ||
GetFullSnapshotLeaseName() string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these methods should be removed. Instead, DeltaSnapshotLeaseName
and FullSnapshotLeaseName
could be added to Values
, set together with other Values
fields and used wherever needed. This is also the approach used in most g/g components. Get
methods should be used only when it's not possible to know a value in advance, e.g. you need to actually deploy an object in order to get a value from its status.
pkg/component/etcd/leases.go
Outdated
} | ||
} | ||
|
||
func (e *etcd) reconcileLease(ctx context.Context, lease *coordinationv1.Lease, enabled bool) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not reconciling a Lease object, just creating, updating, or deleting it (as part of the reconcilation of an Etcd object). The actual reconciliation of a Lease will be performed by the respective controller. Let's use a name that matches more closely what this method is doing. I propose deployOrDestroy
here though I guess deploy
, update
or createOrUpdate
would also be ok - but not reconcile
which has a very different meaning and is therefore confusing.
func (e *etcd) reconcileLease(ctx context.Context, lease *coordinationv1.Lease, enabled bool) error { | |
func (e *etcd) deployOrDestroyLease(ctx context.Context, lease *coordinationv1.Lease, enabled bool) error { |
pkg/component/etcd/leases.go
Outdated
return err | ||
} | ||
|
||
func (e *etcd) reconcileMemberLease(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is not reconciling a Lease object, it's creating, updating, or deleting multiple Lease objects (as part of the reconcilation of an Etcd object). The actual reconciliation of the Leases will be performed by the respective controller. Let's use a name that matches more closely what this method is doing. I propose deployOrDestroy
here though I guess deploy
, update
or createOrUpdate
would also be ok - but not reconcile
which has a very different meaning and is therefore confusing.
func (e *etcd) reconcileMemberLease(ctx context.Context) error { | |
func (e *etcd) deployOrDestroyMemberLeases(ctx context.Context) error { |
@@ -1206,6 +1218,10 @@ func (r *EtcdReconciler) getMapFromEtcd(etcd *druidv1alpha1.Etcd) (map[string]in | |||
etcdValues["etcdDefragTimeout"] = etcd.Spec.Etcd.EtcdDefragTimeout | |||
} | |||
|
|||
if etcd.Spec.Etcd.MemberHeartbeat != nil { | |||
etcdValues["heartbeatDuration"] = etcd.Spec.Etcd.MemberHeartbeat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Could we have matching names here, e.g. either both "heartbeatDuration" or both "memberHeartbeat" (or "memberHeartbeatDuration")?
pkg/component/etcd/etcd.go
Outdated
return fmt.Sprintf("full-snapshot-%s", e.values.EtcdName) | ||
} | ||
|
||
func (e *etcd) Deploy(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add doc comments on these exported methods? (Deploy
, Destroy
, etc.)
pkg/component/etcd/etcd.go
Outdated
func (e *etcd) Destroy(_ context.Context) error { | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is Destroy
not implemented? Shouldn't it delete all the leases deployed by Deploy
?
I will continue working on this PR after the |
560a431
to
32e47ad
Compare
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 byetcd-backup-restore
.Which issue(s) this PR fixes:
Fixes part of #158
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
vsPATCH
) or how merging objects will be implemented. The same approach was taken ingardener/gardener
.Please see this comment for more information.
I changed the
role
,rolebinding
andserviceaccount
names along the way (721f412) to more comply with the scheme used in Gardener components (/cc @aaronfern)Release note: