From 9c86b54bd8392994907262a22c5f0a88717f4c3d Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Thu, 25 Jul 2019 19:53:48 -0500 Subject: [PATCH 1/5] [ansible_runner] Update status on repo create When a `EmbeddedAnsible::AutomationManager::ConfigurationScriptSource` is created and has been sync'd, have it update the `.status` field to be "successful", matching the `awx` results. --- .../automation_manager/configuration_script_source.rb | 2 +- .../automation_manager/configuration_script_source_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb index 267deab0900..4106bd8c35f 100644 --- a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb +++ b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb @@ -60,7 +60,7 @@ def sync configuration_script_payloads.reload end - true + update_attributes!(:status => "successful") end def sync_queue(auth_user = nil) diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb index b371fea5880..5ad4839295b 100644 --- a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb +++ b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb @@ -88,6 +88,7 @@ def files_in_repository(git_repo_dir) expect(result).to be_an(described_class) expect(result.scm_type).to eq("git") expect(result.scm_branch).to eq("master") + expect(result.status).to eq("successful") git_repo_dir = repo_dir.join(result.git_repository.id.to_s) expect(files_in_repository(git_repo_dir)).to eq ["hello_world.yaml"] From 6ea399a8c13b3556efca8c153c2ff75c4475b38e Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Thu, 25 Jul 2019 20:41:18 -0500 Subject: [PATCH 2/5] [ansible_runner] Default repo status is 'new' To simulate ansible/awx, set `.status` for ConfigurationScriptSource to 'new' when a new EmbeddedAnsible repo is first created. To do this, `CrudCommon.create_in_provider` was overwritten to append the `sync` action to the `super` call (instead of in `raw_create_in_provider`. Since these are two atomic actions, and a `Rugged::NetworkError` shouldn't cause the whole process to fail, splitting the code up like this allows a creation to happen, but it to fail on the sync without the record being lost and deleted in the process. As a result, there are a minor amount of extra DB calls in a successful use case, but it means that a user isn't required to resubmit the form fields to try again and can just do a provider refresh when there is a failure to re-attempt a sync. --- .../configuration_script_source.rb | 11 ++++++++++- .../configuration_script_source_spec.rb | 19 ++++++++++++++++++- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb index 4106bd8c35f..0137e451356 100644 --- a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb +++ b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb @@ -24,7 +24,15 @@ def self.raw_create_in_provider(manager, params) params.delete(:scm_type) if params[:scm_type].blank? params.delete(:scm_branch) if params[:scm_branch].blank? - transaction { create!(params.merge(:manager => manager)).tap(&:sync) } + transaction { create!(params.merge(:manager => manager, :status => "new")) } + end + + def self.create_in_provider(manager_id, params) + super.tap do |repo| + notify("syncing", manager_id, {}) do + repo.sync + end + end end def raw_update_in_provider(params) @@ -48,6 +56,7 @@ def git_repository end def sync + update_attributes!(:status => "running") transaction do current = configuration_script_payloads.index_by(&:name) diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb index 5ad4839295b..b1f16c43a31 100644 --- a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb +++ b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb @@ -82,6 +82,7 @@ def files_in_repository(git_repo_dir) context "with valid params" do it "creates a record and initializes a git repo" do expect(Notification).to receive(:create!).with(notify_creation_args) + expect(Notification).to receive(:create!).with(notification_args("syncing", {})) result = described_class.create_in_provider(manager.id, params) @@ -93,6 +94,22 @@ def files_in_repository(git_repo_dir) git_repo_dir = repo_dir.join(result.git_repository.id.to_s) expect(files_in_repository(git_repo_dir)).to eq ["hello_world.yaml"] end + + # NOTE: Second `.notify` stub below prevents `.sync` from getting fired + it "sets the status to 'new' on create" do + expect(Notification).to receive(:create!).with(notify_creation_args) + expect(described_class).to receive(:notify).with(any_args).and_call_original + expect(described_class).to receive(:notify).with("syncing", any_args).and_return(true) + + result = described_class.create_in_provider(manager.id, params) + + expect(result).to be_an(described_class) + expect(result.scm_type).to eq("git") + expect(result.scm_branch).to eq("master") + expect(result.status).to eq("new") + + expect(repos).to be_empty + end end context "with invalid params" do @@ -217,7 +234,7 @@ def files_in_repository(git_repo_dir) end def build_record - expect(Notification).to receive(:create!).with(any_args) + expect(Notification).to receive(:create!).with(any_args).twice described_class.create_in_provider manager.id, params end From c5ad60cd6b43ecb96bf274f37f96631f14f29d89 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Thu, 25 Jul 2019 20:59:45 -0500 Subject: [PATCH 3/5] [ansible_runner] Update status on repo#sync errors Provides "error" status when a `ConfigurationScriptSource#sync` fails. --- .../configuration_script_source.rb | 3 +++ .../configuration_script_source_spec.rb | 24 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb index 0137e451356..c0d7852f95b 100644 --- a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb +++ b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb @@ -70,6 +70,9 @@ def sync configuration_script_payloads.reload end update_attributes!(:status => "successful") + rescue => error + update_attributes!(:status => "error") + raise error end def sync_queue(auth_user = nil) diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb index b1f16c43a31..7efed17acac 100644 --- a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb +++ b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb @@ -127,6 +127,30 @@ def files_in_repository(git_repo_dir) expect(repos).to be_empty end end + + context "when there is a network error fetching the repo" do + it "sets the status to 'error' if syncing has a network error" do + sync_notification_args = notification_args("syncing", {}) + sync_notification_args[:type] = :tower_op_failure + + expect(Notification).to receive(:create!).with(notify_creation_args) + expect(Notification).to receive(:create!).with(sync_notification_args) + expect(GitRepository).to receive(:create!).and_raise(::Rugged::NetworkError) + + expect do + described_class.create_in_provider(manager.id, params) + end.to raise_error(::Rugged::NetworkError) + + result = described_class.last + + expect(result).to be_an(described_class) + expect(result.scm_type).to eq("git") + expect(result.scm_branch).to eq("master") + expect(result.status).to eq("error") + + expect(repos).to be_empty + end + end end describe ".create_in_provider_queue" do From c4b8d93ff4524d346874d1ccc449b634d2c24580 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Thu, 25 Jul 2019 21:19:32 -0500 Subject: [PATCH 4/5] [ansible_runner] Update last_update_error repo Unlike the ansible_tower provider, we haven't been implementing the `last_update_error` and `last_updated_on` values when syncing. This does the following: - Always updates `#last_updated_on` when completing a `#sync` - On error: Formats and sets `#last_update_error` - On success: Removes any previous errors from `#last_update_error` The ERROR_MAX_SIZE value is pulled from the `AnsibleTower` provider. --- .../configuration_script_source.rb | 16 ++++++++++-- .../configuration_script_source_spec.rb | 25 ++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb index c0d7852f95b..0407a93f654 100644 --- a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb +++ b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb @@ -69,9 +69,13 @@ def sync configuration_script_payloads.reload end - update_attributes!(:status => "successful") + update_attributes!(:status => "successful", + :last_updated_on => Time.zone.now, + :last_update_error => nil) rescue => error - update_attributes!(:status => "error") + update_attributes!(:status => "error", + :last_updated_on => Time.zone.now, + :last_update_error => format_sync_error(error)) raise error end @@ -88,4 +92,12 @@ def checkout_git_repository(target_directory) git_repository.update_repo git_repository.checkout(scm_branch, target_directory) end + + ERROR_MAX_SIZE = 50.kilobytes + def format_sync_error(error) + result = error.message.dup + result << "\n\n" + result << error.backtrace.join("\n") + result.mb_chars.limit(ERROR_MAX_SIZE) + end end diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb index 7efed17acac..82836a1ee05 100644 --- a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb +++ b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb @@ -90,6 +90,8 @@ def files_in_repository(git_repo_dir) expect(result.scm_type).to eq("git") expect(result.scm_branch).to eq("master") expect(result.status).to eq("successful") + expect(result.last_updated_on).to be_an(Time) + expect(result.last_update_error).to be_nil git_repo_dir = repo_dir.join(result.git_repository.id.to_s) expect(files_in_repository(git_repo_dir)).to eq ["hello_world.yaml"] @@ -107,6 +109,8 @@ def files_in_repository(git_repo_dir) expect(result.scm_type).to eq("git") expect(result.scm_branch).to eq("master") expect(result.status).to eq("new") + expect(result.last_updated_on).to be_nil + expect(result.last_update_error).to be_nil expect(repos).to be_empty end @@ -129,7 +133,7 @@ def files_in_repository(git_repo_dir) end context "when there is a network error fetching the repo" do - it "sets the status to 'error' if syncing has a network error" do + before do sync_notification_args = notification_args("syncing", {}) sync_notification_args[:type] = :tower_op_failure @@ -140,16 +144,35 @@ def files_in_repository(git_repo_dir) expect do described_class.create_in_provider(manager.id, params) end.to raise_error(::Rugged::NetworkError) + end + it "sets the status to 'error' if syncing has a network error" do result = described_class.last expect(result).to be_an(described_class) expect(result.scm_type).to eq("git") expect(result.scm_branch).to eq("master") expect(result.status).to eq("error") + expect(result.last_updated_on).to be_an(Time) + expect(result.last_update_error).to start_with("Rugged::NetworkError") expect(repos).to be_empty end + + it "clears last_update_error on re-sync" do + result = described_class.last + + expect(result.status).to eq("error") + expect(result.last_updated_on).to be_an(Time) + expect(result.last_update_error).to start_with("Rugged::NetworkError") + + expect(GitRepository).to receive(:create!).and_call_original + + result.sync + + expect(result.status).to eq("successful") + expect(result.last_update_error).to be_nil + end end end From 1738d6066606f2a642d0c4a44122ef68f32bb9c5 Mon Sep 17 00:00:00 2001 From: Nick LaMuro Date: Fri, 26 Jul 2019 22:44:09 -0500 Subject: [PATCH 5/5] [ansible_runner] Update status on repo#update_* Splits up the actions for updating the database and refreshing the git repository into two atomic actions, similar to repo#create_in_provider. Also, to slightly DRY things up a bit, #sync_and_notify was added as well and was used in both the create and update methods. --- .../configuration_script_source.rb | 15 ++++--- .../configuration_script_source_spec.rb | 45 +++++++++++++++++++ 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb index 0407a93f654..e126aab9d29 100644 --- a/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb +++ b/app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb @@ -28,20 +28,19 @@ def self.raw_create_in_provider(manager, params) end def self.create_in_provider(manager_id, params) - super.tap do |repo| - notify("syncing", manager_id, {}) do - repo.sync - end - end + super.tap(&:sync_and_notify) end def raw_update_in_provider(params) transaction do update_attributes!(params.except(:task_id, :miq_task_id)) - sync end end + def update_in_provider(params) + super.tap(&:sync_and_notify) + end + def raw_delete_in_provider destroy! end @@ -79,6 +78,10 @@ def sync raise error end + def sync_and_notify + notify("syncing") { sync } + end + def sync_queue(auth_user = nil) queue("sync", [], "Synchronizing", auth_user) end diff --git a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb index 82836a1ee05..91610bde326 100644 --- a/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb +++ b/spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb @@ -201,6 +201,7 @@ def files_in_repository(git_repo_dir) record = build_record expect(Notification).to receive(:create!).with(notify_update_args) + expect(Notification).to receive(:create!).with(notification_args("syncing", {})) result = record.update_in_provider update_params @@ -226,6 +227,50 @@ def files_in_repository(git_repo_dir) end.to raise_error(ActiveRecord::RecordInvalid) end end + + context "when there is a network error fetching the repo" do + before do + record = build_record + + sync_notification_args = notification_args("syncing", {}) + sync_notification_args[:type] = :tower_op_failure + + expect(Notification).to receive(:create!).with(notify_update_args) + expect(Notification).to receive(:create!).with(sync_notification_args) + expect(record.git_repository).to receive(:update_repo).and_raise(::Rugged::NetworkError) + + expect do + # described_class.last.update_in_provider update_params + record.update_in_provider update_params + end.to raise_error(::Rugged::NetworkError) + end + + it "sets the status to 'error' if syncing has a network error" do + result = described_class.last + + expect(result).to be_an(described_class) + expect(result.scm_type).to eq("git") + expect(result.scm_branch).to eq("other_branch") + expect(result.status).to eq("error") + expect(result.last_updated_on).to be_an(Time) + expect(result.last_update_error).to start_with("Rugged::NetworkError") + end + + it "clears last_update_error on re-sync" do + result = described_class.last + + expect(result.status).to eq("error") + expect(result.last_updated_on).to be_an(Time) + expect(result.last_update_error).to start_with("Rugged::NetworkError") + + expect(result.git_repository).to receive(:update_repo).and_call_original + + result.sync + + expect(result.status).to eq("successful") + expect(result.last_update_error).to be_nil + end + end end describe "#update_in_provider_queue" do