[OCPCLOUD-960] Add support for dedicated instance tenancy #360
Conversation
There was a problem hiding this comment.
Looks good, left a couple of suggestions for possible improvements.
Do you plan to have a follow up PR that validates the options in the validating webhook for AWS? This is a very viable field for validating there IMO
Have you done any manual testing for this feature yet? Do we need any extra config on the AWS account to be able to do this?
| @@ -88,6 +88,9 @@ type AWSMachineProviderConfig struct { | |||
|
|
|||
| // SpotMarketOptions allows users to configure instances to be run using AWS Spot instances. | |||
| SpotMarketOptions *SpotMarketOptions `json:"spotMarketOptions,omitempty"` | |||
|
|
|||
| // Tenancy indicates if instance should run on shared or single-tenant hardware. | |||
| Tenancy string `json:"tenancy,omitempty"` | |||
There was a problem hiding this comment.
Might be worth setting this as a typed string and having the valid options be part of this package as exported variables? WDYT?
I would also suggest adding a comment to this to explain the valid and default options
| @@ -342,6 +342,25 @@ func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsprovid | |||
| } | |||
| } | |||
|
|
|||
| instanceTenancy := machineProviderConfig.Tenancy | |||
|
|
|||
| switch instanceTenancy { | |||
There was a problem hiding this comment.
I know this can be validated in a webhook, but I'd leave the check serverside too.
There was a problem hiding this comment.
Agreed, we should check in both places
|
/retest |
| @@ -342,6 +342,25 @@ func launchInstance(machine *machinev1.Machine, machineProviderConfig *awsprovid | |||
| } | |||
| } | |||
|
|
|||
| instanceTenancy := machineProviderConfig.Tenancy | |||
|
|
|||
| switch instanceTenancy { | |||
There was a problem hiding this comment.
Agreed, we should check in both places
| // Tenancy indicates if instance should run on shared or single-tenant hardware. There are | ||
| // supported 3 options: default, dedicated and host. |
There was a problem hiding this comment.
Might be worth adjusting this and marking it optional (just for future readers, I realise it doesn't achieve anything)
| // Tenancy indicates if instance should run on shared or single-tenant hardware. There are | |
| // supported 3 options: default, dedicated and host. | |
| // Tenancy indicates if instance should run on shared or single-tenant hardware. | |
| // There are 3 supported options: default, dedicated and host. | |
| // +optional |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/retitle [OCPCLOUD-960] Add support for dedicated instance tenancy |
openshift-ci-robot
changed the title
|
/retest Please review the full test history for this PR and help us cut down flakes. |
This PR adds support for dedicated instance tenancy.