Synchronize provider with server when zone changes #18272

jrafanie · 2018-12-06T22:12:11Z

If an appliance's zone (or hostname/ipaddress) changes, we don't update the related "hidden" provider's zone (and URL), requiring users to restart the embedded ansible role or reboot the whole appliance. This PR checks for these changes and updates the provider.

I added a commit to check for URL changes, in case they renamed the appliance (hostname) or the ip address. I can do this as a followup if need be, but it's a tiny change so I included it.

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

carbonin · 2018-12-06T22:48:49Z

spec/models/embedded_ansible_worker/runner_spec.rb

@@ -99,7 +99,13 @@
        runner.instance_variable_set(:@job_data_retention, ::Settings.embedded_ansible.job_data_retention_days)
      end

+      after do


Why do we need this block? Doesn't the runner get re-evaluated for each example? It's defined in a let

Ah, good point... I could have sworn they were class methods but clearly not... I'll verify but yeah, I think the runner should be new for each example.

carbonin · 2018-12-06T22:50:34Z

spec/models/embedded_ansible_worker/runner_spec.rb

@@ -133,12 +139,48 @@

        it "sets the embedded ansible job data retention value when the setting changes" do
          allow(embedded_ansible_instance).to receive(:alive?).and_return(true)
-          allow(runner).to receive(:provider).and_return(provider)


Why can this be removed here, but not in the other examples above it?

It was part of my change to add the after....I can remove this change if I remove the after.

We only needed this line in the other examples because we set an expectation on the provider object in those examples... therefore, we needed to make sure the runner.provider returns this object and not the completely different ruby object returned from first_or_initialize from the "normal" provider method.

Yeah, I think I'd prefer to remove the after block and add this back.

Sounds good

carbonin · 2018-12-06T22:53:32Z

app/models/embedded_ansible_worker/runner.rb

+    provider.save!
+  end
+
+  def provider_matches_server?


I think we could probably name this better as it's handling both the time aspect as well as the matching.

Unfortunately the best I could come up with was synchronize_provider_with_server?

It was first matches_stuff?, I renamed it and gave up because it won't matter until a few eyes come up with a better name.

I'm glad you win the 🏆 of "find the part Joe bailed on trying to name correctly."

I like synchronize_provider_with_server

hmm, that's funny... looking at it now, I like provider_out_of_sync_with_server?

lol, double negative... provider_in_sync_with_server?

jrafanie · 2018-12-06T23:07:59Z

spec/models/embedded_ansible_worker/runner_spec.rb

@@ -109,7 +115,7 @@
      end

      context "with a provider" do
-        let(:provider) { FactoryGirl.create(:provider_embedded_ansible, :with_authentication) }
+        let!(:provider) { FactoryGirl.create(:provider_embedded_ansible, :with_authentication) }


This was added because all examples were already calling provider and my new example wasn't, but still needed it created.

carbonin · 2018-12-07T14:11:57Z

As for updating just the zone (fixing only the bug) vs updating the zone and URL, I prefer fixing both.

The simplicity of this change which refactors existing, tested, code into a new method which is called additional times feels more stable than implementing something new, or changing the existing code block to update only the zone while leaving the URL alone.

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

miq-bot · 2018-12-07T16:45:53Z

Checked commits jrafanie/manageiq@62049ee~...52497e4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

jrafanie · 2018-12-07T16:49:13Z

app/models/embedded_ansible_worker/runner.rb

@@ -22,6 +23,7 @@ def heartbeat
  end

  def do_work
+    synchronize_provider_with_server unless provider_in_sync_with_server?


I think this new method name looks better... @carbonin what do you think?

I think the same problem is still there.

The name only mentions that we're checking the properties, not that we are also returning true based on some timeframe. It doesn't really matter though. I'm not going to hold this up for a better name though.

…ver_when_zone_changes Synchronize provider with server when zone changes (cherry picked from commit a2592e8) https://bugzilla.redhat.com/show_bug.cgi?id=1656308

simaishi · 2018-12-11T14:01:27Z

Hammer backport details:

$ git log -1
commit fd990a963179821d5f1aa1d5c8d00424435635c1
Author: Nick Carboni <[email protected]>
Date:   Fri Dec 7 14:45:57 2018 -0500

    Merge pull request #18272 from jrafanie/synchronize_provider_with_server_when_zone_changes
    
    Synchronize provider with server when zone changes
    
    (cherry picked from commit a2592e80358a82c550eda205267e1bd421dcc97a)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1656308

Extract a method to sync provider with server.

62049ee

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

jrafanie added the bug label Dec 6, 2018

jrafanie requested a review from carbonin December 6, 2018 22:12

jrafanie added the hammer/yes label Dec 6, 2018

jrafanie force-pushed the synchronize_provider_with_server_when_zone_changes branch from 579a921 to f4725de Compare December 6, 2018 22:18

carbonin reviewed Dec 6, 2018

View reviewed changes

jrafanie commented Dec 6, 2018

View reviewed changes

jrafanie force-pushed the synchronize_provider_with_server_when_zone_changes branch from f4725de to 4685f22 Compare December 7, 2018 16:41

jrafanie added 2 commits December 7, 2018 11:42

Sync provider with appliance zone if it changes

0a981f6

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

Detect a hostname/ipaddress change and update the provider

52497e4

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

jrafanie force-pushed the synchronize_provider_with_server_when_zone_changes branch from 4685f22 to 52497e4 Compare December 7, 2018 16:42

jrafanie commented Dec 7, 2018

View reviewed changes

carbonin self-assigned this Dec 7, 2018

carbonin added the changelog/yes label Dec 7, 2018

carbonin approved these changes Dec 7, 2018

View reviewed changes

carbonin merged commit a2592e8 into ManageIQ:master Dec 7, 2018

carbonin added this to the Sprint 101 Ending Dec 17, 2018 milestone Dec 7, 2018

jrafanie deleted the synchronize_provider_with_server_when_zone_changes branch December 10, 2018 15:53

simaishi added the blocker label Dec 11, 2018

simaishi added hammer/backported and removed hammer/yes labels Dec 11, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronize provider with server when zone changes #18272

Synchronize provider with server when zone changes #18272

jrafanie commented Dec 6, 2018 •

edited

Loading

carbonin Dec 6, 2018

jrafanie Dec 6, 2018

carbonin Dec 6, 2018

jrafanie Dec 6, 2018

carbonin Dec 7, 2018

jrafanie Dec 7, 2018

carbonin Dec 6, 2018 •

edited

Loading

jrafanie Dec 6, 2018

jrafanie Dec 6, 2018

jrafanie Dec 7, 2018

jrafanie Dec 7, 2018

jrafanie Dec 6, 2018

carbonin commented Dec 7, 2018

miq-bot commented Dec 7, 2018

jrafanie Dec 7, 2018

carbonin Dec 7, 2018

simaishi commented Dec 11, 2018

Synchronize provider with server when zone changes #18272

Synchronize provider with server when zone changes #18272

Conversation

jrafanie commented Dec 6, 2018 • edited Loading

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

carbonin Dec 6, 2018 • edited Loading

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

carbonin commented Dec 7, 2018

miq-bot commented Dec 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simaishi commented Dec 11, 2018

jrafanie commented Dec 6, 2018 •

edited

Loading

carbonin Dec 6, 2018 •

edited

Loading