JobProxyDispatcher should use all container image classes #15519

enoodle · 2017-07-06T08:33:36Z

With the addition of new container image classes in ManageIQ/manageiq-providers-openshift#23 the JobProxyDispatcher should now address all of them.

This changes the way that Jobs are identified by the dispatcher so that they will be found by the type of job instead of the job's target class.

enoodle · 2017-07-06T08:34:18Z

@moolitayer please review

@miq-bot add_label bug

enoodle · 2017-07-06T08:49:22Z

This changes active_scans_by_zone and pending_jobs to use the input argument instead of calling base_class on it. Those two functions were only used in this file and only with ContainerImage and VmOrTemplate which are base classes so they both won't be effected by this.

moolitayer · 2017-07-06T09:38:12Z

app/models/job_proxy_dispatcher.rb

@@ -193,12 +195,11 @@ def self.waiting?
  end

  def pending_jobs(target_class = VmOrTemplate)
-    class_name = target_class.base_class.name


any idea what was the result of this line?

For both ContainerImage and VmOrTemplate the result was their class name ("ContainerImage", "VmOrTemplate" as strings).
#15519 (comment)

moolitayer · 2017-07-06T09:39:29Z

spec/models/job_proxy_dispatcher_spec.rb

@@ -174,6 +174,7 @@
    end

    context "with container and vms jobs" do
+      let (:container_image_classes) { ContainerImage.descendants.collect(&:name).append('ContainerImage') }


please test for a list as well

👍 here: https://github.com/ManageIQ/manageiq/pull/15519/files#diff-b36aacfb241b305bc73f3a934af1ea53R195

moolitayer · 2017-07-06T09:39:53Z

@jrafanie can you please review this change?

moolitayer · 2017-07-06T09:40:08Z

app/models/job_proxy_dispatcher.rb

@@ -1,5 +1,7 @@
 class JobProxyDispatcher
  include Vmdb::Logging
+
+  CONTAINER_IMAGE_CLASSES = ContainerImage.descendants.append(ContainerImage)


I don't think its safe to do this once in require time, please move to a method

jrafanie · 2017-07-07T15:56:35Z

app/models/job_proxy_dispatcher.rb

@@ -93,6 +94,10 @@ def dispatch
    _log.info "Complete - Timings: #{t.inspect}"
  end

+  def container_image_classes
+    ContainerImage.descendants.append(ContainerImage)


I know the current pattern does this but it feels like the dispatcher shouldn't have this knowledge. Additionally, it would cause us to load each of these classes and any requires they do in the schedule worker... a process that probably shouldn't load container dependencies. I'm not sure of a better solution but if you have an idea, do let us know.

We can check for jobs with target != VmOrTemplate.
It is a hack, maybe if we add a comment that states that if we ever get a target that isn't one of the two we should rethink.

Maybe we can look at job.type instead

hi @moolitayer, thanks... One thing that still confuses me is it feels like the vm scans are really VmOrTemplateScanJob things (hiding as the Job class) and the container image scans are really ContainerImageScanJob things (hiding as a list of descendent classes + self here)... I don't know why we didn't use single table inheritance on this table in the past but I'm wondering if we should do it now. Perhaps fix this without a migration like you are here for any backports... but then, fix it right on master with STI. What do you think? cc @Fryguy

The nice thing is then you can do ContainerImageScanJob.where... or VmOrTemplateScanJob.where as you would expect. This might break the UI in many places where they're expecting Job classes by name, but it feels like we're doing it the hard way and fixing this might help maintenance in the future.

@gtanzillo ^

I don't know why we didn't use single table inheritance on this table in the past but I'm wondering if we should do it now.

We already do have STI on the jobs table.

enoodle · 2017-07-13T11:03:51Z

I added a commit to use the Job table STI instead of searching by the target class. @jrafanie @gtanzillo can you take a look?
I am not sure if it plays well with NativeOperationWorkflow @agrare if you can take a look.

miq-bot · 2017-07-16T10:33:36Z

Checked commits enoodle/manageiq@c8a16c6~...3bbab0d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 4 offenses detected

spec/models/job_proxy_dispatcher_spec.rb

⚠️ - Line 177, Col 10 - Lint/ParenthesesAsGroupedExpression - (...) interpreted as grouped expression.
❗ - Line 231, Col 52 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 240, Col 52 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
❗ - Line 243, Col 52 - Style/MultilineMethodCallBraceLayout - Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

moolitayer

LGTM 👍

jrafanie · 2017-07-17T01:45:08Z

app/models/job_proxy_dispatcher.rb

-             state != 'finished' and
-             (zone is null or zone = ?) and
-             sync_key is not NULL)", @zone)
+    job_class.order(:id)


@hsong-rh Please review the change here to use VmScan by default to look up Job objects instead of using Job.where(:target_class => "VmOrTemplate"). Looks good to me but I want to confirm with you.

ping @hsong-rh ?

