Fixing subservice task creation for service bundles #18283

d-m-u · 2018-12-11T20:58:36Z

Subservices aren't being correctly retired. The task creation should be using src_ids, and the recursive call to create tasks for subservices is wrong. Also, the service retireable check should handle subservices for bundles, and the request and task should be able to mark themselves as finished after the status update to "retired", just like we do for provisioning.

req_state = (states.length == 1) ? states.keys.first : "active", the check we're currently using at https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_request.rb#L383 is being overriden here to set the request to finished to work for bundles. We should be checking state and status for all the child tasks and if they're all ok and finished, then the parent can be updated.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1608958
Also fixes https://bugzilla.redhat.com/show_bug.cgi?id=1653648

They're related.

d-m-u · 2018-12-11T21:01:45Z

@miq-bot add_label bug

d-m-u · 2019-01-03T18:20:52Z

@miq-bot remove_label wip
@miq-bot add_label blocker
@miq-bot assign @gmcculloug
@miq-bot add_reviewer @tinaafitz

gmcculloug · 2019-01-03T18:48:58Z

app/models/service.rb

@@ -218,10 +218,6 @@ def composite?
    children.present?
  end

-  def retireable?
-    type.present?
-  end


This method is now included from the CiFeatureMixin which always returns true. 👍

The rational for changing this check is that, while the blanket check for type being present has some logical issues since not all services have types but all are retireable, the part that's being omitted at the moment is subservices. Per #17317 (comment), I think checking to see if the service is a subservice first makes more sense.

d-m-u · 2019-01-03T18:50:33Z

@miq-bot add_label hammer/yes

app/models/service_retire_task.rb

d-m-u · 2019-01-08T14:13:21Z

@miq-bot remove_label wip

d-m-u · 2019-01-08T15:52:17Z

Failures look unrelated to this code.

app/models/miq_retire_request.rb

app/models/service_retire_task.rb

app/models/vm_retire_task.rb

bdunne · 2019-01-08T16:42:51Z

spec/models/mixins/ci_feature_mixin_spec.rb

-        end
-      end
-
-      context "without type" do


Doesn't this still need to be tested?

It's here: https://github.com/ManageIQ/manageiq/pull/18283/files#diff-92a7338185f2ababc762a33994ba1eebR687

spec/models/service_spec.rb

miq-bot · 2019-01-08T18:15:53Z

Checked commit d-m-u@97537a4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
8 files checked, 0 offenses detected
Everything looks fine. 🏆

bdunne · 2019-01-08T18:23:28Z

app/models/service_retire_task.rb

@@ -16,7 +16,9 @@ def update_and_notify_parent(*args)
  end

  def task_finished
-    update_attributes(:status => status == 'Ok' ? 'Completed' : 'Failed')
+    if status != 'Ok'
+      update_attributes(:status => 'Error')


Is this already set to "Completed" before this method is called?

Well, we needed the completed gone, because it's not a valid state for the task. It should either be ok or error.

…for_bundled_service_bundles Fixing subservice task creation for service bundles (cherry picked from commit 7889bd0) Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1608958 Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1653648

simaishi · 2019-01-08T18:59:21Z

Hammer backport details:

