hashicorp / hashicorp/consul

Release state lock before catalog operations in State.SyncChanges

Open
#6,616 4 comments 1 reaction 0 assignees View on GitHub
theme/health-checks theme/reliability type/enhancement
Dominant language
Go
Stars
30.1k
Forks
4.6k
Avg merge
1d 18h
Merged PRs (30d)
39

Description

Currently, [`State.SyncChanges`](https://github.com/hashicorp/consul/blob/master/agent/local/state.go#L966) holds the state lock for the entire duration of the function. This means that the time holding the lock is largely dependent on the time to sync or delete services/checks in the catalog.

When there are network delays this function can wait up to 10s (RPC dial timeout) for each catalog request.

As shown in [this issue](https://github.com/hashicorp/consul/issues/4460#issuecomment-534476416), that can lead to flapping TTL tests even if the service is healthy. For example:
1. A TTL check is updated to `passing` via the API
2. Network latency leads to long timeouts during `SyncChanges` while holding the state lock
3. The next TTL update fails because it can't acquire the lock and update the check before the TTL interval expires
4. The TTL check is marked as critical, even if it successfully issued a request to update itself as `passing`

~This seems like it could be improved by only holding the state lock while constructing lists of services and checks that are out of sync or were deleted. The catalog operations like `l.syncService` and `l.deleteService` could then be done separately while iterating over those lists.~ Edit: This actually leads to races on concurrent updates, needs additional investigation.

-------

Another related issue: When the state lock is held for remote syncs, the agent endpoints cannot serve local reads either.

Contributor guide

Open the contributing guide

Research direction

Start with State.SyncChanges in agent/local/state.go around line 966, then inspect the referenced l.syncService and l.deleteService operations and the related TTL behavior in issue 4460. The proposed lock split was withdrawn because it introduces races, so first determine the safe synchronization approach; done should avoid long catalog operations blocking TTL updates and local reads without causing concurrent-update races.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
distributed-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.