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 #35740 - Composite content view versions can be emptied out during same-repo merging #10350

Merged

Conversation

ianballou
Copy link
Member

What are the changes introduced in this pull request?

This PR stops repositories from being completely cleared out at composite CV publish time. The remove_all flag comes from a bit of the CopyAllUnits action that only runs when the related CV is composite.

Considerations taken when implementing this change?

I was considering refactoring the part of CopyAllUnits that pertains to composite CVs to use copy_all, since filters shouldn't be present. However, since this piece of code may make it back to older versions of Katello, I wanted to keep it as simple as possible. I left a TODO for getting to this down the road.

What are the testing steps for this pull request?

Follow the steps from the issue:

  1. Create two component content views with the same repository in it
  2. Add a filter to one of the repositories to clear out all content
  3. Publish both content views
  4. Create a composite content view with both component content views in it
  5. Publish that content view
  6. If there is any content in the composite content view version, add source_repositories = source_repositories.reverse here: https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/orchestration/repository/copy_all_units.rb#L21
  7. The next publish of the composite version should result in no content being available in the component content view version

Also, it would be work running an incremental update on a component CV version that is part of a composite and ensuring that the resulting composite incremental CV version makes sense. In particular, try running incremental update twice on a version with different RPM packages. For example, incrementally add foo.rpm to version 1.0, creating 1.1, and then incrementally add bar.rpm to version 1.0, creating version 1.2. I want to ensure that, in the incremental update case, the content is cleared out appropriately. There have been issues in the past with incremental versions affecting each other (e.g. version 1.1 gets foo.rpm, version 1.2 gets bar.rpm and foo.rpm when it was only requested to get bar.rpm).

@theforeman-bot
Copy link

Issues: #35740

@sjha4
Copy link
Member

sjha4 commented Nov 14, 2022

@ianballou : Can you expand on this.

If there is any content in the composite content view version, add source_repositories = source_repositories.reverse here: https://github.com/Katello/katello/blob/master/app/lib/actions/pulp3/orchestration/repository/copy_all_units.rb#L21
The next publish of the composite version should result in no content being available in the component content view version

When 2 components have the same repo, and one has all content filtered out, we want the composite to get it's content from the component that does have all the content. Is that right?

@ianballou
Copy link
Member Author

When 2 components have the same repo, and one has all content filtered out, we want the composite to get it's content from the component that does have all the content. Is that right?

To simplify this, when N components have the same repo, we want to merge them all together. It shouldn't matter if some are completely empty.

I say to reverse the source_repositories because the order in which they get processed in the CopyAllUnits matters. I'm assuming you only have two components. If the empty repo comes first, then just the version will be copied, and then the correct repository will have its contents copied into that version. If the empty repo comes last, then the "good" repo will have its version copied, but then the empty repo will trigger the incorrect clearing out of the composite repository. What I am describing is the code in this else statement: https://github.com/Katello/katello/pull/10350/files#diff-9165e49eb367f9d37a3b6674540d5a37dd9966c8e05205673b6d2f30d11e29a4R20-R31

Also my tests steps are for reproducing the error. To fix, just apply the PR and then try again. Make sure you keep the source_repositories order when you test again.

Copy link
Member

@sjha4 sjha4 left a comment

Choose a reason for hiding this comment

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

Works for incremental updates with a cleared out repo and incremental update on component CV.
Screenshot from 2022-11-16 15-25-07
Composite CV contents matching the CV with repo after auto-publishes.
Screenshot from 2022-11-16 15-24-44

@ianballou ianballou merged commit d793dda into Katello:master Nov 16, 2022
@ianballou ianballou deleted the 35740-composite-clearing-repo-issue branch November 16, 2022 21:47
chris1984 pushed a commit to chris1984/katello that referenced this pull request Dec 8, 2022
chris1984 pushed a commit that referenced this pull request Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants