Fixes #3073 - Fix mismatched and missing Reboot/Reset power actions #10561
Conversation
|
@lzap san The content of the closed PR #9082 is addressed in the new PR #10561. All three PRs (one in Foreman and two in smart-proxy) are ready for review. |
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
I'm thinking about compatibility here. Leaving this on the PR here, but it will apply to the Smart Proxy too.
We support running Foreman with older Smart Proxies so I'd like to detect if the proxy actually supports these (in other words, runs with or without your bug fixes). There's a mechanism with capabilities and in https://theforeman.org/2019/04/smart-proxy-capabilities-explained.html I've tried to describe how it works.
On the Smart Proxy side it will be easy: simply add a capability statement to signal it supports these. On the Foreman side you can use proxy.has_capability?(:BMC, :my_capability) to detect it.
Do you think you could take a look at this?
|
I will leave the review on my colleagues. Thank you so much for your contribution! |
|
@ekohl san Thank you for your comment. 1. Support policy of version mixingAccording to Foreman's release policy, if latest Foreman release is Then, following combination of Foreman core & smart proxy is also supported, is this correct?
But this isn't supported, right?
2. Should capability continue to remain in code, even after all supported version of Foreman core&proxy apply our modification?We'll follow your advice and will add capability (e.g., If our modification is included at Foreman version 3. What is expected behavior regarding compatibility?At current Foreman (before our PR is merged), pushing Regarding compatibility, expected behavior is following, right? If our modification is merged at
Thank you. |
That is correct: we only support Foreman >= Smart Proxy.
We have a downstream called Red Hat Satellite that has a wider support cycle. There can be 4 versions difference. That's why it's probably best to keep it for some time. I'd consider it sufficient to leave a code comment in Foreman that's something like Smart Proxy x.y introduced capability C so in the future we don't need to dig too deep.
That sounds good. I'd also accept a clear error message that the requested operation is not supported. I'll admit I'm not too familiar with the exact BMC implementation(s) to say what's best. |
|
Thank you for your response. |
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
from
0ce97e8 to
230bb3b
Compare
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
from
230bb3b to
99b5afb
Compare
spesnova717
changed the title
spesnova717
changed the title
|
Thank you for the feedback.
This ensures proper behavior with both updated and legacy proxies, maintaining backward compatibility while enabling the new functionality where supported. The CI error originates from the Thank you for your support. |
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
4 times, most recently
from
1835536 to
c0179de
Compare
|
This is a gentle ping for review on the following PRs: These address mismatched and missing Reboot / Reset BMC power actions. Although the CI currently shows ❌, the failure appears to be unrelated to this PR. If someone reviews it, it would be much appreciated. If there’s anything blocking the review or merge process, I look forward to hearing from you. |
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
2 times, most recently
from
4a44ff3 to
3136dfb
Compare
|
Hi, I am currently looking into this topic together with @stejskalleos and must admit I am a bit lost on vocabulary, so hoped you can help me a bit here. Looking at our current BMC API in the Smart Proxy, we support two "reset" options: What I am lost is what these words supposed to mean. Skimming the IPMI 2.0 spec, especially chapter "Chassis Control Command", I see that there is an command of "power cycle" and one for "hard reset", with the former being marked as optional. Both are exposed in our IPMI implementation on the Smart Proxy. Now, looking at Redfish Resource and Schema Guide, especially ResetType property, it can have We also have "Shell" and "SSH" pseudo-BMCs, that only expose Do we agree on the meaning of "Reboot" (soft) and "Reset" (hard)?
And based on that, we could define the new proxy capability as Edit: after talking to friends from OpenStack - we could do a "soft off, on" sequence for the soft reboot on IPMI? |
|
Hello, @evgeni and @stejskalleos. Thanks for review this PR. (Edit: Sorry, I could not afford to write down root cause/detailed explanation in text format at the time when I post this comment. So I added/modified following) --- Addition/modification Start ---------
So I created following table with hope that it explains the situation well. In brief, this is not only about vocabulary, it is about vocabulary & wrong implementation of Foreman core/Proxy, I think. So we named capability as
I agree with you about what "Reboot" and "Reset" means. About Reboot at IPMI Provider "soft off, on" sequence is good approach to Reboot on IPMI, but it is bit complex & expensive operation:
If we decide to implement Reboot functionality against IPMI, it should be good to make another PR because above requirements for Reboot on IPMI is bit complex. --- Addition/modification End ---------
|
app/services/power_manager/bmc.rb
Outdated
|
|
||
| attr_reader :proxy | ||
|
|
||
| def has_poweraction_fix_3_17? |
There was a problem hiding this comment.
I don't know if mentioning the 3_17 version in the code is a good practice.
What if we named it power_action_v2 and put deprecation methods in the original power_action?
That way, we can in follow-up releases remove the deprecated method and keep only one.
There was a problem hiding this comment.
This matches the new capability defined in the smart-proxy PRs, but I've been thinking to suggest two destinct capabilities reboot and reset that then can be used if available (falling back to cycle for setups not having the capability yet)?
There was a problem hiding this comment.
Thanks for comment.
Naming of capability is bit kind of preference. But whether to create 1 capability or 2 capabilities is important point.
If we create 2 separate capabilities, determination logic at [1] becomes bit complex, I think. So we made 1 capability because we expect all PRs[2][3][4][5] here will be merged at one Foreman release version, and not expecting cherry-picking part of these PRs at downstream.
[1] https://github.com/theforeman/foreman/pull/10561/files#diff-ff81ee231c9e9f24cd1650ca805a5fc1d6991ab0598234c6ea05764099420e3fR30-R31
[2] #10561
[3] theforeman/smart-proxy#916
[4] theforeman/smart-proxy#917
[5] theforeman/smart-proxy#919
There was a problem hiding this comment.
We had a discussion with colleagues about putting the Foreman version into the function names, and the general consensus was that it's not a good practice.
Please rename it to something else. My suggestion is power_action_v2
|
Thanks @vanou! Yeah, I agree, "soft off, on" for a soft reboot on IPMI is quite some work to get right, so I'd skip that for now too. And huge thanks for the overview table and links, this helps a lot in understanding the facets of this issue. One thing I noticed, |
Thanks you.
I'm glad to help understanding this complex issue.
Thanks. Actually, at theforeman/smart-proxy#916, we define |
|
Thanks @stejskalleos for review. I'll reply to your comment at Sep. 16 because Sep. 15 is national holiday. |
|
Found another place where this is mixed up: The way the API doc is written implies |
|
And another one: (those constants are currently unused tho) |
@evgeni |
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
from
3136dfb to
e9344c1
Compare
|
Thank you for your review. |
app/services/power_manager/bmc.rb
Outdated
|
|
||
| attr_reader :proxy | ||
|
|
||
| def has_poweraction_fix_3_17? |
There was a problem hiding this comment.
We had a discussion with colleagues about putting the Foreman version into the function names, and the general consensus was that it's not a good practice.
Please rename it to something else. My suggestion is power_action_v2
app/services/power_manager/bmc.rb
Outdated
| nic = host.bmc_nic | ||
|
|
||
| case nic&.subnet&.bmc&.has_capability?(:BMC, :poweraction_fix_3_17) | ||
| in nil | ||
| Foreman::Logging.logger('app').error( | ||
| "Failed to determine capability of BMC proxy. Cancel power operation against #{host.name} because associated BMC NIC, Subnet or BMC Proxy is not found." | ||
| ) | ||
| raise ::Foreman::BMCFeatureException.new( | ||
| N_("BMC NIC associated to %{host}, or Subnet/BMC Proxy associated to the BMC NIC not found.") % | ||
| { host: host.name } | ||
| ) | ||
| in true | ||
| true | ||
| in false | ||
| false | ||
| end | ||
| rescue ActiveRecord::ActiveRecordError => e | ||
| Foreman::Logging.exception( | ||
| "Failed to determine capability of BMC proxy. Cancel power operation against #{host.name} due to unexpected ActiveRecord behavior.", | ||
| e, | ||
| level: :error | ||
| ) | ||
| raise |
There was a problem hiding this comment.
Although a defensive approach is a good way to go, I don't think we need the nil check here.
Calling host.power.action:
- If the host doesn't have an associated
power, the error will be catchedERF42-9958 [Foreman::Exception]: Unknown power management support - can't continue - If the subnet doesn't have an BMC proxy, the error is
Foreman::BMCFeatureException: ERF88-9474 [Foreman::BMCFeatureException]: There is no proxy with BMC feature set up. Associate a BMC feature with a subnet.
| nic = host.bmc_nic | |
| case nic&.subnet&.bmc&.has_capability?(:BMC, :poweraction_fix_3_17) | |
| in nil | |
| Foreman::Logging.logger('app').error( | |
| "Failed to determine capability of BMC proxy. Cancel power operation against #{host.name} because associated BMC NIC, Subnet or BMC Proxy is not found." | |
| ) | |
| raise ::Foreman::BMCFeatureException.new( | |
| N_("BMC NIC associated to %{host}, or Subnet/BMC Proxy associated to the BMC NIC not found.") % | |
| { host: host.name } | |
| ) | |
| in true | |
| true | |
| in false | |
| false | |
| end | |
| rescue ActiveRecord::ActiveRecordError => e | |
| Foreman::Logging.exception( | |
| "Failed to determine capability of BMC proxy. Cancel power operation against #{host.name} due to unexpected ActiveRecord behavior.", | |
| e, | |
| level: :error | |
| ) | |
| raise | |
| smart_proxy = host.smart_proxies.with_features(:BMC).first | |
| return false unless smart_proxy | |
| return smart_proxy.has_capability?(:BMC, :poweraction_fix_3_17) |
What do you think?
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
from
e9344c1 to
2299ac5
Compare
|
@stejskalleos I'd appreciate your review. Thank you. |
stejskalleos
left a comment
stejskalleos
left a comment
There was a problem hiding this comment.
There are a few missing rename changes, and the smart proxy PRs also need to be updated; otherwise, the PR is ready for merge.
test/unit/power_manager_test.rb
Outdated
| assert host.power.reset | ||
| end | ||
|
|
||
| test "should fallback to soft and cycle when poweraction_fix_3_17 is not supported" do |
There was a problem hiding this comment.
| test "should fallback to soft and cycle when poweraction_fix_3_17 is not supported" do | |
| test "should fallback to soft and cycle when power_action_v2 is not supported" do |
app/services/power_manager/bmc.rb
Outdated
| smart_proxy = host.smart_proxies.with_features(:BMC).first | ||
|
|
||
| return false unless smart_proxy | ||
| smart_proxy.has_capability?(:BMC, :poweraction_fix_3_17) |
There was a problem hiding this comment.
| smart_proxy.has_capability?(:BMC, :poweraction_fix_3_17) | |
| smart_proxy.has_capability?(:BMC, :power_action_v2) |
We should rename the method in the smart proxy as well (theforeman/smart-proxy#916 & theforeman/smart-proxy#917)
test/unit/power_manager_test.rb
Outdated
| actions.uniq | ||
| end | ||
|
|
||
| test "should call reboot and reset directly when poweraction_fix_3_17 is supported" do |
There was a problem hiding this comment.
| test "should call reboot and reset directly when poweraction_fix_3_17 is supported" do | |
| test "should call reboot and reset directly when power_action_v2 is supported" do |
… actions Bug theforeman#3073 is caused by 3 things: 1. Foreman core doesn't call BMC proxy's API correctly - Changing power state to 'Reboot' via Web GUI calls BMC proxy's 'soft' power action - Changing power state to 'Reset' via Web GUI calls BMC proxy's 'cycle' power action 2. Foreman BMC proxy doesn't support reboot at all BMC provider 3. Foreman BMC proxy doesn't support reset(powerreset) at Redfish provider This PR fixes first item. For backward compatibility, power_action_v2 capability will be introduced to smart proxy at PRs related to #38498. And this commit introduces logic to determine if smart proxy supports that capability. Co-Authored-By: Vanou Ishii <ishii.vanou@fujitsu.com>
spesnova717
force-pushed
the
fix_3073_bmc_reboot_reset_patch1
branch
from
2299ac5 to
6a96843
Compare
|
@stejskalleos Huge thanks for kind review and comment! |
| attr_reader :proxy | ||
|
|
||
| def power_action_v2 | ||
| smart_proxy = host.smart_proxies.with_features(:BMC).first |
There was a problem hiding this comment.
Found a bug #10697, the smart_proxies method does not include Proxies that are assigned as BMC only.
stejskalleos
left a comment
stejskalleos
left a comment
There was a problem hiding this comment.
🍏 LGTM
Tested two scenarios:
[2025-09-23 13:35:00,183] INFO in main: System "202ca45a-09eb-4906-a0ef-926697668b44" power state set to "ForceRestart"
[2025-09-23 13:30:19,953] INFO in main: System "202ca45a-09eb-4906-a0ef-926697668b44" power state set to "GracefulRestart"
Both scenarios meet expectations and work well. I found one issue during testing, #10697, which was not caused by these changes.
🍏 Works as expected
🍏 Code LGTM
🍏 Tests
🍏 CI is green
|
Thanks @spesnova717 @vanou @evgeni |
6 participants
Bug #3073 is caused by 3 things:
This PR fixes first item.
Modifications in this PR
(1) Removal of Incorrect Power Action Mappings
In
bmc.rb, the:rebootand:resetactions were incorrectly mapped tosoftandcycle, respectively.These incorrect mappings are removed to eliminate the discrepancy between the Web UI representation and the actual behavior.
(2) Implement Reboot and Reset Actions
In the
powermethod ofproxy_api/bmc.rb, therebootandresetactions were not previously implemented.These actions are implemented, enabling Foreman Core to send
rebootandresetpower control requests to the Smart Proxy.