Sort machines deterministically before scaling down

[ingvagabund]

What this PR does / why we need it:

Sort the machines deterministically to delete machines in the same order. In general the machines are sequenced randomly depending on Machines().List() result. When number of replicas goes down, the reconcilication loop is called multiple times, each time taking a different subset of machines to be deleted. Which may result in deletion of more machines than requested (followed by recreation of new machines). Given each machine represents a node, deleting more nodes than expected can be very disruptive.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ingvagabund
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mkjelland

If they are not already assigned, you can assign the PR to them by writing /assign @mkjelland in a comment when ready.

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

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

[ingvagabund]

So far the machines were listed randomly. Given a machine name is generated randomly, the original deletion policy (deleting first machines in a list) is preserved.

[frobware]

Given a machine name is generated randomly, the original deletion policy (deleting first machines in a list) is preserved.

How does the final sort preserve the original deletion policy?

[ingvagabund]

The original deletion policy takes first items from a list where whose order is generated randomly. Reordering the items based on a name that is generated randomly as well changes order if items from one random sequence to another one.

[ingvagabund]

@frobware @enxebre @roberthbailey @hardikdr

[alvaroaleman]

// When number of replicas goes down, the reconcilication loop is called multiple
// times, each time taking a different subset of machines to be deleted.
// Which may result in deletion of more machines than requested (followed
 // by recreation of new machines

Why is it called multiple times? It gets called once for the change of .Spec.Replicas and then once again every time a machine gets deleted or am I missing something here?

WTR to the issue that too many machines may be deleted: Wouldn't just filtering out machines in shouldExcludeMachine that have a non-nil deletionTimestamp fix that?

The only possible issue I see here is a racecondition in scaling down when a machine gets deleted by one Reconsile, that deletion is not propagated back to the lister yet while another Reconsile gets called, resulting in that other Reconsile to delete more machines. However this is an issue that can only prevented by using some locking mechanism that blocks further deletions until the first deletions arrived in the lister (And filtering properly, as mentioned above).

Does that make sense or did I get something wrong?

[roberthbailey]

Closing as per #558 (comment).

/close

[k8s-ci-robot]

@roberthbailey: Closing this PR.

In response to this:

Closing as per #558 (comment).

/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.