Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #37139 - Block content view publish during repository publication tasks #10879

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Feb 2, 2024

What are the changes introduced in this pull request?

Blocks content view publish task while certain repo actions (Sync/upload/remove content) are in progress. Similarly block these repo actions when corresponding CV publish is in progress.

Considerations taken when implementing this change?

There are multiple places in CV publish orchestration where we fetch repository contents. If repository contents are changed during a publication, different steps in the publish workflow will receive different versions of the repository with possibly different contents. This would lead to improper cv publications.

What are the testing steps for this pull request?

Have a few repos and cvs with those repos.
Try a combination of actions like sync/upload/remove content/generate metadata and associated CV publish to make sure the 2-way blocking works as expected.

@sjha4 sjha4 marked this pull request as ready for review February 5, 2024 15:00
@wbclark wbclark self-assigned this Feb 5, 2024
@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch from 8df888e to 2899e12 Compare February 6, 2024 15:03
app/models/katello/content_view.rb Outdated Show resolved Hide resolved
app/models/katello/content_view.rb Outdated Show resolved Hide resolved
@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch 2 times, most recently from 40ffc43 to 9297d0c Compare February 8, 2024 18:05
@sjha4
Copy link
Member Author

sjha4 commented Feb 9, 2024

[test katello]

@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch from 9297d0c to 755f8d8 Compare February 12, 2024 19:17
@wbclark
Copy link
Contributor

wbclark commented Feb 14, 2024

Attempted to publish a CV while one if its repos was syncing:

500
Pending tasks detected in repositories of this content view. Please wait for the tasks: - http://centos8-stream-wclark-katello-devel.example.com/foreman_tasks/tasks/7590b00d-5532-49bf-9cbc-2377d1bca5de before publishing.

The CV publish task immediately went to stopped/error. The repository sync task remained running/pending.

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing an offline chat we had about this:

  1. Should this blocking behavior be controlled by a setting, allowing users with hundreds of repos, automated sync plans, etc to opt out of the blocking behavior to avoid these collisions? Is the current pre-blocking behavior that the CV can be published with the exact repo content that existed prior to the sync, or is there any data inconsistency possible due to the repo contents changing during publishing?

  2. Is there a way that we can block these tasks, still clearly informing the user of what is happening when they try to initiate it, without having a "scary" red stopped/error task showing up on the monitor tasks page thereafter? E.g. could we check for blockers and surface them before the task even gets created, or else have the task go to stopped/warning instead (without changing the entire rescue_strategy for other modes of failure, and still surfacing a UI notification explaining what is happening?) @adamruzicka do you have any ideas about this?

@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch 2 times, most recently from 8dcd78b to 06f038b Compare February 16, 2024 15:32
@sjha4
Copy link
Member Author

sjha4 commented Feb 16, 2024

Should this blocking behavior be controlled by a setting, allowing users with hundreds of repos, automated sync plans, etc to opt out of the blocking behavior to avoid these collisions? Is the current pre-blocking behavior that the CV can be published with the exact repo content that existed prior to the sync, or is there any data inconsistency possible due to the repo contents changing during publishing?

After looking at the CV publish code flow, I am leaning towards blocking it for everyone to prevent any changes between repo contents when the publish starts to when it finishes. Cause many actions in the code path independently looks up repository and contents during the flow and changing repository between these sub actions would lead to unexpected results in the published version.

E.g. could we check for blockers and surface them before the task even gets created.

We could potentially move the validations to the API layer and block creation of task. However syncs and other blocked repo actions are started from multiple places outside of API. ex: sync plans which directly trigger sync action. Having the blocking at action layer would handle all cases including foreman-rake invocations or any rake tasks. I don't think we need exclusive locks on repo during sync or any other actions however and a custom validation to only block publish and nothing else would suffice for this.

@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch 2 times, most recently from f6362ae to adb4189 Compare February 20, 2024 18:39
@sjha4 sjha4 force-pushed the cv_publish_repo_actions branch from adb4189 to 2a2ef80 Compare February 20, 2024 19:10
@sjha4
Copy link
Member Author

sjha4 commented Feb 28, 2024

[test katello]

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We can change one commit from "Fixes" to "Refs" or squash on merge

.for_resource(self)
.order(:started_at)
.last
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you anticipate that we may use this in other areas in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily..However, this is the list of tasks with potential to create a new publication. So if we need to block anything on itthe future, a repo.check_ready_to_act! should work for that use-case.

@sjha4 sjha4 merged commit ebfd49b into Katello:master Mar 4, 2024
14 of 15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants