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

Add OrchestrationTemplateRunner to queue up orchestration stack deployment #18374

Merged

Conversation

lfu
Copy link
Member

@lfu lfu commented Jan 18, 2019

Provider specific provisions executed as part of a service provision are supposed to be queued to the provider zone.

Blocks ManageIQ/manageiq-content#500.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1656351

@miq-bot assign @gmcculloug
@miq-bot add_label bug, gaprindashvili/yes, hammer/yes, services, orchestration

@tinaafitz
Copy link
Member

@bzwei @mkanoor Please review.

}
MiqTask.generic_action_with_callback(task_opts, queue_opts)
end

def update_orchestration_stack
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly we should have a queue version for update too. It can be either included here or with a separate PR.

@@ -35,6 +35,23 @@ def deploy_orchestration_stack
save_create_options
end

def deploy_orchestration_stack_queue
userid ||= User.current_user.try(:userid)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is userid a local variable? || will be no effect.

@lfu lfu force-pushed the service_orchestration_region_1656351 branch from e48c5a1 to c117a9d Compare January 22, 2019 16:14
@lfu lfu changed the title Add method deploy_orchestration_stack_queue. [WIP] Add method deploy_orchestration_stack_queue. Jan 24, 2019
@miq-bot miq-bot added the wip label Jan 24, 2019
@lfu lfu force-pushed the service_orchestration_region_1656351 branch from c117a9d to 2178ef8 Compare January 30, 2019 21:59
@lfu lfu changed the title [WIP] Add method deploy_orchestration_stack_queue. Add OrchestrationTemplateRunner to queue up orchestration stack deployment Jan 30, 2019
@lfu lfu force-pushed the service_orchestration_region_1656351 branch from 2178ef8 to ac9505d Compare January 30, 2019 22:07
@miq-bot miq-bot removed the wip label Jan 30, 2019
@miq-bot
Copy link
Member

miq-bot commented Feb 12, 2019

This pull request is not mergeable. Please rebase and repush.

@lfu lfu force-pushed the service_orchestration_region_1656351 branch from ac9505d to 5cd8b7d Compare February 18, 2019 16:57
@lfu lfu force-pushed the service_orchestration_region_1656351 branch from 5cd8b7d to 7b2fdd5 Compare February 19, 2019 21:59
@lfu
Copy link
Member Author

lfu commented Feb 25, 2019

@bzwei @mkanoor Please review.

@bzwei
Copy link
Contributor

bzwei commented Feb 25, 2019

The goal of this refactoring is that every promise ServiceOrchestration has made does not compromise so that the automate can continue to work.
@lfu Please rework ServiceOrchestration to fulfill the goal.

@lfu lfu force-pushed the service_orchestration_region_1656351 branch from 7b2fdd5 to 136b53e Compare February 27, 2019 23:57
end
end

def service
Copy link
Contributor

Choose a reason for hiding this comment

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

Try our best to decouple modules. Avoid service in this class. The job options should contain only parameters necessary to deploy a stack. For update the options can include stack_id too.

self.name = "#{name}, Orchestration Stack ID: #{@orchestration_stack.id}"
miq_task.update_attributes(:name => name)
save!
service.add_resource(@orchestration_stack)
Copy link
Contributor

Choose a reason for hiding this comment

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

move this line back to service class. You can add orchestraiton_stack_id to job's options so that the service can read back the stack.

my_signal(minimize_indirect, :post_stack_run, err.message, 'error')
ensure
# create options may never be saved before unless they were overridden
service.save_create_options
Copy link
Contributor

Choose a reason for hiding this comment

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

move back to service class

time = Time.zone.now
update_attributes(:started_on => time)
miq_task.update_attributes(:started_on => time)
my_signal(false, :deploy_orchestration_stack)
Copy link
Contributor

Choose a reason for hiding this comment

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

increase the queue priority to high

@lfu lfu force-pushed the service_orchestration_region_1656351 branch 2 times, most recently from af1562c to 3735f09 Compare March 4, 2019 21:43
@lfu
Copy link
Member Author

lfu commented Mar 4, 2019

@bzwei Updated.

creation_options = stack_options
@orchestration_stack = ManageIQ::Providers::CloudManager::OrchestrationStack.create_stack(
orchestration_manager, stack_name, orchestration_template, creation_options
job_options = {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not place all options together without a merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

stack_options might change orchestration_manager and orchestration_template.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is hard to understand. In this case, we can do

  deploy_stack_options = stack_options
  job_options = {
    :create_options => deploy_stack_options,
    :orchestration_manager_id => orchestration_manager.id,
    ...
  }

@@ -47,6 +68,8 @@ def orchestration_stack
if @orchestration_stack.nil? && options.fetch_path(:orchestration_stack, 'ems_id')
@orchestration_stack = OrchestrationStack.new(options[:orchestration_stack])
end

@orchestration_stack ||= orchestration_runner_job.orchestration_stack if orchestration_runner_job
Copy link
Contributor

Choose a reason for hiding this comment

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

@orchestration_stack ||= orchestration_runner_job.try(:orchestration_stack)

end

def wait_on_orchestration_stack
while orchestration_stack.blank?
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to modify orchestration_stack method (for performance reason). Change this method to

  while deploy_stack_job.orchestration_stack.blank?
    ...
  end
  @orchestration_stack = deploy_stack_job.orchestration_stack

This is the only place you need to get the stack from the runner job.

orchestration_manager, stack_name, orchestration_template, creation_options
job_options = {
:create_options => stack_options,
:priority => MiqQueue::HIGH_PRIORITY,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to set priority always to high. We need high priority to start the job because the current thread is waiting on it. Once it starts, the priority should reduce to normal.

@lfu lfu force-pushed the service_orchestration_region_1656351 branch 3 times, most recently from c708c20 to 5bbe071 Compare March 5, 2019 20:41
end

def wait_on_orchestration_stack
while orchestration_runner_job.orchestration_stack.blank?
Copy link
Contributor

Choose a reason for hiding this comment

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

clearly this only applies to deploy_stack_job. update_stack_job does not need to wait on the stack.

creation_options = stack_options
@orchestration_stack = ManageIQ::Providers::CloudManager::OrchestrationStack.create_stack(
orchestration_manager, stack_name, orchestration_template, creation_options
job_options = {
Copy link
Contributor

Choose a reason for hiding this comment

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

It is hard to understand. In this case, we can do

  deploy_stack_options = stack_options
  job_options = {
    :create_options => deploy_stack_options,
    :orchestration_manager_id => orchestration_manager.id,
    ...
  }

@lfu lfu force-pushed the service_orchestration_region_1656351 branch from 5bbe071 to d37428f Compare March 5, 2019 21:07
# Code running with Rails QueryCache enabled,
# need to disable caching for the reload to see updates.
self.class.uncached { reload }
orchestration_runner_job.class.uncached { orchestration_runner_job.reload }
Copy link
Contributor

Choose a reason for hiding this comment

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

deploy_stack_job.class.uncached

self.class.uncached { reload }
orchestration_runner_job.class.uncached { orchestration_runner_job.reload }
end
@orchestration_stack = orchestration_runner_job.orchestration_stack
Copy link
Contributor

Choose a reason for hiding this comment

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

deploy_stack_job.orchestration_stack

lfu added 2 commits March 5, 2019 16:27
All provider calls should be placed in the queue with the correct provider zone.
Add OrchestrationTemplateRunner to handle the provider calls via MiqQueue.

https://bugzilla.redhat.com/show_bug.cgi?id=1656351
@lfu lfu force-pushed the service_orchestration_region_1656351 branch from d37428f to 08ab7f4 Compare March 5, 2019 21:28
@miq-bot
Copy link
Member

miq-bot commented Mar 5, 2019

Checked commits lfu/manageiq@18f6f21~...08ab7f4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

@gmcculloug gmcculloug merged commit bb4f8bf into ManageIQ:master Mar 5, 2019
@gmcculloug gmcculloug added this to the Sprint 107 Ending Mar 18, 2019 milestone Mar 5, 2019
simaishi pushed a commit that referenced this pull request Mar 8, 2019
Add OrchestrationTemplateRunner to queue up orchestration stack deployment

(cherry picked from commit bb4f8bf)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686891
@simaishi
Copy link
Contributor

simaishi commented Mar 8, 2019

Hammer backport details:

$ git log -1
commit 455d2702937ea597b40cba7a791096370eea4e9c
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Mar 5 18:10:25 2019 -0500

    Merge pull request #18374 from lfu/service_orchestration_region_1656351
    
    Add OrchestrationTemplateRunner to queue up orchestration stack deployment
    
    (cherry picked from commit bb4f8bf36b54ebd31823704426ecdaccee2fdbee)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686891

@lfu lfu deleted the service_orchestration_region_1656351 branch November 4, 2019 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants