Adds Oldest and Newest delete policies and annotation-based Simple delete policy #726

erstaples · 2019-02-04T15:36:23Z

What this PR does / why we need it:
This PR adds new delete policies to the MachineSet spec: "Newest" and "Oldest," and it modifies the "Simple" delete policy to add an annotation check, which addresses the issue of defining specific machines when scaling down (#75).

Simple delete policy has been modified to select a Machine for scale down with the annotation machineset.clusters.k8s.io/delete-me=yes.
Oldest and Newest delete policies map the CreationTimestamp of the Machine to a 0-100 delete priority score.

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 #75

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:

Added "Oldest" and "Newest" delete policies on MachineSet spec and added "machineset.clusters.k8s.io/delete-me=yes" annotation check on nodes to delete specific machines when scaling down machine sets

k8s-ci-robot · 2019-02-04T15:36:36Z

Hi @erstaples. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

vincepri · 2019-02-04T16:22:55Z

/ok-to-test

roberthbailey · 2019-02-04T18:50:19Z

/assign @maisem

k8s-ci-robot · 2019-02-04T18:50:21Z

@roberthbailey: GitHub didn't allow me to assign the following users: maisem.

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @maisem

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.

vincepri

Thank you for the contribution! Looking promising and definitely a welcome feature. Left some comments throughout the code.

pkg/apis/cluster/v1alpha1/machineset_types.go

pkg/controller/machineset/delete_policy.go

pkg/controller/machineset/controller.go

vincepri · 2019-02-05T21:59:39Z

pkg/controller/machineset/controller.go

+		case clusterv1alpha1.OldestMachineSetDeletePolicy:
+			deletePriorityFunc = oldestDeletePriority
+		default:
+			klog.Errorf("Unsupported delete policy %s. Defaulting to Simple delete policy.", msdp)


I'm not sure if defaulting to the Simple policy might be the way to go. Users might expect a different behavior and need to debug through the manager logs to find this out. I'd rather return an error here and make the Set not scale at all and add some basic validation to the field.

pkg/apis/cluster/v1alpha1/machineset_types.go

pkg/controller/machineset/controller.go

hardikdr

Few pointers:

This seems to be introducing a configuration-dependency !?!, eg. Autoscaler works only if Simple policy is set. We could try to avoid this if possible.
- Probable solution while scaling-down:
  - default-policy : prefers unhealthy machines first, then random.
  - oldest-policy: prefers oldest machines first
  - newest-policy: prefers latest machines first.
  - machines with certain annotation[some-keyword] gets deleted first in any case, irrespective of the deletion-policy mentioned.
The concern of atomicity: Autoscaler annotates the machine and dies before it really attempts to scale-down machine-set/deployment. By the time it gets back, the state of the workload is slightly changed and the machine is not required to be deleted.
- Probable solution:
  - Live with it, it's a rare case. If we agree on the philosophy that, VMs are ephemeral resources and could go down and come back again unless explicitly instructed through autoscaler-means, controller-logic will reconcile the desired-state anyways later.
  - Feature request from autoscaler, where DeleteNodes interface, cross-checks/updates the current-state where only intended set of machines have some-keyword annotation on them - before every-call. This can also have side-effects of being authoritative while updating annotations !!

Anyways, let's discuss this in next meeting.

ncdc · 2019-03-07T15:39:16Z

Could we please get a status update on this PR? Is there consensus to proceed with it for v1alpha1?

hardikdr · 2019-03-07T17:20:59Z

Yes, I am on it. I am not sure about keeping it in v1alpha1, but we wanted to align with few folks from auto-scaler before merging it.

pkg/controller/machineset/delete_policy.go

ncdc · 2019-03-13T17:53:33Z

/milestone v1alpha1

detiber · 2019-03-15T18:33:27Z

@erstaples it looks like a rebase is still needed for this PR

config/crds/cluster_v1alpha1_machineset.yaml

ncdc · 2019-03-15T18:50:16Z

config/crds/cluster_v1alpha1_machineset.yaml

+                        get registered as Kubernetes nodes. With cluster-api as a
+                        generic out-of-tree provider for autoscaler, this field is
+                        required by autoscaler to be able to have a provider view
+                        of the list of machines. Another list of nodes is queries


queries -> queried?

config/crds/cluster_v1alpha1_machineset.yaml

pkg/apis/cluster/v1alpha1/machineset_types.go

pkg/controller/machineset/delete_policy.go

- Add validation to deletePolicy field on MachineSet CRD - Misc cleanup and comments

- Renamed Simple deletepolicy to Random - Updated Newest and Oldest policies to prioritize annotations and unhealthy nodes - Removed unnecessary validation - Renamed delete annotation

ncdc · 2019-03-18T16:41:04Z

@erstaples do you think you'll have time to rebase + address comments soon? Thanks!

- Added godoc to MachineSetDeletePolicy type

erstaples · 2019-03-18T16:49:44Z

Hey @ncdc, just did!

ncdc · 2019-03-18T17:12:06Z

pkg/controller/machineset/delete_policy.go

+	if d.Seconds() < 0 {
+		return mustNotDelete
+	}
+	return deletePriority(float64(mustDelete) * (1.0 - math.Exp(-d.Seconds()/secondsPerTenDays)))


I'm curious - why seconds per 10 days?

The function maps [0, Inf) -> [0, 1). With exponential decay, the parameter puts the half-life at just about a week, i.e. 1 week ~= 0.5 priority, 2 weeks ~= 0.75 priority etc. Arbitrary but reasonable?

You mean 0-100, right?

scaled by float64(mustDelete) to the appropriate range, yeah.

ntfrnzn · 2019-03-21T18:50:00Z

pkg/controller/machineset/delete_policy.go

+	case v1alpha1.OldestMachineSetDeletePolicy:
+		return oldestDeletePriority, nil
+	default:
+		return nil, errors.Errorf("Unsupported delete policy %s. Must be one of 'Random', 'Newest', or 'Oldest'", msdp)


late comment, but we have to default to a policy if the value is not set, i.e.,

case "": return randomDeletePolicy, nil

@ntfrnzn (not sure if this change went in after your comment, but) if you look a couple of lines up, there is a default for unset/"". The default case is for when the user specifies an invalid policy name such as abc123.

ncdc · 2019-03-25T14:40:27Z

@vincepri any other comments on this?

vincepri

Great work, thank you for the contribution!

/approve

I noticed that some BAZEL.build files are being deleted, could you run make generate and make sure that no updates are shown in git status?

k8s-ci-robot · 2019-03-25T17:52:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erstaples, vincepri

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

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

ncdc · 2019-03-26T18:06:06Z

@erstaples did you get a chance to check out Vince's comment about the Bazel files?

erstaples · 2019-03-27T17:27:37Z

@ncdc @vincepri Sorry about that, Github failed to notify me about these comments, or they got lost in the noise. I re-ran make generate and there are quite a few changes. Want me to push them?

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   pkg/controller/machineset/BUILD.bazel

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	pkg/client/clientset_generated/clientset/BUILD.bazel
	pkg/client/clientset_generated/clientset/fake/BUILD.bazel
	pkg/client/clientset_generated/clientset/scheme/BUILD.bazel
	pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1/BUILD.bazel
	pkg/client/clientset_generated/clientset/typed/cluster/v1alpha1/fake/BUILD.bazel
	pkg/client/informers_generated/externalversions/BUILD.bazel
	pkg/client/informers_generated/externalversions/cluster/BUILD.bazel
	pkg/client/informers_generated/externalversions/cluster/v1alpha1/BUILD.bazel
	pkg/client/informers_generated/externalversions/internalinterfaces/BUILD.bazel
	pkg/client/listers_generated/cluster/v1alpha1/BUILD.bazel

vincepri · 2019-03-27T17:29:20Z

Please do! :)

