Bug and stability fixes - #871
Merged
gabriel-samfira merged 18 commits intoSep 8, 2026
Merged
Conversation
Use a wait group to wait for each individual notification to be sent in parralel to all consumers. This prevents out of order notifications caused by slow consumers. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
If a websocket client disconnected or timed out or had its network connection dropped, this would trigger a Stop() in the websocket worker. When stopping the websocket worker servicing that client, there was the potential to block for at least 10 seconds as we attempted to send a writeMessage(), during which time we also held c.mux locked. While this happens, the database watcher consumer stops draining messages, cascading upwards, blocking the producer as it attempts to keep sending messages to all consumers (including our now blocked consumer). A misbehaving client should not be able to DoS the watcher. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This change increases the per consummer channel buffer from 1 to 128, allowing more time for slower consummers to catch up. Normally, consummers should consume messages at memory speed and handle parallelization in their own routines, but under extreme load, funky things can still happen. Increasing the buffer gives us a bit more time under load, while still ensuring ordering of messages. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This commit changes the consolidate function to act on a snapshot of the instance it manages. The global mux lock is swapped out for a consolidate lock. We want to only run one consolidate on the instance at any given point in time. This way, as we act on consolidating the state in the provider, updates to the instance object in memory are not blocked. The next time consolidate runs, we can act upon the new state. This change also guards the write-back of the provider result to the in-memory copy of the instance. While the provider call is in flight, a newer update (like a user requested force delete) may have landed on the in-memory instance via the watcher. Both the persisted result and watcher events are full row re-reads from the database, so we only overwrite the cached instance if it is not newer than the row we just persisted. Overwriting a newer state would revert it, and no future event would re-deliver it. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Renamed updateArgsFromProviderInstance to persistProviderInstanceState. The old name didn't properly suggest that the new state would persist to DB. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This change adds some timeouts for the HTTP client used to fetch the release index and actual tools. The timeouts are set on connect, TLS handshakes, responses etc. This change also adds a new reconcile loop that consumes a buffered channel of at most 1 update/event. The trigger function that writes to the channel will always make sure to always only leave the last event to the channel, so even if the reconcile loop takes a long time to process the previous event and we get a burst of updates, only the last update will be left in the channel buffer. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This change removes dead client sessions when we fail to write to it. This change also uses CompareAndSwap for the `running` atomic.Bool. That way multiple calls to stop won't block as the first Stop() to run procedes to tare down the worker. We also close the DB watcher consumer as soon as possible to make sure that as we close the worker, the producer doesn't stall due to a potentially full consumer notification channel. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The parameters to slog.Debug were evaluated at defer time. To get the desired effect, we need to add the log line to a closure and defer that. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
We need to update the forge client in 2 situations: * The credentials for a forge are spawwed out * The credentials themselves are updated (new PAT/app) When that happens, the wather generates an event that the pool filters for via its consumer. Database watcher events must be consumed in a non blocking manner. So when a new update event comes in that signals the need for an update to the client, we must consume that event and make sure that something (a loop in this case) wakes up and actually updates the client. This change makes sure that the client gets updated, regardless of how long that takes, in a non blocking way. If an update "storm" comes in (the user updates the credentials multiple times), only the last signal is added to the queue. So the last update is always guaranteed to signal that we need to update the client. Aditionally, this change (and a few more incomming) sets a timeout on the HTTP client we use. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Add separate timeouts for the scaleset client HTTP client and the message queue longpoll client. The scaleset client now uses a 60s overall request timeout and the longpoll client uses a 100s timeout. This change also uses a goroutine for the initial ratelimit population, unblocking the construction of the client if a forge responds slowly or is simply unavailable. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This change is inspired by cloudbase#850. We currently have 2 major problems here and potentially in other places throughout the code: * We do have the real risk of messages being dropped due to slow consumers * Using a goroutine to send messages to consumers, runs the real risk of messages being processed out of order. The initial thought was that consumers be built in a way that would read the message from the channel, and hand that work off imediately to some worker without blocking. In practice, well...I failed at that. While consumers should still be non blocking when reading watcher events, we need to guard against blocking consumers closer to the source of the events. This change does several things to address that: * We buffer the channels to more than just 1 message (128 in this commit) * We build an ordered, unbound queue of ordered messages per consumer. This is a tradeoff between not losing messages and potentially growing memory usage forever due to a potentially dead consumer. A dead consumer is less likely. A slow one is more likely. It is up to the consumers if they coalesce messages as they come in, drop them (not all may be actionable, as is the case where we trigger a tools sync), or if all messages MUST be acted upon (as is the case for runner states, where a transition from running to pending_delete requires the worker to actually do something. Consumers MUST of course do everything possible to either service each request or error out due to timeouts or whatever other failure mode is appropriate. Some logging was added. So if a consumer becomes slow, operators can set up alerts for that and potentially act upon it. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
When an agent notices that the runner has finished, it sends a status update back to GARM and we transition the instance to pending_delete. We used a forced transition for this, which can move an instance backwards from deleting to pending_delete, behind the back of the provider worker which may be in the middle of removing it. Force transitions should be done only for recovery cases. We now use a validated update. If the transition is rejected because the instance is already being deleted, the outcome the agent asked for is already under way, so we treat it as success. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The consolidate loop had no case for instances in deleting, so a manager whose cached state landed there would idle forever. Restarts are recovered by the scaleset worker on startup, but the manager can strand its own cached state at runtime: if a pass errors out after setting deleting but before recording the outcome (the deleted write or the pending_delete requeue fails transiently), no further event will move the status. Deleting is now treated as an interrupted delete and re-driven. Providers report a missing instance as success, so this is safe. Provider errors requeue the instance for every non forced path. If the deleted write is refused because the row regressed onto the deletion lane, we retry it with force, as the provider resource is confirmed gone at that point. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
We used to spawn a goroutine for each event consumed from the watcher. The mutex inside the handlers guarantees mutual exclusion but not ordering, so two updates for the same instance could be applied in reverse, leaving stale or ghost entries in the runners map. Events are now forwarded to an unbounded queue and applied in order by a single goroutine, the same pattern the provider worker uses. The watcher drain is never blocked. Scale down had two problems, observed live when a runner was removed 60 milliseconds after its provider create returned: * Victim selection skipped only active and terminated runners, so a runner that was still bootstrapping (status running, runner status pending) was a valid target. Scale down removes idle capacity, nothing else. Only idle runners are considered now. Stuck bootstraps are the reaper's job. * Runners already on their way out satisfy part of the delta, but they were counted during the removal loop, which made the outcome depend on map iteration order. They are now counted before any victim is picked. runnerCount() now only excludes deleted instances. Anything else, including instances in pending_delete or deleting, is still physically present in the IaaS and counts as capacity. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The listener context was created from context.Background() and the slog values were copied over from the worker context. The only thing the detached context bought us was preserving the log attributes. Deriving from the worker context keeps the log values, means the longpoll dies together with the worker, and the cancel func still allows restarting the listener without restarting the worker. The per attempt timeout around GetMessage() was also removed. The longpoll HTTP client now enforces its own timeout, so the context only needs to carry cancellation. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
The watcher dispatch goroutine now owns closing the messages channel. Loops that did not check the ok value on receive would spin on zero value payloads once the channel closed, forwarding empty events to their queues. The loops now exit cleanly when the consumer channel is closed. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This was referenced Sep 8, 2026
Member
Author
|
Ok. I have a bunch of typos in the commit messages. Too lazy to fix them. |
The loop could only terminate because loop() is the single caller. That invariant is implicit and easy to break. Two non blocking sends with an evict in between give the same latest-wins behavior without depending on who calls this. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Account for the fact that between the time ListRunners() runs and consolidateRunnerState gets a chance to iterate over the runners in the DB, we might have new runners added. This change adds a timestamp at which we listed runners in the GH API so we can ignore any newer runners added to the DB. We also attempt to remove the runner from GH anyway, in case our code failed to guard against accidental removals. This will prevent removing active runners, at the cost of extra API calls and possible runner churn for idle runners. Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses a bunch of issues in the scaleset workers, HTTP clients, database watcher and a few other places. Overall the changes include:
These changes (some inspired from some of the PRs) should fix issues recently reported in: