Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #22546 - Return power action status for ovirt #5369

Closed
wants to merge 1 commit into from

Conversation

orrabin
Copy link
Member

@orrabin orrabin commented Mar 28, 2018

The right way is fixing this in fog-ovirt but until: fog/fog-ovirt#11 is merged and a new gem version released I added a temporary fix.

I can close this and open PRs for 1.17 and 1.16 instead since we will likely have a fixed fog-ovirt version for 1.18, or merge this and cherry-pick to both and I'll revert develop when introducing the new gem version. Leaving the decision up to reviewers.
There are also at least 2 other workarounds I can think of instead of this but this was probably the easiest to later revert and the bug is ovirt specific so it makes sense not to change the general behavior.

@iNecas
Copy link
Member

iNecas commented Mar 28, 2018

I'm for fixing this in develop by updating the dependency and doing this path only in 1.16/17

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests are green, we are good to go, please open 1.16/1.17 PRs.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lemme explain, PowerManager::Base calls this after the method call in method_missing:

result = send(action_entry[:output], result) if action_entry[:output]

So if there is output entry present, it will actually call this with result. Here is the oVirt mapping:

    def action_map
      super.deep_merge({
                         :on       => 'start',
                         :off      => 'stop',
                         :soft     => 'reboot',
                         :cycle    => 'reset',
                         :status   => {:action => :virt_state, :output => :state_output},
                         :state    => {:action => :virt_state, :output => :state_output}
                       })
    end

So for start/stop the result will not be filtered through the method, but for status/state it will. And here is the problem, you return nil but this is the method which will be called to format the result:

    def state_output(result)
      result = result.to_s
      return 'on' if result =~ /started/i
      return 'off' if result =~ /paused/i
      translate_status(result) # unknown output
    end

This will lead to "can't call to_s on nil" Ruby error if I am not mistaken. So I think the patch is not safe, you should return some dummy string. Options:

  • empty string - should work for all actions
  • original string with password filtered out - this is I think better option - it will not cripple output

if action_status.is_a?(Fog::Compute::Ovirt::Server)
return true
end
action_status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I take my approval back. I haven't tested, but after re-reading the code base, I see that there is a possibility for nil exception.

Copy link
Member

@lzap lzap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disregard, ENOCOFFEE

You return true not nil, that will indeed work because true.to_s is "true" :-)

@mmoll
Copy link
Contributor

mmoll commented Mar 29, 2018

As this got merged into the 1.1x-stable branches, this PR should get closed, I presume?

@orrabin
Copy link
Member Author

orrabin commented Mar 31, 2018

Closing in favor of: #5383
Stable branched need the workaround, develop can get the fixed version on fog-ovirt that was released today.

@orrabin orrabin closed this Mar 31, 2018
@orrabin
Copy link
Member Author

orrabin commented Mar 31, 2018

@mmoll I'll reopen if the dependency bump is not approved. Thanks for fixing the stable branches!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants