Fixes #23859 - Fix queue orchestration compute

[tristanrobert]

Hello,

as you asked me for, here is the PR intended to fix the #23859 bug I have submitted to you.
This fix could help me to develop my foreman-proxmox plugin.
I am pleased to help you.

[theforeman-bot]

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

[theforeman-bot]

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

[theforeman-bot]

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

[theforeman-bot]

Issues: #23859

[timogoebel]

Thanks for this, please also incorporate the changes to vm_exists? (and the respective tests) from https://github.com/theforeman/foreman/pull/5569/files into this commit. Thanks.

cc: @leewaa

[theforeman-bot]

@tristanrobert, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

• check #23589: Service command operates with the local Postgres even when Satellite uses remote DB is the correct one
• move ticket #23589 from Foreman Maintain to the Foreman project
• or file a new ticket in the Foreman project

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).

---

This message was auto-generated by Foreman's prprocessor

[mmoll]

ok to test

[mmoll]

Untested, but looks good visually.

@tristanrobert @timogoebel What are the actual (user facing) consequences of this issue with the current code (which, AFACT is in effect since 1.15)?

[tristanrobert]

This PR could be seen as refactoring only. Using it could ease to understand the code, but it does not change actually its features.
So it's up to you to merge it or not.

My actual issue still remains.
Indeed, to know if a vm is to be created, foreman uses vm_exists? from app/models/concerns/orchestration/compute.rb which uses persisted? from app/models/concerns/fog_extensions/model.rb which uses !!identity from fog.
My issue is that proxmox requires vmid to be submitted by the client in order to create a new vm.
So if I use vmid as identity of compute object, my vm will always be persisted to foreman and it never could create a new vm in proxmox this way.
If I can't find a way to change identity in fog-proxmox gem, I may need a new implementation of the persisted? method.
I will tell you when I find a good solution.

[timogoebel]

Thanks, looks good. I concur this is a cosmetic change only. I believe it's important, though as it already caused a lot of confusion. Thanks.

[timogoebel]

My issue is that proxmox requires vmid to be submitted by the client in order to create a new vm.
So if I use vmid as identity of compute object, my vm will always be persisted to foreman and it never could create a new vm in proxmox this way.

@tristanrobert: From a quick glance you should be able to overwrite persisted? for proxmox either via a concern in your gem (that is foreman_proxmox, see https://github.com/theforeman/foreman-digitalocean/blob/d41cafbf406283535c43d3c00b203973402661fd/lib/foreman_digitalocean/engine.rb#L37-L38 for an example) or in fog directly. The concern you mentioned (app/models/concerns/fog_extensions/model.rb) should not be included by default. Are you sure this is the method foreman uses for proxmox?

But let's move this discussion somewhere else please, e.g. an issue in your gem. Ping me if you want, happy to help if I find the time.

[timogoebel]

@tristanrobert: Another sidenote: I'd love to see the inline cache invalidation in Foreman core so other compute resources can make use of this. :-)

[mmoll]

allright, let's bring this to develop without cherry-picking down then 👍

[mmoll]

merged, merci @tristanrobert!