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.

Sign up for GitHub

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

Jump to bottom

Add queue to delete site builds older than 180 days #4352

Closed
wants to merge 7 commits into from
Closed

Add queue to delete site builds older than 180 days #4352

wants to merge 7 commits into from

Conversation

svenaas
Copy link
Contributor

@svenaas svenaas commented Jan 4, 2024 •
edited
Loading

Resolves #4336

Changes proposed in this pull request:

  • Adds a job, queued and run nightly, that deletes builds older than 180 days
  • Changes the language shown in the client for sites which have no builds to reflect that they have had no builds "in the past 180 days" (was "not yet" any builds)

security considerations

None. The new nightly job will remove build records from the application database for which build logs have already expired and been removed from S3.

@svenaas
Copy link
Contributor Author

svenaas commented Jan 4, 2024

Open questions:

  1. Should we, at this time, as @drewbo suggested, have the build log archive job run in dev and staging as well? As of this writing the new older build deletion job introduced with this PR will run in all environments, both for cleanup and for peace of mind in having it run there before production.
  2. Where's the best place to add a test for the deletion code? Currently, following the pattern of the archive build logs queue, this is in deleteOlderBuilds.js. jobProcessors.test.js verifies that it calls the expected Sequelize method and handles errors, but not that it removes the correct builds. Would it be better to add that to jobProcessors.test.js (the other queued jobs aren't tested in that way there) or to refactor the deletion call into a method on the Build model (or into a new service as with build logs) so that it can be covered by unit testing?

@svenaas
Copy link
Contributor Author

svenaas commented Jan 4, 2024

Going with yes on question 1.

@svenaas
Copy link
Contributor Author

svenaas commented Jan 8, 2024

Going with jobProcessors.test.js on question 2.

@svenaas svenaas changed the title (WIP) Add queue to delete site builds older than 180 days Add queue to delete site builds older than 180 days Jan 8, 2024
@svenaas svenaas marked this pull request as ready for review January 8, 2024 19:06
@svenaas svenaas requested a review from apburnes January 8, 2024 19:10
apburnes
apburnes reviewed Jan 9, 2024
View reviewed changes
Copy link
Contributor

@apburnes apburnes left a comment

Choose a reason for hiding this comment

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

It looks good just a couple of comments.

Also, let's add a Queues section to OPERATIONS.md with the subsection on this delete older builds job. We need to start documenting more of the platform.

api/workers/index.js Outdated
@@ -85,6 +87,8 @@ function pagesWorker(connection) {
new QueueWorker(ArchiveBuildLogsQueueName, connection,
path.join(__dirname, 'jobProcessors', 'archiveBuildLogsDaily.js')),
new QueueWorker(BuildTasksQueueName, connection, buildTasksProcessor),
new QueueWorker(DeleteOlderBuildsQueueName, connection,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an older syntax and we hadn't updated the queue worker for ArchiveBuildLogsQueueName yet. Let's import the processor function and then create a new job function in this file like how we did the buildTasksProcessor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done.

api/workers/index.js
appEnv === 'production'
? archiveBuildLogsQueue.add('archiveBuildLogsDaily', {}, nightlyJobConfig)
: Promise.resolve(),
archiveBuildLogsQueue.add('archiveBuildLogsDaily', {}, nightlyJobConfig),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we create the buckets for log archives in dev and staging and added the bucket service to the app manifest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only change I have made is this provisional code change. I didn't realize the buckets didn't exist, so that probably has to happen.

But FWIW the bucket service is already parameterized in manifest.yml as federalist-((env))-s3-build-logs. Would that not allow for the presence of these dev and staging buckets?

Copy link
Contributor

Choose a reason for hiding this comment

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

it looks like non of the archive build log jobs were successful in dev so I was flagging as a possible culprit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked dev and build logs aren't archiving nightly and builds over 180 days aren't deleting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. It also looks like the migration on this PR hasn't run:

[...] column Build.deletedAt does not exist

svenaas added a commit that referenced this pull request Jan 10, 2024
@svenaas
chore: Add Queues section to OPERATIONS.md, starting with delete-olde…
52a3b37 
…r-builds #4352
@svenaas svenaas mentioned this pull request Jan 10, 2024
Closed
svenaas added a commit that referenced this pull request Jan 11, 2024
@svenaas
chore: Add Queues section to OPERATIONS.md, starting with delete-olde…
352aa12 
…r-builds #4352
@svenaas svenaas force-pushed the 4336-add-queue-to-delete-older-builds branch from 52a3b37 to 352aa12 Compare January 11, 2024 15:34
@svenaas
Copy link
Contributor Author

svenaas commented Jan 11, 2024

@apburnes As of this writing this is back on dev, and build.deleted_at is present there. We'll see tonight whether the jobs are successful.

@svenaas svenaas force-pushed the 4336-add-queue-to-delete-older-builds branch from 352aa12 to 440ec49 Compare January 11, 2024 16:37
@svenaas
Copy link
Contributor Author

svenaas commented Jan 11, 2024

Also, let's add a Queues section to OPERATIONS.md with the subsection on this delete older builds job. We need to start documenting more of the platform.

This is also done.

@svenaas
Copy link
Contributor Author

svenaas commented Jan 16, 2024

@apburnes, it looks like both archive-build-logs and delete-old-builds have been completing on dev.

@svenaas svenaas requested a review from apburnes January 16, 2024 13:16
This was referenced Jan 18, 2024
Closed
Closed
Merged
@svenaas
Copy link
Contributor Author

svenaas commented Jan 18, 2024

Closing this, since we cancelled #4336 and are going a different route.

@svenaas svenaas closed this Jan 18, 2024
@svenaas svenaas mentioned this pull request Jan 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@apburnes apburnes Awaiting requested review from apburnes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add queue to delete site builds from database after 180 days
2 participants
@svenaas @apburnes