From a38d4a6ec0a9bac9cff1bace1db12642f388929c Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Wed, 22 Oct 2025 19:40:13 +0000 Subject: [PATCH 1/9] Fixes #38856 - chain composite CV publishes to wait on children --- .../actions/katello/content_view/publish.rb | 6 +- .../incremental_update.rb | 2 +- app/models/katello/content_view_version.rb | 101 +++++++- .../events/auto_publish_composite_view.rb | 7 + config/initializers/foreman_tasks_chaining.rb | 44 ++++ lib/katello/engine.rb | 2 +- test/actions/katello/content_view_test.rb | 30 ++- .../content_view_version_auto_publish_test.rb | 237 ++++++++++++++++++ 8 files changed, 409 insertions(+), 20 deletions(-) create mode 100644 config/initializers/foreman_tasks_chaining.rb create mode 100644 test/models/content_view_version_auto_publish_test.rb diff --git a/app/lib/actions/katello/content_view/publish.rb b/app/lib/actions/katello/content_view/publish.rb index f23eb70d666..0954fc02e0e 100644 --- a/app/lib/actions/katello/content_view/publish.rb +++ b/app/lib/actions/katello/content_view/publish.rb @@ -43,7 +43,7 @@ def plan(content_view, description = "", options = {importing: false, syncable: :action => ::Katello::ContentViewHistory.actions[:publish], :task => self.task, :notes => description, - :triggered_by => options[:triggered_by] + :triggered_by_id => options[:triggered_by_id] || options[:triggered_by]&.id ) source_repositories = [] content_view.publish_repositories(options[:override_components]) do |repositories| @@ -113,7 +113,9 @@ def humanized_name def run version = ::Katello::ContentViewVersion.find(input[:content_view_version_id]) - version.auto_publish_composites! + # Pass the current task's execution plan ID so auto_publish can coordinate + # with other component CV publishes using Dynflow chaining + version.auto_publish_composites!(task.external_id) output[:content_view_id] = input[:content_view_id] output[:content_view_version_id] = input[:content_view_version_id] diff --git a/app/lib/actions/katello/content_view_version/incremental_update.rb b/app/lib/actions/katello/content_view_version/incremental_update.rb index de56a89103e..cb16693f604 100644 --- a/app/lib/actions/katello/content_view_version/incremental_update.rb +++ b/app/lib/actions/katello/content_view_version/incremental_update.rb @@ -275,7 +275,7 @@ def finalize end if version.latest? && !version.content_view.composite? - version.auto_publish_composites! + version.auto_publish_composites!(task.external_id) end end diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 99f6b03ebc3..727a89fde1e 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -361,18 +361,105 @@ def update_content_counts! save! end - def auto_publish_composites! - metadata = { - description: _("Auto Publish - Triggered by '%s'") % self.name, - triggered_by: self.id, - } + def auto_publish_composites!(component_task_id) + description = _("Auto Publish - Triggered by '%s'") % self.name + self.content_view.auto_publish_components.pluck(:composite_content_view_id).each do |composite_id| - ::Katello::EventQueue.push_event(::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, composite_id) do |attrs| - attrs[:metadata] = metadata + composite_cv = ::Katello::ContentView.find(composite_id) + + # Find all currently running publish tasks for sibling component CVs + # that belong to this composite CV + sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) + + # Also find any currently running composite CV publish tasks + composite_task_ids = find_composite_publish_tasks(composite_cv) + + # Combine all tasks we need to wait for + # Composite tasks first, then component tasks - this ensures we wait for + # any already-running composite publish before waiting for component siblings + all_task_ids = (composite_task_ids + sibling_task_ids).uniq + + begin + # Only use chaining if there are other tasks to wait for + # (current task is excluded from sibling_task_ids, so > 0 means there are siblings or composite tasks) + if all_task_ids.any? + # Chain the composite publish to wait for all sibling component publishes and composite publishes + # This ensures all component CVs finish publishing and any prior composite publishes complete + # before the new composite publish starts + ForemanTasks.chain( + all_task_ids, + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + else + # No siblings or composite tasks currently running, trigger composite publish immediately + ForemanTasks.async_task( + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + end + rescue ForemanTasks::Lock::LockConflict => e + # Composite publish already scheduled/running - this is expected when + # multiple component CVs finish around the same time + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + rescue StandardError => e + Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise e end end end + private + + # Find all currently running publish tasks for component CVs that belong to the given composite CV + # @param composite_cv [Katello::ContentView] The composite content view + # @param current_task_id [String] The execution plan ID of the current component publish task + # @return [Array] Array of execution plan IDs for all running component publish tasks + def find_sibling_component_publish_tasks(composite_cv, current_task_id) + # Get all component CV IDs for this composite + component_cv_ids = composite_cv.components.pluck(:content_view_id) + + # Find all currently running publish tasks for these component CVs + running_tasks = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: ['planning', 'planned', 'running']) + .select do |task| + # Check if task is publishing one of the component CVs + task_input = task.input + task_input && component_cv_ids.include?(task_input.dig('content_view', 'id')) + end + + task_ids = running_tasks.map(&:external_id) + + # Exclude the current task - if we're running this code, the current task + # has already reached its Finalize step and doesn't need to be waited for + task_ids.reject { |id| id == current_task_id } + end + + # Find all currently running publish tasks for the composite CV itself + # @param composite_cv [Katello::ContentView] The composite content view + # @return [Array] Array of execution plan IDs for all running composite publish tasks + def find_composite_publish_tasks(composite_cv) + running_tasks = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: ['planning', 'planned', 'running']) + .select do |task| + # Check if task is publishing the composite CV + task_input = task.input + task_input && task_input.dig('content_view', 'id') == composite_cv.id + end + + running_tasks.map(&:external_id) + end + + public + def repository_type_counts_map counts = {} Katello::RepositoryTypeManager.enabled_repository_types.keys.each do |repo_type| diff --git a/app/models/katello/events/auto_publish_composite_view.rb b/app/models/katello/events/auto_publish_composite_view.rb index 1090c1431bb..713c20e37ea 100644 --- a/app/models/katello/events/auto_publish_composite_view.rb +++ b/app/models/katello/events/auto_publish_composite_view.rb @@ -1,5 +1,12 @@ module Katello module Events + # DEPRECATED: This event class is no longer used after implementing Dynflow chaining + # for auto-publish composite views. EventQueue.push_event calls have been removed. + # This class is kept temporarily for reference but should be removed in a future release + # along with: + # - test/models/events/auto_publish_composite_view_test.rb + # - EventQueue registration in lib/katello/engine.rb (already commented out) + # See: ContentViewVersion#auto_publish_composites! which now uses ForemanTasks.chain class AutoPublishCompositeView EVENT_TYPE = 'auto_publish_composite_view'.freeze diff --git a/config/initializers/foreman_tasks_chaining.rb b/config/initializers/foreman_tasks_chaining.rb new file mode 100644 index 00000000000..c79805dd9d2 --- /dev/null +++ b/config/initializers/foreman_tasks_chaining.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +# FIXME: This monkey-patch adds execution plan chaining support to ForemanTasks. +# This should be submitted upstream to the foreman-tasks gem and removed from here +# once it's available in a released version. +# +# See: https://github.com/Dynflow/dynflow/pull/446 + +# Defer extension until after ForemanTasks module is loaded +Rails.application.config.to_prepare do + module ForemanTasks + # Chain execution plans so that a new plan waits until prerequisite plans finish before executing. + # This is useful for coordinating dependent tasks where one task should only run after + # other tasks have completed successfully. + # + # The chained plan will remain in 'scheduled' state until all prerequisite plans + # reach 'stopped' state (regardless of success/failure). + # + # @param plan_uuids [String, Array] UUID(s) of prerequisite execution plan(s) + # @param action [Class] Action class to execute + # @param args Arguments to pass to the action + # @return [ForemanTasks::Task::DynflowTask] The chained task that will wait for prerequisites + # + # @example Chain a task to wait for another task + # task1 = ForemanTasks.async_task(SomeAction) + # task2 = ForemanTasks.chain(task1.external_id, AnotherAction, arg1, arg2) + # # task2 will only execute after task1 completes + # + # @example Chain a task to wait for multiple tasks + # task1 = ForemanTasks.async_task(Action1) + # task2 = ForemanTasks.async_task(Action2) + # task3 = ForemanTasks.chain([task1.external_id, task2.external_id], Action3) + # # task3 will only execute after both task1 and task2 complete + def self.chain(plan_uuids, action, *args) + result = dynflow.world.chain(plan_uuids, action, *args) + # The ForemanTasks record may not exist yet for delayed plans, + # so we need to find or create it + ForemanTasks::Task.find_by(:external_id => result.id) || + ForemanTasks::Task::DynflowTask.new(:external_id => result.id).tap do |task| + task.save! + end + end + end +end diff --git a/lib/katello/engine.rb b/lib/katello/engine.rb index e2193457383..edbc87ec765 100644 --- a/lib/katello/engine.rb +++ b/lib/katello/engine.rb @@ -223,7 +223,7 @@ class Engine < ::Rails::Engine load 'katello/scheduled_jobs.rb' Katello::EventQueue.register_event(Katello::Events::ImportPool::EVENT_TYPE, Katello::Events::ImportPool) - Katello::EventQueue.register_event(Katello::Events::AutoPublishCompositeView::EVENT_TYPE, Katello::Events::AutoPublishCompositeView) + # Katello::Events::AutoPublishCompositeView is obsolete - now uses ForemanTasks.chain() directly Katello::EventQueue.register_event(Katello::Events::DeleteLatestContentViewVersion::EVENT_TYPE, Katello::Events::DeleteLatestContentViewVersion) Katello::EventQueue.register_event(Katello::Events::GenerateHostApplicability::EVENT_TYPE, Katello::Events::GenerateHostApplicability) Katello::EventQueue.register_event(Katello::Events::DeletePool::EVENT_TYPE, Katello::Events::DeletePool) diff --git a/test/actions/katello/content_view_test.rb b/test/actions/katello/content_view_test.rb index cc1abec80d2..ab4f73c0aff 100644 --- a/test/actions/katello/content_view_test.rb +++ b/test/actions/katello/content_view_test.rb @@ -249,7 +249,7 @@ class PublishTest < TestBase end context 'run phase' do - it 'creates auto-publish events for non-composite views' do + it 'triggers auto-publish for composite views using chaining' do composite_view = katello_content_views(:composite_view) action.stubs(:task).returns(success_task) @@ -258,23 +258,35 @@ class PublishTest < TestBase composite_content_view: composite_view, content_view: content_view) + # Mock the task relation to simulate no other component tasks running + task_relation = mock('task_relation') + task_relation.expects(:select).returns([]) + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + + # Expect async_task to be called (no siblings, so no chaining) + ForemanTasks.expects(:async_task).with( + ::Actions::Katello::ContentView::Publish, + composite_view, + anything, + triggered_by: anything + ) + plan_action action, content_view run_action action - - event = Katello::Event.find_by(event_type: Katello::Events::AutoPublishCompositeView::EVENT_TYPE, object_id: composite_view.id) - version = content_view.versions.last - - assert_equal event.metadata[:triggered_by], version.id - assert_equal event.metadata[:description], "Auto Publish - Triggered by '#{version.name}'" end it 'does nothing for non-composite view' do action.stubs(:task).returns(success_task) + # Should not trigger any auto-publish tasks + ForemanTasks.expects(:async_task).never + ForemanTasks.expects(:chain).never + plan_action action, katello_content_views(:no_environment_view) run_action action - - assert_empty Katello::Event.all end end diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb new file mode 100644 index 00000000000..6f88dacd80a --- /dev/null +++ b/test/models/content_view_version_auto_publish_test.rb @@ -0,0 +1,237 @@ +require 'katello_test_helper' + +module Katello + class ContentViewVersionAutoPublishTest < ActiveSupport::TestCase + def setup + User.current = User.find(users(:admin).id) + @org = FactoryBot.create(:katello_organization) + + # Create two component content views + @component_cv1 = FactoryBot.create(:katello_content_view, :organization => @org, :name => "Component CV 1") + @component_cv2 = FactoryBot.create(:katello_content_view, :organization => @org, :name => "Component CV 2") + + # Create a composite content view with auto-publish enabled + @composite_cv = FactoryBot.create(:katello_content_view, + :organization => @org, + :composite => true, + :auto_publish => true, + :name => "Composite CV") + + # Add components to composite + @component1_version = FactoryBot.create(:katello_content_view_version, + :content_view => @component_cv1, + :major => 1, + :minor => 0) + @component2_version = FactoryBot.create(:katello_content_view_version, + :content_view => @component_cv2, + :major => 1, + :minor => 0) + + # For latest: true, set content_view (not content_view_version) + # Validation requires: either (latest=true + content_view) OR (content_view_version) + FactoryBot.create(:katello_content_view_component, + :composite_content_view => @composite_cv, + :content_view => @component_cv1, + :latest => true) + FactoryBot.create(:katello_content_view_component, + :composite_content_view => @composite_cv, + :content_view => @component_cv2, + :latest => true) + end + + def test_auto_publish_with_no_sibling_tasks_triggers_immediately + task_id = SecureRandom.uuid + + # Mock that no other tasks are running + task_relation = mock('task_relation') + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice + task_relation.expects(:select).returns([]).twice # No component or composite tasks + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + .twice # Once for component check, once for composite check + + # Should trigger async_task since no siblings are running + ForemanTasks.expects(:async_task).with( + ::Actions::Katello::ContentView::Publish, + @composite_cv, + anything, + triggered_by_id: @component1_version.id + ).returns(stub(id: SecureRandom.uuid)) + + # Should not call chain + ForemanTasks.expects(:chain).never + + @component1_version.auto_publish_composites!(task_id) + end + + def test_auto_publish_with_sibling_tasks_uses_chaining + task_id1 = SecureRandom.uuid + task_id2 = SecureRandom.uuid + + # Create mock running task for sibling component + sibling_task = mock('sibling_task') + sibling_task.stubs(:external_id).returns(task_id2) + sibling_task.stubs(:input).returns({ + 'content_view' => { 'id' => @component_cv2.id } + }) + + # Mock that sibling task is running + task_relation = mock('task_relation') + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice + task_relation.expects(:select).returns([sibling_task]).once # component check - sibling found + task_relation.expects(:select).returns([]).once # composite check - no composite tasks + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + .twice + + # Should use chain since sibling is running + # Current task is excluded, only sibling remains + # Order: composite_task_ids (empty) + sibling_task_ids ([sibling only, current excluded]) + ForemanTasks.expects(:chain).with( + [task_id2], # only sibling task, current task excluded + ::Actions::Katello::ContentView::Publish, + @composite_cv, + anything, + triggered_by_id: @component1_version.id + ).returns(stub(id: SecureRandom.uuid)) + + # Should not call async_task + ForemanTasks.expects(:async_task).never + + @component1_version.auto_publish_composites!(task_id1) + end + + def test_auto_publish_waits_for_running_composite_publish + task_id = SecureRandom.uuid + composite_task_id = SecureRandom.uuid + + # Create mock running composite publish task + composite_task = mock('composite_task') + composite_task.stubs(:external_id).returns(composite_task_id) + composite_task.stubs(:input).returns({ + 'content_view' => { 'id' => @composite_cv.id } + }) + + # Mock that no component tasks are running, but composite is + task_relation = mock('task_relation') + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice + task_relation.expects(:select).returns([]).once # No component tasks + task_relation.expects(:select).returns([composite_task]).once # Composite task running + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + .twice + + # Should use chain to wait for composite task to finish + # Order: composite_task_ids + sibling_task_ids (empty, current excluded) + ForemanTasks.expects(:chain).with( + [composite_task_id], # only composite task + ::Actions::Katello::ContentView::Publish, + @composite_cv, + anything, + triggered_by_id: @component1_version.id + ).returns(stub(id: SecureRandom.uuid)) + + # Should not call async_task + ForemanTasks.expects(:async_task).never + + @component1_version.auto_publish_composites!(task_id) + end + + def test_auto_publish_handles_lock_conflict_gracefully + task_id = SecureRandom.uuid + + # Mock that no other tasks are running + task_relation = mock('task_relation') + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice + task_relation.expects(:select).returns([]).twice # No component or composite tasks + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + .twice + + # Simulate lock conflict (composite already being published) + # LockConflict needs 2 args: required_lock and conflicting_locks + # The conflicting locks need to respond to .task for error message generation + lock = mock('required_lock') + conflicting_task = mock('conflicting_task') + conflicting_task.stubs(:id).returns(123) + conflicting_lock = mock('conflicting_lock') + conflicting_lock.stubs(:task).returns(conflicting_task) + + ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(lock, [conflicting_lock])) + + # Should deliver failure notification but not raise + ::Katello::UINotifications::ContentView::AutoPublishFailure.expects(:deliver!).with(@composite_cv) + + # Should not raise exception + assert_nothing_raised do + @component1_version.auto_publish_composites!(task_id) + end + end + + def test_find_sibling_component_publish_tasks_finds_running_tasks + task_id1 = SecureRandom.uuid + task_id2 = SecureRandom.uuid + + # Create mock running tasks + task1 = mock('task1') + task1.stubs(:external_id).returns(task_id1) + task1.stubs(:input).returns({ 'content_view' => { 'id' => @component_cv1.id } }) + + task2 = mock('task2') + task2.stubs(:external_id).returns(task_id2) + task2.stubs(:input).returns({ 'content_view' => { 'id' => @component_cv2.id } }) + + task_relation = mock('task_relation') + task_relation.expects(:select).returns([task1, task2]) + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + + current_task_id = SecureRandom.uuid + result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, current_task_id) + + # Should include both sibling tasks but exclude current task + assert_equal 2, result.length + assert_includes result, task_id1 + assert_includes result, task_id2 + assert_not_includes result, current_task_id + end + + def test_find_sibling_tasks_excludes_non_component_tasks + task_id = SecureRandom.uuid + + # Create mock task for a different CV (not a component) + other_cv = FactoryBot.create(:katello_content_view, :organization => @org) + other_task = mock('other_task') + other_task.stubs(:external_id).returns(SecureRandom.uuid) + other_task.stubs(:input).returns({ 'content_view' => { 'id' => other_cv.id } }) + + task_relation = mock('task_relation') + # The select block will filter out other_task because other_cv.id is not in component_cv_ids + # So we return empty array + task_relation.expects(:select).returns([]) + + ForemanTasks::Task::DynflowTask.expects(:for_action) + .with(::Actions::Katello::ContentView::Publish) + .returns(task_relation) + + task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + + result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, task_id) + + # Should exclude current task and other CV's task + assert_equal [], result + end + end +end From f95740f17f673a3ac935bc586733e0a9344f6315 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Wed, 22 Oct 2025 22:06:19 +0000 Subject: [PATCH 2/9] Refs #38856 - do not run ccv publish unnecessary times --- app/models/katello/content_view_version.rb | 100 +++++++------ config/initializers/foreman_tasks_chaining.rb | 10 +- .../content_view_version_auto_publish_test.rb | 139 ++++-------------- 3 files changed, 93 insertions(+), 156 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 727a89fde1e..ac9e999828b 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -367,50 +367,58 @@ def auto_publish_composites!(component_task_id) self.content_view.auto_publish_components.pluck(:composite_content_view_id).each do |composite_id| composite_cv = ::Katello::ContentView.find(composite_id) - # Find all currently running publish tasks for sibling component CVs - # that belong to this composite CV - sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) - - # Also find any currently running composite CV publish tasks - composite_task_ids = find_composite_publish_tasks(composite_cv) - - # Combine all tasks we need to wait for - # Composite tasks first, then component tasks - this ensures we wait for - # any already-running composite publish before waiting for component siblings - all_task_ids = (composite_task_ids + sibling_task_ids).uniq - - begin - # Only use chaining if there are other tasks to wait for - # (current task is excluded from sibling_task_ids, so > 0 means there are siblings or composite tasks) - if all_task_ids.any? - # Chain the composite publish to wait for all sibling component publishes and composite publishes - # This ensures all component CVs finish publishing and any prior composite publishes complete - # before the new composite publish starts - ForemanTasks.chain( - all_task_ids, - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) - else - # No siblings or composite tasks currently running, trigger composite publish immediately - ForemanTasks.async_task( - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) + # Use a database-level advisory lock to prevent race conditions when multiple + # component CVs finish simultaneously and try to create composite publishes + # The lock is automatically released at the end of the transaction + composite_cv.with_lock do + # Find all currently running publish tasks for sibling component CVs + # that belong to this composite CV + sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) + + # Also find any currently running or scheduled composite CV publish tasks + composite_task_ids = find_composite_publish_tasks(composite_cv) + + # If a composite publish is already scheduled or running, skip creating another one + # The existing publish will see the latest component versions when it runs + if composite_task_ids.any? + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled/running, skipping duplicate") + next + end + + # Combine all tasks we need to wait for (only sibling tasks, since composite_task_ids is empty) + all_task_ids = sibling_task_ids.uniq + + begin + # Only use chaining if there are sibling tasks to wait for + if all_task_ids.any? + # Chain the composite publish to wait for all sibling component publishes + # This ensures all component CVs finish publishing before the composite publish starts + ForemanTasks.chain( + all_task_ids, + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + else + # No siblings currently running, trigger composite publish immediately + ForemanTasks.async_task( + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + end + rescue ForemanTasks::Lock::LockConflict => e + # Composite publish already scheduled/running - this is expected when + # multiple component CVs finish around the same time + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + rescue StandardError => e + Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise e end - rescue ForemanTasks::Lock::LockConflict => e - # Composite publish already scheduled/running - this is expected when - # multiple component CVs finish around the same time - Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled: #{e.message}") - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - rescue StandardError => e - Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - raise e end end end @@ -442,13 +450,13 @@ def find_sibling_component_publish_tasks(composite_cv, current_task_id) task_ids.reject { |id| id == current_task_id } end - # Find all currently running publish tasks for the composite CV itself + # Find all currently running or scheduled publish tasks for the composite CV itself # @param composite_cv [Katello::ContentView] The composite content view - # @return [Array] Array of execution plan IDs for all running composite publish tasks + # @return [Array] Array of execution plan IDs for all running/scheduled composite publish tasks def find_composite_publish_tasks(composite_cv) running_tasks = ForemanTasks::Task::DynflowTask .for_action(::Actions::Katello::ContentView::Publish) - .where(state: ['planning', 'planned', 'running']) + .where(state: ['scheduled', 'planning', 'planned', 'running']) .select do |task| # Check if task is publishing the composite CV task_input = task.input diff --git a/config/initializers/foreman_tasks_chaining.rb b/config/initializers/foreman_tasks_chaining.rb index c79805dd9d2..17935408168 100644 --- a/config/initializers/foreman_tasks_chaining.rb +++ b/config/initializers/foreman_tasks_chaining.rb @@ -34,10 +34,14 @@ module ForemanTasks def self.chain(plan_uuids, action, *args) result = dynflow.world.chain(plan_uuids, action, *args) # The ForemanTasks record may not exist yet for delayed plans, - # so we need to find or create it + # so we need to find or create it and properly initialize it ForemanTasks::Task.find_by(:external_id => result.id) || - ForemanTasks::Task::DynflowTask.new(:external_id => result.id).tap do |task| - task.save! + begin + delayed_plan = dynflow.world.persistence.load_delayed_plan(result.id) + execution_plan = dynflow.world.persistence.load_execution_plan(result.id) + ForemanTasks::Task::DynflowTask.new(:external_id => result.id).tap do |task| + task.update_from_dynflow(execution_plan, delayed_plan) + end end end end diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index 6f88dacd80a..e0a76e4ec3c 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -42,15 +42,8 @@ def setup def test_auto_publish_with_no_sibling_tasks_triggers_immediately task_id = SecureRandom.uuid - # Mock that no other tasks are running - task_relation = mock('task_relation') - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice - task_relation.expects(:select).returns([]).twice # No component or composite tasks - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - .twice # Once for component check, once for composite check + # Stub to return no tasks + ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(ForemanTasks::Task.none) # Should trigger async_task since no siblings are running ForemanTasks.expects(:async_task).with( @@ -71,73 +64,41 @@ def test_auto_publish_with_sibling_tasks_uses_chaining task_id2 = SecureRandom.uuid # Create mock running task for sibling component - sibling_task = mock('sibling_task') - sibling_task.stubs(:external_id).returns(task_id2) - sibling_task.stubs(:input).returns({ - 'content_view' => { 'id' => @component_cv2.id } - }) - - # Mock that sibling task is running - task_relation = mock('task_relation') - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice - task_relation.expects(:select).returns([sibling_task]).once # component check - sibling found - task_relation.expects(:select).returns([]).once # composite check - no composite tasks - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - .twice + sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) + + # First call (sibling check) returns sibling, second call (composite check) returns nothing + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: stub(select: [sibling_task]))) + .then.returns(stub(where: stub(select: []))) # Should use chain since sibling is running - # Current task is excluded, only sibling remains - # Order: composite_task_ids (empty) + sibling_task_ids ([sibling only, current excluded]) ForemanTasks.expects(:chain).with( - [task_id2], # only sibling task, current task excluded + [task_id2], ::Actions::Katello::ContentView::Publish, @composite_cv, anything, triggered_by_id: @component1_version.id ).returns(stub(id: SecureRandom.uuid)) - # Should not call async_task ForemanTasks.expects(:async_task).never @component1_version.auto_publish_composites!(task_id1) end - def test_auto_publish_waits_for_running_composite_publish + def test_auto_publish_skips_when_composite_already_scheduled task_id = SecureRandom.uuid composite_task_id = SecureRandom.uuid - # Create mock running composite publish task - composite_task = mock('composite_task') - composite_task.stubs(:external_id).returns(composite_task_id) - composite_task.stubs(:input).returns({ - 'content_view' => { 'id' => @composite_cv.id } - }) - - # Mock that no component tasks are running, but composite is - task_relation = mock('task_relation') - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice - task_relation.expects(:select).returns([]).once # No component tasks - task_relation.expects(:select).returns([composite_task]).once # Composite task running - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - .twice - - # Should use chain to wait for composite task to finish - # Order: composite_task_ids + sibling_task_ids (empty, current excluded) - ForemanTasks.expects(:chain).with( - [composite_task_id], # only composite task - ::Actions::Katello::ContentView::Publish, - @composite_cv, - anything, - triggered_by_id: @component1_version.id - ).returns(stub(id: SecureRandom.uuid)) + # Create mock scheduled composite publish task + composite_task = stub(external_id: composite_task_id, input: { 'content_view' => { 'id' => @composite_cv.id } }) + + # First call (sibling check) returns nothing, second call (composite check) returns composite task + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: stub(select: []))) + .then.returns(stub(where: stub(select: [composite_task]))) - # Should not call async_task + # Should not create any new task + ForemanTasks.expects(:chain).never ForemanTasks.expects(:async_task).never @component1_version.auto_publish_composites!(task_id) @@ -146,31 +107,17 @@ def test_auto_publish_waits_for_running_composite_publish def test_auto_publish_handles_lock_conflict_gracefully task_id = SecureRandom.uuid - # Mock that no other tasks are running - task_relation = mock('task_relation') - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation).twice - task_relation.expects(:select).returns([]).twice # No component or composite tasks - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - .twice - - # Simulate lock conflict (composite already being published) - # LockConflict needs 2 args: required_lock and conflicting_locks - # The conflicting locks need to respond to .task for error message generation - lock = mock('required_lock') - conflicting_task = mock('conflicting_task') - conflicting_task.stubs(:id).returns(123) - conflicting_lock = mock('conflicting_lock') - conflicting_lock.stubs(:task).returns(conflicting_task) + # Stub to return no tasks + ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(ForemanTasks::Task.none) - ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(lock, [conflicting_lock])) + # Simulate lock conflict + lock = stub('required_lock') + conflicting_task = stub(id: 123) + conflicting_lock = stub(task: conflicting_task) - # Should deliver failure notification but not raise + ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(lock, [conflicting_lock])) ::Katello::UINotifications::ContentView::AutoPublishFailure.expects(:deliver!).with(@composite_cv) - # Should not raise exception assert_nothing_raised do @component1_version.auto_publish_composites!(task_id) end @@ -181,22 +128,10 @@ def test_find_sibling_component_publish_tasks_finds_running_tasks task_id2 = SecureRandom.uuid # Create mock running tasks - task1 = mock('task1') - task1.stubs(:external_id).returns(task_id1) - task1.stubs(:input).returns({ 'content_view' => { 'id' => @component_cv1.id } }) - - task2 = mock('task2') - task2.stubs(:external_id).returns(task_id2) - task2.stubs(:input).returns({ 'content_view' => { 'id' => @component_cv2.id } }) + task1 = stub(external_id: task_id1, input: { 'content_view' => { 'id' => @component_cv1.id } }) + task2 = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) - task_relation = mock('task_relation') - task_relation.expects(:select).returns([task1, task2]) - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: [task1, task2]))) current_task_id = SecureRandom.uuid result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, current_task_id) @@ -213,20 +148,10 @@ def test_find_sibling_tasks_excludes_non_component_tasks # Create mock task for a different CV (not a component) other_cv = FactoryBot.create(:katello_content_view, :organization => @org) - other_task = mock('other_task') - other_task.stubs(:external_id).returns(SecureRandom.uuid) - other_task.stubs(:input).returns({ 'content_view' => { 'id' => other_cv.id } }) - - task_relation = mock('task_relation') - # The select block will filter out other_task because other_cv.id is not in component_cv_ids - # So we return empty array - task_relation.expects(:select).returns([]) - - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) + other_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => other_cv.id } }) - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + # The select block will filter out other_task + ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: []))) result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, task_id) From 6b8b11596d4891e49d1bf556b3c6c2b466022e6b Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Thu, 23 Oct 2025 20:19:44 +0000 Subject: [PATCH 3/9] Refs #38856 - properly look up other scheduled CCV tasks --- app/models/katello/content_view_version.rb | 41 +++++++++++---- config/initializers/foreman_tasks_chaining.rb | 2 +- .../content_view_version_auto_publish_test.rb | 52 ++++++++++++++----- 3 files changed, 73 insertions(+), 22 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index ac9e999828b..24b9bf254cf 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -371,17 +371,38 @@ def auto_publish_composites!(component_task_id) # component CVs finish simultaneously and try to create composite publishes # The lock is automatically released at the end of the transaction composite_cv.with_lock do + # Check if there's already a scheduled delayed plan for this composite CV + # For scheduled tasks, input isn't populated yet, so we check the delayed plan's args + has_scheduled_composite_publish = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: 'scheduled') + .any? do |task| + begin + delayed_plan = ForemanTasks.dynflow.world.persistence.load_delayed_plan(task.external_id) + args = delayed_plan.args + # First arg is the content view - check if it matches our composite CV + args.first.is_a?(::Katello::ContentView) && args.first.id == composite_cv.id + rescue StandardError + false + end + end + + if has_scheduled_composite_publish + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled (delayed plan exists), skipping duplicate") + next + end + # Find all currently running publish tasks for sibling component CVs # that belong to this composite CV sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) - # Also find any currently running or scheduled composite CV publish tasks + # Also find any currently running composite CV publish tasks composite_task_ids = find_composite_publish_tasks(composite_cv) - # If a composite publish is already scheduled or running, skip creating another one - # The existing publish will see the latest component versions when it runs + # If a composite publish is already running, skip creating another one + # The existing publish will pick up the latest component versions if composite_task_ids.any? - Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled/running, skipping duplicate") + Rails.logger.info("Composite CV #{composite_cv.name} publish already running, skipping duplicate") next end @@ -450,20 +471,22 @@ def find_sibling_component_publish_tasks(composite_cv, current_task_id) task_ids.reject { |id| id == current_task_id } end - # Find all currently running or scheduled publish tasks for the composite CV itself + # Find all currently running composite publish tasks for the given composite CV + # NOTE: This does NOT check for scheduled tasks - those are handled separately in auto_publish_composites! + # by inspecting delayed plan args, since scheduled tasks don't have input populated yet. # @param composite_cv [Katello::ContentView] The composite content view - # @return [Array] Array of execution plan IDs for all running/scheduled composite publish tasks + # @return [Array] Array of execution plan IDs for all running composite publish tasks def find_composite_publish_tasks(composite_cv) - running_tasks = ForemanTasks::Task::DynflowTask + relevant_tasks = ForemanTasks::Task::DynflowTask .for_action(::Actions::Katello::ContentView::Publish) - .where(state: ['scheduled', 'planning', 'planned', 'running']) + .where(state: ['planning', 'planned', 'running']) .select do |task| # Check if task is publishing the composite CV task_input = task.input task_input && task_input.dig('content_view', 'id') == composite_cv.id end - running_tasks.map(&:external_id) + relevant_tasks.map(&:external_id) end public diff --git a/config/initializers/foreman_tasks_chaining.rb b/config/initializers/foreman_tasks_chaining.rb index 17935408168..c87631c1eb1 100644 --- a/config/initializers/foreman_tasks_chaining.rb +++ b/config/initializers/foreman_tasks_chaining.rb @@ -39,7 +39,7 @@ def self.chain(plan_uuids, action, *args) begin delayed_plan = dynflow.world.persistence.load_delayed_plan(result.id) execution_plan = dynflow.world.persistence.load_execution_plan(result.id) - ForemanTasks::Task::DynflowTask.new(:external_id => result.id).tap do |task| + ForemanTasks::Task::DynflowTask.new_for_execution_plan(execution_plan).tap do |task| task.update_from_dynflow(execution_plan, delayed_plan) end end diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index e0a76e4ec3c..ee491394f4d 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -42,8 +42,15 @@ def setup def test_auto_publish_with_no_sibling_tasks_triggers_immediately task_id = SecureRandom.uuid - # Stub to return no tasks - ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(ForemanTasks::Task.none) + # Stub to return no scheduled tasks, no sibling tasks, no composite tasks + scheduled_relation = stub(any?: false) + sibling_relation = stub(select: []) + composite_relation = stub(select: []) + + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: scheduled_relation)) # First: scheduled check + .then.returns(stub(where: sibling_relation)) # Second: sibling check + .then.returns(stub(where: composite_relation)) # Third: composite check # Should trigger async_task since no siblings are running ForemanTasks.expects(:async_task).with( @@ -66,10 +73,15 @@ def test_auto_publish_with_sibling_tasks_uses_chaining # Create mock running task for sibling component sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) - # First call (sibling check) returns sibling, second call (composite check) returns nothing + # Stub the three checks: scheduled, sibling, composite + scheduled_relation = stub(any?: false) + sibling_relation = stub(select: [sibling_task]) + composite_relation = stub(select: []) + ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(select: [sibling_task]))) - .then.returns(stub(where: stub(select: []))) + .returns(stub(where: scheduled_relation)) # First: scheduled check + .then.returns(stub(where: sibling_relation)) # Second: sibling check + .then.returns(stub(where: composite_relation)) # Third: composite check # Should use chain since sibling is running ForemanTasks.expects(:chain).with( @@ -89,13 +101,22 @@ def test_auto_publish_skips_when_composite_already_scheduled task_id = SecureRandom.uuid composite_task_id = SecureRandom.uuid - # Create mock scheduled composite publish task - composite_task = stub(external_id: composite_task_id, input: { 'content_view' => { 'id' => @composite_cv.id } }) + # Create mock scheduled composite publish task with delayed plan args + composite_task = stub(external_id: composite_task_id) + delayed_plan = stub(args: [@composite_cv, "description", {}]) + + # Mock the delayed plan lookup - need to allow the real dynflow world through + # but intercept the persistence.load_delayed_plan call + world_stub = ForemanTasks.dynflow.world + persistence_stub = stub(load_delayed_plan: delayed_plan) + world_stub.stubs(:persistence).returns(persistence_stub) + + # Stub scheduled check to return the composite task + scheduled_relation = mock + scheduled_relation.expects(:any?).yields(composite_task).returns(true) - # First call (sibling check) returns nothing, second call (composite check) returns composite task ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(select: []))) - .then.returns(stub(where: stub(select: [composite_task]))) + .returns(stub(where: scheduled_relation)) # Should not create any new task ForemanTasks.expects(:chain).never @@ -107,8 +128,15 @@ def test_auto_publish_skips_when_composite_already_scheduled def test_auto_publish_handles_lock_conflict_gracefully task_id = SecureRandom.uuid - # Stub to return no tasks - ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(ForemanTasks::Task.none) + # Stub to return no scheduled, sibling, or composite tasks + scheduled_relation = stub(any?: false) + sibling_relation = stub(select: []) + composite_relation = stub(select: []) + + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: scheduled_relation)) + .then.returns(stub(where: sibling_relation)) + .then.returns(stub(where: composite_relation)) # Simulate lock conflict lock = stub('required_lock') From 2e3a78938db6a156c34417f75a79424d1db91406 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Thu, 23 Oct 2025 21:16:09 +0000 Subject: [PATCH 4/9] Refs #38856 - refactor CCV chaining logic --- app/models/katello/content_view_version.rb | 145 +++++++++--------- .../content_view_version_auto_publish_test.rb | 43 ++---- 2 files changed, 86 insertions(+), 102 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 24b9bf254cf..4ebe49868ab 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -367,84 +367,91 @@ def auto_publish_composites!(component_task_id) self.content_view.auto_publish_components.pluck(:composite_content_view_id).each do |composite_id| composite_cv = ::Katello::ContentView.find(composite_id) - # Use a database-level advisory lock to prevent race conditions when multiple - # component CVs finish simultaneously and try to create composite publishes - # The lock is automatically released at the end of the transaction composite_cv.with_lock do - # Check if there's already a scheduled delayed plan for this composite CV - # For scheduled tasks, input isn't populated yet, so we check the delayed plan's args - has_scheduled_composite_publish = ForemanTasks::Task::DynflowTask - .for_action(::Actions::Katello::ContentView::Publish) - .where(state: 'scheduled') - .any? do |task| - begin - delayed_plan = ForemanTasks.dynflow.world.persistence.load_delayed_plan(task.external_id) - args = delayed_plan.args - # First arg is the content view - check if it matches our composite CV - args.first.is_a?(::Katello::ContentView) && args.first.id == composite_cv.id - rescue StandardError - false - end - end - - if has_scheduled_composite_publish - Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled (delayed plan exists), skipping duplicate") - next - end + next if composite_publish_already_exists?(composite_cv) - # Find all currently running publish tasks for sibling component CVs - # that belong to this composite CV sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) + trigger_composite_publish(composite_cv, sibling_task_ids, description) + end + end + end - # Also find any currently running composite CV publish tasks - composite_task_ids = find_composite_publish_tasks(composite_cv) + private - # If a composite publish is already running, skip creating another one - # The existing publish will pick up the latest component versions - if composite_task_ids.any? - Rails.logger.info("Composite CV #{composite_cv.name} publish already running, skipping duplicate") - next - end + # Check if a composite publish task already exists (scheduled or running) + # @param composite_cv [Katello::ContentView] The composite content view + # @return [Boolean] true if a publish task already exists + def composite_publish_already_exists?(composite_cv) + # Check scheduled tasks first (they don't have input populated yet) + scheduled_tasks = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: 'scheduled') - # Combine all tasks we need to wait for (only sibling tasks, since composite_task_ids is empty) - all_task_ids = sibling_task_ids.uniq - - begin - # Only use chaining if there are sibling tasks to wait for - if all_task_ids.any? - # Chain the composite publish to wait for all sibling component publishes - # This ensures all component CVs finish publishing before the composite publish starts - ForemanTasks.chain( - all_task_ids, - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) - else - # No siblings currently running, trigger composite publish immediately - ForemanTasks.async_task( - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) - end - rescue ForemanTasks::Lock::LockConflict => e - # Composite publish already scheduled/running - this is expected when - # multiple component CVs finish around the same time - Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled: #{e.message}") - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - rescue StandardError => e - Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - raise e - end - end + if scheduled_tasks.any? { |task| scheduled_task_for_composite?(task, composite_cv) } + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") + return true end + + # Check running tasks (these have input populated) + if find_composite_publish_tasks(composite_cv).any? + Rails.logger.info("Composite CV #{composite_cv.name} publish already running, skipping duplicate") + return true + end + + false end - private + # Check if a scheduled task is for the given composite CV by inspecting delayed plan args + # @param task [ForemanTasks::Task] The task to check + # @param composite_cv [Katello::ContentView] The composite content view + # @return [Boolean] true if task is for this composite CV + def scheduled_task_for_composite?(task, composite_cv) + delayed_plan = ForemanTasks.dynflow.world.persistence.load_delayed_plan(task.external_id) + args = delayed_plan.args + args.first.is_a?(::Katello::ContentView) && args.first.id == composite_cv.id + rescue StandardError + false + end + + # Trigger a composite publish, either immediately or chained to sibling tasks + # @param composite_cv [Katello::ContentView] The composite content view + # @param sibling_task_ids [Array] Task IDs to wait for + # @param description [String] Description for the publish task + def trigger_composite_publish(composite_cv, sibling_task_ids, description) + if sibling_task_ids.any? + trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) + else + trigger_immediate_composite_publish(composite_cv, description) + end + rescue ForemanTasks::Lock::LockConflict => e + Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + rescue StandardError => e + Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise e + end + + # Trigger a chained composite publish that waits for sibling tasks + def trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) + ForemanTasks.chain( + sibling_task_ids, + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + end + + # Trigger an immediate composite publish + def trigger_immediate_composite_publish(composite_cv, description) + ForemanTasks.async_task( + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: self.id + ) + end # Find all currently running publish tasks for component CVs that belong to the given composite CV # @param composite_cv [Katello::ContentView] The composite content view diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index ee491394f4d..30205be9e89 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -42,17 +42,12 @@ def setup def test_auto_publish_with_no_sibling_tasks_triggers_immediately task_id = SecureRandom.uuid - # Stub to return no scheduled tasks, no sibling tasks, no composite tasks - scheduled_relation = stub(any?: false) - sibling_relation = stub(select: []) - composite_relation = stub(select: []) - + # Stub to return no scheduled, no running composite, no sibling tasks ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: scheduled_relation)) # First: scheduled check - .then.returns(stub(where: sibling_relation)) # Second: sibling check - .then.returns(stub(where: composite_relation)) # Third: composite check + .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks + .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: []))) # Sibling check: none - # Should trigger async_task since no siblings are running ForemanTasks.expects(:async_task).with( ::Actions::Katello::ContentView::Publish, @composite_cv, @@ -60,9 +55,6 @@ def test_auto_publish_with_no_sibling_tasks_triggers_immediately triggered_by_id: @component1_version.id ).returns(stub(id: SecureRandom.uuid)) - # Should not call chain - ForemanTasks.expects(:chain).never - @component1_version.auto_publish_composites!(task_id) end @@ -70,20 +62,13 @@ def test_auto_publish_with_sibling_tasks_uses_chaining task_id1 = SecureRandom.uuid task_id2 = SecureRandom.uuid - # Create mock running task for sibling component sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) - # Stub the three checks: scheduled, sibling, composite - scheduled_relation = stub(any?: false) - sibling_relation = stub(select: [sibling_task]) - composite_relation = stub(select: []) - ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: scheduled_relation)) # First: scheduled check - .then.returns(stub(where: sibling_relation)) # Second: sibling check - .then.returns(stub(where: composite_relation)) # Third: composite check + .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks + .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling - # Should use chain since sibling is running ForemanTasks.expects(:chain).with( [task_id2], ::Actions::Katello::ContentView::Publish, @@ -92,8 +77,6 @@ def test_auto_publish_with_sibling_tasks_uses_chaining triggered_by_id: @component1_version.id ).returns(stub(id: SecureRandom.uuid)) - ForemanTasks.expects(:async_task).never - @component1_version.auto_publish_composites!(task_id1) end @@ -128,17 +111,11 @@ def test_auto_publish_skips_when_composite_already_scheduled def test_auto_publish_handles_lock_conflict_gracefully task_id = SecureRandom.uuid - # Stub to return no scheduled, sibling, or composite tasks - scheduled_relation = stub(any?: false) - sibling_relation = stub(select: []) - composite_relation = stub(select: []) - ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: scheduled_relation)) - .then.returns(stub(where: sibling_relation)) - .then.returns(stub(where: composite_relation)) + .returns(stub(where: stub(any?: false))) # Scheduled check: none + .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: []))) # Sibling check: none - # Simulate lock conflict lock = stub('required_lock') conflicting_task = stub(id: 123) conflicting_lock = stub(task: conflicting_task) From 89fbaaac2bc4841acf9b6e698dca5619150330de Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Mon, 27 Oct 2025 17:36:49 +0000 Subject: [PATCH 5/9] Refs #38856 - use event polling to chain running CCV publishes. --- .../actions/katello/content_view/publish.rb | 4 ++- app/models/katello/content_view_version.rb | 34 ++++++++++++++----- .../events/auto_publish_composite_view.rb | 12 +++---- lib/katello/engine.rb | 2 +- .../content_view_version_auto_publish_test.rb | 21 ++++++++++++ .../auto_publish_composite_view_test.rb | 16 +++++++-- 6 files changed, 69 insertions(+), 20 deletions(-) diff --git a/app/lib/actions/katello/content_view/publish.rb b/app/lib/actions/katello/content_view/publish.rb index 0954fc02e0e..018da1bfcd1 100644 --- a/app/lib/actions/katello/content_view/publish.rb +++ b/app/lib/actions/katello/content_view/publish.rb @@ -37,13 +37,15 @@ def plan(content_view, description = "", options = {importing: false, syncable: version = version_for_publish(content_view, options) self.version = version library = content_view.organization.library + triggered_by_id = options[:triggered_by_id] || + (options[:triggered_by].is_a?(Integer) ? options[:triggered_by] : options[:triggered_by]&.id) history = ::Katello::ContentViewHistory.create!(:content_view_version => version, :user => ::User.current.login, :status => ::Katello::ContentViewHistory::IN_PROGRESS, :action => ::Katello::ContentViewHistory.actions[:publish], :task => self.task, :notes => description, - :triggered_by_id => options[:triggered_by_id] || options[:triggered_by]&.id + :triggered_by_id => triggered_by_id ) source_repositories = [] content_view.publish_repositories(options[:override_components]) do |repositories| diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 4ebe49868ab..02fafe557c6 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -368,7 +368,18 @@ def auto_publish_composites!(component_task_id) composite_cv = ::Katello::ContentView.find(composite_id) composite_cv.with_lock do - next if composite_publish_already_exists?(composite_cv) + status = composite_publish_status(composite_cv) + + if status == :scheduled + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") + next + elsif status == :running + # A composite publish is currently running - we need to schedule another one + # after it finishes to include this new component version + Rails.logger.info("Composite CV #{composite_cv.name} publish running, scheduling event for retry") + schedule_auto_publish_event(composite_cv, description) + next + end sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) trigger_composite_publish(composite_cv, sibling_task_ids, description) @@ -380,25 +391,32 @@ def auto_publish_composites!(component_task_id) # Check if a composite publish task already exists (scheduled or running) # @param composite_cv [Katello::ContentView] The composite content view - # @return [Boolean] true if a publish task already exists - def composite_publish_already_exists?(composite_cv) + # @return [Symbol, nil] :scheduled if scheduled, :running if running, nil otherwise + def composite_publish_status(composite_cv) # Check scheduled tasks first (they don't have input populated yet) scheduled_tasks = ForemanTasks::Task::DynflowTask .for_action(::Actions::Katello::ContentView::Publish) .where(state: 'scheduled') if scheduled_tasks.any? { |task| scheduled_task_for_composite?(task, composite_cv) } - Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") - return true + return :scheduled end # Check running tasks (these have input populated) if find_composite_publish_tasks(composite_cv).any? - Rails.logger.info("Composite CV #{composite_cv.name} publish already running, skipping duplicate") - return true + return :running end - false + nil + end + + # Schedule an event to retry composite publish after current one finishes + # @param composite_cv [Katello::ContentView] The composite content view + # @param description [String] Description for the publish task + def schedule_auto_publish_event(composite_cv, description) + ::Katello::EventQueue.push_event(::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, composite_cv.id) do |attrs| + attrs[:metadata] = { description: description, version_id: self.id } + end end # Check if a scheduled task is for the given composite CV by inspecting delayed plan args diff --git a/app/models/katello/events/auto_publish_composite_view.rb b/app/models/katello/events/auto_publish_composite_view.rb index 713c20e37ea..262b1080244 100644 --- a/app/models/katello/events/auto_publish_composite_view.rb +++ b/app/models/katello/events/auto_publish_composite_view.rb @@ -1,12 +1,10 @@ module Katello module Events - # DEPRECATED: This event class is no longer used after implementing Dynflow chaining - # for auto-publish composite views. EventQueue.push_event calls have been removed. - # This class is kept temporarily for reference but should be removed in a future release - # along with: - # - test/models/events/auto_publish_composite_view_test.rb - # - EventQueue registration in lib/katello/engine.rb (already commented out) - # See: ContentViewVersion#auto_publish_composites! which now uses ForemanTasks.chain + # Event handler for retrying composite content view auto-publish when a lock conflict occurs. + # This is used in conjunction with Dynflow chaining: + # - Dynflow chaining coordinates sibling component CV publishes to avoid race conditions + # - Event-based retry handles the case when a composite CV publish is already running + # See: ContentViewVersion#auto_publish_composites! class AutoPublishCompositeView EVENT_TYPE = 'auto_publish_composite_view'.freeze diff --git a/lib/katello/engine.rb b/lib/katello/engine.rb index edbc87ec765..e2193457383 100644 --- a/lib/katello/engine.rb +++ b/lib/katello/engine.rb @@ -223,7 +223,7 @@ class Engine < ::Rails::Engine load 'katello/scheduled_jobs.rb' Katello::EventQueue.register_event(Katello::Events::ImportPool::EVENT_TYPE, Katello::Events::ImportPool) - # Katello::Events::AutoPublishCompositeView is obsolete - now uses ForemanTasks.chain() directly + Katello::EventQueue.register_event(Katello::Events::AutoPublishCompositeView::EVENT_TYPE, Katello::Events::AutoPublishCompositeView) Katello::EventQueue.register_event(Katello::Events::DeleteLatestContentViewVersion::EVENT_TYPE, Katello::Events::DeleteLatestContentViewVersion) Katello::EventQueue.register_event(Katello::Events::GenerateHostApplicability::EVENT_TYPE, Katello::Events::GenerateHostApplicability) Katello::EventQueue.register_event(Katello::Events::DeletePool::EVENT_TYPE, Katello::Events::DeletePool) diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index 30205be9e89..f6dc535aabe 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -108,6 +108,27 @@ def test_auto_publish_skips_when_composite_already_scheduled @component1_version.auto_publish_composites!(task_id) end + def test_auto_publish_schedules_event_when_composite_running + task_id = SecureRandom.uuid + running_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => @composite_cv.id } }) + + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: stub(any?: false))) # Scheduled check: none + .then.returns(stub(where: stub(select: [running_task]))) # Running check: found running task + + # Should schedule event instead of creating task + event_attrs = {} + ::Katello::EventQueue.expects(:push_event).with( + ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, + @composite_cv.id + ).yields(event_attrs) + + ForemanTasks.expects(:chain).never + ForemanTasks.expects(:async_task).never + + @component1_version.auto_publish_composites!(task_id) + end + def test_auto_publish_handles_lock_conflict_gracefully task_id = SecureRandom.uuid diff --git a/test/models/events/auto_publish_composite_view_test.rb b/test/models/events/auto_publish_composite_view_test.rb index 9cce5a93ff3..1280338bf2f 100644 --- a/test/models/events/auto_publish_composite_view_test.rb +++ b/test/models/events/auto_publish_composite_view_test.rb @@ -4,12 +4,20 @@ module Katello module Events class AutoPublishCompositeViewTest < ActiveSupport::TestCase let(:composite_view) { katello_content_views(:composite_view) } + let(:component_version) { katello_content_view_versions(:library_view_version_1) } def test_run_with_publish - ForemanTasks.expects(:async_task) + metadata = { description: "Auto Publish - Test", version_id: component_version.id } + + ForemanTasks.expects(:async_task).with( + ::Actions::Katello::ContentView::Publish, + composite_view, + metadata[:description], + triggered_by: metadata[:version_id] + ) event = AutoPublishCompositeView.new(composite_view.id) do |instance| - instance.metadata = {} + instance.metadata = metadata end event.run @@ -23,10 +31,12 @@ def test_run_with_error end def test_run_with_lock_error + metadata = { description: "Auto Publish - Test", version_id: component_version.id } + ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(mock, [])) instance = AutoPublishCompositeView.new(composite_view.id) do |event| - event.metadata = {} + event.metadata = metadata end assert_raises(ForemanTasks::Lock::LockConflict) { instance.run } From 383b737c508c83483b366ff65232d6834c97e235 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Mon, 27 Oct 2025 18:07:27 +0000 Subject: [PATCH 6/9] Refs #38856 - address rubocop concerns. --- app/models/katello/content_view_version.rb | 5 +- config/initializers/foreman_tasks_chaining.rb | 76 ++++++++++--------- .../content_view_version_auto_publish_test.rb | 22 +++--- 3 files changed, 51 insertions(+), 52 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 02fafe557c6..ccb89a3db1e 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -370,10 +370,11 @@ def auto_publish_composites!(component_task_id) composite_cv.with_lock do status = composite_publish_status(composite_cv) - if status == :scheduled + case status + when :scheduled Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") next - elsif status == :running + when :running # A composite publish is currently running - we need to schedule another one # after it finishes to include this new component version Rails.logger.info("Composite CV #{composite_cv.name} publish running, scheduling event for retry") diff --git a/config/initializers/foreman_tasks_chaining.rb b/config/initializers/foreman_tasks_chaining.rb index c87631c1eb1..c2ce1925aa6 100644 --- a/config/initializers/foreman_tasks_chaining.rb +++ b/config/initializers/foreman_tasks_chaining.rb @@ -6,43 +6,45 @@ # # See: https://github.com/Dynflow/dynflow/pull/446 -# Defer extension until after ForemanTasks module is loaded -Rails.application.config.to_prepare do - module ForemanTasks - # Chain execution plans so that a new plan waits until prerequisite plans finish before executing. - # This is useful for coordinating dependent tasks where one task should only run after - # other tasks have completed successfully. - # - # The chained plan will remain in 'scheduled' state until all prerequisite plans - # reach 'stopped' state (regardless of success/failure). - # - # @param plan_uuids [String, Array] UUID(s) of prerequisite execution plan(s) - # @param action [Class] Action class to execute - # @param args Arguments to pass to the action - # @return [ForemanTasks::Task::DynflowTask] The chained task that will wait for prerequisites - # - # @example Chain a task to wait for another task - # task1 = ForemanTasks.async_task(SomeAction) - # task2 = ForemanTasks.chain(task1.external_id, AnotherAction, arg1, arg2) - # # task2 will only execute after task1 completes - # - # @example Chain a task to wait for multiple tasks - # task1 = ForemanTasks.async_task(Action1) - # task2 = ForemanTasks.async_task(Action2) - # task3 = ForemanTasks.chain([task1.external_id, task2.external_id], Action3) - # # task3 will only execute after both task1 and task2 complete - def self.chain(plan_uuids, action, *args) - result = dynflow.world.chain(plan_uuids, action, *args) - # The ForemanTasks record may not exist yet for delayed plans, - # so we need to find or create it and properly initialize it - ForemanTasks::Task.find_by(:external_id => result.id) || - begin - delayed_plan = dynflow.world.persistence.load_delayed_plan(result.id) - execution_plan = dynflow.world.persistence.load_execution_plan(result.id) - ForemanTasks::Task::DynflowTask.new_for_execution_plan(execution_plan).tap do |task| - task.update_from_dynflow(execution_plan, delayed_plan) - end +module ForemanTasksChaining + # Chain execution plans so that a new plan waits until prerequisite plans finish before executing. + # This is useful for coordinating dependent tasks where one task should only run after + # other tasks have completed successfully. + # + # The chained plan will remain in 'scheduled' state until all prerequisite plans + # reach 'stopped' state (regardless of success/failure). + # + # @param plan_uuids [String, Array] UUID(s) of prerequisite execution plan(s) + # @param action [Class] Action class to execute + # @param args Arguments to pass to the action + # @return [ForemanTasks::Task::DynflowTask] The chained task that will wait for prerequisites + # + # @example Chain a task to wait for another task + # task1 = ForemanTasks.async_task(SomeAction) + # task2 = ForemanTasks.chain(task1.external_id, AnotherAction, arg1, arg2) + # # task2 will only execute after task1 completes + # + # @example Chain a task to wait for multiple tasks + # task1 = ForemanTasks.async_task(Action1) + # task2 = ForemanTasks.async_task(Action2) + # task3 = ForemanTasks.chain([task1.external_id, task2.external_id], Action3) + # # task3 will only execute after both task1 and task2 complete + def chain(plan_uuids, action, *args) + result = dynflow.world.chain(plan_uuids, action, *args) + # The ForemanTasks record may not exist yet for delayed plans, + # so we need to find or create it and properly initialize it + ForemanTasks::Task.find_by(:external_id => result.id) || + begin + delayed_plan = dynflow.world.persistence.load_delayed_plan(result.id) + execution_plan = dynflow.world.persistence.load_execution_plan(result.id) + ForemanTasks::Task::DynflowTask.new_for_execution_plan(execution_plan).tap do |task| + task.update_from_dynflow(execution_plan, delayed_plan) end - end + end end end + +# Defer extension until after ForemanTasks module is loaded +Rails.application.config.to_prepare do + ForemanTasks.singleton_class.prepend(ForemanTasksChaining) +end diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index f6dc535aabe..db2a11391e6 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -44,7 +44,7 @@ def test_auto_publish_with_no_sibling_tasks_triggers_immediately # Stub to return no scheduled, no running composite, no sibling tasks ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks + .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks .then.returns(stub(where: stub(select: []))) # Running composite check: none .then.returns(stub(where: stub(select: []))) # Sibling check: none @@ -65,9 +65,9 @@ def test_auto_publish_with_sibling_tasks_uses_chaining sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks - .then.returns(stub(where: stub(select: []))) # Running composite check: none - .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling + .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks + .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling ForemanTasks.expects(:chain).with( [task_id2], @@ -113,8 +113,8 @@ def test_auto_publish_schedules_event_when_composite_running running_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => @composite_cv.id } }) ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: none - .then.returns(stub(where: stub(select: [running_task]))) # Running check: found running task + .returns(stub(where: stub(any?: false))) # Scheduled check: none + .then.returns(stub(where: stub(select: [running_task]))) # Running check: found running task # Should schedule event instead of creating task event_attrs = {} @@ -133,7 +133,7 @@ def test_auto_publish_handles_lock_conflict_gracefully task_id = SecureRandom.uuid ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: none + .returns(stub(where: stub(any?: false))) # Scheduled check: none .then.returns(stub(where: stub(select: []))) # Running composite check: none .then.returns(stub(where: stub(select: []))) # Sibling check: none @@ -172,17 +172,13 @@ def test_find_sibling_component_publish_tasks_finds_running_tasks def test_find_sibling_tasks_excludes_non_component_tasks task_id = SecureRandom.uuid - # Create mock task for a different CV (not a component) - other_cv = FactoryBot.create(:katello_content_view, :organization => @org) - other_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => other_cv.id } }) - - # The select block will filter out other_task + # The select block will filter out tasks for CVs that aren't components ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: []))) result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, task_id) # Should exclude current task and other CV's task - assert_equal [], result + assert_empty result end end end From f020e7a80d1af41a77675b751d189cc96ff70949 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Mon, 3 Nov 2025 20:39:21 +0000 Subject: [PATCH 7/9] Refs #38856 - remove ForemanTasks patch for chaining --- CLAUDE.md | 6 +++ .../actions/katello/content_view/publish.rb | 4 +- app/models/katello/content_view_version.rb | 48 +++++++----------- config/initializers/foreman_tasks_chaining.rb | 50 ------------------- test/actions/katello/content_view_test.rb | 18 +++---- .../content_view_version_auto_publish_test.rb | 6 +-- 6 files changed, 37 insertions(+), 95 deletions(-) delete mode 100644 config/initializers/foreman_tasks_chaining.rb diff --git a/CLAUDE.md b/CLAUDE.md index 2e82b10b8e2..fd5e6752c2d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -269,6 +269,12 @@ Don't write unnecessary comments in tests. When writing a new test, look at surr - test length, where possible - length and quantity of comments (don't be too wordy) +### Code Documentation + +Avoid YARD-style documentation (`@param`, `@return`) in Ruby code. Use single-line comments describing what the method does, not how. + +Example: `# Returns :scheduled, :running, or nil based on task status` + ### Test Commands Reference **CRITICAL: Never use `bundle exec rake test TEST=...` for individual tests. Always use `ktest`.** diff --git a/app/lib/actions/katello/content_view/publish.rb b/app/lib/actions/katello/content_view/publish.rb index 018da1bfcd1..3932a77aa48 100644 --- a/app/lib/actions/katello/content_view/publish.rb +++ b/app/lib/actions/katello/content_view/publish.rb @@ -115,9 +115,9 @@ def humanized_name def run version = ::Katello::ContentViewVersion.find(input[:content_view_version_id]) - # Pass the current task's execution plan ID so auto_publish can coordinate + # Pass the current execution plan ID so auto_publish can coordinate # with other component CV publishes using Dynflow chaining - version.auto_publish_composites!(task.external_id) + version.auto_publish_composites!(execution_plan_id) output[:content_view_id] = input[:content_view_id] output[:content_view_version_id] = input[:content_view_version_id] diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index ccb89a3db1e..a86d8d1cd2b 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -380,19 +380,18 @@ def auto_publish_composites!(component_task_id) Rails.logger.info("Composite CV #{composite_cv.name} publish running, scheduling event for retry") schedule_auto_publish_event(composite_cv, description) next + when nil + # No composite publish running or scheduled - trigger one now + sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) + trigger_composite_publish(composite_cv, sibling_task_ids, description) end - - sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) - trigger_composite_publish(composite_cv, sibling_task_ids, description) end end end private - # Check if a composite publish task already exists (scheduled or running) - # @param composite_cv [Katello::ContentView] The composite content view - # @return [Symbol, nil] :scheduled if scheduled, :running if running, nil otherwise + # Returns :scheduled, :running, or nil based on composite CV publish task status def composite_publish_status(composite_cv) # Check scheduled tasks first (they don't have input populated yet) scheduled_tasks = ForemanTasks::Task::DynflowTask @@ -404,7 +403,7 @@ def composite_publish_status(composite_cv) end # Check running tasks (these have input populated) - if find_composite_publish_tasks(composite_cv).any? + if find_active_composite_publish_tasks(composite_cv).any? return :running end @@ -412,8 +411,6 @@ def composite_publish_status(composite_cv) end # Schedule an event to retry composite publish after current one finishes - # @param composite_cv [Katello::ContentView] The composite content view - # @param description [String] Description for the publish task def schedule_auto_publish_event(composite_cv, description) ::Katello::EventQueue.push_event(::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, composite_cv.id) do |attrs| attrs[:metadata] = { description: description, version_id: self.id } @@ -421,21 +418,18 @@ def schedule_auto_publish_event(composite_cv, description) end # Check if a scheduled task is for the given composite CV by inspecting delayed plan args - # @param task [ForemanTasks::Task] The task to check - # @param composite_cv [Katello::ContentView] The composite content view - # @return [Boolean] true if task is for this composite CV def scheduled_task_for_composite?(task, composite_cv) delayed_plan = ForemanTasks.dynflow.world.persistence.load_delayed_plan(task.external_id) + return false if delayed_plan.nil? + args = delayed_plan.args args.first.is_a?(::Katello::ContentView) && args.first.id == composite_cv.id - rescue StandardError + rescue NoMethodError, TypeError, Dynflow::Error + Rails.logger.error("Failed to check scheduled task for composite CV #{composite_cv.name}: #{e.message}") false end # Trigger a composite publish, either immediately or chained to sibling tasks - # @param composite_cv [Katello::ContentView] The composite content view - # @param sibling_task_ids [Array] Task IDs to wait for - # @param description [String] Description for the publish task def trigger_composite_publish(composite_cv, sibling_task_ids, description) if sibling_task_ids.any? trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) @@ -443,17 +437,18 @@ def trigger_composite_publish(composite_cv, sibling_task_ids, description) trigger_immediate_composite_publish(composite_cv, description) end rescue ForemanTasks::Lock::LockConflict => e - Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.message}") + Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.class} - #{e.message}") ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) rescue StandardError => e - Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.message}") + Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.class} - #{e.message}") + Rails.logger.debug(e.backtrace.join("\n")) if e.backtrace ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - raise e + raise end # Trigger a chained composite publish that waits for sibling tasks def trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) - ForemanTasks.chain( + ForemanTasks.dynflow.world.chain( sibling_task_ids, ::Actions::Katello::ContentView::Publish, composite_cv, @@ -472,10 +467,7 @@ def trigger_immediate_composite_publish(composite_cv, description) ) end - # Find all currently running publish tasks for component CVs that belong to the given composite CV - # @param composite_cv [Katello::ContentView] The composite content view - # @param current_task_id [String] The execution plan ID of the current component publish task - # @return [Array] Array of execution plan IDs for all running component publish tasks + # Find sibling component publish tasks that should be waited for def find_sibling_component_publish_tasks(composite_cv, current_task_id) # Get all component CV IDs for this composite component_cv_ids = composite_cv.components.pluck(:content_view_id) @@ -497,12 +489,8 @@ def find_sibling_component_publish_tasks(composite_cv, current_task_id) task_ids.reject { |id| id == current_task_id } end - # Find all currently running composite publish tasks for the given composite CV - # NOTE: This does NOT check for scheduled tasks - those are handled separately in auto_publish_composites! - # by inspecting delayed plan args, since scheduled tasks don't have input populated yet. - # @param composite_cv [Katello::ContentView] The composite content view - # @return [Array] Array of execution plan IDs for all running composite publish tasks - def find_composite_publish_tasks(composite_cv) + # Find active (planning/planned/running) composite publish tasks (does NOT check scheduled tasks) + def find_active_composite_publish_tasks(composite_cv) relevant_tasks = ForemanTasks::Task::DynflowTask .for_action(::Actions::Katello::ContentView::Publish) .where(state: ['planning', 'planned', 'running']) diff --git a/config/initializers/foreman_tasks_chaining.rb b/config/initializers/foreman_tasks_chaining.rb deleted file mode 100644 index c2ce1925aa6..00000000000 --- a/config/initializers/foreman_tasks_chaining.rb +++ /dev/null @@ -1,50 +0,0 @@ -# frozen_string_literal: true - -# FIXME: This monkey-patch adds execution plan chaining support to ForemanTasks. -# This should be submitted upstream to the foreman-tasks gem and removed from here -# once it's available in a released version. -# -# See: https://github.com/Dynflow/dynflow/pull/446 - -module ForemanTasksChaining - # Chain execution plans so that a new plan waits until prerequisite plans finish before executing. - # This is useful for coordinating dependent tasks where one task should only run after - # other tasks have completed successfully. - # - # The chained plan will remain in 'scheduled' state until all prerequisite plans - # reach 'stopped' state (regardless of success/failure). - # - # @param plan_uuids [String, Array] UUID(s) of prerequisite execution plan(s) - # @param action [Class] Action class to execute - # @param args Arguments to pass to the action - # @return [ForemanTasks::Task::DynflowTask] The chained task that will wait for prerequisites - # - # @example Chain a task to wait for another task - # task1 = ForemanTasks.async_task(SomeAction) - # task2 = ForemanTasks.chain(task1.external_id, AnotherAction, arg1, arg2) - # # task2 will only execute after task1 completes - # - # @example Chain a task to wait for multiple tasks - # task1 = ForemanTasks.async_task(Action1) - # task2 = ForemanTasks.async_task(Action2) - # task3 = ForemanTasks.chain([task1.external_id, task2.external_id], Action3) - # # task3 will only execute after both task1 and task2 complete - def chain(plan_uuids, action, *args) - result = dynflow.world.chain(plan_uuids, action, *args) - # The ForemanTasks record may not exist yet for delayed plans, - # so we need to find or create it and properly initialize it - ForemanTasks::Task.find_by(:external_id => result.id) || - begin - delayed_plan = dynflow.world.persistence.load_delayed_plan(result.id) - execution_plan = dynflow.world.persistence.load_execution_plan(result.id) - ForemanTasks::Task::DynflowTask.new_for_execution_plan(execution_plan).tap do |task| - task.update_from_dynflow(execution_plan, delayed_plan) - end - end - end -end - -# Defer extension until after ForemanTasks module is loaded -Rails.application.config.to_prepare do - ForemanTasks.singleton_class.prepend(ForemanTasksChaining) -end diff --git a/test/actions/katello/content_view_test.rb b/test/actions/katello/content_view_test.rb index ab4f73c0aff..0ee5c36652b 100644 --- a/test/actions/katello/content_view_test.rb +++ b/test/actions/katello/content_view_test.rb @@ -249,7 +249,7 @@ class PublishTest < TestBase end context 'run phase' do - it 'triggers auto-publish for composite views using chaining' do + it 'triggers auto-publish for composite views' do composite_view = katello_content_views(:composite_view) action.stubs(:task).returns(success_task) @@ -258,20 +258,18 @@ class PublishTest < TestBase composite_content_view: composite_view, content_view: content_view) - # Mock the task relation to simulate no other component tasks running - task_relation = mock('task_relation') - task_relation.expects(:select).returns([]) - ForemanTasks::Task::DynflowTask.expects(:for_action) - .with(::Actions::Katello::ContentView::Publish) - .returns(task_relation) - task_relation.expects(:where).with(state: ['planning', 'planned', 'running']).returns(task_relation) + # Mock the task relations to simulate no scheduled, no running composite, no sibling tasks + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks + .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: []))) # Sibling check: none # Expect async_task to be called (no siblings, so no chaining) ForemanTasks.expects(:async_task).with( ::Actions::Katello::ContentView::Publish, composite_view, anything, - triggered_by: anything + triggered_by_id: anything ) plan_action action, content_view @@ -283,7 +281,7 @@ class PublishTest < TestBase # Should not trigger any auto-publish tasks ForemanTasks.expects(:async_task).never - ForemanTasks.expects(:chain).never + ForemanTasks.dynflow.world.expects(:chain).never plan_action action, katello_content_views(:no_environment_view) run_action action diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index db2a11391e6..98e5e695d9e 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -69,7 +69,7 @@ def test_auto_publish_with_sibling_tasks_uses_chaining .then.returns(stub(where: stub(select: []))) # Running composite check: none .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling - ForemanTasks.expects(:chain).with( + ForemanTasks.dynflow.world.expects(:chain).with( [task_id2], ::Actions::Katello::ContentView::Publish, @composite_cv, @@ -102,7 +102,7 @@ def test_auto_publish_skips_when_composite_already_scheduled .returns(stub(where: scheduled_relation)) # Should not create any new task - ForemanTasks.expects(:chain).never + ForemanTasks.dynflow.world.expects(:chain).never ForemanTasks.expects(:async_task).never @component1_version.auto_publish_composites!(task_id) @@ -123,7 +123,7 @@ def test_auto_publish_schedules_event_when_composite_running @composite_cv.id ).yields(event_attrs) - ForemanTasks.expects(:chain).never + ForemanTasks.dynflow.world.expects(:chain).never ForemanTasks.expects(:async_task).never @component1_version.auto_publish_composites!(task_id) From 30b3b754513cec96f11838bc0a83a14c0d3fceb5 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Mon, 17 Nov 2025 19:28:52 +0000 Subject: [PATCH 8/9] Refs #38856 - simplify auto-publish to always use events * All composite publishes now go through event system (not just :running case) * Event handler performs coordination and chaining * Pass calling_task_id through event metadata for sibling exclusion --- app/models/katello/content_view_version.rb | 102 +++++++-------- .../events/auto_publish_composite_view.rb | 12 +- .../content_view_version_auto_publish_test.rb | 121 +++--------------- .../auto_publish_composite_view_test.rb | 16 ++- 4 files changed, 82 insertions(+), 169 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index a86d8d1cd2b..80a38438812 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -374,16 +374,12 @@ def auto_publish_composites!(component_task_id) when :scheduled Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") next - when :running - # A composite publish is currently running - we need to schedule another one - # after it finishes to include this new component version - Rails.logger.info("Composite CV #{composite_cv.name} publish running, scheduling event for retry") - schedule_auto_publish_event(composite_cv, description) + when :running, nil + # Either composite is running or no composite activity detected + # Schedule event to trigger composite publish with proper coordination + Rails.logger.info("Composite CV #{composite_cv.name} scheduling auto-publish event") + schedule_auto_publish_event(composite_cv, description, component_task_id) next - when nil - # No composite publish running or scheduled - trigger one now - sibling_task_ids = find_sibling_component_publish_tasks(composite_cv, component_task_id) - trigger_composite_publish(composite_cv, sibling_task_ids, description) end end end @@ -411,9 +407,9 @@ def composite_publish_status(composite_cv) end # Schedule an event to retry composite publish after current one finishes - def schedule_auto_publish_event(composite_cv, description) + def schedule_auto_publish_event(composite_cv, description, component_task_id) ::Katello::EventQueue.push_event(::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, composite_cv.id) do |attrs| - attrs[:metadata] = { description: description, version_id: self.id } + attrs[:metadata] = { description: description, version_id: self.id, calling_task_id: component_task_id } end end @@ -429,16 +425,28 @@ def scheduled_task_for_composite?(task, composite_cv) false end - # Trigger a composite publish, either immediately or chained to sibling tasks - def trigger_composite_publish(composite_cv, sibling_task_ids, description) - if sibling_task_ids.any? - trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) - else - trigger_immediate_composite_publish(composite_cv, description) - end + # Trigger a composite publish with coordination for sibling tasks. + # Checks for running component CV publishes and chains if necessary. + def self.trigger_composite_publish_with_coordination(composite_cv, description, triggered_by_version_id, calling_task_id: nil) + # Find currently running component CV publish tasks + component_cv_ids = composite_cv.components.pluck(:content_view_id) + running_tasks = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: ['planning', 'planned', 'running']) + .select do |task| + task_input = task.input + task_input && component_cv_ids.include?(task_input.dig('content_view', 'id')) + end + + sibling_task_ids = running_tasks.map(&:external_id) + # Exclude the calling component task to avoid self-dependency + sibling_task_ids.reject! { |id| id == calling_task_id } if calling_task_id + + trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) rescue ForemanTasks::Lock::LockConflict => e Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.class} - #{e.message}") ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise rescue StandardError => e Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.class} - #{e.message}") Rails.logger.debug(e.backtrace.join("\n")) if e.backtrace @@ -446,48 +454,26 @@ def trigger_composite_publish(composite_cv, sibling_task_ids, description) raise end - # Trigger a chained composite publish that waits for sibling tasks - def trigger_chained_composite_publish(composite_cv, sibling_task_ids, description) - ForemanTasks.dynflow.world.chain( - sibling_task_ids, - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) - end - - # Trigger an immediate composite publish - def trigger_immediate_composite_publish(composite_cv, description) - ForemanTasks.async_task( - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: self.id - ) + # Trigger a composite publish, chaining to sibling tasks if any exist + def self.trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) + if sibling_task_ids.any? + ForemanTasks.dynflow.world.chain( + sibling_task_ids, + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: triggered_by_version_id + ) + else + ForemanTasks.async_task( + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: triggered_by_version_id + ) + end end - # Find sibling component publish tasks that should be waited for - def find_sibling_component_publish_tasks(composite_cv, current_task_id) - # Get all component CV IDs for this composite - component_cv_ids = composite_cv.components.pluck(:content_view_id) - - # Find all currently running publish tasks for these component CVs - running_tasks = ForemanTasks::Task::DynflowTask - .for_action(::Actions::Katello::ContentView::Publish) - .where(state: ['planning', 'planned', 'running']) - .select do |task| - # Check if task is publishing one of the component CVs - task_input = task.input - task_input && component_cv_ids.include?(task_input.dig('content_view', 'id')) - end - - task_ids = running_tasks.map(&:external_id) - - # Exclude the current task - if we're running this code, the current task - # has already reached its Finalize step and doesn't need to be waited for - task_ids.reject { |id| id == current_task_id } - end # Find active (planning/planned/running) composite publish tasks (does NOT check scheduled tasks) def find_active_composite_publish_tasks(composite_cv) diff --git a/app/models/katello/events/auto_publish_composite_view.rb b/app/models/katello/events/auto_publish_composite_view.rb index 262b1080244..576632d14ef 100644 --- a/app/models/katello/events/auto_publish_composite_view.rb +++ b/app/models/katello/events/auto_publish_composite_view.rb @@ -25,10 +25,14 @@ def run return unless composite_view begin - ForemanTasks.async_task(::Actions::Katello::ContentView::Publish, - composite_view, - metadata[:description], - triggered_by: metadata[:version_id]) + # Use the same coordination logic as auto_publish_composites! to check for + # running component tasks and chain if necessary + ::Katello::ContentViewVersion.trigger_composite_publish_with_coordination( + composite_view, + metadata[:description], + metadata[:version_id], + calling_task_id: metadata[:calling_task_id] + ) rescue => e self.retry = true if e.is_a?(ForemanTasks::Lock::LockConflict) deliver_failure_notification diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index 98e5e695d9e..57bdf04599a 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -39,45 +39,37 @@ def setup :latest => true) end - def test_auto_publish_with_no_sibling_tasks_triggers_immediately + def test_auto_publish_schedules_event_when_no_composite_activity task_id = SecureRandom.uuid - # Stub to return no scheduled, no running composite, no sibling tasks + # Stub to return no scheduled, no running composite ForemanTasks::Task::DynflowTask.stubs(:for_action) .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks .then.returns(stub(where: stub(select: []))) # Running composite check: none - .then.returns(stub(where: stub(select: []))) # Sibling check: none - ForemanTasks.expects(:async_task).with( - ::Actions::Katello::ContentView::Publish, - @composite_cv, - anything, - triggered_by_id: @component1_version.id - ).returns(stub(id: SecureRandom.uuid)) + ::Katello::EventQueue.expects(:push_event).with( + ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, + @composite_cv.id + ) @component1_version.auto_publish_composites!(task_id) end - def test_auto_publish_with_sibling_tasks_uses_chaining - task_id1 = SecureRandom.uuid - task_id2 = SecureRandom.uuid - - sibling_task = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) + def test_auto_publish_schedules_event_when_composite_running + task_id = SecureRandom.uuid + running_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => @composite_cv.id } }) + # Stub to return no scheduled but a running composite ForemanTasks::Task::DynflowTask.stubs(:for_action) .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks - .then.returns(stub(where: stub(select: []))) # Running composite check: none - .then.returns(stub(where: stub(select: [sibling_task]))) # Sibling check: found sibling - - ForemanTasks.dynflow.world.expects(:chain).with( - [task_id2], - ::Actions::Katello::ContentView::Publish, - @composite_cv, - anything, - triggered_by_id: @component1_version.id - ).returns(stub(id: SecureRandom.uuid)) - - @component1_version.auto_publish_composites!(task_id1) + .then.returns(stub(where: stub(select: [running_task]))) # Running composite check: found running + + ::Katello::EventQueue.expects(:push_event).with( + ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, + @composite_cv.id + ) + + @component1_version.auto_publish_composites!(task_id) end def test_auto_publish_skips_when_composite_already_scheduled @@ -101,84 +93,11 @@ def test_auto_publish_skips_when_composite_already_scheduled ForemanTasks::Task::DynflowTask.stubs(:for_action) .returns(stub(where: scheduled_relation)) - # Should not create any new task - ForemanTasks.dynflow.world.expects(:chain).never - ForemanTasks.expects(:async_task).never + # Should not schedule event when already scheduled + ::Katello::EventQueue.expects(:push_event).never @component1_version.auto_publish_composites!(task_id) end - def test_auto_publish_schedules_event_when_composite_running - task_id = SecureRandom.uuid - running_task = stub(external_id: SecureRandom.uuid, input: { 'content_view' => { 'id' => @composite_cv.id } }) - - ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: none - .then.returns(stub(where: stub(select: [running_task]))) # Running check: found running task - - # Should schedule event instead of creating task - event_attrs = {} - ::Katello::EventQueue.expects(:push_event).with( - ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, - @composite_cv.id - ).yields(event_attrs) - - ForemanTasks.dynflow.world.expects(:chain).never - ForemanTasks.expects(:async_task).never - - @component1_version.auto_publish_composites!(task_id) - end - - def test_auto_publish_handles_lock_conflict_gracefully - task_id = SecureRandom.uuid - - ForemanTasks::Task::DynflowTask.stubs(:for_action) - .returns(stub(where: stub(any?: false))) # Scheduled check: none - .then.returns(stub(where: stub(select: []))) # Running composite check: none - .then.returns(stub(where: stub(select: []))) # Sibling check: none - - lock = stub('required_lock') - conflicting_task = stub(id: 123) - conflicting_lock = stub(task: conflicting_task) - - ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(lock, [conflicting_lock])) - ::Katello::UINotifications::ContentView::AutoPublishFailure.expects(:deliver!).with(@composite_cv) - - assert_nothing_raised do - @component1_version.auto_publish_composites!(task_id) - end - end - - def test_find_sibling_component_publish_tasks_finds_running_tasks - task_id1 = SecureRandom.uuid - task_id2 = SecureRandom.uuid - - # Create mock running tasks - task1 = stub(external_id: task_id1, input: { 'content_view' => { 'id' => @component_cv1.id } }) - task2 = stub(external_id: task_id2, input: { 'content_view' => { 'id' => @component_cv2.id } }) - - ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: [task1, task2]))) - - current_task_id = SecureRandom.uuid - result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, current_task_id) - - # Should include both sibling tasks but exclude current task - assert_equal 2, result.length - assert_includes result, task_id1 - assert_includes result, task_id2 - assert_not_includes result, current_task_id - end - - def test_find_sibling_tasks_excludes_non_component_tasks - task_id = SecureRandom.uuid - - # The select block will filter out tasks for CVs that aren't components - ForemanTasks::Task::DynflowTask.stubs(:for_action).returns(stub(where: stub(select: []))) - - result = @component1_version.send(:find_sibling_component_publish_tasks, @composite_cv, task_id) - - # Should exclude current task and other CV's task - assert_empty result - end end end diff --git a/test/models/events/auto_publish_composite_view_test.rb b/test/models/events/auto_publish_composite_view_test.rb index 1280338bf2f..24ab9f2be48 100644 --- a/test/models/events/auto_publish_composite_view_test.rb +++ b/test/models/events/auto_publish_composite_view_test.rb @@ -7,13 +7,14 @@ class AutoPublishCompositeViewTest < ActiveSupport::TestCase let(:component_version) { katello_content_view_versions(:library_view_version_1) } def test_run_with_publish - metadata = { description: "Auto Publish - Test", version_id: component_version.id } + calling_task_id = SecureRandom.uuid + metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id } - ForemanTasks.expects(:async_task).with( - ::Actions::Katello::ContentView::Publish, + ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).with( composite_view, metadata[:description], - triggered_by: metadata[:version_id] + metadata[:version_id], + calling_task_id: calling_task_id ) event = AutoPublishCompositeView.new(composite_view.id) do |instance| @@ -31,9 +32,12 @@ def test_run_with_error end def test_run_with_lock_error - metadata = { description: "Auto Publish - Test", version_id: component_version.id } + calling_task_id = SecureRandom.uuid + metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id } - ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(mock, [])) + ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).raises( + ForemanTasks::Lock::LockConflict.new(mock, []) + ) instance = AutoPublishCompositeView.new(composite_view.id) do |event| event.metadata = metadata From 4489865fa3db0de640e4d2d9e190d3f74bf04905 Mon Sep 17 00:00:00 2001 From: Ian Ballou Date: Mon, 17 Nov 2025 20:19:28 +0000 Subject: [PATCH 9/9] Refs #38856 - address rubocop concerns again. --- app/models/katello/content_view_version.rb | 103 +++++++++--------- test/actions/katello/content_view_test.rb | 22 ++-- .../content_view_version_auto_publish_test.rb | 18 ++- 3 files changed, 78 insertions(+), 65 deletions(-) diff --git a/app/models/katello/content_view_version.rb b/app/models/katello/content_view_version.rb index 80a38438812..fe1bb9b5c39 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -385,6 +385,57 @@ def auto_publish_composites!(component_task_id) end end + class << self + # Trigger a composite publish with coordination for sibling tasks. + # Checks for running component CV publishes and chains if necessary. + def trigger_composite_publish_with_coordination(composite_cv, description, triggered_by_version_id, calling_task_id: nil) + # Find currently running component CV publish tasks + component_cv_ids = composite_cv.components.pluck(:content_view_id) + running_tasks = ForemanTasks::Task::DynflowTask + .for_action(::Actions::Katello::ContentView::Publish) + .where(state: ['planning', 'planned', 'running']) + .select do |task| + task_input = task.input + task_input && component_cv_ids.include?(task_input.dig('content_view', 'id')) + end + + sibling_task_ids = running_tasks.map(&:external_id) + # Exclude the calling component task to avoid self-dependency + sibling_task_ids.reject! { |id| id == calling_task_id } if calling_task_id + + trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) + rescue ForemanTasks::Lock::LockConflict => e + Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.class} - #{e.message}") + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise + rescue StandardError => e + Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.class} - #{e.message}") + Rails.logger.debug(e.backtrace.join("\n")) if e.backtrace + ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) + raise + end + + # Trigger a composite publish, chaining to sibling tasks if any exist + def trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) + if sibling_task_ids.any? + ForemanTasks.dynflow.world.chain( + sibling_task_ids, + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: triggered_by_version_id + ) + else + ForemanTasks.async_task( + ::Actions::Katello::ContentView::Publish, + composite_cv, + description, + triggered_by_id: triggered_by_version_id + ) + end + end + end + private # Returns :scheduled, :running, or nil based on composite CV publish task status @@ -420,61 +471,11 @@ def scheduled_task_for_composite?(task, composite_cv) args = delayed_plan.args args.first.is_a?(::Katello::ContentView) && args.first.id == composite_cv.id - rescue NoMethodError, TypeError, Dynflow::Error + rescue NoMethodError, TypeError, Dynflow::Error => e Rails.logger.error("Failed to check scheduled task for composite CV #{composite_cv.name}: #{e.message}") false end - # Trigger a composite publish with coordination for sibling tasks. - # Checks for running component CV publishes and chains if necessary. - def self.trigger_composite_publish_with_coordination(composite_cv, description, triggered_by_version_id, calling_task_id: nil) - # Find currently running component CV publish tasks - component_cv_ids = composite_cv.components.pluck(:content_view_id) - running_tasks = ForemanTasks::Task::DynflowTask - .for_action(::Actions::Katello::ContentView::Publish) - .where(state: ['planning', 'planned', 'running']) - .select do |task| - task_input = task.input - task_input && component_cv_ids.include?(task_input.dig('content_view', 'id')) - end - - sibling_task_ids = running_tasks.map(&:external_id) - # Exclude the calling component task to avoid self-dependency - sibling_task_ids.reject! { |id| id == calling_task_id } if calling_task_id - - trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) - rescue ForemanTasks::Lock::LockConflict => e - Rails.logger.info("Composite CV #{composite_cv.name} publish lock conflict: #{e.class} - #{e.message}") - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - raise - rescue StandardError => e - Rails.logger.error("Failed to auto-publish composite CV #{composite_cv.name}: #{e.class} - #{e.message}") - Rails.logger.debug(e.backtrace.join("\n")) if e.backtrace - ::Katello::UINotifications::ContentView::AutoPublishFailure.deliver!(composite_cv) - raise - end - - # Trigger a composite publish, chaining to sibling tasks if any exist - def self.trigger_publish_with_sibling_tasks(composite_cv, sibling_task_ids, description, triggered_by_version_id) - if sibling_task_ids.any? - ForemanTasks.dynflow.world.chain( - sibling_task_ids, - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: triggered_by_version_id - ) - else - ForemanTasks.async_task( - ::Actions::Katello::ContentView::Publish, - composite_cv, - description, - triggered_by_id: triggered_by_version_id - ) - end - end - - # Find active (planning/planned/running) composite publish tasks (does NOT check scheduled tasks) def find_active_composite_publish_tasks(composite_cv) relevant_tasks = ForemanTasks::Task::DynflowTask diff --git a/test/actions/katello/content_view_test.rb b/test/actions/katello/content_view_test.rb index 0ee5c36652b..792072d5e24 100644 --- a/test/actions/katello/content_view_test.rb +++ b/test/actions/katello/content_view_test.rb @@ -249,7 +249,7 @@ class PublishTest < TestBase end context 'run phase' do - it 'triggers auto-publish for composite views' do + it 'schedules event for composite views' do composite_view = katello_content_views(:composite_view) action.stubs(:task).returns(success_task) @@ -258,18 +258,15 @@ class PublishTest < TestBase composite_content_view: composite_view, content_view: content_view) - # Mock the task relations to simulate no scheduled, no running composite, no sibling tasks + # Mock the task relations to simulate no scheduled composite ForemanTasks::Task::DynflowTask.stubs(:for_action) .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks .then.returns(stub(where: stub(select: []))) # Running composite check: none - .then.returns(stub(where: stub(select: []))) # Sibling check: none - - # Expect async_task to be called (no siblings, so no chaining) - ForemanTasks.expects(:async_task).with( - ::Actions::Katello::ContentView::Publish, - composite_view, - anything, - triggered_by_id: anything + + # Expect event to be scheduled + ::Katello::EventQueue.expects(:push_event).with( + ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, + composite_view.id ) plan_action action, content_view @@ -279,9 +276,8 @@ class PublishTest < TestBase it 'does nothing for non-composite view' do action.stubs(:task).returns(success_task) - # Should not trigger any auto-publish tasks - ForemanTasks.expects(:async_task).never - ForemanTasks.dynflow.world.expects(:chain).never + # Should not trigger any auto-publish events + ::Katello::EventQueue.expects(:push_event).never plan_action action, katello_content_views(:no_environment_view) run_action action diff --git a/test/models/content_view_version_auto_publish_test.rb b/test/models/content_view_version_auto_publish_test.rb index 57bdf04599a..fd7b2e382f0 100644 --- a/test/models/content_view_version_auto_publish_test.rb +++ b/test/models/content_view_version_auto_publish_test.rb @@ -45,7 +45,7 @@ def test_auto_publish_schedules_event_when_no_composite_activity # Stub to return no scheduled, no running composite ForemanTasks::Task::DynflowTask.stubs(:for_action) .returns(stub(where: stub(any?: false))) # Scheduled check: no scheduled tasks - .then.returns(stub(where: stub(select: []))) # Running composite check: none + .then.returns(stub(where: stub(select: []))) # Running composite check: none ::Katello::EventQueue.expects(:push_event).with( ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, @@ -99,5 +99,21 @@ def test_auto_publish_skips_when_composite_already_scheduled @component1_version.auto_publish_composites!(task_id) end + def test_scheduled_task_for_composite_handles_errors_gracefully + task_id = SecureRandom.uuid + composite_task = stub(external_id: task_id) + + # Mock dynflow world to raise an error when loading delayed plan + world_stub = ForemanTasks.dynflow.world + persistence_stub = stub + persistence_stub.stubs(:load_delayed_plan).raises(Dynflow::Error, "Delayed plan not found") + world_stub.stubs(:persistence).returns(persistence_stub) + + # Should log error and return false instead of raising + Rails.logger.expects(:error).with(regexp_matches(/Failed to check scheduled task/)) + + result = @component1_version.send(:scheduled_task_for_composite?, composite_task, @composite_cv) + refute result + end end end