Create Snapshot in Foreman GUI uses API endpoint which does not merge the params correctly

[laugmanuel]

I guess we have three issues here. Feel free to split this into separate Issues as needed.

1. The "Create snapshot" button triggers the API endpoint (defined in app/controllers/api/v2/snapshots_controller.rb) instead of the regular controller (app/controllers/foreman_snapshot_management/snapshots_controller.rb)
2. The parameter include_ram gets ignored because it's called includeRam in the request:

2021-06-23T16:59:58 [I|app|9e1afa82]   Parameters: {"includeRam"=>false, "snapshot"=>{"name"=>"test", "description"=>""}, "apiv"=>"v2", "host_id"=>"19923"}

3. The merge of additional parameters is broken in the API controller

• working: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/foreman_snapshot_management/snapshots_controller.rb#L31
• vs. broken: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/api/v2/snapshots_controller.rb#L52)

[m-bucher]

1. is actually a feature, because we want to avoid having two interfaces that do the same

2. & 3) right, I will look into it, thangs 😃

[laugmanuel]

@m-bucher do you have any updates on this?

[m-bucher]

Apologies for the delay @laugmanuel .
I did try to fix it in #68. However, it still did not work on my forklift.

But feel free to try it out.

[laugmanuel]

To me it looks like there is (another) problem deeper down the stack.
If I call the following for a given host in the rake console, the snapshot also contains the RAM:

host.compute_resource.send(:client).vm_take_snapshot('instance_uuid' => host.uuid, 'name' => 'foobar', 'description' => 'desc', 'memory' => false)

Note: I guess the parameter also needs to be part of the param_group: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/api/v2/snapshots_controller.rb#L40-L43
Also, the old param should be removed then: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/api/v2/snapshots_controller.rb#L48

[m-bucher]

I think I found the underlying problem in fog-vsphere.
Meaning this never worked 🤔

[laugmanuel]

I think it did. I do not have proof, but we should have noticed it when the RAM was included prior (as we did now).

Maybe this has to do with a vSphere update? The API Doc still mentions this parameter: https://vdc-repo.vmware.com/vmwb-repository/dcr-public/b525fb12-61bb-4ede-b9e3-c4a1f8171510/99ba073a-60e9-4933-8690-149860ce8754/doc/vim.VirtualMachine.html#createSnapshot

[m-bucher]

I have created a PR to fix this fog/fog-vsphere#270

[m-bucher]

Note: I guess the parameter also needs to be part of the param_group: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/api/v2/snapshots_controller.rb#L40-L43
Also, the old param should be removed then: https://github.com/ATIX-AG/foreman_snapshot_management/blob/master/app/controllers/api/v2/snapshots_controller.rb#L48

Now I remember, why we added it outside the param_group. I think this was due to it being allowed to be part of the edit-snapshot API.
Therefore, I would like to keep it the way we had it before. I am aware this is not that intuitive, but changing it now would also change the API and thus breaking existing code that might use this REST-API (e.g. foreman-ansible-modules).

[laugmanuel]

I'm fine with that. Then it should be enough to fix the merge of parameters here: https://github.com/ATIX-AG/foreman_snapshot_management/pull/68/files#diff-c85714c47ad02be889f2acadc0c7133fb725288bdc8158e249c05f0ee5c4f747L52

[m-bucher]

Fixed in v2.0.1