$ git log -1
commit a0968c86246cd63fec12eb8e85b8cc5cbc544eb4
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Tue Jan 8 13:37:37 2019 -0500

    Merge pull request #18283 from d-m-u/fixing_subservice_task_creation_for_bundled_service_bundles
    
    Fixing subservice task creation for service bundles
    
    (cherry picked from commit 7889bd00d0305cc9e56c6f918047d75a3cfbfafb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1608958
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1653648

miq-bot added the wip label Dec 11, 2018

miq-bot added the bug label Dec 11, 2018

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 8 times, most recently from fd5ce90 to 1345539 Compare December 18, 2018 20:45

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 2 times, most recently from e220d96 to 10bf5c6 Compare January 3, 2019 18:17

d-m-u changed the title ~~[WIP] Fixing subservice task creation for bundled service bundles~~ Fixing subservice task creation for bundled service bundles Jan 3, 2019

d-m-u changed the title ~~Fixing subservice task creation for bundled service bundles~~ Fixing subservice task creation for service bundles Jan 3, 2019

miq-bot added blocker and removed wip labels Jan 3, 2019

miq-bot assigned gmcculloug Jan 3, 2019

miq-bot requested a review from tinaafitz January 3, 2019 18:21

gmcculloug reviewed Jan 3, 2019

View reviewed changes

miq-bot added the hammer/yes label Jan 3, 2019

gmcculloug reviewed Jan 3, 2019

View reviewed changes

app/models/service_retire_task.rb Outdated Show resolved Hide resolved

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 2 times, most recently from 54cdfa5 to 9da82b4 Compare January 4, 2019 13:45

d-m-u changed the title ~~Fixing subservice task creation for service bundles~~ [WIP] Fixing subservice task creation for service bundles Jan 4, 2019

miq-bot added the wip label Jan 7, 2019

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 2 times, most recently from 225556e to 1cba151 Compare January 8, 2019 00:52

d-m-u changed the title ~~[WIP] Fixing subservice task creation for service bundles~~ Fixing subservice task creation for service bundles Jan 8, 2019

miq-bot removed the wip label Jan 8, 2019

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 6 times, most recently from 5d05837 to 0136071 Compare January 8, 2019 15:24

bdunne requested changes Jan 8, 2019

View reviewed changes

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch 3 times, most recently from 43a3771 to c5ef83f Compare January 8, 2019 17:48

Fix service bundle task creation

97537a4

d-m-u force-pushed the fixing_subservice_task_creation_for_bundled_service_bundles branch from c5ef83f to 97537a4 Compare January 8, 2019 18:06

tinaafitz approved these changes Jan 8, 2019

View reviewed changes

bdunne reviewed Jan 8, 2019

View reviewed changes

bdunne approved these changes Jan 8, 2019

View reviewed changes

bdunne merged commit 7889bd0 into ManageIQ:master Jan 8, 2019

bdunne assigned bdunne and unassigned gmcculloug Jan 8, 2019

bdunne added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 8, 2019

simaishi added hammer/backported and removed hammer/yes labels Jan 8, 2019

d-m-u mentioned this pull request Jan 10, 2019

Add retired to service active states #18348

Merged

d-m-u deleted the fixing_subservice_task_creation_for_bundled_service_bundles branch February 1, 2019 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing subservice task creation for service bundles #18283

Fixing subservice task creation for service bundles #18283

d-m-u commented Dec 11, 2018 •

edited

Loading

d-m-u commented Dec 11, 2018

d-m-u commented Jan 3, 2019

gmcculloug Jan 3, 2019

d-m-u Jan 8, 2019

d-m-u commented Jan 3, 2019

d-m-u commented Jan 8, 2019

d-m-u commented Jan 8, 2019

bdunne Jan 8, 2019

d-m-u Jan 8, 2019

miq-bot commented Jan 8, 2019

bdunne Jan 8, 2019

d-m-u Jan 8, 2019

simaishi commented Jan 8, 2019

Fixing subservice task creation for service bundles #18283

Fixing subservice task creation for service bundles #18283

Conversation

d-m-u commented Dec 11, 2018 • edited Loading

d-m-u commented Dec 11, 2018

d-m-u commented Jan 3, 2019

gmcculloug Jan 3, 2019

Choose a reason for hiding this comment

d-m-u Jan 8, 2019

Choose a reason for hiding this comment

d-m-u commented Jan 3, 2019

d-m-u commented Jan 8, 2019

d-m-u commented Jan 8, 2019

bdunne Jan 8, 2019

Choose a reason for hiding this comment

d-m-u Jan 8, 2019

Choose a reason for hiding this comment

miq-bot commented Jan 8, 2019

bdunne Jan 8, 2019

Choose a reason for hiding this comment

d-m-u Jan 8, 2019

Choose a reason for hiding this comment

simaishi commented Jan 8, 2019

d-m-u commented Dec 11, 2018 •

edited

Loading