added taking over incomplete volume parameters

[libc225so]

Hi,
I found out that when a new VM is to be created with a request for disks that are not in the ComputeProfile but simply added for example like this ( through hammer ) with: --volume='size=10GB' --volume='size=20GB' the disks do not show up properly in the json.

They show up like this:
"volumes_attributes"=>{
"0"=>{"storage_type"=>"hard_disk", "storage"=>"local-lvm", "controller"=>"virtio", "device"=>"0", "cache"=>"none", "size"=>"19GB", "id"=>"virtio0"},
"1"=>{"size"=>"10GB"},
"2"=>{"size"=>"20GB"}},

As an result disk 1 && 2 are disregarded and only the disk that has been mentioned in the ComputeProfile will get the size requested.

This patch adjusts the json to "copy" the settings from the first disk, into the next ones.

I Hope, you can consider this patch.
Thank you and regards,
Kees

[libc225so]

Hi,
I tried to work with the comments from the pipeline, but I guess I need a little help here.
The only once that made sense to me were: "Space missing after comma" also it shows no line number so not too clear for me how to solve this.

[Manisha15]

Hi, I tried to work with the comments from the pipeline, but I guess I need a little help here. The only once that made sense to me were: "Space missing after comma" also it shows no line number so not too clear for me how to solve this.

You can check here which lines are affected: https://github.com/theforeman/foreman_fog_proxmox/pull/359/files

[Manisha15]

it should be if index > 0 && dev_specs.keys.count == 1

[Manisha15]

No need to add return keyword as it is implicit.

[libc225so]

Nice, I didn't know that .. This is one of my firsts ruby spinsels ;-)

[libc225so]

Hi Manisha15, can this be merged into main now ?
Or what is the strategy ?
Thanks !

[Manisha15]

Hi Manisha15, can this be merged into main now ? Or what is the strategy ? Thanks !

I am testing and reviewing it. Then I'll merge it. Can you please squash the commits in one commit and create an issue for it along with appending the commit message with fix: and adding issue no. You can refer to merged PRs for commit message.

[libc225so]

Hi
As requested, I opened a new issue with title: "Taking over incomplete volume parameters" and squashed all commits into 1 with comment: "fix: Taking over incomplete volume parameters"
I hope this is OK

[Manisha15]

@libc225so There is no specific hammer cli command for proxmox. May I know the commands you used that gave the issue redarding volume attributes?

[libc225so]

Hi Manisha15
Hammer is used in Foreman, so when instead of using the web interface and you want to create a new VM, you can issue something like this:
hammer host create --name blah ....

The options for the disk ( configured with --volume ), follow what is configured in the compute profile, but allows posibilities to address for different capacity.

If your compute profile contains 1 disk, then you will get 1 disk.

But there is an option ( simular as to use with VMWare ) to add extra disks, i.e. repeat the "--volume" parameter like this:

--volume="'size=19GB'"
--volume="'size=10GB'"
--volume="'size=20GB'"

So in this example, the 19GB refers to the disk from the compute profile
And the 10 && 20 are extra disks you want which have NO definition in the compute profile

But the parameters from hammer going to Foreman do not allow you to add any other information then just the size ..
The rest of the information comes from the compute profile

So the complete json that in the end is send to fog is this json string for the first disk.

"0"=>{"storage_type"=>"hard_disk", "storage"=>"local-lvm", "controller"=>"virtio", "device"=>"0", "cache"=>"none", "size"=>"19GB", "id"=>"virtio0"},

As said, the parameters for volume do not provide possibilities to add for instance the type / and or storage etc ...

So the only logical thing that happends is, that the json for disk 2 && 3 coming from Foreman to fog only contains the size parameter ( "1"=>{"size"=>"10GB"}, "2"=>{"size"=>"20GB"} )

But when this information is handed to fog, this is insufficient to create the disk in proxmox, because it also needs to know where and how to store that disk.

Normally if you create a second disk, you keep the disks in 1 place, so it makes sense to copy over the information provided in the 1st disk configuration to the requested number 2 and 3.

And that is exactly what the patch does, it fills in the missing parts in the json.

On the other hand, if the information is correct, i.e. you have 2 disks configured in your compute profile, then these will come in simular like that 1st one from above .. and the patch will not touch it, because it only works on the combination for "if index > 0 && dev_specs.keys.count == 1" ..

Hope this helps
Regards,
Kees

[Manisha15]

