Add persistor with configurable saver strategy #73

Ladas · 2017-07-25T13:15:22Z

Add Persistor with configurable saver strategy. Encapsulating ICs to Persistor and exposing the options to Settings. Testing both ;default and :batch strategies

Check with https://github.com/ManageIQ/manageiq-providers-kubernetes/pull/73/files?w=1 to filter out the whitespace changes

Ladas · 2017-07-25T13:15:56Z

cc @agrare @cben

Ladas · 2017-07-26T10:03:37Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

+          :parent               => manager,
+          :builder_params       => {:ems_id => manager.id},
+          :association          => :container_routes,
+          :attributes_blacklist => [:namespace, :tags],


@cben I needed to blacklist tags to have OSE test passing, is there some other change that doesn't require this?

Hmm, didn't notice parse_* functions are setting tags. I'm surprised it worked so far without the blacklist :-)

It might have been a bad rebase, because it was there and then I noticed it was missing when I rebased yesterday. Without it, the OpenShift specs were failing for me.

miq-bot · 2017-07-26T21:14:07Z

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

Expose shared options of the existing ICs and make a Persister connection.

Use a Persistor in the Parser

Make InventoryCollections configurable by Settings

Test :default and :batch saver strategies

Blacklist tags for container routes and builds

Deduplicate spec before blocks

Remove image labels as those are conditionally in the OSE parser

Extract persister class, so it can be redefined by OpenShift

ContainerService id is part of the ContainerServicePortConfig ems_ref

Add todo for :protocol inside of ContainerServicePortConfig manager_ref

miq-bot · 2017-07-27T08:24:21Z

Checked commits Ladas/manageiq-providers-kubernetes@07971aa~...0af5990 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 1 offense detected

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

❗ - Line 49, Col 9 - Style/CommentAnnotation - Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.

cben

Excellent 👍 some small questions

Made me read the base inventory.rb, inventory/persister.rb, so I finally have some idea what these do :-)

cben · 2017-07-27T08:12:34Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

-      initialize_inventory_collections(ems, options)
+      persister = ManageIQ::Providers::Kubernetes::Inventory::Persister::ContainerManager.new(ems)
+      # TODO expose Persistor and use that
+      @inv_collections = persister.collections


https://english.stackexchange.com/questions/206893/persister-or-persistor ?
(unconclusive, some anecdotal usage but no etymological basis for distinct meanings)

cben · 2017-07-27T08:43:58Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

