Introduce Request and Task for firmware update #18801
Conversation
|
Hello guys, this is basically a copy-paste of #18573 where we did physical server provisioning, only this time we do physical server firmware update. |
miha-plesko
force-pushed
the
firmware-update
branch
from
88f0eb0
to
b578d4f
Compare
miha-plesko
changed the title
miha-plesko
force-pushed
the
firmware-update
branch
2 times, most recently
from
8ee090d
to
023cfcd
Compare
miha-plesko
force-pushed
the
firmware-update
branch
2 times, most recently
from
be5db97
to
4261e76
Compare
spec/models/physical_server_firmware_update_task/state_machine_spec.rb
Outdated
Show resolved
Hide resolved
spec/models/physical_server_firmware_update_task/state_machine_spec.rb
Outdated
Show resolved
Hide resolved
miha-plesko
force-pushed
the
firmware-update
branch
from
4261e76
to
5bd59ad
Compare
miha-plesko
changed the title
|
The related PR has been merged yesterday, so I think we're good with this one as well. @gmcculloug any objections? |
|
@miha-plesko Can you add a link to the related PR to the description for future reference. Also, was this PR dependent on it? |
|
Added link to the description. No, this specific PR is not hard-related, but the actual internal state machine implementation (in Redfish provider, this PR here) depends on it. |
|
@gmcculloug going to assign this to you since it is much more your area of expertise |
| @@ -110,6 +110,10 @@ def console_url | |||
| raise MiqException::Error, _("Console not supported") | |||
| end | |||
|
|
|||
| def self.firmware_update_class | |||
| self::FirmwareUpdate | |||
There was a problem hiding this comment.
I only found the FirewareUpdate class defined in the Redfish PR https://github.com/ManageIQ/manageiq-providers-redfish/pull/69/files. I think we should have a FirmwareUpdate class at this intermediate class level and then the Provider class is sub-classed from this new one.
Something like:
class ManageIQ::Providers::PhysicalInfraManager::FirmwareUpdate < ::PhysicalServerFirmwareUpdateTask
# Firmware update is triggered in a single Redfish API call for a list of servers
# hence we don't need more than one task per firmware update request.
SINGLE_TASK = true
include_concern 'StateMachine'
endThen the RedFish FirmwareUpdate class is derived from this new intermediate class.
Also, since this is a task is there a reason you picked the naming FirmwareUpdate? The name sounds more like an inventoried item then a task that would run a state-machine.
There was a problem hiding this comment.
Hi @gmcculloug and sorry for delayed response due to my annual PTO. I've applied both your comments, the reason it was implemented different before is that I took VM provision request as a guideline where they
a) implement but don't ever extend the intermediate class (https://github.com/ManageIQ/manageiq/blob/master/app/models/manageiq/providers/cloud_manager/provision.rb#L1), see how providers then extend the Task directly anyway: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/provision.rb#L1
b) name the task as Provision and not as ProvisionTask https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/provision.rb#L1
But I'm with you on this one, because seeing 'Provision' or 'FirmwareUpdate' inside provider feels like inventory item.
There was a problem hiding this comment.
Thanks @miha-plesko. The MiqProvision class is a little special as it existed before we combined the request and tasks in sub-classed models. The name was kept for backward compatibility.
In newer models we use request/task as the model suffix.
miha-plesko
force-pushed
the
firmware-update
branch
from
5bd59ad
to
d278fc8
Compare
|
|
||
| def mark_as_completed | ||
| update_and_notify_parent(:state => 'finished', :message => msg('firmware update completed')) | ||
| MiqEvent.raise_evm_event(self, 'generic_task_finish', :message => "Done updating firmware on PhysicalServer(s)") |
There was a problem hiding this comment.
Is it required to raise an event here? Are you going to create policies based on this event?
There was a problem hiding this comment.
No requirement currently, no, I copy-pasted from provisioning example thinking it's needed for some reason. Let me remove it then
miha-plesko
force-pushed
the
firmware-update
branch
from
d278fc8
to
90854a9
Compare
We're introducing a new operation on physical server: firmware update. We want to use state machine-driven Request for it because it's a long running procedure and we want to be doing it on more than one server with a single click (and then track its progress). That being said - the PR contains a new Request class and a new Task class (with internal state machine). The actual state machine implementation is done in provider codebase - depending on class of the selected physical server. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>
miha-plesko
force-pushed
the
firmware-update
branch
from
90854a9
to
3fd59c8
Compare
|
Checked commit xlab-si@3fd59c8 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
We're introducing a new operation on physical server: firmware update. We want to use state machine-driven Request for it because it's a long running procedure and we want to be doing it on more than one server with a single click (and then track its progress).
That being said - the PR contains a new Request class and a new Task class (with internal state machine). The actual state machine implementation is done in provider codebase - depending on class of the selected physical server.
Related PR: #18774
@miq-bot add_label enhancement
@miq-bot assign @agrare
/cc @gmcculloug