[OCPCLOUD-778] Add support for autoscaling to/from zero #112

JoelSpeed · 2020-03-09T11:26:22Z

What this PR does / why we need it:

This PR adds a new controller to annotate machinesets with information required to enable autoscaling to/from zero

I haven't tested this yet on a real cluster so will look into doing that

I also haven't added any tests for the controller as whole, happy to add some envtest style tests if that would be useful

enxebre · 2020-03-10T11:17:14Z

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/Azure/go-autorest/autorest/to v0.3.0
 	github.com/Azure/go-autorest/autorest/validation v0.2.0 // indirect
 	github.com/ghodss/yaml v1.0.0


should this be in the vendor commit?

enxebre · 2020-03-10T11:17:18Z

pkg/cloud/azure/actuators/machineset/azure_instance_types.go

+
+// This file was copied from
+// https://github.com/openshift/kubernetes-autoscaler/blob/95d5f0927b9347635f1720d6748503415e6921cb/cluster-autoscaler/cloudprovider/azure/azure_instance_types.go
+// // https://github.com/kubernetes/kubernetes/issues/79384


extra "//"?

pkg/cloud/azure/actuators/machineset/controller.go

enxebre · 2020-03-10T11:20:35Z

can we update all commits to UPSTREAM: <carry>: openshift: blah

enxebre · 2020-03-10T11:22:36Z

units won't pass since there's nothing setting up the environment for envTest yet

enxebre · 2020-03-10T11:48:42Z

pkg/cloud/azure/actuators/machineset/controller_test.go

+	}, nil
+}
+
+func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (machinev1.ProviderSpec, error) {


Is there any exported method in the actuator machine controller we can reuse for this?

Not exported, but it does already exist in the tests

cluster-api-provider-azure/pkg/cloud/azure/actuators/machine_scope_test.go

Lines 42 to 50 in d80999f

func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (*machinev1.ProviderSpec, error) {

bytes, err := json.Marshal(in)

if err != nil {

return nil, err

}

return &machinev1.ProviderSpec{

Value: &runtime.RawExtension{Raw: bytes},

}, nil

}

and

cluster-api-provider-azure/pkg/cloud/azure/actuators/machine/actuator_test.go

Lines 69 to 77 in d80999f

func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (*machinev1.ProviderSpec, error) {

bytes, err := yaml.Marshal(in)

if err != nil {

return nil, err

}

return &machinev1.ProviderSpec{

Value: &runtime.RawExtension{Raw: bytes},

}, nil

}

JoelSpeed

All comments fixed

I've added the make test unit which has the external binaries for env test downloaded as part of it, just need to make this run in CI and should be good! (I copied the scripts from the AWS one merged in openshift/cluster-api-provider-aws#289

JoelSpeed · 2020-03-10T12:22:38Z

pkg/cloud/azure/actuators/machineset/controller_test.go

+	}, nil
+}
+
+func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (machinev1.ProviderSpec, error) {


Not exported, but it does already exist in the tests

cluster-api-provider-azure/pkg/cloud/azure/actuators/machine_scope_test.go

Lines 42 to 50 in d80999f

func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (*machinev1.ProviderSpec, error) {

bytes, err := json.Marshal(in)

if err != nil {

return nil, err

}

return &machinev1.ProviderSpec{

Value: &runtime.RawExtension{Raw: bytes},

}, nil

}

and

cluster-api-provider-azure/pkg/cloud/azure/actuators/machine/actuator_test.go

Lines 69 to 77 in d80999f

func providerSpecFromMachine(in *machineproviderv1.AzureMachineProviderSpec) (*machinev1.ProviderSpec, error) {

bytes, err := yaml.Marshal(in)

if err != nil {

return nil, err

}

return &machinev1.ProviderSpec{

Value: &runtime.RawExtension{Raw: bytes},

}, nil

}

pkg/cloud/azure/actuators/machineset/controller.go

JoelSpeed · 2020-03-10T12:23:02Z

go.mod

@@ -9,6 +9,7 @@ require (
 	github.com/Azure/go-autorest/autorest/to v0.3.0
 	github.com/Azure/go-autorest/autorest/validation v0.2.0 // indirect
 	github.com/ghodss/yaml v1.0.0


enxebre · 2020-03-11T09:33:26Z

pkg/cloud/azure/actuators/machineset/controller.go

+	}
+
+	originalMachineSetToPatch := client.MergeFrom(machineSet.DeepCopy())
+	defer func() {


do we need to defer here?
what's the benefit over calling this explicitly at the end of this func? I'd find that more readable. wdyt?

I did this because I had meant to put a return in the if err != nil block on lines 96-99, but I seemingly forgot to do that.
I was following a pattern from upstream but happy to rework it to avoid the defer

Added a commit to rework this, gonna squash it before merge, WDYT?

sgtm. Please keep revendor commit separate

enxebre · 2020-03-11T10:50:33Z

pkg/cloud/azure/actuators/machineset/controller.go

+		logger.Error(err, "Failed to reconcile MachineSet")
+		r.recorder.Eventf(machineSet, corev1.EventTypeWarning, "ReconcileError", "%v", err)
+	}
+	return result, nil


shouldn't this return err? seems like the err in l94 is ignored otherwise?

Already fixed that, I think you saw an outdated version

JoelSpeed · 2020-03-11T14:58:38Z

/test unit

…tion

enxebre · 2020-03-12T11:44:45Z

/approve

openshift-ci-robot · 2020-03-12T11:45:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

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

alexander-demicev

/lgtm

openshift-bot · 2020-03-12T13:19:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Mar 9, 2020

openshift-ci-robot requested review from bison and ingvagabund March 9, 2020 11:26

enxebre reviewed Mar 10, 2020

View reviewed changes

pkg/cloud/azure/actuators/machineset/controller.go Show resolved Hide resolved

enxebre reviewed Mar 10, 2020

View reviewed changes

JoelSpeed force-pushed the machineset-controller branch from a7c8e80 to 3e499da Compare March 10, 2020 12:27

JoelSpeed commented Mar 10, 2020

View reviewed changes

enxebre reviewed Mar 11, 2020

View reviewed changes

enxebre mentioned this pull request Mar 11, 2020

Machineset annotations controller openshift/cluster-api-provider-aws#301

Merged

JoelSpeed force-pushed the machineset-controller branch 2 times, most recently from 336e982 to 6a33aca Compare March 11, 2020 12:01

JoelSpeed added 4 commits March 11, 2020 13:43

UPSTREAM: <carry>: openshift: Add machineset controller

c9d2187

UPSTREAM: <carry>: openshift: Add tests for reconcile

cbabede

UPSTREAM: <carry>: openshift: Add envTest tests for reconciler

8749218

UPSTREAM: <carry>: openshift: make vendor

81f6fdf

JoelSpeed force-pushed the machineset-controller branch from 6a33aca to 1147e1f Compare March 11, 2020 13:43

enxebre mentioned this pull request Mar 11, 2020

[WIP] UPSTREAM: <carry>: openshift: Implement scale from zero openshift/kubernetes-autoscaler#110

Closed

UPSTREAM: <carry>: openshift: Add envtest binaries for testing

54b56de

JoelSpeed force-pushed the machineset-controller branch from 1147e1f to 54b56de Compare March 11, 2020 15:18

UPSTREAM: <carry>: openshift: Don't re-reconcile on invalid configura…

eca8aff

…tion

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 12, 2020

alexander-demicev approved these changes Mar 12, 2020

View reviewed changes

openshift-ci-robot assigned alexander-demicev Mar 12, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2020

enxebre mentioned this pull request Mar 12, 2020

UPSTREAM: <carry>: openshift: Implement scale from zero openshift/kubernetes-autoscaler#137

Merged

openshift-merge-robot merged commit a0b4361 into openshift:master Mar 12, 2020

JoelSpeed mentioned this pull request Mar 16, 2020

[OCPCLOUD-779] Add Machineset annotations to support autoscaling to/from zero openshift/cluster-api-provider-gcp#77

Merged

SamuelStuchly mentioned this pull request Apr 6, 2021

Vsphere enable autoscaling from/to zero openshift/machine-api-operator#839

Merged

Karthik-K-N mentioned this pull request Jun 17, 2022

Add support scaling from/to zero openshift/machine-api-provider-powervs#21

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OCPCLOUD-778] Add support for autoscaling to/from zero #112

