Skip to content

Commit 7bdef5f

Browse files
RadekManakRadekManak
committed
Implement SPOT instances
1 parent 3a37cb2 commit 7bdef5f

File tree

2 files changed

+166
-60
lines changed

2 files changed

+166
-60
lines changed

‎pkg/cloud/gcp/actuators/machine/reconciler.go‎

Lines changed: 30 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -73,14 +73,26 @@ func containsString(sli []string, str string) bool {
7373
return false
7474
}
7575

76-
func restartPolicyToBool(policy machinev1.GCPRestartPolicyType, preemptible bool) (*bool, error) {
76+
// toComputeProvisioningModel converts the GCPProvisioningModelType to the string expected by the GCP Compute API
77+
func toComputeProvisioningModel(model machinev1.GCPProvisioningModelType) string {
78+
switch model {
79+
case "":
80+
return ""
81+
case machinev1.GCPSpotInstance:
82+
return "SPOT"
83+
default:
84+
return string(model)
85+
}
86+
}
87+
88+
func restartPolicyToBool(policy machinev1.GCPRestartPolicyType, preemptible bool, provisioningModel machinev1.GCPProvisioningModelType) (*bool, error) {
7789
// for more information about how the restart policy works, see the GCP docs at
7890
// https://cloud.google.com/compute/docs/instances/setting-vm-host-options#settingoptions
7991
if len(policy) == 0 {
8092
return nil, nil
8193
} else if policy == machinev1.RestartPolicyAlways {
82-
if preemptible {
83-
return nil, errors.New("preemptible instances cannot be automatically restarted")
94+
if preemptible || provisioningModel == machinev1.GCPSpotInstance {
95+
return nil, errors.New("preemptible or spot instances cannot be automatically restarted")
8496
}
8597
return pointer.Bool(true), nil
8698
} else if policy == machinev1.RestartPolicyNever {
@@ -109,8 +121,8 @@ func (r *Reconciler) checkQuota(guestAccelerators []machinev1.GCPGPUConfig) erro
109121
if metric == "" {
110122
return machinecontroller.InvalidMachineConfiguration("Unsupported accelerator type %s", accelerator.Type)
111123
}
112-
// preemptible instances have separate quota
113-
if r.providerSpec.Preemptible {
124+
// preemptible and spot instances have separate quota with "PREEMPTIBLE_" prefix
125+
if r.providerSpec.Preemptible || r.providerSpec.ProvisioningModel == machinev1.GCPSpotInstance {
114126
metric = "PREEMPTIBLE_" + metric
115127
}
116128

@@ -194,6 +206,7 @@ func (r *Reconciler) create() error {
194206
Scheduling: &compute.Scheduling{
195207
Preemptible: r.providerSpec.Preemptible,
196208
OnHostMaintenance: string(r.providerSpec.OnHostMaintenance),
209+
ProvisioningModel: toComputeProvisioningModel(r.providerSpec.ProvisioningModel),
197210
},
198211
ShieldedInstanceConfig: &compute.ShieldedInstanceConfig{
199212
EnableSecureBoot: false,
@@ -210,7 +223,7 @@ func (r *Reconciler) create() error {
210223
ResourceManagerTags: userTags,
211224
}
212225

213-
if automaticRestart, err := restartPolicyToBool(r.providerSpec.RestartPolicy, r.providerSpec.Preemptible); err != nil {
226+
if automaticRestart, err := restartPolicyToBool(r.providerSpec.RestartPolicy, r.providerSpec.Preemptible, r.providerSpec.ProvisioningModel); err != nil {
214227
return machinecontroller.InvalidMachineConfiguration("failed to determine restart policy: %v", err)
215228
} else {
216229
instance.Scheduling.AutomaticRestart = automaticRestart
@@ -519,8 +532,9 @@ func (r *Reconciler) setMachineCloudProviderSpecifics(instance *compute.Instance
519532
r.machine.Labels[machinecontroller.MachineRegionLabelName] = r.providerSpec.Region
520533
r.machine.Labels[machinecontroller.MachineAZLabelName] = r.providerSpec.Zone
521534

522-
if r.providerSpec.Preemptible {
523-
// Label on the Machine so that an MHC can select Preemptible instances
535+
isSpot := instance.Scheduling != nil && instance.Scheduling.ProvisioningModel == toComputeProvisioningModel(machinev1.GCPSpotInstance)
536+
if r.providerSpec.Preemptible || isSpot {
537+
// Label on the Machine so that an MHC can select Preemptible/Spot instances
524538
r.machine.Labels[machinecontroller.MachineInterruptibleInstanceLabelName] = ""
525539

526540
if r.machine.Spec.Labels == nil {
@@ -565,6 +579,14 @@ func validateMachine(machine machinev1.Machine, providerSpec machinev1.GCPMachin
565579
return machinecontroller.InvalidMachineConfiguration("machine is missing %q label", machinev1.MachineClusterIDLabel)
566580
}
567581

582+
if providerSpec.ProvisioningModel != "" && providerSpec.ProvisioningModel != machinev1.GCPSpotInstance {
583+
return machinecontroller.InvalidMachineConfiguration("Unsupported provisioning model %q, Valid value is %q. Omit provisioningModel for standard.", providerSpec.ProvisioningModel, machinev1.GCPSpotInstance)
584+
}
585+
586+
if providerSpec.Preemptible && providerSpec.ProvisioningModel == machinev1.GCPSpotInstance {
587+
return machinecontroller.InvalidMachineConfiguration("preemptible cannot be used together with 'Spot' provisioning model")
588+
}
589+
568590
return nil
569591
}
570592

‎pkg/cloud/gcp/actuators/machine/reconciler_test.go‎

Lines changed: 136 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010

1111
"github.com/googleapis/gax-go/v2/apierror"
1212
configv1 "github.com/openshift/api/config/v1"
13-
"github.com/openshift/api/machine/v1beta1"
1413
machinev1 "github.com/openshift/api/machine/v1beta1"
1514
machinecontroller "github.com/openshift/machine-api-operator/pkg/controller/machine"
1615
computeservice "github.com/openshift/machine-api-provider-gcp/pkg/cloud/gcp/actuators/services/compute"
@@ -483,9 +482,9 @@ func TestCreate(t *testing.T) {
483482
name: "Always restart policy with a preemptible instance produces an error",
484483
providerSpec: &machinev1.GCPMachineProviderSpec{
485484
Preemptible: true,
486-
RestartPolicy: v1beta1.RestartPolicyAlways,
485+
RestartPolicy: machinev1.RestartPolicyAlways,
487486
},
488-
expectedError: errors.New("failed to determine restart policy: preemptible instances cannot be automatically restarted"),
487+
expectedError: errors.New("failed to determine restart policy: preemptible or spot instances cannot be automatically restarted"),
489488
},
490489
{
491490
name: "Always restart policy with a non-preemptible instance does not produce an error",
@@ -497,7 +496,7 @@ func TestCreate(t *testing.T) {
497496
},
498497
},
499498
Preemptible: false,
500-
RestartPolicy: v1beta1.RestartPolicyAlways,
499+
RestartPolicy: machinev1.RestartPolicyAlways,
501500
},
502501
},
503502
{
@@ -510,7 +509,7 @@ func TestCreate(t *testing.T) {
510509
},
511510
},
512511
Preemptible: true,
513-
RestartPolicy: v1beta1.RestartPolicyNever,
512+
RestartPolicy: machinev1.RestartPolicyNever,
514513
},
515514
},
516515
{
@@ -523,7 +522,7 @@ func TestCreate(t *testing.T) {
523522
},
524523
},
525524
Preemptible: false,
526-
RestartPolicy: v1beta1.RestartPolicyNever,
525+
RestartPolicy: machinev1.RestartPolicyNever,
527526
},
528527
},
529528
{
@@ -745,6 +744,58 @@ func TestCreate(t *testing.T) {
745744
},
746745
expectedError: errors.New("failed to fetch user-defined tags for : failed to fetch openshift/key2/value2 tag details: googleapi: Error 500: Internal error while fetching 'openshift/key2/value2'"),
747746
},
747+
{
748+
name: "Create spot instance successfully",
749+
providerSpec: &machinev1.GCPMachineProviderSpec{
750+
Region: "test-region",
751+
Zone: "test-zone",
752+
MachineType: "n1-standard-4",
753+
ProvisioningModel: machinev1.GCPSpotInstance,
754+
Disks: []*machinev1.GCPDisk{
755+
{
756+
Boot: true,
757+
Image: "projects/fooproject/global/images/uefi-image",
758+
},
759+
},
760+
},
761+
validateInstance: func(t *testing.T, instance *compute.Instance) {
762+
expectedProvisioningModel := toComputeProvisioningModel(machinev1.GCPSpotInstance)
763+
if instance.Scheduling.ProvisioningModel != expectedProvisioningModel {
764+
t.Errorf("expected ProvisioningModel to be %q, Got: %s", expectedProvisioningModel, instance.Scheduling.ProvisioningModel)
765+
}
766+
},
767+
},
768+
{
769+
name: "Fail on invalid provisioning model",
770+
providerSpec: &machinev1.GCPMachineProviderSpec{
771+
Region: "test-region",
772+
Zone: "test-zone",
773+
ProvisioningModel: "InvalidModel",
774+
Disks: []*machinev1.GCPDisk{
775+
{
776+
Boot: true,
777+
Image: "projects/fooproject/global/images/uefi-image",
778+
},
779+
},
780+
},
781+
expectedError: errors.New("failed validating machine provider spec: Unsupported provisioning model \"InvalidModel\", Valid value is \"Spot\". Omit provisioningModel for standard."),
782+
},
783+
{
784+
name: "Fail when both preemptible and spot are set",
785+
providerSpec: &machinev1.GCPMachineProviderSpec{
786+
Region: "test-region",
787+
Zone: "test-zone",
788+
Preemptible: true,
789+
ProvisioningModel: machinev1.GCPSpotInstance,
790+
Disks: []*machinev1.GCPDisk{
791+
{
792+
Boot: true,
793+
Image: "projects/fooproject/global/images/uefi-image",
794+
},
795+
},
796+
},
797+
expectedError: errors.New("failed validating machine provider spec: preemptible cannot be used together with 'Spot' provisioning model"),
798+
},
748799
}
749800

750801
mockTagService := tagservice.NewMockTagService()
@@ -1444,73 +1495,106 @@ func TestSetMachineCloudProviderSpecifics(t *testing.T) {
14441495

14451496
func TestRestartPolicyToBool(t *testing.T) {
14461497
cases := []struct {
1447-
name string
1448-
policy v1beta1.GCPRestartPolicyType
1449-
preemptible bool
1450-
expectedReturn *bool
1451-
expectedError error
1498+
name string
1499+
policy machinev1.GCPRestartPolicyType
1500+
preemptible bool
1501+
provisioningModel machinev1.GCPProvisioningModelType
1502+
expectedReturn *bool
1503+
expectedError error
14521504
}{
14531505
{
1454-
name: "Empty policy with non-preemptible returns nil and no error",
1455-
policy: "",
1456-
preemptible: false,
1457-
expectedReturn: nil,
1458-
expectedError: nil,
1506+
name: "Empty policy with non-preemptible returns nil and no error",
1507+
policy: "",
1508+
preemptible: false,
1509+
provisioningModel: "",
1510+
expectedReturn: nil,
1511+
expectedError: nil,
1512+
},
1513+
{
1514+
name: "Empty policy with preemptible returns nil and no error",
1515+
policy: "",
1516+
preemptible: true,
1517+
provisioningModel: "",
1518+
expectedReturn: nil,
1519+
expectedError: nil,
1520+
},
1521+
{
1522+
name: "Empty policy with spot provisioning model returns nil and no error",
1523+
policy: "",
1524+
preemptible: false,
1525+
provisioningModel: machinev1.GCPSpotInstance,
1526+
expectedReturn: nil,
1527+
expectedError: nil,
1528+
},
1529+
{
1530+
name: "Always policy with non-preemptible returns true and no error",
1531+
policy: machinev1.RestartPolicyAlways,
1532+
preemptible: false,
1533+
provisioningModel: "",
1534+
expectedReturn: pointer.Bool(true),
1535+
expectedError: nil,
14591536
},
14601537
{
1461-
name: "Empty policy with preemptible returns nil and no error",
1462-
policy: "",
1463-
preemptible: true,
1464-
expectedReturn: nil,
1465-
expectedError: nil,
1538+
name: "Always policy with preemptible returns nil and an error",
1539+
policy: machinev1.RestartPolicyAlways,
1540+
preemptible: true,
1541+
provisioningModel: "",
1542+
expectedReturn: nil,
1543+
expectedError: errors.New("preemptible or spot instances cannot be automatically restarted"),
14661544
},
14671545
{
1468-
name: "Always policy with non-preemptible returns true and no error",
1469-
policy: v1beta1.RestartPolicyAlways,
1470-
preemptible: false,
1471-
expectedReturn: pointer.Bool(true),
1472-
expectedError: nil,
1546+
name: "Always policy with spot provisioning model returns nil and an error",
1547+
policy: machinev1.RestartPolicyAlways,
1548+
preemptible: false,
1549+
provisioningModel: machinev1.GCPSpotInstance,
1550+
expectedReturn: nil,
1551+
expectedError: errors.New("preemptible or spot instances cannot be automatically restarted"),
14731552
},
14741553
{
1475-
name: "Always policy with preemptible returns nil and an error",
1476-
policy: v1beta1.RestartPolicyAlways,
1477-
preemptible: true,
1478-
expectedReturn: nil,
1479-
expectedError: errors.New("preemptible instances cannot be automatically restarted"),
1554+
name: "Never policy with non-preemptible returns false and no error",
1555+
policy: machinev1.RestartPolicyNever,
1556+
preemptible: false,
1557+
provisioningModel: "",
1558+
expectedReturn: pointer.Bool(false),
1559+
expectedError: nil,
14801560
},
14811561
{
1482-
name: "Never policy with non-preemptible returns false and no error",
1483-
policy: v1beta1.RestartPolicyNever,
1484-
preemptible: false,
1485-
expectedReturn: pointer.Bool(false),
1486-
expectedError: nil,
1562+
name: "Never policy with preemptible returns false and no error",
1563+
policy: machinev1.RestartPolicyNever,
1564+
preemptible: true,
1565+
provisioningModel: "",
1566+
expectedReturn: pointer.Bool(false),
1567+
expectedError: nil,
14871568
},
14881569
{
1489-
name: "Never policy with preemptible returns false and no error",
1490-
policy: v1beta1.RestartPolicyNever,
1491-
preemptible: true,
1492-
expectedReturn: pointer.Bool(false),
1493-
expectedError: nil,
1570+
name: "Never policy with spot provisioning model returns false and no error",
1571+
policy: machinev1.RestartPolicyNever,
1572+
preemptible: false,
1573+
provisioningModel: machinev1.GCPSpotInstance,
1574+
expectedReturn: pointer.Bool(false),
1575+
expectedError: nil,
14941576
},
14951577
{
1496-
name: "Unknown policy with non-preemptible returns nil and an error",
1497-
policy: "SometimesMaybe",
1498-
preemptible: false,
1499-
expectedReturn: nil,
1500-
expectedError: errors.New("unrecognized restart policy: SometimesMaybe"),
1578+
name: "Unknown policy with non-preemptible returns nil and an error",
1579+
policy: "SometimesMaybe",
1580+
preemptible: false,
1581+
provisioningModel: "",
1582+
expectedReturn: nil,
1583+
expectedError: errors.New("unrecognized restart policy: SometimesMaybe"),
15011584
},
15021585
{
1503-
name: "Unknown policy with preemptible returns nil and an error",
1504-
policy: "SometimesMaybe",
1505-
preemptible: true,
1506-
expectedReturn: nil,
1507-
expectedError: errors.New("unrecognized restart policy: SometimesMaybe"),
1586+
name: "Unknown policy with preemptible returns nil and an error",
1587+
policy: "SometimesMaybe",
1588+
preemptible: true,
1589+
provisioningModel: "",
1590+
expectedReturn: nil,
1591+
expectedError: errors.New("unrecognized restart policy: SometimesMaybe"),
15081592
},
15091593
}
15101594

15111595
for _, tc := range cases {
15121596
t.Run(tc.name, func(t *testing.T) {
1513-
observedReturn, observedError := restartPolicyToBool(tc.policy, tc.preemptible)
1597+
observedReturn, observedError := restartPolicyToBool(tc.policy, tc.preemptible, tc.provisioningModel)
15141598

15151599
if tc.expectedReturn == nil && observedReturn != nil {
15161600
t.Errorf("Expected nil return value, got: %v", *observedReturn)

0 commit comments

Comments
 (0)