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.

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

feat(HMS-2255): launch to location of the resource group by default #730

Merged
merged 1 commit into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions api/openapi.gen.json
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@
"amount": 1,
"image_id": "composer-api-081fc867-838f-44a5-af03-8b8def808431",
"instance_size": "Basic_A0",
"location": "useast_1",
"location": "useast",
lzap marked this conversation as resolved.
Show resolved Hide resolved
"name": "my-instance",
"poweroff": false,
"pubkey_id": 42,
Expand Down Expand Up @@ -680,6 +680,7 @@
"type": "string"
},
"location": {
"description": "Location (also known as region) to deploy the VM into, be aware it needs to be the same as the image location. Defaults to the Resource Group location, or 'eastus' when also creating the resource group.",
"type": "string"
},
"name": {
Expand All @@ -693,7 +694,7 @@
"type": "integer"
},
"resource_group": {
"description": "Azure resource group name to deploy the VM resources into. Optional, defaults to 'redhat-deployed'.",
"description": "Azure resource group name to deploy the VM resources into. Optional, defaults to images resource group and when not found to 'redhat-deployed'.",
"type": "string"
},
"source_id": {
Expand Down Expand Up @@ -1363,7 +1364,7 @@
"name": "GPL-3.0"
},
"title": "provisioning-api",
"version": "1.8.0"
"version": "1.9.0"
},
"openapi": "3.0.0",
"paths": {
Expand Down
7 changes: 4 additions & 3 deletions api/openapi.gen.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ components:
type: string
location:
type: string
description: Location (also known as region) to deploy the VM into, be aware it needs to be the same as the image location. Defaults to the Resource Group location, or 'eastus' when also creating the resource group.
name:
type: string
poweroff:
Expand All @@ -104,7 +105,7 @@ components:
format: int64
resource_group:
type: string
description: Azure resource group name to deploy the VM resources into. Optional, defaults to 'redhat-deployed'.
description: Azure resource group name to deploy the VM resources into. Optional, defaults to images resource group and when not found to 'redhat-deployed'.
source_id:
type: string
v1.AzureReservationResponse:
Expand Down Expand Up @@ -664,7 +665,7 @@ components:
amount: 1
image_id: composer-api-081fc867-838f-44a5-af03-8b8def808431
instance_size: Basic_A0
location: useast_1
location: useast
name: my-instance
poweroff: false
pubkey_id: 42
Expand Down Expand Up @@ -952,7 +953,7 @@ info:
description: Provisioning service API
license:
name: GPL-3.0
version: 1.8.0
version: 1.9.0
paths:
/availability_status/sources:
post:
Expand Down
2 changes: 1 addition & 1 deletion cmd/spec/VERSION
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1.8.0
1.9.0
2 changes: 1 addition & 1 deletion cmd/spec/example_reservation.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ var AwsReservationResponsePayloadDoneExample = payloads.AWSReservationResponse{
var AzureReservationRequestPayloadExample = payloads.AzureReservationRequest{
PubkeyID: 42,
SourceID: "654321",
Location: "useast_1",
Location: "useast",
ResourceGroup: "redhat-hcc",
InstanceSize: "Basic_A0",
Amount: 1,
Expand Down
16 changes: 10 additions & 6 deletions internal/clients/http/azure/create_vm.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ const (
vmPollFrequency = 10 * time.Second
)

func newAzureResourceGroup(group armresources.ResourceGroup) clients.AzureResourceGroup {
return clients.AzureResourceGroup{ID: *group.ID, Name: *group.Name, Location: *group.Location}
}

func (c *client) BeginCreateVM(ctx context.Context, networkInterface *armnetwork.Interface, vmParams clients.AzureInstanceParams, vmName string) (string, error) {
ctx, span := telemetry.StartSpan(ctx, "BeginCreateVM")
defer span.End()
Expand Down Expand Up @@ -154,24 +158,24 @@ func (c *client) prepareVMNetworking(ctx context.Context, subnet *armnetwork.Sub
return networkInterface, publicIP, nil
}

func (c *client) EnsureResourceGroup(ctx context.Context, name string, location string) (*string, error) {
func (c *client) EnsureResourceGroup(ctx context.Context, name string, location string) (clients.AzureResourceGroup, error) {
resourceGroupClient, err := c.newResourceGroupsClient(ctx)
if err != nil {
return nil, err
return clients.AzureResourceGroup{}, err
}

logger := logger(ctx)
getResp, err := resourceGroupClient.Get(ctx, name, nil)
if err == nil {
return getResp.ResourceGroup.ID, nil
return newAzureResourceGroup(getResp.ResourceGroup), nil
}
if err != nil {
var azErr *azcore.ResponseError
if errors.As(err, &azErr) && azErr.StatusCode == http.StatusNotFound {
logger.Debug().Msgf("resource group %s not found, creating", name)
// 404 is expected, continue
} else {
return nil, fmt.Errorf("failed to fetch resource group: %w", err)
return clients.AzureResourceGroup{}, fmt.Errorf("failed to fetch resource group: %w", err)
}
}

Expand All @@ -183,10 +187,10 @@ func (c *client) EnsureResourceGroup(ctx context.Context, name string, location

resp, err := resourceGroupClient.CreateOrUpdate(ctx, name, parameters, nil)
if err != nil {
return nil, fmt.Errorf("cannot create resource group: %w", err)
return clients.AzureResourceGroup{}, fmt.Errorf("cannot create resource group: %w", err)
}

return resp.ResourceGroup.ID, nil
return newAzureResourceGroup(resp.ResourceGroup), nil
}

func (c *client) createVirtualNetwork(ctx context.Context, location string, resourceGroupName string, name string) (*armnetwork.VirtualNetwork, error) {
Expand Down
20 changes: 10 additions & 10 deletions internal/clients/http/image_builder/image_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,46 +88,46 @@ func (c *ibClient) GetAWSAmi(ctx context.Context, composeUUID uuid.UUID, instanc
return uploadStatus.Ami, nil
}

func (c *ibClient) GetAzureImageID(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, error) {
func (c *ibClient) GetAzureImageInfo(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, string, error) {
logger := logger(ctx)
logger.Trace().Msgf("Getting Azure ID of image %v", composeUUID.String())

composeStatus, err := c.getComposeStatus(ctx, composeUUID)
if err != nil {
return "", err
return "", "", err
}
if composeStatus == nil {
logger.Warn().Msg("Compose status is not ready")
return "", fmt.Errorf("getting azure id: %w", http.ErrImageStatus)
return "", "", fmt.Errorf("getting azure id: %w", http.ErrImageStatus)
}

logger.Trace().Msgf("Verifying Azure type")
if composeStatus.ImageStatus.UploadStatus == nil {
return "", fmt.Errorf("%w: upload status is nil", http.ErrUploadStatus)
return "", "", fmt.Errorf("%w: upload status is nil", http.ErrUploadStatus)
}
if composeStatus.ImageStatus.UploadStatus.Type != UploadTypesAzure {
return "", fmt.Errorf("%w: expected image type Azure, got %s", http.ErrUnknownImageType, composeStatus.ImageStatus.UploadStatus.Type)
return "", "", fmt.Errorf("%w: expected image type Azure, got %s", http.ErrUnknownImageType, composeStatus.ImageStatus.UploadStatus.Type)
}
if len(composeStatus.Request.ImageRequests) < 1 {
logger.Error().Msg(http.ErrImageRequestNotFound.Error())
return "", http.ErrImageRequestNotFound
return "", "", http.ErrImageRequestNotFound
}

imageArch, archErr := clients.MapArchitectures(ctx, string(composeStatus.Request.ImageRequests[0].Architecture))
if archErr != nil || imageArch != instanceType.Architecture {
return "", http.ErrImageArchInvalid
return "", "", http.ErrImageArchInvalid
}

uploadOptions, err := composeStatus.ImageStatus.UploadStatus.Options.AsAzureUploadStatus()
if err != nil {
return "", fmt.Errorf("%w: not an Azure status", http.ErrUploadStatus)
return "", "", fmt.Errorf("%w: not an Azure status", http.ErrUploadStatus)
}

azureUploadRequest, err := composeStatus.Request.ImageRequests[0].UploadRequest.Options.AsAzureUploadRequestOptions()
if err != nil {
return "", fmt.Errorf("failed to decode Azure upload request from IB: %w", err)
return "", "", fmt.Errorf("failed to decode Azure upload request from IB: %w", err)
}
return fmt.Sprintf("/resourceGroups/%s/providers/Microsoft.Compute/images/%s", azureUploadRequest.ResourceGroup, uploadOptions.ImageName), nil
return azureUploadRequest.ResourceGroup, uploadOptions.ImageName, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Multiple return values (except error) typically mean we should think about introducing a type. But this is still okay I believe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was just lazy here, let me know if you insist, I agree struct would work better here 🤔


func (c *ibClient) GetGCPImageName(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, error) {
Expand Down
2 changes: 1 addition & 1 deletion internal/clients/instance_params.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type AWSInstanceParams struct {

// AzureInstanceParams define parameters for a single instance launch on Azure.
type AzureInstanceParams struct {
// Location - to deploy into
// Location - to deploy into, defaults to Resource Group location
Location string

// ResourceGroupName to launch the instance in
Expand Down
14 changes: 9 additions & 5 deletions internal/clients/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,12 @@ import (
"github.com/google/uuid"
)

type AzureResourceGroup struct {
ID string
Name string
Location string
Copy link
Member

Choose a reason for hiding this comment

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

You removed bunch of locations to region, shall we remove this one as well?

Copy link
Member Author

@ezr-ondrej ezr-ondrej Oct 20, 2023

Choose a reason for hiding this comment

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

I went back on the decision, lets keep Location, but explain it's region, not region with availability zone, as it souns like it the first look. I've added more description in the OpenAPI spec.

}

// GetSourcesClient returns Sources interface implementation. There are currently
// two implementations available: HTTP and stub
var GetSourcesClient func(ctx context.Context) (Sources, error)
Expand Down Expand Up @@ -39,11 +45,9 @@ type ImageBuilder interface {
// It also verifies the image is built successfully and for the right architecture.
GetAWSAmi(ctx context.Context, composeUUID uuid.UUID, instanceType InstanceType) (string, error)

// GetAzureImageID returns partial image id, that is missing the subscription prefix
// Full name is /subscriptions/<subscription-id>/resourceGroups/<Group>/providers/Microsoft.Compute/images/<ImageName>
// GetAzureImageID returns /resourceGroups/<Group>/providers/Microsoft.Compute/images/<ImageName>
// GetAzureImageInfo returns Resource Group name and image name from the image builder info.
// It also verifies the image is built successfully and for the right architecture.
GetAzureImageID(ctx context.Context, composeUUID uuid.UUID, instanceType InstanceType) (string, error)
GetAzureImageInfo(ctx context.Context, composeUUID uuid.UUID, instanceType InstanceType) (string, string, error)

// GetGCPImageName returns GCP image name
// It also verifies the image is built successfully and for the right architecture.
Expand Down Expand Up @@ -117,7 +121,7 @@ type Azure interface {
TenantId(ctx context.Context) (AzureTenantId, error)

// EnsureResourceGroup makes sure that group with give name exists in a location
EnsureResourceGroup(ctx context.Context, name string, location string) (*string, error)
EnsureResourceGroup(ctx context.Context, name string, location string) (AzureResourceGroup, error)

// CreateVMs creates multiple Azure virtual machines
// Returns array of instance IDs and error if something went wrong
Expand Down
8 changes: 6 additions & 2 deletions internal/clients/stubs/azure_stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,16 +92,20 @@ func (stub *AzureClientStub) WaitForVM(ctx context.Context, resumeToken string)
return "", ErrNotStartedVM
}

func (stub *AzureClientStub) EnsureResourceGroup(ctx context.Context, name string, location string) (*string, error) {
func (stub *AzureClientStub) EnsureResourceGroup(ctx context.Context, name string, location string) (clients.AzureResourceGroup, error) {
id := strconv.Itoa(len(stub.createdRgs) + 1)

if name == "existing" {
return clients.AzureResourceGroup{ID: "/subscriptions/123/resourceGroups/mockedID", Name: "existing", Location: "westeurope"}, nil
}

rg := armresources.ResourceGroup{
ID: &id,
Name: &name,
Location: &location,
}
stub.createdRgs = append(stub.createdRgs, &rg)
return &id, nil
return clients.AzureResourceGroup{ID: id, Name: name, Location: location}, nil
}

func (stub *AzureClientStub) TenantId(ctx context.Context) (clients.AzureTenantId, error) {
Expand Down
4 changes: 2 additions & 2 deletions internal/clients/stubs/image_builder_stub.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ func (mock *ImageBuilderClientStub) GetAWSAmi(ctx context.Context, composeUUID u
return "ami-0c830793775595d4b-test", nil
}

func (mock *ImageBuilderClientStub) GetAzureImageID(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, error) {
return "/resourceGroups/redhat-deployed/providers/Microsoft.Compute/images/composer-api-92ea98f8-7697-472e-80b1-7454fa0e7fa7", nil
func (mock *ImageBuilderClientStub) GetAzureImageInfo(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, string, error) {
return "myTestGroup", "composer-api-92ea98f8-7697-472e-80b1-7454fa0e7fa7", nil
}

func (mock *ImageBuilderClientStub) GetGCPImageName(ctx context.Context, composeUUID uuid.UUID, instanceType clients.InstanceType) (string, error) {
Expand Down
23 changes: 19 additions & 4 deletions internal/jobs/launch_instance_azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package jobs
import (
"context"
"fmt"
"regexp"
"strconv"

"github.com/RHEnVision/provisioning-backend/internal/config"
Expand Down Expand Up @@ -31,7 +32,7 @@ type LaunchInstanceAzureTaskArgs struct {
// Associated reservation
ReservationID int64

// Location to provision the instances into
// Location to provision the instances into when blank, uses the Resource Group location
Location string

// Associated public key
Expand Down Expand Up @@ -72,6 +73,15 @@ func HandleLaunchInstanceAzure(ctx context.Context, job *worker.Job) {
logger.Debug().Msg("Resource group has not been set, defaulting to 'redhat-deployed'")
args.ResourceGroupName = DefaultAzureResourceGroupName
}
if args.Location != "" {
// Match for availability zone suffix and removes it.
// For backwards compatibility, we accept both forms and adjust here,
// but we want to accept only region without the zone info in the future.
res, e := regexp.MatchString(`_[1-6]\z`, args.Location)
if e == nil && res {
args.Location = args.Location[0 : len(args.Location)-2]
}
}
lzap marked this conversation as resolved.
Show resolved Hide resolved

// ensure panic finishes the job
defer func() {
Expand Down Expand Up @@ -116,13 +126,18 @@ func DoEnsureAzureResourceGroup(ctx context.Context, args *LaunchInstanceAzureTa
return fmt.Errorf("cannot create new Azure client: %w", err)
}

resourceGroupID, err := azureClient.EnsureResourceGroup(ctx, args.ResourceGroupName, location)
resourceGroup, err := azureClient.EnsureResourceGroup(ctx, args.ResourceGroupName, location)
if err != nil {
span.SetStatus(codes.Error, "cannot create resource group")
logger.Error().Err(err).Msg("Cannot create resource group")
return fmt.Errorf("failed to ensure resource group: %w", err)
}
logger.Trace().Msgf("Using resource group id=%s", *resourceGroupID)
logger.Trace().Msgf("Using resource group id=%s", resourceGroup)

if args.Location == "" {
logger.Debug().Str("azure_location", resourceGroup.Location).Msg("Using location from Resource Group")
args.Location = resourceGroup.Location
}

return nil
}
Expand Down Expand Up @@ -168,7 +183,7 @@ func DoLaunchInstanceAzure(ctx context.Context, args *LaunchInstanceAzureTaskArg
}

vmParams := clients.AzureInstanceParams{
Location: location,
Location: args.Location,
ResourceGroupName: args.ResourceGroupName,
ImageID: args.AzureImageID,
Pubkey: pubkey,
Expand Down
Loading