python3-labgrid: Add new package for coordinator #66
Conversation
Some devices might also want to start the labgrid-coordinator as a systemd service. but this should not be the default. Move the labgrid-coordinator to a new package, that depends on labgrid. Signed-off-by: Fabian Pflug <[email protected]>
| Environment="LABGRID_COORDINATOR_LISTEN_IP=0.0.0.0" | ||
| Environment="LABGRID_COORDINATOR_PORT=20408" |
There was a problem hiding this comment.
This seems to be redundant to the contents of the EnvironmentFile. Why do we need to set them here again?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
If everything is configured correctly in the environment, then yes. But i always like to have sane defaults in case something is missing.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, and if you forget to declare a variable in /etc/labgrid/environment, should there be a default value, or should the service fail?
| PACKAGE_BEFORE_PN += "${PN}-coordinator" | ||
| RDEPENDS:${PN}-coordinator = "${PN}" | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| inherit python_setuptools_build_meta systemd | ||
|
|
||
| SYSTEMD_SERVICE:${PN} = "labgrid-exporter.service" | ||
| SYSTEMD_SERVICE:${PN}-coordinator = "labgrid-coordinator.service" |
There was a problem hiding this comment.
Should just be added to SYSTEMD_SERVICE:${PN} above.
| install -m 0644 ${UNPACKDIR}/labgrid-coordinator.service ${D}${systemd_system_unitdir}/ | ||
| } | ||
|
|
||
| FILES:${PN}-coordinator += "${systemd_system_unitdir}/labgrid-coordinator.service /usr/bin/labgrid-coordinator" |
There was a problem hiding this comment.
I don't see the need for this.
|
|
||
| ### The coordinator service will listen to the following ip, if the service is also | ||
| ### enabled. | ||
|
|
||
| LABGRID_COORDINATOR_LISTEN_IP=0.0.0.0 |
There was a problem hiding this comment.
Why can't we simply use ${LABGRID_COORDINATOR_IP}:${LABGRID_COORDINATOR_PORT} from /etc/labgrid/environment?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| Environment="LABGRID_COORDINATOR_LISTEN_IP=0.0.0.0" | ||
| Environment="LABGRID_COORDINATOR_PORT=20408" |
| EnvironmentFile=/etc/labgrid/environment | ||
| ExecStart=/usr/bin/labgrid-coordinator -l ${LABGRID_COORDINATOR_LISTEN_IP}:${LABGRID_COORDINATOR_PORT} | ||
| Restart=on-failure | ||
| DynamicUser=yes |
There was a problem hiding this comment.
I'm not sure that's a good idea. The state directory permissions will probably cause issues after an update.
| ExecStart=/usr/bin/labgrid-coordinator -l ${LABGRID_COORDINATOR_LISTEN_IP}:${LABGRID_COORDINATOR_PORT} | ||
| Restart=on-failure | ||
| DynamicUser=yes | ||
| StateDirectory=labgrid-coordinator |
There was a problem hiding this comment.
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?
| Restart=on-failure | ||
| DynamicUser=yes | ||
| StateDirectory=labgrid-coordinator | ||
| # Set WorkingDirectory to StateDirectory, this works in DynamicUser mode since symlinks are created |
There was a problem hiding this comment.
If we drop DynamicUser=yes, the comment won't apply anymore.
There was a problem hiding this comment.
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.
|
@gumulka Could you address the feedback? |
3 participants
Some devices might also want to start the labgrid-coordinator as a systemd service. but this should not be the default. Move the labgrid-coordinator to a new package, that depends on labgrid.