Project

General

Profile

Bug #23859

Ternary operation with vm_exists? in orchestration compute

Added by Tristan Robert about 1 month ago. Updated 2 days ago.

Status:
Closed
Priority:
Normal
Assignee:
-
Category:
Compute resources
Target version:
Difficulty:
Triaged:
No
Bugzilla link:
Team Backlog:
Fixed in Releases:
Found in Releases:

Description

In app/models/concerns/orchestration/compute.rb file there is this method:

def queue_compute
    return unless compute? && errors.empty?
    # Create a new VM if it doesn't already exist or update an existing vm
    vm_exists? ? queue_compute_create : queue_compute_update
end

When I read the ternary operator it seems to do the exact contrary of what it is explained in the comment.
And when I test it, indeed it does not create the VM in compute provider.

I think it should be:

def queue_compute
    return unless compute? && errors.empty?
    # Create a new VM if it doesn't already exist or update an existing vm
    !vm_exists? ? queue_compute_create : queue_compute_update
end

Then it works and it finally creates the new VM in compute provider.
I am surprised that nobody else found this before.
Maybe I misunderstood something?


Related issues

Related to Foreman - Feature #18064: Ability to import and provision existing VMsClosed2017-01-13

Associated revisions

Revision 2a6bce4d (diff)
Added by Tristan Robert about 1 month ago

Fixes #23859 - Queue orchestration compute

Revision f57d42f5 (diff)
Added by Tristan Robert about 1 month ago

Fixes #23859 - Fix vm_exists? method

Fix related tests

History

#1 Updated by Michael Moll about 1 month ago

  • Related to Feature #18064: Ability to import and provision existing VMs added

#2 Updated by Michael Moll about 1 month ago

Could you open a pull request whith this, please?

Personally I'd find this easier to follow the logic:

    vm_exists? ? queue_compute_update : queue_compute_create

#3 Updated by Timo Goebel about 1 month ago

We already have a fix for this as part of https://github.com/theforeman/foreman/pull/5569.

#4 Updated by Michael Moll about 1 month ago

I'm unsure about the actual impact of this bug.

If it's relevant, it would good to have it as a separate commit to cherry-pick it down to the supported releases.

#5 Updated by The Foreman Bot about 1 month ago

  • Status changed from New to Ready For Testing
  • Pull request https://github.com/theforeman/foreman/pull/5675 added

#6 Updated by Tristan Robert about 1 month ago

The PR refactors vm_exists? but my original issue still remains. I will make another issue (feature enhancement exactly) and a new PR.

#7 Updated by Michael Moll about 1 month ago

  • Legacy Backlogs Release (now unused) set to 353

#8 Updated by Tristan Robert about 1 month ago

  • % Done changed from 0 to 100
  • Status changed from Ready For Testing to Closed

#9 Updated by Ewoud Kohl van Wijngaarden 2 days ago

  • Triaged set to No
  • Category set to Compute resources

Also available in: Atom PDF