convert vm specs to provider specific instance type name #277

anjannath · 2024-08-12T17:56:01Z

fixes #281

adrianriobo · 2024-08-14T08:37:05Z

cmd/mapt/cmd/aws/hosts/fedora.go

+				if viper.GetString(params.LinuxArch) == "arm64" {
+					arch = resources.Arm64
+				}
+				ec2Types, _ = resources.GetAwsMachineTypes(viper.GetInt32(params.CPUs), viper.GetInt32(params.Memory), arch)


This logic needs to be moved to the pkg; inside the actions...

the rational behind... is at the moment the interface to interact with mapt is through cmd, but in case wanna support the k8s controller model it will directly use the logic on actions as so we need to accept the requested features there and search for the type of machine there

so iiuc the required cpus and memories should be part of the Request struct on actions and these will be what passed from the cmd layer and pkg will figure out the instance types based on the cpus,memory and arch

yeah, probably the search module will have an struct with those values you use to query (CPU, Ram, Nested Virtualization, GPU and arch) that struct will be part of the request struct

key part any logic should be encapsulated within actions, then cmd or controllers just fill the request structs there

added CPUs and Memory fields to the Request struct itself, and functions to fetch the instance types

but as you suggest encapsulating these in a new request structs would be better, then we can have methods on it to fetch the instance types.. e.g named AwsInstanceTypeRequest AzureInstanceTypeRequest

adrianriobo · 2024-09-16T08:25:09Z

pkg/provider/util/instancetypes/instancetypes.go

