Added some configurable options for ProxMox API via CloudConfig #1370

asyslinux · 2022-09-03T20:55:35Z

1356 - Had some questions

Added full support of configurable options (CloudConfig) with backward compatibility:

Arch (Not work correctly through ProxMox API, but may be need later for other Clouds) (default: "" ; autodetect by ProxMox)
Machine (Not used if empty) (default: "" ; autodetect by ProxMox)
Sockets (default: "1")
Cores (default: "1")
Numa (default: "0")
Memory (default: "512M")
BridgeName (BridgeName0) (default: "vmbr0")
BridgeName1 (default: "") (Not used yet)
IsoStorageName (default: "local")
StorageName (default: "local-lvm")
Onboot (default: "0")
Protection (default: "0")

Added support for timestamps in instance names (if not used -i argument from command line).
Also fixed and extended some checks.
Added import dep of "github.com/docker/go-units" in proxmox/proxmox_instance.go for converting human-readable M,G memory settings to bytes for ProxMox

Short example of configuration file:

{
    "Dirs": ["static"],
    "NameServers": ["172.21.1.1"],
    "CloudConfig": {
        "BridgeName": "vmbr0",
        "ImageName": "mytest",
        "IsoStorageName": "local",
        "StorageName": "local-lvm"
    },
    "RunConfig": {
        "Accel": true,
        "InstanceName": "mytest",
        "ImageName": "mytest",
        "IPAddress": "172.21.1.75",
        "NetMask": "255.255.255.0",
        "Gateway": "172.21.1.1",
        "Verbose": true,
        "ShowErrors": true,
        "ShowWarnings": true
    }
}

Full example of configuration file:

{
    "Dirs": ["static"],
    "NameServers": ["172.21.1.1"],
    "CloudConfig": {
        "Arch": "x86_64",
        "Machine": "q35",
        "Sockets": 2,
        "Cores": 4,
        "Numa": true,
        "Memory": "4096M",
        "BridgeName0": "vmbr0",
        "ImageName": "mytest",
        "IsoStorageName": "local",
        "StorageName": "local-lvm",
        "Protection": false,
        "Onboot": true
    },
    "RunConfig": {
        "Accel": true,
        "InstanceName": "mytest",
        "ImageName": "mytest",
        "IPAddress": "172.21.1.75",
        "NetMask": "255.255.255.0",
        "Gateway": "172.21.1.1",
        "Verbose": true,
        "ShowErrors": true,
        "ShowWarnings": true
    }
}

Now with fully customizable base options, you can already normally use ops with ProxMox. It may be necessary to add some more options to the command line arguments, but this is after agreement with the Author of ops utility.

eyberg · 2022-09-04T05:51:05Z

so we now have 15 different infrastructure targets, i think it'd be best if cloudconfig/runconfig remain as generic as possible (eg: a given var should be more or less equal across target platforms), open to suggestion on this but kinda wondering if it might make sense to introduce optional platform specific config

we could provide a new interface that supplies custom config pertinent to each platform

for example all for these (which don't have equivalents in other platforms):

BridgeName (BridgeName0) (default: "vmbr0")
IsoStorageName (default: "local")
StorageName (default: "local-lvm")
Onboot (default: "0")
Protection (default: "0")

could all be a part of a proxmox specific config so we don't pollute all the other platform's config

eyberg · 2022-09-04T05:56:27Z

proxmox/proxmox_instance.go

+		memoryHmn = "512M"
+	}
+
+	memoryInt, err := units.RAMInBytes(memoryHmn)


we're hoping to use less imports (and could use a deepclean on the ones that exist) but if possible it be good to just copy in whatever method you need here and plop it in here https://github.com/nanovms/ops/blob/master/lepton/helpers.go

