-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixes #38948 - Add power state bulk action to new host overview page #10795
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 #38948 - Add power state bulk action to new host overview page #10795
Conversation
4a1e36d to
10df08a
Compare
sbernhard
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Omar. Very cool addition.
|
It looks great @EnigmaXV ! |
|
@chris1984 is going to give this a quick test, stand by :) |
...ts/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js
Outdated
Show resolved
Hide resolved
|
Tested and working good as far as turning on and off the hosts. I did notice what @lfu mentioned as well with it being called twice. |
|
Thx @chris1984 and @lfu for your feedback I tried a few things to rule out unintended re-renders (memoizing inputs, guarding bumpRefresh with refs, removing the hosts list refetch to avoid extra renders, and checking Redux DevTools + Network to confirm there’s no polling or interval involved). From what I can tell, the behavior comes from the current design: each row renders a HostPowerStatus component that fetches its state on mount, and after a successful bulk power change we trigger a refresh so the icons update. That results in one request per host initially, and one additional request per host after the change. It’s also worth mentioning that the second request is returned as 304 Not Modified, so it’s served from cache and doesn’t re-hit the backend. Because of that, I don’t think this is a major performance issue right now. Happy to hear if I missed something or if there’s a preferred approach here. |
...ack/assets/javascripts/react_app/components/HostsIndex/Columns/components/HostPowerStatus.js
Outdated
Show resolved
Hide resolved
...ts/javascripts/react_app/components/HostsIndex/BulkActions/powerState/BulkPowerStateModal.js
Outdated
Show resolved
Hide resolved
|
Thanks @EnigmaXV for taking time looking into the issue.
That is the behavior I would expect. But I noticed multiple 'power' requests per host after power state change. If no experts with |
10df08a to
47d1648
Compare
|
@chris1984 / @jeremylenz Should we then finish / merge this PR after the explanation from @EnigmaXV ? |
|
We are about to enter Foreman 3.18 stabilization: https://community.theforeman.org/t/foreman-3-18-branching-process/45265 Not making any judgement if it should or should not be merged but reminding you the merge window for features closes at the end of the month. The sooner a decision is made, the better |
|
I'm going to see if anyone can show me these multiple requests so I can take a look, but I really would like to merge this before stabilization. If it works otherwise we can improve it later. |
|
Note we have https://github.com/theforeman/smart-proxy/blob/develop/docs/bmc.md which can help you to set up the power management features without the physical hardware. |
jeremylenz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do see 2 requests per host to GET api/v2/hosts/:id/power. (They all return 200 OK.) It should be at most 1 request. However, since this PR is well tested and works as expected otherwise, I am going to open a followup issue for that. https://projects.theforeman.org/issues/39009

Uh oh!
There was an error while loading. Please reload this page.