member removal part two #768

p0lyn0mial · 2022-03-23T16:05:49Z

No description provided.

p0lyn0mial · 2022-03-24T07:02:05Z

/retest

p0lyn0mial · 2022-03-24T11:48:12Z

/assing @hasbro17 @deads2k

p0lyn0mial · 2022-03-28T11:54:04Z

/assing @hasbro17 @deads2k

p0lyn0mial · 2022-03-28T14:27:11Z

/retest

hasbro17

Some questions/notes about how we detect an excess of voting members when deciding to scale-down.

hasbro17 · 2022-03-28T23:07:40Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+	if len(memberMachines) <= desiredControlPlaneReplicasCount {
+		klog.V(4).Infof("haven't found a replacement machine, the number of desired control plane replicas: %d must be greater than the current number of machines that host an etcd member: %d", desiredControlPlaneReplicasCount, len(memberMachines))
+		return nil
+	}


the number of desired control plane replicas: %d must be greater than the current number of machines that host an etcd member

It's actually the opposite since we want an excess of voting member etcd machines or greater than the desired control-plane size, in order to select a replacement member to scale-down.
So we can probably make this log statement more accurate e.g:

Suggested change

if len(memberMachines) <= desiredControlPlaneReplicasCount {

klog.V(4).Infof("haven't found a replacement machine, the number of desired control plane replicas: %d must be greater than the current number of machines that host an etcd member: %d", desiredControlPlaneReplicasCount, len(memberMachines))

return nil

}

if len(memberMachines) <= desiredControlPlaneReplicasCount {

klog.V(4).Infof("Ignoring scale-down since the number of etcd voting member machines (%d) < desired number of control-plane replicas (%d) ", len(memberMachines), desiredControlPlaneReplicasCount)

return nil

}

hasbro17 · 2022-03-28T23:14:32Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+	if err != nil {
+		return err
+	}
+	memberMachines := ceohelpers.FilterMachinesWithMachineDeletionHook(masterMachines)


When deciding whether or not to scale-down a voting member this count should only be voting member machines.

Otherwise given desiredControlPlaneReplicasCount = 3 and if we have 3 voting + 1 learner members, then len(memberMachines) = 4 since learner member machines also have a deletion hook.
Deleting the voting member would trigger a scale-down since len(memberMachines) > desiredControlPlaneReplicasCount but now we've just replaced a voting member with a learner member (which may not get promoted).

So we also need to filter out the learner members here and change memberMachines => votingMemberMachines when considering scaling down a voting machine.

With the above you would still need to handle the cleanup of learner members pending deletion. That does not depend on len(memberMachines) <= desiredControlPlaneReplicasCount, but rather we remove the learner member whenever its machine is pending deletion.

I would suggest moving the learner member removal logic to its own function to make it simpler, but your call on addressing that wherever makes sense.

yes, you are right, I have updated the PR, please see if you like it.

it attempts to remove the member only once we have identified that a Machine resource is being deleted and a replacement member has been created

…roller it attempts to remove a learning member pending deletion regardless of whether a replacement member has been found

p0lyn0mial · 2022-03-29T10:36:31Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+	if err != nil {
+		return err
+	}
+	for _, member := range members {


at this point I don't think we have to check the type of the member (voting vs learner)

…ks for master machines the Machine Deletion Hook this controller reconciles is a mechanism within the Machine API that allow this operator to hold up removal of a machine

…eletion

p0lyn0mial · 2022-03-30T16:45:33Z

/retest

openshift-ci · 2022-03-30T18:05:41Z

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

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-five-control-plane-replicas	`5146ae7`	link	false	`/test e2e-gcp-five-control-plane-replicas`

Full PR test history. Your PR dashboard.

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

hasbro17

/approve

Still going through the unit tests but the member removal controller logic makes sense.

hasbro17 · 2022-03-31T05:56:11Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+			learningMachines = append(learningMachines, memberMachine)
+		}
+	}
+	return c.removeMemberPendingDeletion(ctx, learningMachines, "learning")


Just a general comment, not a suggestion to change :)
There is a simpler way to tell if members are learning machines by just doing a member list and then just filtering on the member.IsLearner field from the response.
Although the benefit of your approach is that it doesn't require a live call to the etcd server so this is fine too.

Although the benefit of your approach is that it doesn't require a live call to the etcd server so this is fine too.

yeah, I wanted to avoid a live call on every interation.

wallylewis · 2022-03-31T16:36:29Z

/lgtm

openshift-ci · 2022-03-31T16:38:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hasbro17, p0lyn0mial, wallylewis

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~~ [hasbro17]

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

deads2k · 2022-04-01T15:05:31Z

pkg/operator/clustermembercontroller/clustermembercontroller.go

+	}
+
+	if machineForEtcdHost, hasMachine := ceohelpers.IndexMachinesByNodeInternalIP(masterMachines)[etcdHost]; hasMachine && machineForEtcdHost.DeletionTimestamp != nil {
+		klog.V(4).Infof("won't add member: %v to the cluster because its machine is pending deletion: %v", etcdHost, machineForEtcdHost.Name)


I suggest v(2). This shouldn't happen often. I can probably agree one that one event every time may cause more problems than it fixes (though a rate limit may be in order), but you'll want to know this is happening when it happens.

i am okay with v(2)

deads2k · 2022-04-01T15:07:49Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

-	// stop if the machine API is not functional
+	var errs []error
+
+	if err := c.removeMemberWithoutMachine(ctx); err != nil {


the first check used to be, "do we have a machine API". Why would we remove a member without a machine if we don't have a machine API?

why not? A member without a machine won't' work.

ah, unless there won't be machine objects when the machine API is not functional

deads2k · 2022-04-01T15:12:02Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+	if err != nil {
+		return err
+	}
+	if currentVotingMemberIPListSet.Len() <= desiredControlPlaneReplicasCount {


I'm guessing you're doing this because deletionTimestamp set on a machine can mean either

this member should scale down now, or

this member should scale down after something else comes up.

However, there needs to be a better signal of this than a static installConfig that is never changed. Where is this intent actually present.

deads2k · 2022-04-01T15:12:49Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

@@ -149,6 +226,42 @@ func (c *clusterMemberRemovalController) sync(ctx context.Context, _ factory.Syn
 	return nil
 }

+func (c *clusterMemberRemovalController) removeMemberPendingDeletion(ctx context.Context, memberMachines []*machinev1beta1.Machine, memberType string) error {
+	memberMachinesPendingDeletion := ceohelpers.FilterMachinesPendingDeletion(memberMachines)


this method is missing in this PR, is it DeletionTimestamp != nil?

yes, https://github.com/openshift/cluster-etcd-operator/blob/master/pkg/operator/ceohelpers/common.go#L93

deads2k · 2022-04-01T15:14:46Z

pkg/operator/clustermemberremovalcontroller/clustermemberremovalcontroller.go

+			return err
+		}
+		if hasInternalIP(memberMachineToDelete, memberIP) {
+			memberLocator := fmt.Sprintf("[ url: %v, name: %v, id: %v ]", memberIP, member.Name, member.ID)


this is event worthy

MemberRemove fires an event

…al-part-two" This reverts commit 85c402e, reversing changes made to c48fd04.

Revert "Merge pull request #768 from p0lyn0mial/member-removal-part-two"

p0lyn0mial force-pushed the member-removal-part-two branch from 84743ea to b555615 Compare March 24, 2022 11:47

p0lyn0mial mentioned this pull request Mar 24, 2022

adds helpers functions used by the vertical scaling controllers #769

Merged

p0lyn0mial force-pushed the member-removal-part-two branch from b555615 to 76bd1b8 Compare March 28, 2022 10:42

p0lyn0mial changed the title ~~WIP: member removal part two~~ member removal part two Mar 28, 2022

p0lyn0mial force-pushed the member-removal-part-two branch from 76bd1b8 to 075082f Compare March 28, 2022 12:05

p0lyn0mial force-pushed the member-removal-part-two branch from 075082f to fddfdaf Compare March 28, 2022 15:35

hasbro17 requested changes Mar 28, 2022

View reviewed changes

hasbro17 mentioned this pull request Mar 28, 2022

reconciles machine deletion hooks #742

Closed

p0lyn0mial added 2 commits March 29, 2022 11:40

adds attemptToScaleDown to the memberremoval controller

1e0150a

it attempts to remove the member only once we have identified that a Machine resource is being deleted and a replacement member has been created

adds attemptToRemoveLearningMember to the cluster member removal cont…

e7e6468

…roller it attempts to remove a learning member pending deletion regardless of whether a replacement member has been found

p0lyn0mial force-pushed the member-removal-part-two branch from fddfdaf to e7e6468 Compare March 29, 2022 10:35

p0lyn0mial commented Mar 29, 2022

View reviewed changes

p0lyn0mial added 3 commits March 30, 2022 13:43

introduces MachineDeletionHooksController that reconciles machine hoo…

7e15521

…ks for master machines the Machine Deletion Hook this controller reconciles is a mechanism within the Machine API that allow this operator to hold up removal of a machine

vendor

904e2e0

wires MachineDeletionHooksController

50c92f5

p0lyn0mial force-pushed the member-removal-part-two branch 2 times, most recently from 3a56cc8 to c272151 Compare March 30, 2022 12:29

p0lyn0mial added 3 commits March 30, 2022 16:54

updates TestScalingUpAndDownSingleNode

f4fde63

ClusterMemberController adds members whose machines are not pending d…

6222e07

…eletion

vendor update

5146ae7

p0lyn0mial force-pushed the member-removal-part-two branch from c272151 to 5146ae7 Compare March 30, 2022 14:54

hasbro17 approved these changes Mar 31, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2022

openshift-ci bot assigned wallylewis Mar 31, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2022

openshift-merge-robot merged commit 85c402e into openshift:master Mar 31, 2022

deads2k reviewed Apr 1, 2022

View reviewed changes

p0lyn0mial mentioned this pull request Apr 1, 2022

clusterMemberRemovalController: run removeMemberWithoutMachine only when the machine API is on #775

Closed

stbenjam added a commit to stbenjam/cluster-etcd-operator that referenced this pull request Apr 4, 2022

Revert "Merge pull request openshift#768 from p0lyn0mial/member-remov…

14eaa16

…al-part-two" This reverts commit 85c402e, reversing changes made to c48fd04.

deads2k added a commit that referenced this pull request Apr 4, 2022

Merge pull request #778 from stbenjam/revert-vsphere-upi

98bc759

Revert "Merge pull request #768 from p0lyn0mial/member-removal-part-two"

p0lyn0mial deleted the member-removal-part-two branch April 4, 2022 15:46

p0lyn0mial restored the member-removal-part-two branch April 4, 2022 15:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

member removal part two #768

member removal part two #768

p0lyn0mial commented Mar 23, 2022

p0lyn0mial commented Mar 24, 2022

p0lyn0mial commented Mar 24, 2022

p0lyn0mial commented Mar 28, 2022

p0lyn0mial commented Mar 28, 2022

hasbro17 left a comment

hasbro17 Mar 28, 2022

hasbro17 Mar 28, 2022

hasbro17 Mar 28, 2022

p0lyn0mial Mar 29, 2022

p0lyn0mial Mar 29, 2022

p0lyn0mial commented Mar 30, 2022

openshift-ci bot commented Mar 30, 2022

hasbro17 left a comment

hasbro17 Mar 31, 2022

p0lyn0mial Mar 31, 2022

wallylewis commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

deads2k Apr 1, 2022

p0lyn0mial Apr 1, 2022

deads2k Apr 1, 2022

p0lyn0mial Apr 1, 2022

p0lyn0mial Apr 1, 2022

deads2k Apr 1, 2022

deads2k Apr 1, 2022

p0lyn0mial Apr 1, 2022

deads2k Apr 1, 2022

p0lyn0mial Apr 1, 2022

member removal part two #768

member removal part two #768

Conversation

p0lyn0mial commented Mar 23, 2022

p0lyn0mial commented Mar 24, 2022

p0lyn0mial commented Mar 24, 2022

p0lyn0mial commented Mar 28, 2022

p0lyn0mial commented Mar 28, 2022

hasbro17 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

p0lyn0mial commented Mar 30, 2022

openshift-ci bot commented Mar 30, 2022

hasbro17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallylewis commented Mar 31, 2022

openshift-ci bot commented Mar 31, 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