erstaples · 2019-03-27T17:30:40Z

@vincepri Done!

vincepri · 2019-03-27T17:50:15Z

@detiber @ncdc leaving final LGTM to you

detiber · 2019-03-27T19:11:44Z

/lgtm

…lete policy (kubernetes-sigs#726) * add oldest and newest delete policy implementations * delete policy checkpoint * wire up spec.deletePolicy to sort delegate * gofmt * - Refactor deletePriorityFunc selector to separate function - Add validation to deletePolicy field on MachineSet CRD - Misc cleanup and comments * Address PR feedback: - Renamed Simple deletepolicy to Random - Updated Newest and Oldest policies to prioritize annotations and unhealthy nodes - Removed unnecessary validation - Renamed delete annotation * - Cleaned up grammar on providerID field comments - Added godoc to MachineSetDeletePolicy type * Add empty string case * Add bazel files

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 4, 2019

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 4, 2019

k8s-ci-robot requested review from justinsb and roberthbailey February 4, 2019 15:36

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 4, 2019

vincepri reviewed Feb 5, 2019

View reviewed changes

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 6, 2019

detiber reviewed Feb 6, 2019

View reviewed changes

pkg/apis/cluster/v1alpha1/machineset_types.go Show resolved Hide resolved

pkg/apis/cluster/v1alpha1/machineset_types.go Outdated Show resolved Hide resolved

pkg/controller/machineset/controller.go Outdated Show resolved Hide resolved

hardikdr mentioned this pull request Feb 19, 2019

MachineSet: support for defining specific machines when scaling down #75

Closed

hardikdr reviewed Feb 19, 2019

View reviewed changes

justinsb reviewed Mar 13, 2019

View reviewed changes

pkg/controller/machineset/delete_policy.go Outdated Show resolved Hide resolved

ncdc mentioned this pull request Mar 13, 2019

Delete specific machine from a machineset when scaling down #513

Closed

k8s-ci-robot added this to the v1alpha1 milestone Mar 13, 2019

ncdc reviewed Mar 15, 2019

View reviewed changes

ntfrnzn and others added 5 commits March 18, 2019 08:53

add oldest and newest delete policy implementations

eec67ea

delete policy checkpoint

5ce93e8

wire up spec.deletePolicy to sort delegate

bcdc828

gofmt

0a075a6

- Refactor deletePriorityFunc selector to separate function

c100f30

- Add validation to deletePolicy field on MachineSet CRD - Misc cleanup and comments

Address PR feedback:

df17a92

- Renamed Simple deletepolicy to Random - Updated Newest and Oldest policies to prioritize annotations and unhealthy nodes - Removed unnecessary validation - Renamed delete annotation

- Cleaned up grammar on providerID field comments

81ccbe1

- Added godoc to MachineSetDeletePolicy type

ncdc reviewed Mar 18, 2019

View reviewed changes

vincepri self-assigned this Mar 20, 2019

ntfrnzn reviewed Mar 21, 2019

View reviewed changes

Add empty string case

aae49af

vincepri approved these changes Mar 25, 2019

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2019

Add bazel files

2672eb8

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 27, 2019

k8s-ci-robot assigned detiber Mar 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 27, 2019

k8s-ci-robot merged commit f6aa237 into kubernetes-sigs:master Mar 27, 2019

mhrivnak mentioned this pull request Apr 2, 2019

Research Machine selection when scaling down a MachineSet metal3-io/cluster-api-provider-baremetal#10

Closed

alvaroaleman mentioned this pull request Apr 3, 2019

Update cluster-api to add selective downscaling kubermatic/machine-controller#477

Merged

frobware mentioned this pull request Apr 25, 2019

Add cluster-api based cloudprovider kubernetes/autoscaler#1866

Merged

rhockenbury mentioned this pull request Mar 8, 2020

MachineDeployment should support spec.deletePolicy (like MS) #2585

Closed

rajgani mentioned this pull request Aug 6, 2024

Scale down specific node is not working #11026

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Oldest and Newest delete policies and annotation-based Simple delete policy #726

Adds Oldest and Newest delete policies and annotation-based Simple delete policy #726

erstaples commented Feb 4, 2019

k8s-ci-robot commented Feb 4, 2019

vincepri commented Feb 4, 2019

roberthbailey commented Feb 4, 2019

k8s-ci-robot commented Feb 4, 2019

vincepri left a comment

vincepri Feb 5, 2019

hardikdr left a comment

ncdc commented Mar 7, 2019

hardikdr commented Mar 7, 2019

ncdc commented Mar 13, 2019

detiber commented Mar 15, 2019

ncdc Mar 15, 2019

ncdc commented Mar 18, 2019

erstaples commented Mar 18, 2019

ncdc Mar 18, 2019

ntfrnzn Mar 18, 2019

ncdc Mar 18, 2019

ntfrnzn Mar 18, 2019

ntfrnzn Mar 21, 2019

ncdc Mar 25, 2019

ncdc commented Mar 25, 2019

vincepri left a comment

k8s-ci-robot commented Mar 25, 2019

ncdc commented Mar 26, 2019

erstaples commented Mar 27, 2019

vincepri commented Mar 27, 2019

erstaples commented Mar 27, 2019

vincepri commented Mar 27, 2019

detiber commented Mar 27, 2019

Adds Oldest and Newest delete policies and annotation-based Simple delete policy #726

Adds Oldest and Newest delete policies and annotation-based Simple delete policy #726

Conversation

erstaples commented Feb 4, 2019

k8s-ci-robot commented Feb 4, 2019

vincepri commented Feb 4, 2019

roberthbailey commented Feb 4, 2019

k8s-ci-robot commented Feb 4, 2019

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardikdr left a comment

Choose a reason for hiding this comment

ncdc commented Mar 7, 2019

hardikdr commented Mar 7, 2019

ncdc commented Mar 13, 2019

detiber commented Mar 15, 2019

Choose a reason for hiding this comment

ncdc commented Mar 18, 2019

erstaples commented Mar 18, 2019

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

ncdc commented Mar 25, 2019

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Mar 25, 2019

ncdc commented Mar 26, 2019

erstaples commented Mar 27, 2019

vincepri commented Mar 27, 2019

erstaples commented Mar 27, 2019

vincepri commented Mar 27, 2019

detiber commented Mar 27, 2019