Skip to content

Prune all operations in a recurring task - #18516

Draft
markylaing wants to merge 23 commits into
canonical:mainfrom
markylaing:ops-delete-task
Draft

Prune all operations in a recurring task#18516
markylaing wants to merge 23 commits into
canonical:mainfrom
markylaing:ops-delete-task

Conversation

@markylaing

Copy link
Copy Markdown
Contributor

Updates the operation prune task to run every minute. Operations with children are kept for 24 hours, but all other operations are deleted if they are complete and last updated more than 5 seconds ago.

This means that all operations will hang around for a bit longer, but none should be left lingering forever.

Closes #18475

Checklist

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts LXD’s operation cleanup behavior to prevent completed operations from lingering indefinitely in the database under heavy load by moving deletion responsibility into the recurring prune task and running it more frequently.

Changes:

  • Run the expired-operation prune task every minute instead of hourly.
  • Update the prune logic to retain parent (bulk) operations for 24 hours, while pruning other completed operations after a short grace period (~5s).
  • Remove immediate DB deletion from individual operation completion in favor of centralized pruning.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
lxd/operations/operations.go Changes in-memory operation cleanup on completion (removes prior delayed/DB cleanup goroutine).
lxd/operations/linux.go Updates pruning criteria to differentiate parent (bulk) vs other operations and introduces 5-second grace window for non-bulk.
lxd/operations.go Switches prune task scheduling from hourly to every minute.

Comment thread lxd/operations/operations.go Outdated
Comment thread lxd/operations/db.go
Comment thread lxd/operations/linux.go Outdated
@github-actions

github-actions Bot commented Jul 3, 2026

Copy link
Copy Markdown

This PR has no domain label assigned. @canonical/lxd-maintainers please set the appropriate domain labels.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.

Comment thread lxd/db/cluster/operations.go
Comment thread lxd/operations.go
Comment thread test/suites/clustering.sh
Comment thread lxd/operations/db.go Outdated
Comment thread lxd/operations/operations.go Outdated
@markylaing
markylaing force-pushed the ops-delete-task branch 3 times, most recently from 4f3bb35 to 8c453ba Compare July 10, 2026 14:36
Comment thread lxd/db/cluster/operations.go Outdated
Comment thread lxd/operations/operations.go Outdated
@markylaing
markylaing marked this pull request as ready for review July 13, 2026 13:00
@github-actions
github-actions Bot requested a review from simondeziel July 13, 2026 13:00
@markylaing

Copy link
Copy Markdown
Contributor Author

@tomponline @kadinsayani this is also now rebased and ready for review. There will be conflicts with operation stages so I'll fix up and rebase whichever one is merged second.

Comment thread lxd/operations/db.go
// This is the only place where operations are deleted from the in-memory map.
// They are also deleted from the database at startup and shutdown, and when a member is offline.
func SynchronizeAndPruneExpiredOperations(ctx context.Context, s *state.State) error {
now := time.Now()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need to consider operation statuses both locally and in the database.

A good heuristic would be to look at the updatedAt timestamp for both and update those that don't match (or delete those that are not present).

Consider the local map as the source of truth.

Comment thread lxd/operations.go
continue
}

// Skip operations that finished more than 5 seconds ago.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Why??????

Comment thread test/suites/operations.sh Outdated
lxc operation list --project default

# Prune any leftover operations before starting the test.
lxc query -X POST /internal/testing/prune-operations

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should look at why the test is failing and make the test more precise

@tomponline
tomponline marked this pull request as draft July 23, 2026 08:53
And don't spawn a goroutine or delete the operation from the
internal map or database.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
… is readonly.

This check is present for `op.{Update,Extend}Metadata` but not on
commit.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Delete in batches of 500 to avoid issues with SQLite max bind
parameters.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
And update other unit tests to use the new function.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
This deletes all given operations with the given IDs and ensures
done() is called.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…ate timestamp.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
"updateStatus" is ambiguous in that it sounds like it only updates the
operation status. In fact, it changes the operation status and then
persists all changes to the database.

The "CommitMetadata" function is similarly ambiguous because it
checkpoints the whole operation. Additionally, since the operation is
not edited. It is odd that this function sets the updatedAt timestamp.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…cancelling.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Changes:
- Run the task whether clustered or not.
- Don't skip if not the leader.
- Run the task every minute.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
The operation synchronization task will run on start up and every minute
thereafter. It will clear any errant operations.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
…resent.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Instance placement relies on finding which cluster member has the fewest
instances. This also checks for instance create operations.

Since we now save operation status to the database, we can use this to
be more precise about the number of pending instances by querying only
for operations that are running. In fact, since we now only prune
operations in a recurring task, we need to do this.

This commit also improves efficiency of the function by querying only
once, rather than twice per cluster member.

Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
Signed-off-by: Mark Laing <mark.laing@canonical.com>
@markylaing

Copy link
Copy Markdown
Contributor Author

Annoyingly I've goofed this PR just before leaving for two weeks.

Aside from the panic 🤦‍♂️ it's in a good place. Next steps when I return are:

  • fix panic
  • factor out synchronization function into smaller logical chunks that can be unit tested
  • address other outstanding comments
  • look at precision of get_operations test and assess whether the testing handler is needed

This comment is mainly to remind me what's going on when I'm back in two weeks

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.

Stale operationg are left lingering under heavy load

4 participants