Poor evict performance

[BradNicolle]

Describe the bug
The queries performed by the evict tool are quite inefficient and produce significant contention with active users of the DSS instance. For example, in a DB where we had ~25k expired rows in the identification_service_areas table, each time the dss-rid-evict job ran for around 20-40min and was unable to make any progress due to contention, before failing with:

failed to execute db-manager: failed to execute RID transaction: timeout: context already done: context deadline exceeded

This also led to failed RID queries - it appears the interactions of these queries are effectively locking all rows in each table.

After killing the dss-rid-evict job, it was possible to delete the expired rows using manual SQL queries:

root@localhost:26257/rid> DELETE FROM identification_service_areas WHERE ends_at <= '<date>' AND writer = '<writer>';
DELETE 24504

Time: 33.266s total (execution 33.266s / network 0.000s)

and

root@localhost:26257/rid> DELETE FROM subscriptions WHERE ends_at <= '<date>' AND writer = '<writer>';
DELETE 800

Time: 1.144s total (execution 1.143s / network 0.000s)

These queries were able to execute quickly and without any observed contention with active traffic.

Note that we have also experienced similar behaviour for SCD.

It seems that we could run these same DELETE queries from within the evict tool to achieve this same significant increase in performance. Currently, the evict tool runs a "list", followed by an individual "delete" for each row returned from the list inside a transaction. This adds up to a large number of individual queries, as well as their accompanying network traffic to/from the DB client. I don't have firm evidence, but it also appears that any API queries on unrelated rows of the table force the transaction to retry.

Is there any reason why these could not be simplified into a single DELETE for ISAs and another for subscriptions (and correspondingly for SCD, operational intents and subscriptions) to keep all this processing within the DB?

Additionally, is there any need to run ISA and subscription deletion together in a single transaction? I imagine we could achieve this without a transaction at all, given each query is implicitly its own transaction and they don't seem to be dependent on each other for consistency. Is that an accurate assumption?

[mickmis]

Hi @BradNicolle,

Thanks for the detailed report.
What you describe is a known behavior of the tool, extract from its documentation:

All expired entities are identified and removed within a single transaction. When the system is under heavy load, lock contention with concurrent transactions may cause the cleanup to fail. There is no risk of data inconsistency in this case - the cleanup may simply be retried.

To mitigate this:

• Run the cleanup during low-intensity periods (e.g. at night).
• Clean up iteratively, starting with a lower TTL and progressively increasing it. If this becomes a recurring issue, batching removals could be considered as a future improvement.

When this tool was developed the focus was on making it reliable and shipped quickly, thus why it was kept dumb and no optimizations were implemented. But yes it has ample room for improvements. And given your feedback, it looks like now is the time for that :)
I will bring that up at the InterUSS weekly today for prioritization.

[mickmis]

Hi @BradNicolle,

May I ask what is the operational impact on your end? This will determine the priority of addressing this issue.
In addition, may I ask how come there was such large an accumulation of expired entities?
And have you tried playing with the TTL arguments to possibly reduce the amount of entities to be removed?

And to answer more specifically to some of your initial questions:

Is there any reason why these could not be simplified into a single DELETE for ISAs and another for subscriptions (and correspondingly for SCD, operational intents and subscriptions) to keep all this processing within the DB?

Entirely possible, yes.

is there any need to run ISA and subscription deletion together in a single transaction?

For the ISA repo, indeed the ISAs and the subscriptions do not refer to each other so that would be possible, yes. For the SCD repo, the operational intents are referencing subscriptions, so not trivially.

[BradNicolle]

Thanks @mickmis for the response.

May I ask what is the operational impact on your end?

Due to contention, sometimes the evict tool will fail to clear entities, which seems to snowball onto the next run (even more entities to clear next time, with an even higher chance of failing again). This eventually leads to evict consistently failing, and requiring manual intervention to recover. The manual intervention is either: running the raw DELETE SQL query above, or running the evict tool in TTL increments. Our instance has enough volume of traffic that TTL increments as small as a few hours are required to allow the tool to make progress.

how come there was such large an accumulation of expired entities?

In this case, the job was failing for a number of days which allowed such a large accumulation. We are implementing an alert to detect the above condition and intervene earlier.

have you tried playing with the TTL arguments to possibly reduce the amount of entities to be removed?

We currently use a 30min TTL for RID and run it on a 30min schedule. The snowballing occurs even with this configuration (which should minimise the number of entities to delete on each run). I imagine a longer TTL would be more susceptible (due to a larger number of entities eligible) and a smaller TTL might be too recent to consider the entities "expired".

[mickmis]

Thanks for the feedback. Given the impact you describe I think this warrants a fairly high priority. The assumption made when this was developed that it could it be run daily during low-traffic period does not hold in your case, since you need such a low TTL.

[mickmis]

Given the feedback that a single DELETE query offers acceptable performances, I think it is reasonable to go down that road.

Proposal for implementation items:

• Add those methods to the stores (implement for sqlstore only, memstore and raftstore may remain unimplemented for now, c.f. [raft][gate 2] Production readiness #1718) :
  • Add DeleteExpiredOperationalIntents to scd repository
  • Add DeleteExpiredSubscriptions to scd repository
    • Note: the ON DELETE CASCADE on scd_operations.subscription_id may end up deleting subscription too. But that should be OK because any subscription tied to an operational intent must encompass it. So if the subscription is expired, the operational intent tied necessarily is as well.
  • Add DeleteExpiredISAs to rid repository
  • Add DeleteExpiredSubscriptions to rid repository
• Wire those up in the evict tool, retain the ability to list only the expired entities
• Do not run anymore RID and SCD ISA/OI and subscription in a transaction: all of them can be independent from each other (for scd as well contrary to what I've written in Poor evict performance #1717 (comment))
• From RID Eviction - Pool-Level Cleanup and Transaction Contention #1726: add option to CLI to allow evicting entities belonging to any other locality
• Optional, and also suggested in RID Eviction - Pool-Level Cleanup and Transaction Contention #1726: it might be useful to include a 'limit' parameter to the tool, to be passed to the SQL as a LIMIT, that would allow finer control on the eviction execution in case there are again performance issues in the future.

Cleanup query could e.g. look like for ISAs:

WITH expired AS (
        SELECT id, updated_at
        FROM identification_service_areas
        WHERE ends_at <= $1
        AND writer = $2
        LIMIT $3
)
DELETE FROM identification_service_areas
  WHERE id = expired.id
  AND updated_at = expired.updated_at
  RETURNING id, owner, url, cells, starts_at, ends_at, writer, updated_at;

[utmverse]

Thank you for agreeing to implement this feature, @mickmis.

Having the capability to evict expired entities across the pool, while retaining the option to restrict eviction to the operator’s own entities, makes sense. The governance aspect of using pool-wide cleanup is already being raised with the Technical Governance team

[utmverse]

Thank you for agreeing to implement this feature, @mickmis.

Having the capability to evict expired entities across the pool, while retaining the option to restrict eviction to the operator’s own entities, makes sense. The governance aspect of using pool-wide cleanup is already being raised with the Technical Governance team

@mickmis - Just checking in to see if you have any updates on this feature implementation, as discussed in 1726.

[mickmis]

@mickmis - Just checking in to see if you have any updates on this feature implementation, as discussed in 1726.

We have an open PR pending review (#1725) implementing this. Given the priority it is among the next ones to be handled so it should be completed and merged sometime this week / early next week. After that we will be able to make a new release (candidate) for users to upgrade to.

[mickmis]

Issue closed too early due to #1725 being linked to it.