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.

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

Collect workers immediately #252

Merged
merged 5 commits into from
Sep 5, 2024

Conversation

subpop
Copy link
Collaborator

@subpop subpop commented Aug 22, 2024

The current implementation of yggd could be described as "lazy discovery of workers"; a worker is only discovered when a message arrives destined for it and the message is routed. If and only if yggd is able to successfully deliver a message to a worker, is it then added to the dispatchers table, triggering a publish of an updated "connection-status" message. This lazy-loading causes yggctl to output incorrect worker status under certain conditions. If yggctl workers list is run immediately after starting yggd, the list will be empty.

This PR changes the procedure yggd uses to detect and collect workers. After connecting to the bus (in the (Dispatcher).Connect method), a goroutine is run that scans the list of names on the bus, collecting any that begin with "com.redhat.Yggdrasil1.Worker1". After the workers are found, a "connection-status" message is published.

Additionally, a MatchSignal is added that watches for name ownership change (specifically the org.freedesktop.DBus.NameOwnerChanged signal) within the "com.redhat.Yggdrasil1.Worker1" namespace. If name ownership changes occur, the features are deleted or updated, causing a new "connection-status" message to publish.

I also discovered a related bug in the table and text output formats. If a worker had an empty "features" map, the "text" and "table" would erroneously omit those workers from the output because output was based on a for-loop over the features map. I chose to drop the "text" output format entirely, since it's functionally identical to "table". I also reformatted how the features map is included in the table output.

Old

WORKER  FIELD         VALUE
echo    DispatchedAt
echo    Version       1

New

WORKER  FEATURES
echo    {"DispatchedAt":"","Version":"1"}

Fixes: #135
Card ID: CCT-115

The error being logged is not backed by another error, so it is
incorrect to include 'err' in the reported error string in this
situation.
@subpop subpop requested a review from jirihnidek August 22, 2024 18:27
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

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

Thanks for the updates, this works great and it almost LGTM.
I have some small nitpicks before merging though.

internal/work/dispatcher.go Outdated Show resolved Hide resolved
internal/work/dispatcher.go Outdated Show resolved Hide resolved
internal/work/dispatcher.go Outdated Show resolved Hide resolved
After connecting to the bus, scan the bus names immediately for known
worker names.
Add a match signal to observe when worker name ownership changes.
The text format of the worker list subcommand printed worker/feature
details in an arbitrary format. Since "table" already exists, "text" is
redundant.
Change the table output format to print all features in a single cell.
This allows workers with empty feature maps to still appear in the table
output.
As of Go 1.20, it is no longer necessary to call rand.Seed with a random
value.
@subpop subpop force-pushed the immediate-worker-list branch from dff4aa6 to ebe1c43 Compare September 4, 2024 13:40
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@jirihnidek jirihnidek left a comment

Choose a reason for hiding this comment

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

LGTM

@jirihnidek jirihnidek merged commit d6b2b3b into RedHatInsights:main Sep 5, 2024
30 of 34 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of workers can give wrong result
3 participants