Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

asset/installconfig/Azure: Validate install-config instance types #4419

Merged
merged 1 commit into from
Dec 3, 2020
Merged

asset/installconfig/Azure: Validate install-config instance types #4419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions pkg/asset/installconfig/azure/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ type API interface {
GetControlPlaneSubnet(ctx context.Context, resourceGroupName, virtualNetwork, subnet string) (*aznetwork.Subnet, error)
ListLocations(ctx context.Context) (*[]azsubs.Location, error)
GetResourcesProvider(ctx context.Context, resourceProviderNamespace string) (*azres.Provider, error)
GetVirtualMachineSku(ctx context.Context, name, region string) (*azsku.ResourceSku, error)
GetDiskSkus(ctx context.Context, region string) ([]azsku.ResourceSku, error)
GetGroup(ctx context.Context, groupName string) (*azres.Group, error)
ListResourceIDsByGroup(ctx context.Context, groupName string) ([]string, error)
Expand Down Expand Up @@ -211,3 +212,34 @@ func (c *Client) ListResourceIDsByGroup(ctx context.Context, groupName string) (
}
return res, nil
}

// GetVirtualMachineSku retrieves the resource SKU of a specified virtual machine SKU in the specified region.
func (c *Client) GetVirtualMachineSku(ctx context.Context, name, region string) (*azsku.ResourceSku, error) {
client := azsku.NewResourceSkusClientWithBaseURI(c.ssn.Environment.ResourceManagerEndpoint, c.ssn.Credentials.SubscriptionID)
client.Authorizer = c.ssn.Authorizer
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()

for page, err := client.List(ctx); page.NotDone(); err = page.NextWithContext(ctx) {
jstuever marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, errors.Wrap(err, "error fetching SKU pages")
}
for _, sku := range page.Values() {
// Filter out resources that are not virtualMachines
if !strings.EqualFold("virtualMachines", *sku.ResourceType) {
continue
}
// Filter out resources that do not match the provided name
if !strings.EqualFold(name, *sku.Name) {
continue
}
// Return the resource from the provided region
for _, location := range to.StringSlice(sku.Locations) {
if strings.EqualFold(location, region) {
return &sku, nil
}
}
}
}
return nil, nil
}
15 changes: 15 additions & 0 deletions pkg/asset/installconfig/azure/mock/azureclient_generated.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

86 changes: 86 additions & 0 deletions pkg/asset/installconfig/azure/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"net"
"sort"
"strconv"
"strings"

azdns "github.com/Azure/azure-sdk-for-go/profiles/latest/dns/mgmt/dns"
Expand All @@ -17,15 +18,100 @@ import (
aztypes "github.com/openshift/installer/pkg/types/azure"
)

type resourceRequirements struct {
minimumVCpus int64
minimumMemory int64
}

var controlPlaneReq = resourceRequirements{
minimumVCpus: 4,
minimumMemory: 16,
}

var computeReq = resourceRequirements{
minimumVCpus: 2,
minimumMemory: 8,
}

// Validate executes platform-specific validation.
func Validate(client API, ic *types.InstallConfig) error {
allErrs := field.ErrorList{}

allErrs = append(allErrs, validateNetworks(client, ic.Azure, ic.Networking.MachineNetwork, field.NewPath("platform").Child("azure"))...)
allErrs = append(allErrs, validateRegion(client, field.NewPath("platform").Child("azure").Child("region"), ic.Azure)...)
allErrs = append(allErrs, validateInstanceTypes(client, ic)...)
return allErrs.ToAggregate()
}

// ValidateInstanceType ensures the instance type has sufficient Vcpu and Memory.
func ValidateInstanceType(client API, fieldPath *field.Path, region, instanceType string, req resourceRequirements) field.ErrorList {
allErrs := field.ErrorList{}

typeMeta, err := client.GetVirtualMachineSku(context.TODO(), instanceType, region)
if err != nil {
return append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, err.Error()))
}

if typeMeta == nil {
errMsg := fmt.Sprintf("not found in region %s", region)
return append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, errMsg))
}

for _, capability := range *typeMeta.Capabilities {

if strings.EqualFold(*capability.Name, "vCPUs") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we willing to accept a SKU that does not have the "vCPUs" or "MemoryGB" capability?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if the capability is not present, then we won't show any error. We filter out any SKU that is not of type "virtualMachines". I am unable to find anything in the docs where this type of SKU is guaranteed to have these capabilities specified (or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move forward as-is. We can always make changes if we find cu issues later on.

cpus, err := strconv.ParseInt(*capability.Value, 10, 0)
if err != nil {
return append(allErrs, field.InternalError(fieldPath, err))
}
if cpus < req.minimumVCpus {
errMsg := fmt.Sprintf("instance type does not meet minimum resource requirements of %d vCPUs", req.minimumVCpus)
allErrs = append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, errMsg))
}
} else if strings.EqualFold(*capability.Name, "MemoryGB") {
memory, err := strconv.ParseInt(*capability.Value, 10, 0)
if err != nil {
return append(allErrs, field.InternalError(fieldPath, err))
}
if memory < req.minimumMemory {
errMsg := fmt.Sprintf("instance type does not meet minimum resource requirements of %d GB Memory", req.minimumMemory)
allErrs = append(allErrs, field.Invalid(fieldPath.Child("type"), instanceType, errMsg))
}
}
}

return allErrs
}

// validateInstanceTypes checks that the user-provided instance types are valid.
func validateInstanceTypes(client API, ic *types.InstallConfig) field.ErrorList {
allErrs := field.ErrorList{}

// Default requirements need to be sufficient to support Control Plane instances.
defaultInstanceReq := controlPlaneReq

if ic.ControlPlane != nil && ic.ControlPlane.Platform.Azure != nil && ic.ControlPlane.Platform.Azure.InstanceType != "" {
// Default requirements can be relaxed when the controlPlane type is set explicitly.
defaultInstanceReq = computeReq

allErrs = append(allErrs, ValidateInstanceType(client, field.NewPath("controlPlane", "platform", "azure"), ic.Azure.Region, ic.ControlPlane.Platform.Azure.InstanceType, controlPlaneReq)...)
}

if ic.Platform.Azure.DefaultMachinePlatform != nil && ic.Platform.Azure.DefaultMachinePlatform.InstanceType != "" {
allErrs = append(allErrs, ValidateInstanceType(client, field.NewPath("platform", "azure", "defaultMachinePlatform"), ic.Azure.Region, ic.Platform.Azure.DefaultMachinePlatform.InstanceType, defaultInstanceReq)...)
}

for idx, compute := range ic.Compute {
fieldPath := field.NewPath("compute").Index(idx)
if compute.Platform.Azure != nil && compute.Platform.Azure.InstanceType != "" {
allErrs = append(allErrs, ValidateInstanceType(client, fieldPath.Child("platform", "azure"),
ic.Azure.Region, compute.Platform.Azure.InstanceType, computeReq)...)
}
}

return allErrs
}

