Skip to content

python3-labgrid: Add new package for coordinator #66

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

python3-labgrid: Add new package for coordinator #66

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions recipes-devtools/python/python3-labgrid.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ HOMEPAGE = "https://github.com/labgrid-project"
LICENSE = "LGPL-2.1-or-later"
LIC_FILES_CHKSUM = "file://LICENSE;md5=c0e9407a08421b8c72f578433434f0bd"

PACKAGE_BEFORE_PN += "${PN}-coordinator"
RDEPENDS:${PN}-coordinator = "${PN}"

Comment on lines +6 to +8
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why does this need to be a separate package? Simply installing the systemd service and the labgrid-coordinator entry point (as done before) should not hurt.

Copy link
Author

@gumulka gumulka Jul 21, 2025

Choose a reason for hiding this comment

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

I wanted to reduce the amount of systemd-services, that get started by default and not have it in there, but be a deliberate decision to activate the coordinator on a device.

Copy link
Member

@Bastian-Krause Bastian-Krause Jul 23, 2025

Choose a reason for hiding this comment

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

labgrid-coordinator.service is not started by default, hence installing the coordinator entry point and the systemd service does not change anything. A systemctl enable labgrid-coordinator.service is still needed.

RDEPENDS:${PN} = " \
coreutils \
libgpiod \
Expand All @@ -29,6 +32,7 @@ RDEPENDS:${PN} = " \
SRC_URI = " \
file://configuration.yaml \
file://labgrid-exporter.service \
file://labgrid-coordinator.service \
file://environment \
"

Expand All @@ -40,13 +44,16 @@ DEPENDS += "python3-pytest-runner-native"
inherit python_setuptools_build_meta systemd

SYSTEMD_SERVICE:${PN} = "labgrid-exporter.service"
SYSTEMD_SERVICE:${PN}-coordinator = "labgrid-coordinator.service"
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

Should just be added to SYSTEMD_SERVICE:${PN} above.


do_install:append() {
install -d ${D}${sysconfdir}/labgrid
install -m 0644 ${UNPACKDIR}/configuration.yaml ${D}${sysconfdir}/labgrid
install -m 0644 ${UNPACKDIR}/environment ${D}${sysconfdir}/labgrid
install -d ${D}${systemd_system_unitdir}
install -m 0644 ${UNPACKDIR}/labgrid-exporter.service ${D}${systemd_system_unitdir}/
install -m 0644 ${UNPACKDIR}/labgrid-coordinator.service ${D}${systemd_system_unitdir}/
}

FILES:${PN}-coordinator += "${systemd_system_unitdir}/labgrid-coordinator.service /usr/bin/labgrid-coordinator"
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

I don't see the need for this.

FILES:${PN} += "${sysconfdir} ${systemd_system_unitdir}"
5 changes: 5 additions & 0 deletions recipes-devtools/python/python3-labgrid/environment
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,8 @@

LABGRID_COORDINATOR_IP=127.0.0.1
LABGRID_COORDINATOR_PORT=20408

### The coordinator service will listen to the following ip, if the service is also
### enabled.

LABGRID_COORDINATOR_LISTEN_IP=0.0.0.0
Comment on lines +6 to +10
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

Why can't we simply use ${LABGRID_COORDINATOR_IP}:${LABGRID_COORDINATOR_PORT} from /etc/labgrid/environment?

Copy link
Author

@gumulka gumulka Jul 21, 2025

Choose a reason for hiding this comment

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

Because you might want to listen to 0.0.0.0 and connect to 127.0.0.1 and allow others to connect to your coordinator, even if you don't know its public IP address.

Copy link
Member

@Bastian-Krause Bastian-Krause Jul 23, 2025

Choose a reason for hiding this comment

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

Let's take a step back here: Having the coordinator as part of a device that will usually be the exporter (that's at least what meta-labgrid aims for currently) is a rather special case for situations where no (or little) other labgrid infrastructure exists. That's why I think a simple approach where you would configure the /etc/labgrid/environment like this fits:

LABGRID_COORDINATOR_IP=lxatac-00001
LABGRID_COORDINATOR_PORT=20408

That configuration works for my LXA TAC home setup with..

EnvironmentFile=/etc/labgrid/environment
ExecStart=/usr/bin/labgrid-coordinator --listen ${LABGRID_COORDINATOR_IP}:${LABGRID_COORDINATOR_PORT}

..in my labgrid-coordinator.service for over a year. I don't think we need to cover more elaborate use cases.

Copy link
Author

@gumulka gumulka Sep 1, 2025

Choose a reason for hiding this comment

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

That is where we differ in our goals. I want it be generic, because there is no reason to have it only fit your use case, It hinders people from using meta-labgrid on a generic coordinator. (Might not be the case for everyone, but why not?)
This allows us to have a machine dedicated as being a coordinator for other exporters.

18 changes: 18 additions & 0 deletions recipes-devtools/python/python3-labgrid/labgrid-coordinator.service
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
[Unit]
Description=Labgrid Coordinator
After=network.target

[Service]
Environment="PYTHONUNBUFFERED=1"
Environment="LABGRID_COORDINATOR_LISTEN_IP=0.0.0.0"
Environment="LABGRID_COORDINATOR_PORT=20408"
Comment on lines +7 to +8
Copy link
Member

@jluebbe jluebbe May 26, 2025

Choose a reason for hiding this comment

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

This seems to be redundant to the contents of the EnvironmentFile. Why do we need to set them here again?

Copy link
Author

@gumulka gumulka May 26, 2025

Choose a reason for hiding this comment

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

These are the defaults, that can be overwritten with the environment file. If there are no entries in environment, there are used. Otherwise they have to be specified.

Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

Shouldn't be needed.

Copy link
Author

@gumulka gumulka Jul 21, 2025

Choose a reason for hiding this comment

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

If everything is configured correctly in the environment, then yes. But i always like to have sane defaults in case something is missing.

Copy link
Member

@Bastian-Krause Bastian-Krause Jul 23, 2025

Choose a reason for hiding this comment

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

I can't follow. /etc/labgrid/environment is the central configuration place. Having more configuration scattered in other places is surprising. Let's not do this.

Copy link
Author

@gumulka gumulka Sep 1, 2025

Choose a reason for hiding this comment

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

Yes, and if you forget to declare a variable in /etc/labgrid/environment, should there be a default value, or should the service fail?

EnvironmentFile=/etc/labgrid/environment
ExecStart=/usr/bin/labgrid-coordinator -l ${LABGRID_COORDINATOR_LISTEN_IP}:${LABGRID_COORDINATOR_PORT}
Restart=on-failure
DynamicUser=yes
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

I'm not sure that's a good idea. The state directory permissions will probably cause issues after an update.

StateDirectory=labgrid-coordinator
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

The default for this is /var/lib/. This looks wrong. Isn't that simply something a BSP using meta-labgrid will set to an absolute path, e.g. on its central data partition?

# Set WorkingDirectory to StateDirectory, this works in DynamicUser mode since symlinks are created
Copy link
Member

@Bastian-Krause Bastian-Krause Jul 21, 2025

Choose a reason for hiding this comment

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

If we drop DynamicUser=yes, the comment won't apply anymore.

Copy link
Author

@gumulka gumulka Jul 21, 2025

Choose a reason for hiding this comment

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

Copied from https://github.com/labgrid-project/labgrid/blob/master/contrib/systemd/labgrid-coordinator.service
Happy to change to a better suited version for yocto. But we should first clarify if we want a separate package or not.

WorkingDirectory=%S/labgrid-coordinator

[Install]
WantedBy=multi-user.target