Physical server provisioning with internal state machine #18573

miha-plesko · 2019-03-19T14:48:32Z

We implement PhysicalServerProvisionRequest with one or more tasks - one per server to be provisioned. Each task is provider-specific (depending on PhysicalServer instance type). Tasks are implemented with internal state machine; each provider implements its own steps for provisioning like this:

:run_provision
:start_provisioning
     #
...  # provider-specific steps here
     #
:done_provisioning
:mark_as_completed
:finish

Please note that internal state machine gets triggered by external one to allow for more customization.

Related to
ManageIQ/manageiq-automation_engine#306
ManageIQ/manageiq-content#516

@miq-bot add_label enhancement
@miq-bot assign @agrare

app/models/physical_server.rb

gmcculloug · 2019-03-28T23:41:10Z

app/models/physical_server_provision_request.rb

@@ -11,7 +11,7 @@ def src_configured_systems
  end

  def self.request_task_class_from(_attribs)
-    ManageIQ::Providers::Lenovo::PhysicalInfraManager::ProvisionTask
+    PhysicalServerProvisionTask


Does this method need to be overridden in sub-classes?

Okay so as discussed on a call a week ago we're now allowing Task override from within provider repo (see L19 below). Overriden Task should inherit from this PhysicalServerProvisionTask.

Here in Request, however, we cannot allow override because there is no way we can figure what provider does the Request belong to. But this is not a problem, because self.request_task_class is only used by Request to render Task description prefix, so IMO it's safe to leave it like this.

app/models/physical_server_provision_task.rb

miha-plesko · 2019-04-19T09:15:13Z

Retrying add_label and assign:

@miq-bot add_label enhancement
@miq-bot assign @agrare

agrare

I'm good with this from a providers POV, I'll leave it up to @gmcculloug to ack the provision request changes

miha-plesko · 2019-04-23T14:23:35Z

app/models/physical_server.rb

@@ -33,6 +33,7 @@ class PhysicalServer < ApplicationRecord

  virtual_column :v_availability, :type => :string, :uses => :host
  virtual_column :v_host_os, :type => :string, :uses => :host
+  virtual_delegate :ems_type, :to => "ext_management_system", :allow_nil => true


@gmcculloug here's the virtual attribute definition that the external state machine depends on (for routing via messages).

As stated above, I think this should be emstype.

miha-plesko · 2019-04-23T14:33:33Z

app/models/physical_server_provision_task.rb

+  end
+
+  def self.base_model
+    PhysicalServerProvisionTask


This is the change @lfu @gmcculloug that introduces standalone task name instead shared one (was miq_provision_task, is physical_server_provision_task now). Not sure why I didn't do it in first place, sorry.

gmcculloug

Need to look into the emstype vs ems_type comments, otherwise this is looking good.

gmcculloug · 2019-04-26T21:58:39Z

app/models/physical_server_provision_request.rb

+    source = PhysicalServer.find_by(:id => source_id)
+    raise MiqException::MiqProvisionError, "Unable to find source PhysicalServer with id [#{source_id}]" if source.nil?
+    raise MiqException::MiqProvisionError, "Source PhysicalServer with id [#{source_id}] has no EMS, unable to provision" if source.ext_management_system.nil?
+    source


Not needed, but when I see the pattern

var = set_var raise '' raise '' return var

I want to use tap

PhysicalServer.find_by(:id => source_id).tap do |source| raise MiqException::MiqProvisionError, "Unable to find source PhysicalServer with id [#{source_id}]" if source.nil? raise MiqException::MiqProvisionError, "Source PhysicalServer with id [#{source_id}] has no EMS, unable to provision" if source.ext_management_system.nil? end

Thanks, @lfu demonstrated something similar on the content PR and I like it, not sure why I did it oldschool here again.

gmcculloug · 2019-04-26T22:14:03Z

app/models/manageiq/providers/physical_infra_manager.rb

@@ -34,6 +34,7 @@ class << model_name
    def self.ems_type
      @ems_type ||= "physical_infra_manager".freeze
    end
+    delegate :ems_type, :to => :class


I don't think you need this if your virtual_delegate uses emstype instead of ems_type.

With a fake provider added to the ext_manage_systems table you see:

irb(main):037:0> p => #<ManageIQ::Providers::Redfish::PhysicalInfraManager id: 50, name: "RedFish", created_on: "2019-04-26 22:08:06", updated_on: "2019-04-26 22:08:06", guid: "ffec646a-3145-4e63-8bbf-546c4525b344", zone_id: nil, type: "ManageIQ::Providers::Redfish::PhysicalInfraManager", api_version: nil, uid_ems: nil, host_default_vnc_port_start: nil, host_default_vnc_port_end: nil, provider_region: nil, last_refresh_error: nil, last_refresh_date: nil, provider_id: nil, realm: nil, tenant_id: 1, project: nil, parent_ems_id: nil, subscription: nil, last_metrics_error: nil, last_metrics_update_date: nil, last_metrics_success_date: nil, tenant_mapping_enabled: nil, enabled: true, options: nil, zone_before_pause_id: nil, last_inventory_date: nil> irb(main):038:0> p.emstype => "redfish_ph_infra" irb(main):039:0> p.class.ems_type => "redfish_ph_infra"

If this works you will also need to adjust the automate engine and content PRs.

Nice, works like a charm, updated, thanks 👍

gmcculloug · 2019-04-26T22:14:34Z

app/models/physical_server.rb

@@ -33,6 +33,7 @@ class PhysicalServer < ApplicationRecord

  virtual_column :v_availability, :type => :string, :uses => :host
  virtual_column :v_host_os, :type => :string, :uses => :host
+  virtual_delegate :ems_type, :to => "ext_management_system", :allow_nil => true


As stated above, I think this should be emstype.

We implement PhysicalServerProvisionRequest with one or more tasks - one per server to be provisioned. Each task is provider-specific (depending on PhysicalServer instance type). Tasks are implemented with internal state machine; each provider implements its own steps for provisioning like this: ``` :run_provision :start_provisioning # ... # provider-specific steps here # :done_provisioning :mark_as_completed :finish ``` Please note that internal state machine gets triggered by external one to allow for more customization. With this commit we also add virtual attribute `ems_type` to PhysicalServer model because external state machine needs it for routing. Signed-off-by: Miha Pleško <miha.plesko@xlab.si>

miq-bot · 2019-04-29T12:40:53Z

Checked commit xlab-si@f25de09 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
9 files checked, 3 offenses detected

spec/factories/miq_request.rb

❗ - Line 18, Col 48 - Layout/ExtraSpacing - Unnecessary spacing detected.

spec/models/physical_server_provision_task/state_machine_spec.rb

❗ - Line 40, Col 24 - Rails/SkipsModelValidations - Avoid using update_attribute because it skips validations.
❗ - Line 48, Col 24 - Rails/SkipsModelValidations - Avoid using update_attribute because it skips validations.

agrare

Can you address that one thing in a follow-up @miha-plesko ?

agrare · 2019-04-29T18:35:53Z

app/models/physical_server_provision_task/state_machine.rb

+  end
+
+  def start_provisioning
+    update_and_notify_parent(:message => msg('start provisioning'))


@miha-plesko why go through the trouble of updating the parent message if you're just going to raise immediately after? The way this is now you can't even do super in the child class to get the benefit of sharing the update_and_notify_parent calls across subclasses because it would just raise.

I'm going to merge as is because this case shouldn't ever happen

@agrare my thinking was to improve logs for unimplemented provider (e.g. Lenovo): first log says "start provisioning" then it explodes "NotImplemented" which kind of reads nicely as "start provisioning is not implemented".

However, I see your point - the piece of code is unusable as soon as one performs override from the provider. Followup here #18706, thanks for merging.

Physical server provisioning with internal state machine

miha-plesko mentioned this pull request Mar 19, 2019

Physical server provisioning via StateMachine ManageIQ/manageiq-providers-redfish#51

Closed

7 tasks

miq-bot added the wip label Mar 19, 2019

gmcculloug reviewed Mar 28, 2019

View reviewed changes

miha-plesko force-pushed the pxe-state-machines branch from 5f59c21 to 6099ec5 Compare April 5, 2019 11:50

miha-plesko force-pushed the pxe-state-machines branch from 6099ec5 to 7207b35 Compare April 17, 2019 13:49

miha-plesko changed the title ~~[WIP] Physical server provisioning via StateMachine~~ Physical server provisioning with internal state machine Apr 17, 2019

miq-bot removed the wip label Apr 17, 2019

miq-bot added the enhancement label Apr 19, 2019

miq-bot assigned agrare Apr 19, 2019

agrare reviewed Apr 22, 2019

View reviewed changes

gmcculloug mentioned this pull request Apr 16, 2019

Physical server provisioning via StateMachine ManageIQ/manageiq-automation_engine#306

Merged

miha-plesko force-pushed the pxe-state-machines branch from 7207b35 to b39b21d Compare April 23, 2019 14:22

miha-plesko commented Apr 23, 2019

View reviewed changes

This was referenced Apr 25, 2019

Strange padding at Request page ManageIQ/manageiq-ui-classic#5490

Closed

Introduce PhysicalServerProvision StateMachine ManageIQ/manageiq-content#516

Merged

gmcculloug reviewed Apr 26, 2019

View reviewed changes

miha-plesko force-pushed the pxe-state-machines branch from b39b21d to f25de09 Compare April 29, 2019 12:38

gmcculloug approved these changes Apr 29, 2019

View reviewed changes

agrare approved these changes Apr 29, 2019

View reviewed changes

agrare merged commit f25de09 into ManageIQ:master Apr 29, 2019

agrare added a commit that referenced this pull request Apr 29, 2019

Merge pull request #18573 from xlab-si/pxe-state-machines

05c0432

Physical server provisioning with internal state machine

rodneyhbrown7 mentioned this pull request May 6, 2019

WIP: Initial PR for using the provisioning state machine ManageIQ/manageiq-providers-lenovo#265

Closed

miha-plesko mentioned this pull request May 23, 2019

Introduce Request and Task for firmware update #18801

Merged

agrare added this to the Sprint 110 Ending Apr 29, 2019 milestone Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Physical server provisioning with internal state machine #18573

Physical server provisioning with internal state machine #18573

miha-plesko commented Mar 19, 2019 •

edited by gmcculloug

Loading

gmcculloug Mar 28, 2019

miha-plesko Apr 17, 2019

miha-plesko commented Apr 19, 2019

agrare left a comment

miha-plesko Apr 23, 2019

gmcculloug Apr 26, 2019

miha-plesko Apr 23, 2019

gmcculloug left a comment

gmcculloug Apr 26, 2019

miha-plesko Apr 29, 2019

gmcculloug Apr 26, 2019

miha-plesko Apr 29, 2019

gmcculloug Apr 26, 2019

miq-bot commented Apr 29, 2019

agrare left a comment

agrare Apr 29, 2019

miha-plesko Apr 30, 2019

Physical server provisioning with internal state machine #18573

Physical server provisioning with internal state machine #18573

Conversation

miha-plesko commented Mar 19, 2019 • edited by gmcculloug Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Apr 19, 2019

agrare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmcculloug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 29, 2019

agrare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miha-plesko commented Mar 19, 2019 •

edited by gmcculloug

Loading