-    {
-      :strategy       => strategy,
-      :targeted       => targeted,
-      :saver_strategy => saver_strategy


So if you don't set saver_strategy in settings, would it still use default or is it a mandatory setting now?

Can you add the possible/interesting options to settings.yml? Or are there too many to list?
At least saver_strategy? I'd like at least those settings we expect performance team to measure to appear in Advanced Settings.

cben · 2017-07-27T08:46:51Z

spec/models/manageiq/providers/kubernetes/container_manager/refresher_spec.rb

+    context "with :batch saver" do
+      before(:each) do
+        stub_settings_merge(
+          :ems_refresh => {:kubernetes => {:inventory_collections => {:saver_strategy => :batch}}}


Will it work with string "batch" as more likely to come from settings.yml ? (I think our settings parser does understand :symbol syntax but I don't want to require it.)

Settings can handle symbol, we use in on few places now. But yeah, the interface should probably handle string too. Should be done in coming PRs, where I will be refactoring the interface.

cben · 2017-07-27T08:56:00Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

@@ -41,8 +40,12 @@ def ems_inv_to_hashes(inventory, _options = Config::Options.new)
      @data
    end

+    def persister_class
+      ManageIQ::Providers::Kubernetes::Inventory::Persister::ContainerManager


A job for Inventory.persister_class_for() ?
(I'm actually happy as written — less magic to follow, but if you already have the magic lookup pattern in other places...)

possibly, but the next step should be to expose persistor, instead of @inv_collections. So yeah, we could abstract that on the Inventory.

cben · 2017-07-27T10:14:28Z

codeclimate is all about InventoryCollection construction, ignorable.

cben · 2017-07-27T10:27:13Z

@miq-bot add-label performance
(a lot of performance :-)

cben · 2017-07-27T11:02:20Z

@moolitayer @enoodle @zeari please review.
(?w=1 link in PR description helps.)

moolitayer · 2017-07-27T11:18:45Z

app/models/manageiq/providers/kubernetes/container_manager/refresh_parser.rb

    def ems_inv_to_inv_collections(ems, inventory, options = Config::Options.new)
-      initialize_inventory_collections(ems, options)
+      persister = persister_class.new(ems)
+      # TODO expose Persistor and use that


@Ladas can you please explain this TODO?

just that we should replace the usage of @inv_collections, that we have in many files by the Persister object, that should be the interface

ahh okay, was a little confused by the fact that here the todo is already done...

yeah, we just need to fix all the other places. :-)

moolitayer · 2017-07-27T11:39:25Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

+  end
+
+  def shared_options
+    settings_options = options[:inventory_collections].try(:to_hash) || {}


Where is options coming from?

that is on a base class Persistor, taking it from the advanced settings

moolitayer · 2017-07-27T11:46:12Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

+
+    settings_options.merge(
+      :strategy => strategy,
+      :targeted => targeted,


Is it intentional these two can't be configured?

yes, those 2 will be overwritten by the Persistor class used for subgraph saving. The interface itself, I still need to redesign it, trying to figure it out now. :-)

moolitayer · 2017-07-27T11:46:31Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

+  end
+
+  def strategy
+    nil


what is the meaning of a nil strategy ? can we be explicit ?

This is the DB searching strategy, default is nil. We will redefine it for the sub-graph saving Persistor

oh I see it on InventoryCollection now, nice doc!

yeah, I need to refactor those though, since the number of settings to cover all the corner cases has grown a lot :-) So that interface + connection to Settings refactoring is on the roadmap next.

moolitayer · 2017-07-27T11:53:59Z

app/models/manageiq/providers/kubernetes/container_manager/inventory_collections.rb

+          :association          => :container_services,
+          :secondary_refs       => {:by_namespace_and_name => [:namespace, :name]},
+          :attributes_blacklist => [:namespace],
+          :saver_strategy       => :default # TODO(perf) Can't use batch strategy because of usage of M:N container_groups relation


M:N = M:M ? if not what does it mean?

the relation table container_groups_container_services, we need to model it as a separate InventoryCollection, then the batch saving will work, for both container_groups and container_groups_container_services

moolitayer

LGTM 👍

moolitayer

LGTM 👍

moolitayer · 2017-07-27T12:28:18Z

BTW @Ladas it would be nice to have an empty inventory_collections in the settings for visibility

Ladas · 2017-07-27T12:40:04Z

@moolitayer right, I haven't figured out the exact format yet, once i do, I'll push it to the settings. We will probably need to be able to set the option for specific Persistor and specific InventoryCollection.

bdunne · 2017-08-08T20:37:49Z

@moolitayer Any reason there's no milestone here?

moolitayer · 2017-08-09T13:16:39Z

@bdunne see #72 (comment)

there's no particular reason, just update for update sake https://github.com/ManageIQ/more_core_extensions/compare/179bf40..e5b4501 - Added Ruby 2.7 support [[ManageIQ#79](ManageIQ/more_core_extensions#79)] - Added Process#pause, Process#resume, and Process#alive? [[ManageIQ#73](ManageIQ/more_core_extensions#73)] array added * `#compact_map` - Collect non-nil results from the block array added `#tabular_sort` - Sorts an Array of Hashes by specific columns hierarchy added `#descendant_get` - Returns the descendant with a given name the two breaking changes: - **BREAKING**: Moved Object#descendant_get to Class#descendant_get [[ManageIQ#75](ManageIQ/more_core_extensions#75)] - **BREAKING**: Removed deprecated Enumerable#stable_sort_by [[ManageIQ#76](ManageIQ/more_core_extensions#76)] a minor header output change was made that hasn't been released yet to make tableize more markdown compliant see ManageIQ/linux_admin#221

Ladas added the enhancement label Jul 25, 2017

Ladas mentioned this pull request Jul 25, 2017

[WIP] fixing graph refresh and adding persistor #64

Closed

Ladas closed this Jul 25, 2017

Ladas reopened this Jul 25, 2017

Ladas force-pushed the add_persistor_with_configurable_saver_strategy branch 2 times, most recently from 91d0489 to e7b0d2f Compare July 26, 2017 09:52

Ladas mentioned this pull request Jul 26, 2017

Add openshift persister and test also batch saver strategy ManageIQ/manageiq-providers-openshift#39

Merged

2 tasks

Ladas commented Jul 26, 2017

View reviewed changes

cben mentioned this pull request Jul 26, 2017

Openshift graph refresh ManageIQ/manageiq-providers-openshift#34

Merged

2 tasks

miq-bot added the unmergeable label Jul 26, 2017

agrare and others added 9 commits July 27, 2017 09:44

Add inventory persister for kubernetes

07971aa

Expose shared options of the existing ICs

dd12ac7

Expose shared options of the existing ICs and make a Persister connection.

Use a Persistor in the Parser

9c21f48

Use a Persistor in the Parser

Make InventoryCollections configurable by Settings

3d20bac

Make InventoryCollections configurable by Settings

Test :default and :batch saver strategies

a378d68

Test :default and :batch saver strategies

Blacklist tags for container routes and builds

5bfd4f7

Blacklist tags for container routes and builds

Deduplicate spec before blocks

3a2e8bd

Deduplicate spec before blocks

Remove image labels as those are conditionally in the OSE parser

b2b618d

Remove image labels as those are conditionally in the OSE parser

Extract persister class, so it can be redefined by OpenShift

f22f947

Extract persister class, so it can be redefined by OpenShift

Ladas force-pushed the add_persistor_with_configurable_saver_strategy branch from e7b0d2f to f22f947 Compare July 27, 2017 07:51

miq-bot removed the unmergeable label Jul 27, 2017

Ladas added 2 commits July 27, 2017 10:13

ContainerService id is part of the ContainerServicePortConfig ems_ref

8da447f

ContainerService id is part of the ContainerServicePortConfig ems_ref

Add todo for :protocol inside of ContainerServicePortConfig manager_ref

0af5990

Add todo for :protocol inside of ContainerServicePortConfig manager_ref

cben reviewed Jul 27, 2017

View reviewed changes

cben approved these changes Jul 27, 2017

View reviewed changes

miq-bot added the performance label Jul 27, 2017

cben mentioned this pull request Jul 27, 2017

Containers graph refresh #27

Closed

24 tasks

moolitayer reviewed Jul 27, 2017

View reviewed changes

moolitayer added the refactoring label Jul 27, 2017

moolitayer approved these changes Jul 27, 2017

View reviewed changes

moolitayer merged commit 0f62a51 into ManageIQ:master Jul 27, 2017

moolitayer added this to the Sprint 66 Ending Aug 7, 2017 milestone Aug 9, 2017

Add persistor with configurable saver strategy #73

Add persistor with configurable saver strategy #73

Conversation

Ladas commented Jul 25, 2017 • edited Loading

Ladas commented Jul 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Jul 26, 2017

miq-bot commented Jul 27, 2017

cben 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

Ladas Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

cben commented Jul 27, 2017

cben commented Jul 27, 2017

cben commented Jul 27, 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

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

Choose a reason for hiding this comment

Ladas Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer left a comment

Choose a reason for hiding this comment

moolitayer commented Jul 27, 2017

Ladas commented Jul 27, 2017

bdunne commented Aug 8, 2017

moolitayer commented Aug 9, 2017

Ladas commented Jul 25, 2017 •

edited

Loading

Ladas Jul 27, 2017 •

edited

Loading

Ladas Jul 27, 2017 •

edited

Loading