// validateNetworks checks that the user-provided VNet and subnets are valid.
func validateNetworks(client API, p *aztypes.Platform, machineNetworks []types.MachineNetworkEntry, fieldPath *field.Path) field.ErrorList {
allErrs := field.ErrorList{}
Expand Down
70 changes: 70 additions & 0 deletions pkg/asset/installconfig/azure/validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"net"
"testing"

azsku "github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2018-10-01/compute"
aznetwork "github.com/Azure/azure-sdk-for-go/services/network/mgmt/2018-12-01/network"
azres "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-05-01/resources"
azsubs "github.com/Azure/azure-sdk-for-go/services/resources/mgmt/2019-06-01/subscriptions"
Expand Down Expand Up @@ -35,6 +36,34 @@ var (
validResourceGroupResourceType = "resourceGroups"
validResourceSkuRegions = "southeastasia"

instanceTypeSku = []*azsku.ResourceSku{
{Name: to.StringPtr("Standard_A1_v2"), Capabilities: &[]azsku.ResourceSkuCapabilities{{Name: to.StringPtr("vCPUs"), Value: to.StringPtr("1")}, {Name: to.StringPtr("MemoryGB"), Value: to.StringPtr("2")}}},
{Name: to.StringPtr("Standard_D2_v4"), Capabilities: &[]azsku.ResourceSkuCapabilities{{Name: to.StringPtr("vCPUs"), Value: to.StringPtr("2")}, {Name: to.StringPtr("MemoryGB"), Value: to.StringPtr("8")}}},
{Name: to.StringPtr("Standard_D4_v4"), Capabilities: &[]azsku.ResourceSkuCapabilities{{Name: to.StringPtr("vCPUs"), Value: to.StringPtr("4")}, {Name: to.StringPtr("MemoryGB"), Value: to.StringPtr("16")}}},
}

validInstanceTypes = func(ic *types.InstallConfig) {
ic.Platform.Azure.DefaultMachinePlatform.InstanceType = "Standard_D2_v4"
ic.ControlPlane.Platform.Azure.InstanceType = "Standard_D4_v4"
ic.Compute[0].Platform.Azure.InstanceType = "Standard_D2_v4"
}

invalidateDefaultInstanceTypes = func(ic *types.InstallConfig) {
ic.Platform.Azure.DefaultMachinePlatform.InstanceType = "Standard_A1_v2"
}

invalidateControlPlaneInstanceTypes = func(ic *types.InstallConfig) {
ic.ControlPlane.Platform.Azure.InstanceType = "Standard_A1_v2"
}

invalidateComputeInstanceTypes = func(ic *types.InstallConfig) {
ic.Compute[0].Platform.Azure.InstanceType = "Standard_A1_v2"
}

undefinedDefaultInstanceTypes = func(ic *types.InstallConfig) {
ic.Platform.Azure.DefaultMachinePlatform.InstanceType = "Dne_D2_v4"
}

invalidateMachineCIDR = func(ic *types.InstallConfig) {
_, newCidr, _ := net.ParseCIDR("192.168.111.0/24")
ic.MachineNetwork = []types.MachineNetworkEntry{
Expand Down Expand Up @@ -105,6 +134,16 @@ func validInstallConfig() *types.InstallConfig {
DefaultMachinePlatform: &azure.MachinePool{},
},
},
ControlPlane: &types.MachinePool{
Platform: types.MachinePoolPlatform{
Azure: &azure.MachinePool{},
},
},
Compute: []types.MachinePool{{
Platform: types.MachinePoolPlatform{
Azure: &azure.MachinePool{},
},
}},
}
}

Expand Down Expand Up @@ -149,6 +188,31 @@ func TestAzureInstallConfigValidation(t *testing.T) {
edits: editFunctions{invalidateControlPlaneSubnet, invalidateComputeSubnet},
errorMsg: "failed to retrieve compute subnet",
},
{
name: "Valid instance types",
edits: editFunctions{validInstanceTypes},
errorMsg: "",
},
{
name: "Invalid default machine type",
edits: editFunctions{invalidateDefaultInstanceTypes},
errorMsg: `\[platform.azure.defaultMachinePlatform.type: Invalid value: "Standard_A1_v2": instance type does not meet minimum resource requirements of 4 vCPUs, platform.azure.defaultMachinePlatform.type: Invalid value: "Standard_A1_v2": instance type does not meet minimum resource requirements of 16 GB Memory\]`,
},
{
name: "Invalid control plane instance types",
edits: editFunctions{invalidateControlPlaneInstanceTypes},
errorMsg: `[controlPlane.platform.azure.type: Invalid value: "n1\-standard\-1": instance type does not meet minimum resource requirements of 4 vCPUs, controlPlane.platform.azure.type: Invalid value: "n1\-standard\-1": instance type does not meet minimum resource requirements of 16 GB Memory]`,
},
{
name: "Invalid compute instance types",
edits: editFunctions{invalidateComputeInstanceTypes},
errorMsg: `\[compute\[0\].platform.azure.type: Invalid value: "Standard_A1_v2": instance type does not meet minimum resource requirements of 2 vCPUs, compute\[0\].platform.azure.type: Invalid value: "Standard_A1_v2": instance type does not meet minimum resource requirements of 8 GB Memory\]`,
},
{
name: "Undefined default instance types",
edits: editFunctions{undefinedDefaultInstanceTypes},
errorMsg: `platform.azure.defaultMachinePlatform.type: Invalid value: "Dne_D2_v4": not found in region`,
},
{
name: "Invalid region",
edits: editFunctions{invalidateRegion},
Expand All @@ -171,6 +235,12 @@ func TestAzureInstallConfigValidation(t *testing.T) {

azureClient := mock.NewMockAPI(mockCtrl)

// InstanceType
for _, value := range instanceTypeSku {
azureClient.EXPECT().GetVirtualMachineSku(gomock.Any(), to.String(value.Name), gomock.Any()).Return(value, nil).AnyTimes()
}
azureClient.EXPECT().GetVirtualMachineSku(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()

// VirtualNetwork
azureClient.EXPECT().GetVirtualNetwork(gomock.Any(), validNetworkResourceGroup, validVirtualNetwork).Return(virtualNetworkAPIResult, nil).AnyTimes()
azureClient.EXPECT().GetVirtualNetwork(gomock.Any(), gomock.Not(validNetworkResourceGroup), gomock.Not(validVirtualNetwork)).Return(&aznetwork.VirtualNetwork{}, fmt.Errorf("invalid network resource group")).AnyTimes()
Expand Down