jrafanie

LGTM, just waiting for approval from the SSA team.

simon3z · 2017-07-20T08:57:58Z

@enoodle @moolitayer @jrafanie any consideration on the Jobs already present in the db? Should we be concerned about those? (When/if they'll fail will they just disappear or will they remain in the db forever?)

enoodle · 2017-07-20T09:18:54Z

@enoodle @moolitayer @jrafanie any consideration on the Jobs already present in the db? Should we be concerned about those? (When/if they'll fail will they just disappear or will they remain in the db forever?)

We are changing here the way that we locate the Jobs, Instead of locating jobs by the target class we are locating them by the class of the job. "old" jobs would still be found by this so I don't see it creating any problems.

moolitayer · 2017-07-20T10:02:33Z

We are changing here the way that we locate the Jobs, Instead of locating jobs by the target class we are locating them by the class of the job. "old" jobs would still be found by this so I don't see it creating any problems.

That is my understanding as well

moolitayer · 2017-07-24T12:09:39Z

@roliveri @hsong-rh ping

moolitayer · 2017-07-25T17:46:32Z

cc @Loicavenel

roliveri

Looks good to me.

miq-bot added the bug label Jul 6, 2017

enoodle force-pushed the pending_container_scan_jobs_for_all_images branch from 889af09 to 681ff98 Compare July 6, 2017 08:52

moolitayer reviewed Jul 6, 2017

View reviewed changes

enoodle force-pushed the pending_container_scan_jobs_for_all_images branch from 681ff98 to 68a2271 Compare July 6, 2017 09:48

job proxy dispatcher should use all container image classes

c8a16c6

enoodle force-pushed the pending_container_scan_jobs_for_all_images branch from 68a2271 to c8a16c6 Compare July 6, 2017 10:39

chessbyte assigned jrafanie Jul 6, 2017

chessbyte added core/smart state providers/containers labels Jul 6, 2017

jrafanie reviewed Jul 7, 2017

View reviewed changes

use Job class STI

cccdfe4

enoodle mentioned this pull request Jul 13, 2017

Adding openshift_container_images ManageIQ/manageiq-providers-openshift#23

Merged

using all EXISTING container image classes

3bbab0d

moolitayer approved these changes Jul 16, 2017

View reviewed changes

jrafanie reviewed Jul 17, 2017

View reviewed changes

jrafanie approved these changes Jul 17, 2017

View reviewed changes

enoodle mentioned this pull request Jul 20, 2017

ActiveRecord::SubclassNotFound for ContainerImage ManageIQ/manageiq-providers-kubernetes#70

Closed

enoodle mentioned this pull request Jul 20, 2017

Move Openshift specific code into its provider directory. #15523

Merged

roliveri approved these changes Jul 26, 2017

View reviewed changes

jrafanie merged commit 9a58921 into ManageIQ:master Jul 26, 2017

jrafanie added this to the Sprint 66 Ending Aug 7, 2017 milestone Jul 26, 2017

jrafanie added the fine/no label Jul 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JobProxyDispatcher should use all container image classes #15519

JobProxyDispatcher should use all container image classes #15519

enoodle commented Jul 6, 2017 •

edited

Loading

enoodle commented Jul 6, 2017

enoodle commented Jul 6, 2017

moolitayer Jul 6, 2017

enoodle Jul 6, 2017

moolitayer Jul 6, 2017

enoodle Jul 6, 2017

moolitayer commented Jul 6, 2017

moolitayer Jul 6, 2017

jrafanie Jul 7, 2017

moolitayer Jul 7, 2017

jrafanie Jul 10, 2017

jrafanie Jul 10, 2017

Fryguy Jul 11, 2017

enoodle commented Jul 13, 2017

miq-bot commented Jul 16, 2017

moolitayer left a comment

jrafanie Jul 17, 2017

enoodle Jul 20, 2017

jrafanie left a comment

simon3z commented Jul 20, 2017

enoodle commented Jul 20, 2017

moolitayer commented Jul 20, 2017

moolitayer commented Jul 24, 2017

moolitayer commented Jul 25, 2017

roliveri left a comment

JobProxyDispatcher should use all container image classes #15519

JobProxyDispatcher should use all container image classes #15519

Conversation

enoodle commented Jul 6, 2017 • edited Loading

enoodle commented Jul 6, 2017

enoodle commented Jul 6, 2017

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

moolitayer commented Jul 6, 2017

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

enoodle commented Jul 13, 2017

miq-bot commented Jul 16, 2017

moolitayer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie left a comment

Choose a reason for hiding this comment

simon3z commented Jul 20, 2017

enoodle commented Jul 20, 2017

moolitayer commented Jul 20, 2017

moolitayer commented Jul 24, 2017

moolitayer commented Jul 25, 2017

roliveri left a comment

Choose a reason for hiding this comment

enoodle commented Jul 6, 2017 •

edited

Loading