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

Add support for D-Bus-based yggdrasil IPC #21

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

subpop
Copy link
Contributor

@subpop subpop commented Aug 5, 2024

Add parallel support for communicating with yggd over D-Bus. In order to support both gRPC and D-Bus, the gRPC-based yggdrasil package is included in the source tree of this project and a go module replace directive is used to replace the external package lookup with a local one.

When the program starts, it decides to use gRPC only if the YGG_SOCKET_ADDR variable is present in the environment. If that variable is absent, the worker connects to D-Bus using the upstream yggdrasil "worker" package API.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks like this is vendoring in a complete yggdrasil version. Is that intended?

To make testing easier, I started to add Packit in #20.

@subpop
Copy link
Contributor Author

subpop commented Aug 5, 2024

Yes, this is deliberate. This is how one worker can support both the D-Bus and the gRPC versions of yggdrasil. You'll notice one of the commits pulls the yggdrasil 0.2.x source code into the repository and renames it to "_v0".

@adamruzicka
Copy link
Contributor

Could we add yggdrasil as a git submodule instead of carrying a copy of it?

@subpop
Copy link
Contributor Author

subpop commented Aug 12, 2024

Could we add yggdrasil as a git submodule instead of carrying a copy of it?

Good idea. I'll refactor to do that.

@subpop
Copy link
Contributor Author

subpop commented Aug 12, 2024

One obstacle to using a submodule properly is that the vendored yggdrasil needs its module name edited in order for both yggdrasil-0.2.x and yggdrasil-0.4 to work. I did this "by hand" with the code carried directly in the repository in the PR. Switching to a submodule would mean we would need to first rename the upstream module name to include a version suffix. This is probably good to do, but it will require upstream work first.

@subpop
Copy link
Contributor Author

subpop commented Aug 12, 2024

See RedHatInsights/yggdrasil#250.

@subpop subpop force-pushed the port-dbus branch 2 times, most recently from 5088e72 to eac1a23 Compare August 28, 2024 17:32
@subpop
Copy link
Contributor Author

subpop commented Aug 28, 2024

I updated the PR to use a submodule. It turns out I was able to do it with a combination of submodules and go module replace directives. The changes I thought were required upstream (RedHatInsights/yggdrasil#250) were not needed for this particular implementation.

@subpop subpop requested a review from ekohl August 28, 2024 17:34
@ekohl
Copy link
Member

ekohl commented Aug 28, 2024

I kicked off CI, but it's not happy. For the future I also relaxed the repo's GitHub Actions settings from Require approval for first-time contributors to Require approval for first-time contributors who are new to GitHub which should allow you to iterate easier. And to be honest, because I'm lazy and don't want to approve every CI run after you push ;)

@subpop
Copy link
Contributor Author

subpop commented Aug 29, 2024

Tests are passing now.

@ekohl ekohl requested a review from adamruzicka August 29, 2024 16:17
@ekohl
Copy link
Member

ekohl commented Aug 29, 2024

I'm looking at @adamruzicka for the real code. I only look at the build system/CI side of things.

@subpop could you rebase on top of main so that we get packit builds as well?

Read yggdrasil/v1 into the source tree as a submodule. This is a
prerequisite for supporting both gRPC and D-Bus based yggdrasil.
Message models the pb.Data type internally. Types like
ExternalCommunciation now use Message instead of pb.Data. This
encapsulates the ExternalCommunication interface so that it can be
implemented by other types.
The runner functions and other types (UpdateAggregator) use this Message
type instead of pb.Data.
Add parallel support for communicating with yggd over D-Bus.

When the program starts, it decides to use gRPC only if the
YGG_SOCKET_ADDR variable is present in the environment. If that variable
is absent, the worker connects to D-Bus using the upstream yggdrasil
"worker" package API.
@subpop subpop force-pushed the port-dbus branch 2 times, most recently from 4d33af7 to 5e86897 Compare August 29, 2024 16:55
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Nice, having working RPMs should make testing easier.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
data/com.redhat.Yggdrasil1.Worker1.foreman.service.in Outdated Show resolved Hide resolved
data/com.redhat.Yggdrasil1.Worker1.foreman.service.in Outdated Show resolved Hide resolved
Makefile Outdated
Comment on lines 20 to 23
install -D -m 755 build/foreman_worker $(DESTDIR)/$(LIBEXECDIR)/foreman_worker
install -D -m 644 build/data/com.redhat.Yggdrasil1.Worker1.foreman.conf $(DESTDIR)/usr/share/dbus-1/system.d/com.redhat.Yggdrasil1.Worker1.foreman.conf
install -D -m 644 data/dbus_com.redhat.Yggdrasil1.Worker1.foreman.service $(DESTDIR)/usr/share/dbus-1/system-services/com.redhat.Yggdrasil1.Worker1.foreman.service
install -D -m 644 build/data/com.redhat.Yggdrasil1.Worker1.foreman.service $(DESTDIR)/usr/lib/systemd/system/com.redhat.Yggdrasil1.Worker1.foreman.service
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, interesting. I would expect the rpm build to fail due to additional files. But the build seems to pass and those files are note present in the resulting rpm.

Makefile Outdated Show resolved Hide resolved
These files enable the worker to start up explicity (via the system
service) or automatically via bus-activation.
@ekohl
Copy link
Member

ekohl commented Sep 4, 2024

From a build perspective 👍 but I'm not qualified to review the Go code.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

With yggdrasil-0.2.1 this seems to work.

With yggdrasil-0.4.4, with the same configuration:

  • yggdrasil takes the content field from the mqtt notification, discards the host path and replaces it with the value of default data host (compile-time configuration?) and tries to fetch it. The url ends up being something like https://$some_rh_service/ssh/jobs/$uuid, which, funnily enough gets greeted by a 200 OK
  • the resulting html is passed to the worker which tries to execute it, believing it to be a script, this fails
  • the worker reports back to yggdrasil, yggdrasil tries to call back, but again replaces the hostname from the return_url with the default data host and tries to POST to the resulting url, failing again

Setting data-host = "" in the config file seems to resolve this particular issue, but are we expected to do that?

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

None of the things I complained in the previous comment can really be addressed in this PR, so I take that back.

The change proposed here look sane and I was able to get the entire flow to work end to end with yggdrasil-0.2.1 and 0.4.4, let's get this in.

@adamruzicka adamruzicka merged commit dd68f8c into theforeman:main Sep 25, 2024
7 checks passed
@adamruzicka
Copy link
Contributor

Thank you @subpop & @ekohl !

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.

3 participants