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 f23eb70d666..3932a77aa48 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 => options[:triggered_by] + :triggered_by_id => triggered_by_id ) source_repositories = [] content_view.publish_repositories(options[:override_components]) do |repositories| @@ -113,7 +115,9 @@ def humanized_name def run version = ::Katello::ContentViewVersion.find(input[:content_view_version_id]) - version.auto_publish_composites! + # Pass the current execution plan ID so auto_publish can coordinate + # with other component CV publishes using Dynflow chaining + 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/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..fe1bb9b5c39 100644 --- a/app/models/katello/content_view_version.rb +++ b/app/models/katello/content_view_version.rb @@ -361,18 +361,137 @@ 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) + + composite_cv.with_lock do + status = composite_publish_status(composite_cv) + + case status + when :scheduled + Rails.logger.info("Composite CV #{composite_cv.name} publish already scheduled, skipping duplicate") + next + 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 + end + end + 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 + 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) } + return :scheduled + end + + # Check running tasks (these have input populated) + if find_active_composite_publish_tasks(composite_cv).any? + return :running + end + + nil + end + + # Schedule an event to retry composite publish after current one finishes + 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, calling_task_id: component_task_id } + end + end + + # Check if a scheduled task is for the given composite CV by inspecting delayed plan args + 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 NoMethodError, TypeError, Dynflow::Error => e + Rails.logger.error("Failed to check scheduled task for composite CV #{composite_cv.name}: #{e.message}") + false + 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 + .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 + + relevant_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..576632d14ef 100644 --- a/app/models/katello/events/auto_publish_composite_view.rb +++ b/app/models/katello/events/auto_publish_composite_view.rb @@ -1,5 +1,10 @@ module Katello module Events + # 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 @@ -20,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/actions/katello/content_view_test.rb b/test/actions/katello/content_view_test.rb index cc1abec80d2..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 'creates auto-publish events for non-composite views' do + it 'schedules event for composite views' do composite_view = katello_content_views(:composite_view) action.stubs(:task).returns(success_task) @@ -258,23 +258,29 @@ class PublishTest < TestBase composite_content_view: composite_view, content_view: content_view) - plan_action action, content_view - run_action action + # 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 - event = Katello::Event.find_by(event_type: Katello::Events::AutoPublishCompositeView::EVENT_TYPE, object_id: composite_view.id) - version = content_view.versions.last + # Expect event to be scheduled + ::Katello::EventQueue.expects(:push_event).with( + ::Katello::Events::AutoPublishCompositeView::EVENT_TYPE, + composite_view.id + ) - assert_equal event.metadata[:triggered_by], version.id - assert_equal event.metadata[:description], "Auto Publish - Triggered by '#{version.name}'" + plan_action action, content_view + run_action action end it 'does nothing for non-composite view' do action.stubs(:task).returns(success_task) + # 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 - - 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..fd7b2e382f0 --- /dev/null +++ b/test/models/content_view_version_auto_publish_test.rb @@ -0,0 +1,119 @@ +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_schedules_event_when_no_composite_activity + task_id = SecureRandom.uuid + + # 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 + + ::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_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_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 + task_id = SecureRandom.uuid + composite_task_id = SecureRandom.uuid + + # 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) + + ForemanTasks::Task::DynflowTask.stubs(:for_action) + .returns(stub(where: scheduled_relation)) + + # Should not schedule event when already scheduled + ::Katello::EventQueue.expects(:push_event).never + + @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 diff --git a/test/models/events/auto_publish_composite_view_test.rb b/test/models/events/auto_publish_composite_view_test.rb index 9cce5a93ff3..24ab9f2be48 100644 --- a/test/models/events/auto_publish_composite_view_test.rb +++ b/test/models/events/auto_publish_composite_view_test.rb @@ -4,12 +4,21 @@ 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) + calling_task_id = SecureRandom.uuid + metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id } + + ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).with( + composite_view, + metadata[:description], + metadata[:version_id], + calling_task_id: calling_task_id + ) event = AutoPublishCompositeView.new(composite_view.id) do |instance| - instance.metadata = {} + instance.metadata = metadata end event.run @@ -23,10 +32,15 @@ def test_run_with_error end def test_run_with_lock_error - ForemanTasks.expects(:async_task).raises(ForemanTasks::Lock::LockConflict.new(mock, [])) + calling_task_id = SecureRandom.uuid + metadata = { description: "Auto Publish - Test", version_id: component_version.id, calling_task_id: calling_task_id } + + ::Katello::ContentViewVersion.expects(:trigger_composite_publish_with_coordination).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 }