+	return fmt.Errorf("Invalid values for CPUs: %d, Memory: %d and Arch: %s", cpus, memory, arch)
+}
+
+func (r *AwsInstanceRequest) GetMachineTypes() ([]string, error) {


May I would suggest change a bit the structure of the pkg here, I would create a interface for

func GetMachineTypes() ([]string, error) {

And would be implemented by azure and aws, each specific code would be on its own file i.e:

instancesTypes, instancesTypesAzure, instancesTypesAWS, probably for the interface approach you would require an extra module on the same pkg (api) similar to https://github.com/adrianriobo/goax/tree/main/pkg/goax/app

does this make sense to you? I think it would organize the functions a bit better as right now they are kind of mixed on instancesTypes, and eventually if we add gpc support it would add more code on same file

yeah, i can try this approach will update the PR

adrianriobo · 2024-09-16T08:27:44Z

cmd/mapt/cmd/aws/hosts/windows.go

@@ -101,6 +101,7 @@ func getWindowsCreate() *cobra.Command {
 	flagSet.Bool(spot, false, spotDesc)
 	flagSet.Bool(amiKeepCopy, false, amiKeepCopyDesc)
 	flagSet.AddFlagSet(params.GetGHActionsFlagset())
+	flagSet.AddFlagSet(params.GetCpusAndMemoryFlagset())


Here paras are added but not process of the data?

ahh yes, this is missing the corresponding code to make use of the flags, will fix

adrianriobo · 2024-09-18T10:16:42Z

pkg/provider/util/instancetypes/azure.go

+	EncryptionAtHostSupported    bool
+}
+
+func (vm *virtualMachine) NestedVirtSupported() bool {


Are any of this functions public?

yeah the functions are public but only used in the same file, those could be made private, is that your suggestion to make them private?

i was also thinking maybe this file could be in pkg/provider/azure/instancetypes.go instead

is that your suggestion to make them private?

Yeah, that it is

i was also thinking maybe this file could be in pkg/provider/azure/instancetypes.go instead

Yeah an option would be something like that or keep the api for the providers outside and each implementation under the provider, but for the time being it is fine for me to keep it here

okay, PR is updated now, PTALA :)

adrianriobo · 2024-09-18T13:46:07Z

Also I did not check this yet, I would like to know if is there any dependecy on this module from pulumi or is everything only sdk based.

I would like it sdk based ...otherwise if someone try to use may require the aws cli installed down the line, which may prevent for using this module.

anjannath · 2024-09-19T07:00:41Z

Also I did not check this yet, I would like to know if is there any dependecy on this module from pulumi or is everything only sdk based.

I would like it sdk based ...otherwise if someone try to use may require the aws cli installed down the line, which may prevent for using this module.

it uses only the SDK and no dependency on pulumi, here's a small code snippet to use the instancetypes module:

package main

import (
    "fmt"
    "github.com/redhat-developer/mapt/pkg/provider/util/instancetypes"
)

func main() {
    azureInstanceRequest := instancetypes.AzureInstanceRequest{
        CPUs: 4,
        MemoryGib: 8,
        NestedVirt: true,
        Arch: instancetypes.Arm64,
    }
    if vms, err := azureInstanceRequest.GetMachineTypes(); err == nil {
        for _, vm := range vms {
            fmt.Println(vm)
        }
    }
}

and the go.mod for it don't have any pulumi dependencies

 % cat go.mod
module machinenames

go 1.22.4

replace github.com/redhat-developer/mapt => ../../

require github.com/redhat-developer/mapt v0.0.0-00010101000000-000000000000

require (
        github.com/Azure/azure-sdk-for-go/sdk/azcore v1.13.0 // indirect
        github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.7.0 // indirect
        github.com/Azure/azure-sdk-for-go/sdk/internal v1.10.0 // indirect
        github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v6 v6.0.0 // indirect
        github.com/AzureAD/microsoft-authentication-library-for-go v1.2.2 // indirect
        github.com/atotto/clipboard v0.1.4 // indirect
        github.com/aws/amazon-ec2-instance-selector/v2 v2.4.2-0.20231006140257-d989c5d76f0e // indirect
        github.com/aws/aws-sdk-go v1.45.6 // indirect
        github.com/aws/aws-sdk-go-v2 v1.27.2 // indirect
        github.com/aws/aws-sdk-go-v2/config v1.27.18 // indirect
        github.com/aws/aws-sdk-go-v2/credentials v1.17.18 // indirect
        github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.5 // indirect
        github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.9 // indirect
        github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.9 // indirect
        github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect
        github.com/aws/aws-sdk-go-v2/service/ec2 v1.142.0 // indirect
        github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.2 // indirect
        github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.11 // indirect
        github.com/aws/aws-sdk-go-v2/service/pricing v1.21.6 // indirect
        github.com/aws/aws-sdk-go-v2/service/sso v1.20.11 // indirect
        github.com/aws/aws-sdk-go-v2/service/ssooidc v1.24.5 // indirect
        github.com/aws/aws-sdk-go-v2/service/sts v1.28.12 // indirect
        github.com/aws/smithy-go v1.20.2 // indirect
        github.com/aymanbagabas/go-osc52/v2 v2.0.1 // indirect
        github.com/blang/semver/v4 v4.0.0 // indirect
        github.com/charmbracelet/bubbles v0.16.1 // indirect
        github.com/charmbracelet/bubbletea v0.25.0 // indirect
        github.com/charmbracelet/lipgloss v0.7.1 // indirect
        github.com/containerd/console v1.0.4 // indirect
        github.com/evertras/bubble-table v0.15.2 // indirect
        github.com/golang-jwt/jwt/v5 v5.2.1 // indirect
        github.com/google/uuid v1.6.0 // indirect
        github.com/imdario/mergo v0.3.16 // indirect
        github.com/jmespath/go-jmespath v0.4.0 // indirect
        github.com/kylelemons/godebug v1.1.0 // indirect
        github.com/lucasb-eyer/go-colorful v1.2.0 // indirect
        github.com/mattn/go-isatty v0.0.20 // indirect
        github.com/mattn/go-localereader v0.0.1 // indirect
        github.com/mattn/go-runewidth v0.0.15 // indirect
        github.com/mitchellh/go-homedir v1.1.0 // indirect
        github.com/muesli/ansi v0.0.0-20230316100256-276c6243b2f6 // indirect
        github.com/muesli/cancelreader v0.2.2 // indirect
        github.com/muesli/reflow v0.3.0 // indirect
        github.com/muesli/termenv v0.15.2 // indirect
        github.com/oliveagle/jsonpath v0.0.0-20180606110733-2e52cf6e6852 // indirect
        github.com/patrickmn/go-cache v2.1.0+incompatible // indirect
        github.com/pkg/browser v0.0.0-20240102092130-5ac0b6a4141c // indirect
        github.com/rivo/uniseg v0.4.7 // indirect
        github.com/sahilm/fuzzy v0.1.0 // indirect
        go.uber.org/multierr v1.11.0 // indirect
        golang.org/x/crypto v0.25.0 // indirect
        golang.org/x/net v0.27.0 // indirect
        golang.org/x/sync v0.7.0 // indirect
        golang.org/x/sys v0.22.0 // indirect
        golang.org/x/term v0.22.0 // indirect
        golang.org/x/text v0.16.0 // indirect
)

Signed-off-by: Anjan Nath <[email protected]>

openshift-ci bot added the do-not-merge/work-in-progress label Aug 12, 2024

anjannath force-pushed the spec-to-vmtypes branch 2 times, most recently from a0d52bd to 14d8432 Compare August 13, 2024 19:11

anjannath changed the title ~~[wip] convert vm specs to provider specific instance type name~~ convert vm specs to provider specific instance type name Aug 13, 2024

openshift-ci bot removed the do-not-merge/work-in-progress label Aug 13, 2024

adrianriobo reviewed Aug 14, 2024

View reviewed changes

anjannath force-pushed the spec-to-vmtypes branch 6 times, most recently from b9aba0e to aa9fd25 Compare August 21, 2024 09:52

anjannath force-pushed the spec-to-vmtypes branch 4 times, most recently from 3b2ef23 to 7a2978b Compare August 27, 2024 08:09

anjannath mentioned this pull request Sep 2, 2024

Spot module as a separate package #284

Merged

anjannath force-pushed the spec-to-vmtypes branch 2 times, most recently from 1786c81 to 409f981 Compare September 3, 2024 09:41

adrianriobo reviewed Sep 16, 2024

View reviewed changes

anjannath force-pushed the spec-to-vmtypes branch from 409f981 to c1c16a1 Compare September 18, 2024 07:52

adrianriobo reviewed Sep 18, 2024

View reviewed changes

adrianriobo approved these changes Sep 19, 2024

View reviewed changes

convert vm specs to provider specific instance type name

51981df

Signed-off-by: Anjan Nath <[email protected]>

anjannath force-pushed the spec-to-vmtypes branch from c1c16a1 to 51981df Compare September 19, 2024 07:19

adrianriobo self-requested a review September 19, 2024 07:23

adrianriobo approved these changes Sep 19, 2024

View reviewed changes

adrianriobo merged commit 0971b20 into redhat-developer:main Sep 19, 2024
4 checks passed

adrianriobo mentioned this pull request Oct 3, 2024

[Bug] Missing params for search instance type #301

Closed

adrianriobo mentioned this pull request Oct 14, 2024

[BUG] Documentation is missing information on new parameters to set type of machine based on args #310

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert vm specs to provider specific instance type name #277

convert vm specs to provider specific instance type name #277

anjannath commented Aug 12, 2024 •

edited

Loading

adrianriobo Aug 14, 2024

anjannath Aug 19, 2024

adrianriobo Aug 19, 2024

adrianriobo Aug 19, 2024

anjannath Aug 19, 2024

adrianriobo Sep 16, 2024

anjannath Sep 18, 2024

adrianriobo Sep 16, 2024

anjannath Sep 18, 2024

adrianriobo Sep 18, 2024

anjannath Sep 18, 2024

anjannath Sep 18, 2024

adrianriobo Sep 18, 2024

anjannath Sep 19, 2024

adrianriobo commented Sep 18, 2024

anjannath commented Sep 19, 2024

convert vm specs to provider specific instance type name #277

convert vm specs to provider specific instance type name #277

Conversation

anjannath commented Aug 12, 2024 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrianriobo commented Sep 18, 2024

anjannath commented Sep 19, 2024

anjannath commented Aug 12, 2024 •

edited

Loading