[OCPCLOUD-778] Add support for autoscaling to/from zero #112

JoelSpeed commented Mar 9, 2020

enxebre Mar 10, 2020

JoelSpeed Mar 10, 2020

enxebre Mar 10, 2020

enxebre commented Mar 10, 2020

enxebre commented Mar 10, 2020

enxebre Mar 10, 2020

JoelSpeed Mar 10, 2020

JoelSpeed left a comment

JoelSpeed Mar 10, 2020

JoelSpeed Mar 10, 2020

enxebre Mar 11, 2020 •

edited

JoelSpeed Mar 11, 2020

JoelSpeed Mar 11, 2020

enxebre Mar 11, 2020

enxebre Mar 11, 2020 •

edited

JoelSpeed Mar 11, 2020

JoelSpeed commented Mar 11, 2020

enxebre commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

alexander-demicev left a comment

openshift-bot commented Mar 12, 2020

	func providerSpecFromMachine(in machineproviderv1.AzureMachineProviderSpec) (machinev1.ProviderSpec, error) {
	bytes, err := json.Marshal(in)
	if err != nil {
	return nil, err
	}
	return &machinev1.ProviderSpec{
	Value: &runtime.RawExtension{Raw: bytes},
	}, nil
	}

	func providerSpecFromMachine(in machineproviderv1.AzureMachineProviderSpec) (machinev1.ProviderSpec, error) {
	bytes, err := yaml.Marshal(in)
	if err != nil {
	return nil, err
	}
	return &machinev1.ProviderSpec{
	Value: &runtime.RawExtension{Raw: bytes},
	}, nil
	}

[OCPCLOUD-778] Add support for autoscaling to/from zero #112

[OCPCLOUD-778] Add support for autoscaling to/from zero #112

Conversation

JoelSpeed commented Mar 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented Mar 10, 2020

enxebre commented Mar 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Mar 11, 2020 • 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

enxebre Mar 11, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Mar 11, 2020

enxebre commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

alexander-demicev left a comment

Choose a reason for hiding this comment

openshift-bot commented Mar 12, 2020

enxebre Mar 11, 2020 •

edited

enxebre Mar 11, 2020 •

edited