Ok, i`ll be move part of code (units.RAMInBytes) to lepton/helpers.go, and will remove import.

eyberg · 2022-09-04T06:00:14Z

proxmox/proxmox_instance.go

+	storageName := config.CloudConfig.StorageName
+	isoStorageName := config.CloudConfig.IsoStorageName
+	bridgeName := config.CloudConfig.BridgeName
+	bridgeName0 := config.CloudConfig.BridgeName0


what's the rationale for 2 of these? the expectation for multiple nics?

Yes, because often for production apps need to be have 2 nics, later i want add bridgeName1 for secondary nic. And not only use with DHCP, also will need add a static settings IPs for secondary nic in RunConfig, for advanced users like me this is important, for simple users enough DHCP + 1 nic. Two variables (bridgeName and bridgeName0) I use for make a simpler configuration examples, where need only one nic (can use bridgeName without number). But we can remove bridgeName variable if You want, for me this is not necessary.

so we apparently do support multiple nics, nanovms/nanos#1461 , #1009 however, ops needs more support to enable this fully, in particular network settings for static assignment need to be added in ops but this should prob be done in a different pr

yes, this is we can make via other pr after solve this pr

asyslinux · 2022-09-04T06:59:01Z

so we now have 15 different infrastructure targets, i think it'd be best if cloudconfig/runconfig remain as generic as possible (eg: a given var should be more or less equal across target platforms), open to suggestion on this but kinda wondering if it might make sense to introduce optional platform specific config

we could provide a new interface that supplies custom config pertinent to each platform

for example all for these (which don't have equivalents in other platforms):

Are you mean like this? Ok, i will try to add separate ProxmoxConfig part, please wait a little, but this is broke backward compatibility:

{
    "Dirs": ["static"],
    "NameServers": ["172.21.1.1"],
    "CloudConfig": {
        "ImageName": "mytest",
        "Arch": "x86_64",
        "Machine": "q35",
        "Sockets": 2,
        "Cores": 4,
        "Numa": true,
        "Memory": "4096M"
    },
    "ProxmoxConfig": {
        "BridgeName0": "vmbr0",
        "IsoStorageName": "local",
        "StorageName": "local-lvm",
        "Protection": false,
        "Onboot": true
    },
    "RunConfig": {
        "Accel": true,
        "InstanceName": "mytest",
        "ImageName": "mytest",
        "IPAddress": "172.21.1.75",
        "NetMask": "255.255.255.0",
        "Gateway": "172.21.1.1",
        "Verbose": true,
        "ShowErrors": true,
        "ShowWarnings": true
    }
}

…on/helpers.go

asyslinux · 2022-09-04T07:46:54Z

I changed the code as you indicated. If You prefer to move other options to ProxmoxConfig, please say me.

Now configuration files is like as:
Full example of configuration file:

{
    "Dirs": ["static"],
    "NameServers": ["172.21.1.1"],
    "CloudConfig": {
        "ImageName": "mytest",
        "Arch": "x86_64",
        "Machine": "q35",
        "Sockets": 2,
        "Cores": 4,
        "Numa": true,
        "Memory": "4096M"
    },
    "ProxmoxConfig": {
        "BridgeName0": "vmbr0",
        "IsoStorageName": "local",
        "StorageName": "local-lvm",
        "Protection": false,
        "Onboot": true
    },
    "RunConfig": {
        "Accel": true,
        "InstanceName": "mytest",
        "ImageName": "mytest",
        "IPAddress": "172.21.1.75",
        "NetMask": "255.255.255.0",
        "Gateway": "172.21.1.1",
        "Verbose": true,
        "ShowErrors": true,
        "ShowWarnings": true
    }
}

Short example of configuration file:

{
    "Dirs": ["static"],
    "NameServers": ["172.21.1.1"],
    "CloudConfig": {
        "ImageName": "mytest"
    },
    "ProxmoxConfig": {
        "BridgeName": "vmbr0",
        "IsoStorageName": "local",
        "StorageName": "local-lvm"
    },
    "RunConfig": {
        "Accel": true,
        "InstanceName": "mytest",
        "ImageName": "mytest",
        "IPAddress": "172.21.1.75",
        "NetMask": "255.255.255.0",
        "Gateway": "172.21.1.1",
        "Verbose": true,
        "ShowErrors": true,
        "ShowWarnings": true
    }
}

francescolavra · 2022-09-04T12:16:48Z

types/config.go

@@ -146,6 +155,15 @@ type ProviderConfig struct {
 	// you can use to pass role information to an EC2 instance when the instance starts.
 	InstanceProfile string

+	// Machine specifies the type of machine (pc or q35) (Used only for ProxMox yet)
+	Machine string `cloud:"machine"`


The ProviderConfig struct already has a "Flavor" member, which is used to specify a machine type for other cloud providers (such as for example aws and gcp). It seems to me you could use that instead of adding a new field to the struct.

The ProviderConfig struct already has a "Flavor" member, which is used to specify a machine type for other cloud providers (such as for example aws and gcp). It seems to me you could use that instead of adding a new field to the struct.

Maybe for Proxmox, in general, create all fields in ProxmoxConfig struct, and not use CloudConfig at all?

"ImageName": "mytest", "Arch": "x86_64", "Machine": "q35", "Sockets": 2, "Cores": 4, "Numa": true "Memory": "4096M"

It just seems to me that if you add settings in the course of development, then you can add a bunch of additional settings to any providers. And in ProxmoxConfig, I have displayed far from everything that is configured there via Api.

So that I just do not interfere with adding fields in CloudConfig during development. That is, only the, ProxmoxConfig and RunConfig structures will be used.

For CloudConfig and other providers to be separately, ProxmoxConfig separately. Then there will be no restrictions on adding the required fields and you will not have to share the names of fields. Like this, and config be more simpler with two structs for end users:

{ "Dirs": ["static"], "NameServers": ["172.21.1.1"], "ProxmoxConfig": { "ImageName": "mytest", "Arch": "x86_64", "Machine": "q35", "Sockets": 2, "Cores": 4, "Numa": true, "Memory": "4096M", "BridgeName0": "vmbr0", "IsoStorageName": "local", "StorageName": "local-lvm", "Protection": false, "Onboot": true }, "RunConfig": { "Accel": true, "InstanceName": "mytest", "ImageName": "mytest", "IPAddress": "172.21.1.75", "NetMask": "255.255.255.0", "Gateway": "172.21.1.1", "Verbose": true, "ShowErrors": true, "ShowWarnings": true } }

…oxConfig struct

asyslinux

Latest commit revert changes in CloudConfig struct, and all new fields is created in ProxmoxConfig struct. This is necessary to simplify the config for end users of ProxMox, and not to interfere with extra fields in the CloudConfig structure.

eyberg · 2022-09-04T15:36:05Z

types/config.go

@@ -80,6 +80,9 @@ type Config struct {
 	// attach/detach.
 	ProgramPath string

+	// ProxmoxConfig configures various attributes about the ProxMox provider.
+	ProxmoxConfig ProxmoxConfig


So this is the right path, however, since we have 15 different infrastructure targets I don't think we want to include 15 separate ones into each top level config.

I was thinking this could be implemented via an interface so each target infrastructure can have its own cloud specific config but we only have one reference to it from here.

I'm afraid that a lot of code rework will be required in many places with the config, because everything is initially taken from c.Config, if I understand correctly. I'm not sure this is necessary, because provider selected from command line (-t arg) and if has correct error checking in providers, then user can not use wrong config. But this is my opinion, I just wanted to help get the ops working with proxmox.

the problem is the same as the original one - this is all config for a specific target platform that can be arbitrarily large and it doesn't make sense to include for other users who won't use those platforms, i can prob take a look at this in the nxt few days as I do see your need but I just wouldn't want someone else to come in and start adding in the same thing for gcp, aws, azure, etc.

we have the notion of a Provider interface here -> https://github.com/nanovms/ops/blob/master/lepton/provider.go#L21 - I think adding the config into this so each provider can have one would be a good way to go about implementing this

No problem. It's up to you, I can wait if you rewrite it the way you want it yourself. You will make what architecture you need yourself for processing configs. When you implement, just reject the pull request and take what is required from my repository, that I wrote for proxmox and even that you can change to your liking.

so as an example would this work for you? master...targetConfig - that way that config is re-usable by any target platform and can be arb. large, in the past when we've had a number of target specific vars (such as Azure) we've utilized env vars which would also be an option here

Yes, good solution. Big Thanks.

eyberg · 2022-09-07T22:06:18Z

chatted offline - closing in favor of #1373

Added some configurable options for ProxMox API via CloudConfig

add1377

eyberg reviewed Sep 4, 2022

View reviewed changes

Added ProxmoxConfig provider property, move additional import to lept…

9780fb3

…on/helpers.go

francescolavra reviewed Sep 4, 2022

View reviewed changes

CloudConfig struct is reverted back, all new fields is moved to Proxm…

25da801

…oxConfig struct

asyslinux commented Sep 4, 2022

View reviewed changes

eyberg reviewed Sep 4, 2022

View reviewed changes

eyberg mentioned this pull request Sep 7, 2022

Target cloud config #1373

Merged

eyberg closed this Sep 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added some configurable options for ProxMox API via CloudConfig #1370

Added some configurable options for ProxMox API via CloudConfig #1370

asyslinux commented Sep 3, 2022 •

edited

Loading

eyberg commented Sep 4, 2022

eyberg Sep 4, 2022

asyslinux Sep 4, 2022

eyberg Sep 4, 2022

asyslinux Sep 4, 2022 •

edited

Loading

eyberg Sep 4, 2022

asyslinux Sep 5, 2022

asyslinux commented Sep 4, 2022 •

edited

Loading

asyslinux commented Sep 4, 2022 •

edited

Loading

francescolavra Sep 4, 2022

asyslinux Sep 4, 2022 •

edited

Loading

asyslinux left a comment

eyberg Sep 4, 2022

asyslinux Sep 4, 2022

eyberg Sep 4, 2022

asyslinux Sep 4, 2022

eyberg Sep 6, 2022

asyslinux Sep 6, 2022

eyberg commented Sep 7, 2022

Added some configurable options for ProxMox API via CloudConfig #1370

Added some configurable options for ProxMox API via CloudConfig #1370

Conversation

asyslinux commented Sep 3, 2022 • edited Loading

eyberg commented Sep 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asyslinux Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asyslinux commented Sep 4, 2022 • edited Loading

asyslinux commented Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

asyslinux Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

asyslinux 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eyberg commented Sep 7, 2022

asyslinux commented Sep 3, 2022 •

edited

Loading

asyslinux Sep 4, 2022 •

edited

Loading

asyslinux commented Sep 4, 2022 •

edited

Loading

asyslinux commented Sep 4, 2022 •

edited

Loading

asyslinux Sep 4, 2022 •

edited

Loading