ETCD-349: add etcdmemberlister controller #962

Elbehery · 2022-11-07T18:03:24Z

resolves https://issues.redhat.com/browse/ETCD-349

cc @tjungblu @hasbro17 minimal controller to discuss on this PR the right directions

Elbehery · 2022-11-07T19:53:05Z

pkg/operator/clustermembercontroller/clustermembercontroller.go

-func (c *ClusterMemberController) allNodesMapToVotingMembers(nodes []*corev1.Node) ([]*corev1.Node, error) {
+// allNodesMapToNonVotingMembers returns nodes that don't map to voting members (i.e. non learner) in the etcd cluster membership.
+// The voting members are read from the etcd cluster membership.
+func (c *ClusterMemberController) allNodesMapToNonVotingMembers(nodes []*corev1.Node) ([]*corev1.Node, error) {


@hasbro17 I think this name reflects more what the function does, isn't it ?

cc @tjungblu

tjungblu · 2022-11-08T09:24:11Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+type EtcdMemberListerController struct {
+	operatorClient v1helpers.OperatorClient
+	etcdClient     etcdcli.EtcdClient
+	membersCache   map[string]*etcdserverpb.Member


I think this will need some kind of locking, given this will be consumed by other controllers?

tjungblu · 2022-11-08T09:25:47Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+	err := c.syncEtcdMembers(ctx, syncCtx.Recorder())
+	if err != nil {
+		_, _, updateErr := v1helpers.UpdateStatus(ctx, c.operatorClient, v1helpers.UpdateConditionFn(operatorv1.OperatorCondition{
+			Type:    "EtcdMemberListerControllerDegraded",


we should only degrade if this fails for several consecutive times, a transient failure every 30s shouldn't be a big issue to degrade the whole operator on

done 👍🏽

Elbehery · 2022-11-08T10:20:40Z

@tjungblu thanks a lot for your review ..

I just want to clarify what this controller is going to do

cache All etcd members
cache healthy etcd members
cache unhealthy etcd members

I have chosen to use t map[string]*Member. If this choice is not adequate please let me know your thoughts

Also please correct me if I am wrong :)

tjungblu · 2022-11-08T10:22:26Z

this 👍 cache All etcd members

Elbehery · 2022-11-08T12:42:13Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+	errKeyNotExist     = errors.New("key does not exist")
+)
+
+type membersCache struct {


@tjungblu would this make sense ? .. I am gonna embed this within the controller

I think the struct makes sense, I'm not sure why you wanted to have it as a dedicated controller (or maybe I did).

it's more sensible to me to put this into etcdClientGetter struct in the etcdcli, then MemberList can always return a cached response or fetch when outdated.

alternatively, for choice of different consumers, we could also have another layer on top of etcdcli that would implement a cache for the MemberLister / HealthyMemberLister/UnhealthyMemberLister interface in the same package

something along those lines:

package etcdcli import ( "context" "go.etcd.io/etcd/api/v3/etcdserverpb" ) type CachedMembers struct { // some caching logic } func (c CachedMembers) MemberList(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") } func (c CachedMembers) VotingMemberList(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") } func (c CachedMembers) HealthyMembers(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") } func (c CachedMembers) HealthyVotingMembers(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") } func (c CachedMembers) UnhealthyMembers(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") } func (c CachedMembers) UnhealthyVotingMembers(ctx context.Context) ([]*etcdserverpb.Member, error) { //TODO implement me panic("implement me") }

Elbehery · 2022-11-08T13:01:56Z

/label tide/merge-method-squash

Elbehery · 2022-11-08T17:39:28Z

/retest-required

hasbro17 · 2022-11-09T05:12:11Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+func (mc *membersCache) add(key string, value *etcdserverpb.Member) (bool, error) {
+	if mc == nil || mc.cache == nil {
+		return false, errNilMembersCache
+	}
+	defer mc.mux.Unlock()
+
+	mc.mux.Lock()
+	_, ok := mc.cache[key]
+	if ok {
+		return false, errKeyExist
+	}
+	// safe to add
+	mc.cache[key] = *value
+	return true, nil
+}
+
+func (mc *membersCache) update(key string, value *etcdserverpb.Member) (bool, error) {
+	if mc == nil || mc.cache == nil {
+		return false, errNilMembersCache
+	}
+	defer mc.mux.Unlock()
+
+	mc.mux.Lock()
+	_, ok := mc.cache[key]
+	if !ok {
+		return false, errKeyNotExist
+	}
+	// safe to update
+	mc.cache[key] = *value
+	return true, nil
+}


Do we really need add/update/delete/get methods on the member cache if we're just outright replacing the cache when the membership changes?
If not, then we can remove these.

yes u r right, initially i was using them to update the cache with new entries and remove stale entries .. but we dont need them now, removing ...

hasbro17 · 2022-11-09T05:24:23Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+	// build map from current live members
+	currentMembersMap, errs := c.currentMembersMap(members)
+	if len(errs) > 0 {
+		syncErrs = append(syncErrs, errs...)


Why are we continuing to check the cache here if we have errors from building the current members map?
Shouldn't we return the errors?

well I did not know what to do tbh :)

So I can proceed with building the map while accumulating errors as I do now, or fail-fast once i encounter an error

wdyt ?

cc @tjungblu

fail-fast, but only degrade the operator after several consecutive failures.

tjungblu · 2022-11-09T09:29:37Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+
+	// check membership changes against members cache
+	if !reflect.DeepEqual(currentMembersMap, c.membersCache) {
+		klog.V(2).Infof("detected changes in etcd cluster membership, updating controller cache")


can you add the previous members and the current members? I've done something similar in the client and it's super useful when going through must gathers

tjungblu · 2022-11-09T09:30:18Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+var (
+	errNilMembersCache = errors.New("member cache is nil")
+	errKeyExist        = errors.New("key already exist")
+	errKeyNotExist     = errors.New("key does not exist")


I think the errors can also be deleted now that you've removed the diff/add/remove logic

tjungblu · 2022-11-09T15:09:53Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+		c.lastConsecutiveFailures++
+		if c.lastConsecutiveFailures >= 5 {
+			// reset counter before going degraded
+			c.lastConsecutiveFailures = 0


this doesn't count ConsecutiveFailures, but accumulates all failures ever happened until you reset after five times. You need to reset the counter when it was successful too.

+1, thanks a lot 👍🏽

Elbehery · 2022-11-09T17:36:37Z

/retest-required

Elbehery · 2022-11-09T18:25:44Z

pkg/operator/clustermembercontroller/clustermembercontroller.go

@@ -52,6 +55,9 @@ type ClusterMemberController struct {

 	masterNodeLister   corev1listers.NodeLister
 	masterNodeSelector labels.Selector
+	memberListerCtrl   *etcdmemberlistercontroller.EtcdMemberListerController
+	members            []etcdserverpb.Member


this controller has two usage of memberlist()

one uses to validate if a member is already part of the cluster

one iterates over the members and attempt to promote learners

Therefore i added both these fields and i update them on notifications from etcdmemberlistercontroller

please let me know wdyt cc @tjungblu @hasbro17

Elbehery · 2022-11-09T18:28:02Z

pkg/operator/starter.go

@@ -316,6 +323,7 @@ func RunOperator(ctx context.Context, controllerContext *controllercmd.Controlle
 		configInformers.Config().V1().Networks(),
 		etcdClient,
 		controllerContext.EventRecorder,
+		&etcdmemberlistercontroller,


this line does not compile, i do not know how to fix it :/

So the NewClusterMemberController returns a factory.Controller type . I need to have a ref for the etcdmemberlistercontroller to update the local cache in each controller upon notifications

please let me know wdyt

I suggest you to create an interface that is returned by NewEtcdMemberListerController instead. You're also missing to actually run that controller.

Again, just mimick what envVarController already does here.

fixed 👍🏽

Elbehery · 2022-11-09T18:34:22Z

pkg/operator/clustermembercontroller/clustermembercontroller.go

-	if err != nil {
-		return fmt.Errorf("could not get etcd member list: %v", err)
-	}
-


this call will be removed with my changes .. so instead of issuing a request to etcd-server, the controller retrieve them locally .. etcdmemberlistercontroller makes sure other controllers do not have stale cached data

tjungblu · 2022-11-10T09:19:50Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

@@ -9,6 +9,7 @@ import (
 	"go.etcd.io/etcd/api/v3/etcdserverpb"

 	errorsutil "k8s.io/apimachinery/pkg/util/errors"
+	interfaces "k8s.io/apiserver/pkg/server/dynamiccertificates"


that looks like a weird thing to import, why is that needed?

i found Notifier and Listener already defined there so I used them

I can also create my own interface

what's the envvar controller using?

it's own interfaces .. I created mine, I can use others .. maybe move them into commons ?

tjungblu · 2022-11-10T09:21:34Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+		}
+	}
+	c.listeners = append(c.listeners, listener)
+	return


why return?

tjungblu · 2022-11-10T09:22:46Z

pkg/operator/etcdmemberlistercontroller/etcdmemberlistercontroller.go

+	members := make([]etcdserverpb.Member, 0, len(c.membersCache.cache))
+	defer c.membersCache.mux.RUnlock()
+
+	c.membersCache.mux.RLock()


it's not wrong what you wrote here, but for better readability I would put the locking to the start of the method:

c.membersCache.mux.RLock() defer c.membersCache.mux.RUnlock() members := make([]etcdserverpb.Member, 0, len(c.membersCache.cache)) ...

So when people add more lines later, they have an easier time to reason about the scope of the lock.

tjungblu · 2022-11-10T09:29:17Z

pkg/operator/clustermembercontroller/clustermembercontroller.go

-		if peerURL == m.PeerURLs[0] {
-			return true
-		}
+func (c *ClusterMemberController) Enqueue() {


I think this can create a race condition. Imagine this controller is running every 10s on a different goroutine than the member lister controller.

The updates done in this Enqueue method might not be fully visible during the controller sync. I like this idea with the set, but you maybe just want to implement it in the boring way by listing all members when the controller sync runs and just reuse that return throughout the whole lifecycle of the sync.

OR alternatively add some locking around it, not sure it's really worth it because the controller runs never the less. If you were to tie the controller sync period to only run when the member listing changes, that would be useful but likely not necessary.

I am a bit confused now :D

So Enqueue is invoked by the etcdmemberlistercontroller when the member list changes only. it updates it own cache and then sync Listener's caches by invoking Enqueue() .. Moreover, Enqueue calls
(c *EtcdMemberListerController) GetMembers() which uses the lock before reading the cache and returning a copy to the Listener

Listener's Sync will use the local updated cache to reconcile

Where is the race condition ?

simply put, you're adjusting the internal structure of clustermembercontroller here that runs on a different controller goroutine.

Enqueue is called from the etcdmemberlistercontroller controller goroutine, so you need to lock here in this controller as well if you want to update the struct that it's reading. Otherwise you'd have stale reads from cpu cache, that's the race condition.

I'm sure if you were to run both controllers in goroutines in a test with -race it would flag that.

i see it now, but what is the right way then, without using extra locking ? .. i think we should reduce this, or ?

in clustermembercontroller.sync I would just call getMembers() as it was with the client. This is properly locked and you always get consistent data back for the lifetime of the sync call. We can add the notification fanciness later to only run the controller when the member list changes.

yes I agree, I am also using []Member instead of map[string]Member since the controller need not lookup by memberIP ..

I am adding also GetHealthyMember and GetUnhealthyMembers so that other controllers need not to use the etcdclient directly for any member listing

WIP .. :D

Elbehery · 2022-11-12T18:42:36Z

/retest-required

Elbehery · 2022-11-12T19:45:03Z

/retest-required

Elbehery · 2022-11-12T20:31:19Z

i gave up :D

Elbehery · 2022-11-15T21:02:45Z

/retest-required

openshift-bot · 2023-02-21T09:01:09Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2023-03-24T00:34:16Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2023-04-23T08:00:36Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2023-04-23T08:01:08Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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/test-infra repository.

Elbehery · 2024-02-14T17:14:40Z

/reopen

Elbehery · 2024-02-14T17:14:47Z

/remove-lifecycle rotten

Elbehery · 2024-02-14T17:24:02Z

/reopen

openshift-ci-robot · 2024-02-14T17:26:46Z

@Elbehery: This pull request references ETCD-349 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

resolves https://issues.redhat.com/browse/ETCD-349

cc @tjungblu @hasbro17 minimal controller to discuss on this PR the right directions

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-02-14T17:27:19Z

@Elbehery: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

openshift-ci · 2024-02-14T17:35:35Z

@Elbehery: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

openshift-ci · 2024-02-14T17:40:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Elbehery

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Elbehery]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 7, 2022

openshift-ci bot requested review from hasbro17 and tjungblu November 7, 2022 18:04

Elbehery commented Nov 7, 2022

View reviewed changes

tjungblu reviewed Nov 8, 2022

View reviewed changes

Elbehery commented Nov 8, 2022

View reviewed changes

Elbehery force-pushed the add-etcdmemberlister-controller branch from 18709d2 to eadaf17 Compare November 8, 2022 12:45

openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 8, 2022

Elbehery force-pushed the add-etcdmemberlister-controller branch from de78602 to 4065364 Compare November 8, 2022 16:39

hasbro17 reviewed Nov 9, 2022

View reviewed changes

Elbehery force-pushed the add-etcdmemberlister-controller branch from d83c682 to c99c538 Compare November 9, 2022 05:50

tjungblu reviewed Nov 9, 2022

View reviewed changes

Elbehery force-pushed the add-etcdmemberlister-controller branch from 9eac35d to eec479c Compare November 9, 2022 18:23

Elbehery commented Nov 9, 2022

View reviewed changes

tjungblu reviewed Nov 10, 2022

View reviewed changes

Elbehery force-pushed the add-etcdmemberlister-controller branch from eec479c to d71b42f Compare November 10, 2022 09:29

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 21, 2023

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 24, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 23, 2023

openshift-ci bot closed this Apr 23, 2023

openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 14, 2024

openshift-ci bot reopened this Feb 14, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 14, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2024

Elbehery force-pushed the add-etcdmemberlister-controller branch from 8f8dac4 to 058308c Compare February 14, 2024 18:17

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2024

ETCD-349: add etcd cachedmembers

374195c

Elbehery force-pushed the add-etcdmemberlister-controller branch from 058308c to 374195c Compare February 21, 2024 15:05

Elbehery closed this Feb 21, 2024

Elbehery deleted the add-etcdmemberlister-controller branch February 21, 2024 15:10

ETCD-349: add etcdmemberlister controller #962

ETCD-349: add etcdmemberlister controller #962

Conversation

Elbehery commented Nov 7, 2022

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

Elbehery commented Nov 8, 2022

tjungblu commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjungblu Feb 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Elbehery commented Nov 8, 2022

Elbehery commented Nov 8, 2022

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

Choose a reason for hiding this comment

Elbehery commented Nov 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjungblu Nov 10, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjungblu Nov 10, 2022 • edited

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

Elbehery commented Nov 12, 2022

Elbehery commented Nov 12, 2022

Elbehery commented Nov 12, 2022

Elbehery commented Nov 15, 2022

openshift-bot commented Feb 21, 2023

openshift-bot commented Mar 24, 2023

openshift-bot commented Apr 23, 2023

openshift-ci bot commented Apr 23, 2023

Elbehery commented Feb 14, 2024

Elbehery commented Feb 14, 2024

Elbehery commented Feb 14, 2024

openshift-ci-robot commented Feb 14, 2024 • edited by openshift-ci bot

openshift-ci bot commented Feb 14, 2024

openshift-ci bot commented Feb 14, 2024

openshift-ci bot commented Feb 14, 2024

tjungblu Feb 21, 2024 •

edited

tjungblu Nov 10, 2022 •

edited

tjungblu Nov 10, 2022 •

edited

openshift-ci-robot commented Feb 14, 2024 •

edited by openshift-ci bot