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

[WIP] Purge miq requests every 6 months #21577

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

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Nov 29, 2021

We have no process to clear out miq requests.

If we do not want to automatically delete these, I would feel more comfortable putting most of this PR in but removing the scheduler to automatically create them.

I am concerned about MiqRequestTask.where().destroy_all. I remember there was some trick with the tasks that do have child tasks, but can't remember the actual nuance. For this reason, I stuck with destroy. But we are going to bring a lot of records over the wire. MiqApproval is more straight forward and we can use delete_all

@jrafanie
Copy link
Member

I am concerned about MiqRequestTask.where().destroy_all

I wonder if we need to look for the related miq_requests rows associated with the tasks. If all tasks associated with the request have been removed, we might want to delete that too. I caution you that I started on this but was stuck because I have no idea how to test this for real other than made up specs.

You can see my similar work, but for queue work that was in flight when a worker was killed, here: https://github.com/ManageIQ/manageiq/pull/21485/files

@jrafanie
Copy link
Member

I am concerned about MiqRequestTask.where().destroy_all

I wonder if we need to look for the related miq_requests rows associated with the tasks. If all tasks associated with the request have been removed, we might want to delete that too. I caution you that I started on this but was stuck because I have no idea how to test this for real other than made up specs.

You can see my similar work, but for queue work that was in flight when a worker was killed, here: #21485 (files)

Note, I'm ok with waiting on that. I honestly just don't know enough of what the various tables hold and how they're used with each type of thing, provision, services, etc. to say what's right or wrong.

Comment on lines +22 to +23
MiqRequestTask.where(:miq_request_id => ids).destroy_all
MiqApproval.where(:miq_request_id => ids).delete_all
Copy link
Member

Choose a reason for hiding this comment

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

Should this be done in batches or does the mixin already do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

the miq requests are batched. So I was thinking that we would just get rid of all the associated approvals.
Since this is a delete_all, no records will come back over the wire so I was less concerned around this one.

(it is the destroy_all that is concerning to me)

Copy link
Member

Choose a reason for hiding this comment

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

yeah I was talking about the destroy_all in batches - didn't realize I commented on the wrong line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm guessing there are probably 1 task per request. So the batching size should be about the same.
Not sure if this does it one by one.

Since we implemented purge_associated_records, we download all the ids.

@jrafanie
Copy link
Member

@kbrock did you get a chance to look at #21577 (comment) ? I don't know if we can clean up some of these without getting their associated other requests/tasks. Maybe it will just work since they will all get purged after 6 months anyway? I don't know. Maybe this PR is just simpler.

@kbrock kbrock changed the title Purge miq requests every 6 months [WIP] Purge miq requests every 6 months Jan 10, 2022
@kbrock kbrock added the wip label Jan 10, 2022
@kbrock
Copy link
Member Author

kbrock commented Jan 10, 2022

WIP: I do not know how to detect if this workflow is still running. (per @jrafanie comments) So it is possible that we destroy a long running workflow. Although, a workflow that runs for 6 months is quite long.

Not purging associated records but purging all tasks/requests older than a certain date would work. I just worry that we delete a request that is part of a service template or something. I seem to remember workflows being creative in how they stored the templates.

@kbrock
Copy link
Member Author

kbrock commented Feb 2, 2022

@jrafanie do we want to merge this with #21485 ?

There was more thought for what records want to be deleted over there
Here I focused on the purging infrastructure.

I think the main thing that needs to be done with this pr is we need to change purged_scope to properly determine which records are able to be removed. (separate from what time frame we want to remove and batching)

We possibly want to change the way associated records need to be purged, but that would be an optimization

@miq-bot
Copy link
Member

miq-bot commented Feb 27, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock
Copy link
Member Author

kbrock commented Jul 27, 2023

outstanding: not sure how to determine which workflows are still running, so can't determine if it is safe to delete a workflow.

@kbrock kbrock reopened this Jul 27, 2023
@miq-bot miq-bot closed this Jul 31, 2023
@miq-bot
Copy link
Member

miq-bot commented Jul 31, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@Fryguy Fryguy reopened this Jul 31, 2023
@kbrock
Copy link
Member Author

kbrock commented Aug 1, 2023

update

  • rebase to fix conflict

Still WIP: just like #21485 - tricky to determine whether a workflow is still in use, and therefore don't want to delete a request out from underneath the workflow

@miq-bot miq-bot closed this Nov 6, 2023
@miq-bot
Copy link
Member

miq-bot commented Nov 6, 2023

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

@kbrock kbrock reopened this Nov 13, 2023
@miq-bot
Copy link
Member

miq-bot commented Nov 13, 2023

Checked commit kbrock@61971c6 with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
5 files checked, 0 offenses detected
Everything looks fine. 🍰

@miq-bot
Copy link
Member

miq-bot commented Jan 5, 2024

This pull request is not mergeable. Please rebase and repush.

@miq-bot
Copy link
Member

miq-bot commented Apr 8, 2024

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

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.

None yet

4 participants