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

ezr-ondrej · 2023-10-18T13:59:25Z

We should allow users to follow Azure default and use the resource group location by default.

This PR also makes the default Resource Group the image resource group.

ezr-ondrej · 2023-10-19T07:50:01Z

/retest

lzap · 2023-10-19T09:12:49Z

Failed: response not updated with instances

Maybe we need to update a test - images are now being launched in the location of the resource group rather than some hardcoded default, @akhil-jha

akhil-jha · 2023-10-19T14:12:23Z

Failed: response not updated with instances

Maybe we need to update a test - images are now being launched in the location of the resource group rather than some hardcoded default, @akhil-jha

Probably true.
I'll look at it tomorrow.

ezr-ondrej · 2023-10-19T14:58:53Z

/retest

lets try once more for good luck 🤷

lzap · 2023-10-19T16:16:42Z

Not your day: Failed: response not updated with instances

ezr-ondrej · 2023-10-20T08:36:33Z

Ah, with so many failures in the pipeline, I've just not stopped to think that it might be a regression 🤦

"InvalidResourceLocation\",\n    \"message\": \"The specified location 'eastus_1' is invalid. A location must consist of characters, whitespace, digit, or following symbols

internal/preload/instance_type.go

ezr-ondrej · 2023-10-20T09:51:45Z

internal/services/azure_reservation_service.go

@@ -147,9 +149,10 @@ func CreateAzureReservation(w http.ResponseWriter, r *http.Request) {
 		EdgeID:    logging.EdgeRequestId(r.Context()),
 		AccountID: identity.AccountId(r.Context()),
 		Args: jobs.LaunchInstanceAzureTaskArgs{
+			// Ignore location for now
+			//Location:          reservation.Detail.Location,


We have ignored location previously and always substituted it for eastus, now we're just explicit about it.

ezr-ondrej · 2023-10-20T10:55:33Z

internal/services/azure_reservation_service.go

+		if preload.AzureInstanceType.ValidateRegion(payload.Location) {
+			logger.Warn().Msgf("Azure region passed with location suffix (%s), this is deprecated behaviour format", payload.Location)


⚠️ change for the future, we now validate region, not location.

In the past, we've validated for location, but then actually ignored the param down the line and forced eastus, but we actually need regions, not locations in all the calls (even tho azure calls them Location 🤦)

Does this need to be part of this PR? Also I am not sure if I am following here. Can you explain?

Well, we required region+zone in the past, but then we dropped the value and used hard coded eastus, so it worked, but our client expects only region (without the zone suffix).

So this patch accepts both, but when user passes the version with suffix, we now warn about it, so we can keep track of how many usages of that there is.

Ok this is fine, if you want to go an extra mile I think you can refactor ValidateRegion to accept variadic string and concatenate by _ because this is exactly how it works for all the clouds.

It's actually dash (-) for EC2 and GCP and underscore _ for Azure 😮‍💨 xD

lzap · 2023-10-20T11:52:09Z

iqe_provisioning_api.exceptions.ApiAttributeError: V1AWSReservationResponseDetail has no attribute 'public_ipv4' at ['['received_data', 'instances', 0, 'detail']']['public_ipv4']

Not related (out of quota).

lzap

I am not sure about the terminology. We have the "location" term and then there are "region" and "zone". To me, it looks like "location" is simply "region" and "zone" concatenated into one string, maybe we could adopt this in the codebase.

We need to figure out a good approach, the current proposal simply ditches the location completely, however, it is possible to technically have a VM in a different zone than resource group. Sure, it is not recommended, but it is possible. I would suggest that we keep this possibility at least in the API.

There are some changes I do not understand.

lzap · 2023-10-20T13:28:59Z

internal/clients/http/image_builder/image_client.go

 	}
-	return fmt.Sprintf("/resourceGroups/%s/providers/Microsoft.Compute/images/%s", azureUploadRequest.ResourceGroup, uploadOptions.ImageName), nil
+	return azureUploadRequest.ResourceGroup, uploadOptions.ImageName, nil
 }


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

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

lzap · 2023-10-20T13:32:01Z

internal/clients/interface.go

+type AzureResourceGroup struct {
+	ID       string
+	Name     string
+	Location string


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

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.

internal/jobs/launch_instance_azure.go

lzap · 2023-10-20T13:55:53Z

internal/services/azure_reservation_service.go

+		if preload.AzureInstanceType.ValidateRegion(payload.Location) {
+			logger.Warn().Msgf("Azure region passed with location suffix (%s), this is deprecated behaviour format", payload.Location)


Does this need to be part of this PR? Also I am not sure if I am following here. Can you explain?

api/openapi.gen.json

lzap

All right, needs rebase tho.

We should allow users to follow Azure default and use the resource group location by default.

ezr-ondrej · 2023-10-23T15:16:08Z

Rebased and all 🍏

lzap · 2023-10-24T06:21:07Z

Okay, thanks a bunch for working on this.

ezr-ondrej force-pushed the default_azure_location branch from 5082f94 to e7342e6 Compare October 18, 2023 17:41

ezr-ondrej force-pushed the default_azure_location branch from e7342e6 to dae1c21 Compare October 19, 2023 13:48

ezr-ondrej force-pushed the default_azure_location branch from dae1c21 to 6231399 Compare October 20, 2023 09:09

ezr-ondrej commented Oct 20, 2023

View reviewed changes

internal/preload/instance_type.go Outdated Show resolved Hide resolved

ezr-ondrej commented Oct 20, 2023

View reviewed changes

ezr-ondrej force-pushed the default_azure_location branch from 6231399 to 128afe2 Compare October 20, 2023 10:51

ezr-ondrej commented Oct 20, 2023

View reviewed changes

ezr-ondrej force-pushed the default_azure_location branch from 128afe2 to 83a74e3 Compare October 20, 2023 10:57

lzap requested changes Oct 20, 2023

View reviewed changes

ezr-ondrej force-pushed the default_azure_location branch 2 times, most recently from e843775 to f1a09be Compare October 20, 2023 14:33

ezr-ondrej commented Oct 20, 2023

View reviewed changes

api/openapi.gen.json Show resolved Hide resolved

lzap approved these changes Oct 23, 2023

View reviewed changes

ezr-ondrej force-pushed the default_azure_location branch 2 times, most recently from e5d2cce to 1a34c67 Compare October 23, 2023 14:46

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

008fded

We should allow users to follow Azure default and use the resource group location by default.

ezr-ondrej force-pushed the default_azure_location branch from 1a34c67 to 008fded Compare October 23, 2023 14:53

lzap merged commit f26e2b5 into RHEnVision:main Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ezr-ondrej commented Oct 18, 2023 •

edited

Loading

ezr-ondrej commented Oct 19, 2023

lzap commented Oct 19, 2023

akhil-jha commented Oct 19, 2023

ezr-ondrej commented Oct 19, 2023

lzap commented Oct 19, 2023

ezr-ondrej commented Oct 20, 2023

ezr-ondrej Oct 20, 2023

ezr-ondrej Oct 20, 2023

lzap Oct 20, 2023

ezr-ondrej Oct 20, 2023

lzap Oct 23, 2023

ezr-ondrej Oct 23, 2023

lzap commented Oct 20, 2023 •

edited

Loading

lzap left a comment

lzap Oct 20, 2023

ezr-ondrej Oct 20, 2023

lzap Oct 20, 2023

ezr-ondrej Oct 20, 2023 •

edited

Loading

lzap Oct 20, 2023

lzap left a comment

ezr-ondrej commented Oct 23, 2023

lzap commented Oct 24, 2023

		if preload.AzureInstanceType.ValidateRegion(payload.Location) {
		logger.Warn().Msgf("Azure region passed with location suffix (%s), this is deprecated behaviour format", payload.Location)

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

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

Conversation

ezr-ondrej commented Oct 18, 2023 • edited Loading

ezr-ondrej commented Oct 19, 2023

lzap commented Oct 19, 2023

akhil-jha commented Oct 19, 2023

ezr-ondrej commented Oct 19, 2023

lzap commented Oct 19, 2023

ezr-ondrej commented Oct 20, 2023

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Oct 20, 2023 • edited Loading

lzap left a comment

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

ezr-ondrej Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap left a comment

Choose a reason for hiding this comment

ezr-ondrej commented Oct 23, 2023

lzap commented Oct 24, 2023

ezr-ondrej commented Oct 18, 2023 •

edited

Loading

lzap commented Oct 20, 2023 •

edited

Loading

ezr-ondrej Oct 20, 2023 •

edited

Loading