Hello @libc225so , thanks for the detailed explanation. I ran the command and was able to configure the desired disks by providing comma separated values like in other compute resources. For example,
hammer host create --name="proxmox-test-host" --hostgroup=proxmox_almalinux8 --volume="size=5G,id=ide1" --location="Munich" --organization="Example"
Thus, I don't understand the need to have this feature where to have to provide it additionally. The important thing to notice here is you need to provide the id for the volume and it will take the controller type from there.

[libc225so]

Hi Manisha,
Did you try adding more then 1 disk ?
If you create a second disk, i.e. add another time "--volume" in hammer .. then it fails to create this because the json is then incomplete.

Secondly, I will try to see if the second disk picks up with your suggestion to add the id for the controller, I have not tried that yet ..

[Manisha15]

Hey @libc225so , yeah I created with two volumes but for second one you also have to specify the storage type as it cloud be anything from cd to cloudinit and storage. The first volume works without specifying the storage_type because by default we add one storage of type hard_disk to vm so it re-writes its config.

[libc225so]

Ok, I figured out what you mean, let me resume it.

If you have your compute_profile configured in Foreman with 1 disk like this:

Storage: local-lvm
Controller: virtio
Device: [greyedout_counted_device_number]
Cache: none
Size(GB): 21

Then this translates in fog on the incoming request in json like this:

"volumes_attributes"=>{
"0"=>{
"storage_type"=>"hard_disk",
"storage"=>"local-lvm",
"controller"=>"virtio",
"device"=>"0",
"cache"=>"none",
"size"=>"21GB",
"id"=>"virtio0"
}
}

If you don't change anything to this and this is actually what is required then creating a host with hammer will deliver this disk according to this compute_profile without the need to specify any "--volume" setting.

If you want to override the size, you only need to specify this with the "--volume" parameter. So this becomes an option like this:

--volume="'size=50GB'"

Which will deliver the same json, but with the updated size.

However if you want to have a secondary disk, and you fully specify all the arguments this works indeed. But there is some logic required to get the id && device enumerated. An example of adding a second disk works indeed like this:

--volume="'size=50GB'"
--volume="'storage_type=hard_disk,storage=local-lvm,controller=virtio,device=1,size=19GB,id=virtio1'" \

In this second line you have to enumerate the device && id properly. Secondly the additional arguments needed are hardly documented ( I could not find it easy, maybe I was not looking in the right place, could be ) and it is quite some details to get it right.

So I would say, yes this is a "feature" (instead of a fix) which makes life easier if you look at this from the hammer cli point of view, because then it would only require this:

--volume="'size=50GB'"
--volume="'size=19GB'" \

What do you think ?

[Manisha15]

Ok, I figured out what you mean, let me resume it.

If you have your compute_profile configured in Foreman with 1 disk like this:

Storage: local-lvm Controller: virtio Device: [greyedout_counted_device_number] Cache: none Size(GB): 21

Then this translates in fog on the incoming request in json like this:

"volumes_attributes"=>{ "0"=>{ "storage_type"=>"hard_disk", "storage"=>"local-lvm", "controller"=>"virtio", "device"=>"0", "cache"=>"none", "size"=>"21GB", "id"=>"virtio0" } }

If you don't change anything to this and this is actually what is required then creating a host with hammer will deliver this disk according to this compute_profile without the need to specify any "--volume" setting.

If you want to override the size, you only need to specify this with the "--volume" parameter. So this becomes an option like this:

--volume="'size=50GB'"

Which will deliver the same json, but with the updated size.

However if you want to have a secondary disk, and you fully specify all the arguments this works indeed. But there is some logic required to get the id && device enumerated. An example of adding a second disk works indeed like this:

--volume="'size=50GB'" --volume="'storage_type=hard_disk,storage=local-lvm,controller=virtio,device=1,size=19GB,id=virtio1'" \

In this second line you have to enumerate the device && id properly. Secondly the additional arguments needed are hardly documented ( I could not find it easy, maybe I was not looking in the right place, could be ) and it is quite some details to get it right.

So I would say, yes this is a "feature" (instead of a fix) which makes life easier if you look at this from the hammer cli point of view, because then it would only require this:

--volume="'size=50GB'" --volume="'size=19GB'" \

What do you think ?

I understand you want to ease the process of adding a new volume but in api/ hammer-cli providing the id is required, and the device attribute is optional. With your code, it would automatically add the volume to the first controller and storage available which is blind-sided for the user. I agree its not well documented anywhere which fields are required. So the best place to have this feature is in hammer-cli-foreman where we can specify which fields are required in order to add a volume.