Remove AutoPublishCompositeView event in favor of a task

[jturel]

What are the changes introduced in this pull request?

Removes the AutoPublishCompositeView event with a task that emulates the same behavior of the event + event queue retry function. Is the design totally insane or is there something to this?

Considerations taken when implementing this change?

Maintain the intention of the event in that the task will continue trying to trigger the Publish task until it's not locked by another task.

What are the testing steps for this pull request?

Publish a CV:
ForemanTasks.async_task(Actions::Katello::ContentView::AutoPublish, Katello::ContentView.find(2), description: 'fake', triggered_by: 49)

And while it's running:

ForemanTasks.async_task(Actions::Katello::ContentView::AutoPublish, Katello::ContentView.find(2), description: 'fake', triggered_by: 49)

Of course, you need your own values for the CV and triggered by id (which is a CVV)

Summary by Sourcery

Replace the AutoPublish composite view event mechanism with a task-based workflow that triggers composite content view auto-publish via ForemanTasks.

New Features:

• Introduce an AutoPublish task action for content views that retries publish when locks are present and integrates with BulkAction for batching composite publishes.

Bug Fixes:

• Ensure composite auto-publish is skipped when no eligible composite content views are configured, avoiding unnecessary background work.

Enhancements:

• Trigger composite auto-publish from the publish action via an execution plan hook instead of the run phase.
• Add helper methods on content view and content view version to compute composite views to auto-publish and the options passed to async tasks.

Tests:

• Update publish action tests to cover the new auto-publish task behavior and remove event-based expectations.
• Add unit tests for content view version auto_publish_composites! behavior, including the no-composites case.

Chores:

• Remove the AutoPublishCompositeView event class, its registration, and associated tests now that the behavior is handled by tasks instead of events.

[jturel]

AutoPublish can change to handle N >1 sub plans so a BulkAction probably shouldn't / needn't be used

[ianballou]

I haven't tested this yet, but the logic makes sense. I don't think the more internal Dynflow / Foreman Tasks methods being used here are risky APIs to rely on since they're pretty core to the plugins.

My bigger question is if we are losing any important safety mechanisms by moving to Foreman Tasks for CVV auto publish. The event queue has event de-duplication and a MAX_RETRY_AGE for rescheduled events.

The de-duplication seems to only matter if two content view children finish publishing at the same time and trigger the same auto-publish event. Unlikely, but not impossible, especially for smaller content views. I think I may address this in #11540 by not creating a new auto-publish event when there is already a CCV publish scheduled.

The MAX_RETRY_AGE is nice so that the retries don't get stuck forever, but I suppose users could just delete the suspended task if it's stuck. The events are generally hidden without the user knowing what's going on, so having an age for event retrying made sense. With Dynflow, users can see what is scheduled, so I think it may be less important to have a timeout.

[jturel]

Thanks for taking a look!

My bigger question is if we are losing any important safety mechanisms by moving to Foreman Tasks for CVV auto publish. The event queue has event de-duplication and a MAX_RETRY_AGE for rescheduled events.

Good points. My thoughts:

The retry mechanism was added to handle the LockConflict errors in the same PR which added the auto publish event. It's the only condition that blocks the new action and it's reasonable that the lock will free eventually. If some other error is raised the action should fail, which is good - it'll bring visibility to potential bugs which the event queue hides because it silently throws away events with unhandled exceptions.

The de-duplication seems to only matter if two content view children finish publishing at the same time and trigger the same auto-publish event. Unlikely, but not impossible, especially for smaller content views. I think I may address this in #11540 by not creating a new auto-publish event when there is already a CCV publish scheduled.

The original intent of the event was to serialize auto publishing and avoid locks as the event queue is single threaded. De-dup is more of a side effect. I might wait for some user feedback which necessitates solving that. Devil's advocate: the de-deduplication alters the viewable CV publish history ie "Auto-publish triggered by ..." which could be confusing because the history someone expects wouldn't be reflected.

How long does a really big, slow composite CV publish these days?

The MAX_RETRY_AGE is nice so that the retries don't get stuck forever, but I suppose users could just delete the suspended task if it's stuck. The events are generally hidden without the user knowing what's going on, so having an age for event retrying made sense. With Dynflow, users can see what is scheduled, so I think it may be less important to have a timeout.

Agree!

[ianballou]

Devil's advocate: the de-deduplication alters the viewable CV publish history ie "Auto-publish triggered by ..." which could be confusing because the history someone expects wouldn't be reflected.

De-duplication became a bit more of an apparent issue in #11540 because there is no reason to have multiple composite content view publishes if the composite is now waiting for all of its children to stop publishing. Without my chaining PR though, I think it's less important.

How long does a really big, slow composite CV publish these days?

Without dependency solving (which we tell users generally not to turn on), the largest CV publishes should be a matter of hours. The slowest part these days is usually indexing the content units from Pulp.

[jturel]

De-duplication became a bit more of an apparent issue in #11540 because there is no reason to have multiple composite content view publishes if the composite is now waiting for all of its children to stop publishing. Without my chaining PR though, I think it's less important.

Gotcha - I'm still parsing that PR. Let me think about this.

[jturel]

@ianballou pushed a change that should solve deduplication. it works like this:

• New model ContentViewAutoPublishRequest which tracks the CV and CVV. There can only be one request per CV (but a version could have many)
• The pre-auto publish process attempts to create a request per CV. If one exists it means an auto publish is waiting to happen or is currently creating the async Publish task
• The AutoPublish action locks (via action_subject) the publish request object which is then deleted once the Publish task is created, allowing further auto publishes to happen for a CCV

[ianballou]

At first I thought the new deduplication here would fully replace the de-duplication I have to do in #11540, but I realized there are two different cases for de-duplication of composite CV auto publishes

This PR de-duplicates requests for auto publish events that are polling. #11540 de-duplicates requests specifically when there is an auto publish that is scheduled because it has some component children publishing.

Thanks for the update, I am testing it now in combination with my other PR.

[jturel]

Thanks for the update, I am testing it now in combination with my other PR.

Cool, I just pushed a commit that fixes a bug around one of the queries .. you'll want to pull latest!

[ianballou]

I've been testing this in combination with my other PR, so far things are good. I need to test this alone after because it might make more sense for this to merge first, but we'll see.

[ianballou]

Actually, I should ask, do you have a preference about the merge order? If this merges first, this PR will avoid the merge conflicts but some things might change in the other PR when the rebase happens. If this merges last, there'll be more work here but you'll have more control over the combining of chaining + event-less auto publishes.

[jturel]

Actually, I should ask, do you have a preference about the merge order? If this merges first, this PR will avoid the merge conflicts but some things might change in the other PR when the rebase happens. If this merges last, there'll be more work here but you'll have more control over the combining of chaining + event-less auto publishes.

I'm leaning toward this going in first - it feels more natural to me. Since you're already deep into the work of chaining I think you'd be better suited for marrying the two. Seems like this might be closer to being done as well?

[ianballou]

This is working nicely by itself. Tested:

1. Publishing two component children that finish very close to each other an noticed only one composite auto-publish (de-duplication)
2. Published a composite and then one of its component children. Saw that an auto-publish occurred after the first composite publish finished.

[ianballou]

Since this auto publish action is a coordinator of sorts, I was a little confused seeing an auto publish event and a publish event later.

What do you think about updating the humanized name to tell the user that this is just the auto publish scheduler doing its thing?

Maybe something like "Coordinate composite content view auto publish".

[jturel]

Great idea

[ianballou]

I'm leaning toward this going in first - it feels more natural to me. Since you're already deep into the work of chaining I think you'd be better suited for marrying the two. Seems like this might be closer to being done as well?

Sure, this works for me. +1 to this being a quicker merge too.

[ianballou]

Don't worry about these until it's out of draft form.

[ianballou]

Blocking reminder to eventually remove this.

[ianballou]

Reminder that the log statements will need to eventually be removed or turned into debug logs as well.

[jturel]

Closing in favor of #11611