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

Retain old JobResults #8956

Closed
kkthxbye-code opened this issue Mar 24, 2022 · 11 comments · Fixed by #8957
Closed

Retain old JobResults #8956

kkthxbye-code opened this issue Mar 24, 2022 · 11 comments · Fixed by #8957
Assignees
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application

Comments

@kkthxbye-code
Copy link
Contributor

kkthxbye-code commented Mar 24, 2022

NetBox version

v.3.1.9

Feature type

Change to existing functionality

Proposed functionality

Currently all old JobResults are deleted when a new JobResult is in a terminal state. I propose that old JobResults are preserved.

Use case

Deleting old JobResults make it hard to use for automation through the API. Imagine a case where two runs of the same script is triggered via. the API. The API returns an id for the JobResult that can be used to query for the result. The RQ worker will run the scripts one after the other. When the first script finishes, you have the runtime of the second script to get the result. If you do not retrieve the result before the second script finishes, the JobResult is deleted and impossible to retrieve.

Also contributing to the issue is that JobResults are not webhook enabled, which could provide a workaround in some cases. But that's a different feature request.

Database changes

None

External dependencies

No response

@kkthxbye-code kkthxbye-code added the type: feature Introduction of new functionality to the application label Mar 24, 2022
@kkthxbye-code
Copy link
Contributor Author

I have a working implementation shortly as a PR.

@jeremystretch
Copy link
Member

jeremystretch commented Mar 24, 2022

I think this makes sense. The first question that comes to mind is: How will we handle the eventual cleanup of old JobResult instances? Presumably via the housekeeping management command? We'd also need to introduce a configuration parameter to dictate the expiration age similar to CHANGELOG_RETETION.

Also contributing to the issue is that JobResults are not webhook enabled, which could provide a workaround in some cases. But that's a different feature request.

#8958

@jeremystretch jeremystretch added the status: under review Further discussion is needed to determine this issue's scope and/or implementation label Mar 24, 2022
@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented Mar 24, 2022

I actually did consider adding it to the housekeeping command, but figured that as JobResults, opposed to changelog entries, are deletable in the admin panel, it wouldn't be nessecary.

I can add it if you think it would be the best way to handle it. How about default retention? Unlimited would be my preference , but I'm not sure how much data people are actually outputting from their scripts.

@jeremystretch
Copy link
Member

IMO unlimited (None) is a reasonable and safe default.

@kkthxbye-code
Copy link
Contributor Author

I added a JOBRESULT_RETENTION option, added the functionality to the housekeeping script and updated the documentation. Let me know if I missed anything.

@jeremystretch
Copy link
Member

Since this is a fairly substantial change to the existing behavior, I'd like to tag this for v3.2 (due to be released the week of April 4th).

@kkthxbye-code would you mind rebasing your PR off the feature branch? I don't think you'll run into much conflict, but please let me know if you need any help.

@kkthxbye-code
Copy link
Contributor Author

kkthxbye-code commented Mar 25, 2022

@jeremystretch - I haven't rebased to a different parent before, could you maybe give a little hint. My google-fu comes short here, as there seems to be a bunch of different ways to get the same result.

I've tried git rebase --onto origin/feature origin/develop and then git add netbox/extras/management/commands/runscript.py as that file was shown as "deleted by us:" for some reason. Then force pushing with git push origin HEAD:save-job-results --force. It looked kinda correct, but when I viewed the pull request on github it indicated merge conflicts and it was still trying to merge into develop.

I'm kinda lost I guess :)

@DanSheps
Copy link
Member

DanSheps commented Apr 8, 2022

Should we target this for v3.3 now or release it as a minor in v3.2?

@jeremystretch
Copy link
Member

I think it's reasonable to implement this in v3.2, given that:

  • There are no changes to the reports or scripts APIs
  • The change will be largely transparent to users
  • I can't think of any way this would disrupt existing workflows

However, perhaps we should alter the default JOBRESULT_RETENTION value from 0 (indefinite) to 90 days (matching the default for CHANGELOG_RETENTION). This will ensure that even an oblivious user doesn't end up with an enormous backlog of job results, provided they are running the housekeeping daemon as advised in the documentation.

@jeremystretch jeremystretch added status: accepted This issue has been accepted for implementation and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels Apr 8, 2022
@kkthxbye-code
Copy link
Contributor Author

@jeremystretch - I changed the default to 90. While I prefer unlimited by default, I can see the rationale for 90.

When merged I'll start looking at scheduling jobs. I also think it might make sense to move job results out of the admin ui at some point, espcially if scheduled tasks are on the table.

Also while I tested this pretty extensively, it would ease my mind if one of you guys would just do a small verification to see if everything functions as intended.

@jeremystretch
Copy link
Member

I changed the default to 90. While I prefer unlimited by default, I can see the rationale for 90.

Thanks! It's all about trying to protect the user ultimately.

I also think it might make sense to move job results out of the admin ui at some point, espcially if scheduled tasks are on the table.

Agreed; we should implement a "proper" UI view as part of the scheduling work.

Also while I tested this pretty extensively, it would ease my mind if one of you guys would just do a small verification to see if everything functions as intended.

Will do! 👍

jeremystretch added a commit that referenced this issue Apr 12, 2022
jeremystretch added a commit that referenced this issue Apr 12, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: feature Introduction of new functionality to the application
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants