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 #37600 - Add params to skip Candlepin content on repository remove #11052

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Jun 26, 2024

What are the changes introduced in this pull request?

  • We currently have 2 params available to skip Candlepin content during repo deletion skip_environment_update destroy_content these params were not exposed to the user.
  • Added skip_candlepin_environment_update which maps to skip_environment_update and skip_candlepin_remove_content which maps to destroy_content
  • Added unit tests

What are the testing steps for this pull request?

  • SSH to reproducer(IP in Jira)
  • Try to delete a repo that causes the error hammer repository destroy --id 1457 --organization-id 1
  • Apply patch file at bottom of PR since it's Katello 4.7
    • cd /usr/share/gems/gems/katello-4.7.0.36
    • patch -p1 < <patchfile>
    • foreman-maintain service restart
  • Try to delete a repo that would throw an error with new params hammer repository destroy --id 1458 --organization-id 1 --skip-candlepin-environment-update true --skip-candlepin-remove-content true

Patch file:
katello.patch

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Seems to work fine, ran the instructions in the PR exactly on the reproducer. Just wanted to check -- 1458 on the reproducer was also an erroring repository?

One question -- how will a user know to run this workflow? I wonder if we can catch the Unable to find content with the ID "8799". error and tell the user to try deleting the repo with those parameters.

@chris1984
Copy link
Member Author

chris1984 commented Jul 15, 2024

Seems to work fine, ran the instructions in the PR exactly on the reproducer. Just wanted to check -- 1458 on the reproducer was also an erroring repository?

One question -- how will a user know to run this workflow? I wonder if we can catch the Unable to find content with the ID "8799". error and tell the user to try deleting the repo with those parameters.

I am not sure the best way, we can try to find those items in candlepin and we can't find them just skip those items in the task as well as have the params I exposed if the user knows the except repos #'s. To answer your other questions, yeah any of the codebuilder repos were failing in that reproducer, prob some others but that is what was in the bug.

@ianballou
Copy link
Member

What would you think about adding a rescue where the missing content error pops up and return a custom error saying something like "Try deleting the repository with blah options"?

@ianballou
Copy link
Member

Otherwise I suppose we'd need to add an entry to the debugging section in the Foreman docs

@chris1984
Copy link
Member Author

What would you think about adding a rescue where the missing content error pops up and return a custom error saying something like "Try deleting the repository with blah options"?

My worry there is then the task will be half complete and the repos will be out of Pulp/Katello so won't it fail the next time on the first part of the delete? Is there a way to do a dry run first before it deletes anything?

@ianballou
Copy link
Member

My worry there is then the task will be half complete and the repos will be out of Pulp/Katello so won't it fail the next time on the first part of the delete? Is there a way to do a dry run first before it deletes anything?
Yeah, it's too bad that the Unable to find content with the ID "8799" error is likely the only way a user will know to try deletion with the new options.

If that's unavoidable, I could see a user hitting the error, having one messed up repo, and then going to some document teaching them how to find the rest of the broken repositories and then fixing them following the guide.

I can't remember, do we have a rake task that'll find these repos that are broken and need deleting? If so, I suppose the rake task could teach the user of the new options.

@ianballou
Copy link
Member

(Also, the PR needs a rebase for the tests to run)

@ianballou
Copy link
Member

we can try to find those items in candlepin and we can't find them just skip those items in the task

I missed this before, I think this totally makes sense so the user wouldn't even have to think about it. Maybe just a warning log to say that the content deletion was skipped.

@chris1984 chris1984 force-pushed the fix-cprepo branch 2 times, most recently from 12be414 to e118736 Compare July 17, 2024 15:56
@Ron-Lavi
Copy link
Contributor

@chris1984 can you rebase? @ianballou mind having another look? thanks

@chris1984
Copy link
Member Author

Ian is helping me get it working, but Sayan said it might be safe to push it to 6.17. I'll keep trying to make it work

@sayan3296
Copy link
Contributor

Based on a few of the recent occurrences of this issue, I believe I would be happy with the implementation if it would be something like this :

  • There are some orphaned repos present and some good repos present and part of the same CV version [ lifecycle ]
  • User started deleting one or more good + orphaned repos
  • Some of their content ids are missing but Katello only logs a warning about it and skips the SetContent step
  • Completes the deletion successfully without any manual intervention needed

As long as this would be the workflow for the deletion of a repo, I am happy to have the fix delivered as soon as possible.

While the issue also could happen during repo creation, 90% of the cases encountered are related to deletion only. So for now, let's fix the deletion part and make it as smooth as we can.

@chris1984 @ianballou jfyi ^^

@chris1984
Copy link
Member Author

Based on a few of the recent occurrences of this issue, I believe I would be happy with the implementation if it would be something like this :

  • There are some orphaned repos present and some good repos present and part of the same CV version [ lifecycle ]
  • User started deleting one or more good + orphaned repos
  • Some of their content ids are missing but Katello only logs a warning about it and skips the SetContent step
  • Completes the deletion successfully without any manual intervention needed

As long as this would be the workflow for the deletion of a repo, I am happy to have the fix delivered as soon as possible.

While the issue also could happen during repo creation, 90% of the cases encountered are related to deletion only. So for now, let's fix the deletion part and make it as smooth as we can.

@chris1984 @ianballou jfyi ^^

That works for me, I am wrapping up 2 6.16 bugs, then will get back to this one so we can get this possibly into a 6.16.z

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants