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 scripts for setting up hoeve #620

Draft
wants to merge 7 commits into
base: staging
Choose a base branch
from
Draft

Conversation

lmbollen
Copy link
Contributor

To make the setup process reproducible

For ethernet connectivity we need to do a few things:

  • Connect all ethernet interfaces to a virtual bridge
  • Assign the virtual bridge a static IP
  • Set up DHCP for the ethernet interfaces

To make the setup process reproducible
Copy link
Contributor

@leonschoorl leonschoorl left a comment

Choose a reason for hiding this comment

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

How do you see this script?

A:

Is it something you intend to run on each CI run within the (docker) container?
Then we should consider adding isc-dhcp-server to the Docker image, instead of installing it each time.
Also do you even have access to the outside network interfaces to setup a bridge like this?
A vanilla docker container definitely does not. But maybe the ones setup by the CI infra are configured differently.

B:

Or is it something run once on a system to set it up for talking ethernet with the FPGAs?
Then it should have a more persistent way of making these changes, possibly with a NetworkManager configuration or with some systemd service or something.

hitl-setup/setup.sh Outdated Show resolved Hide resolved
hitl-setup/setup.sh Outdated Show resolved Hide resolved
hitl-setup/dhcpd.conf Show resolved Hide resolved
hitl-setup/setup.sh Outdated Show resolved Hide resolved
hitl-setup/setup.sh Outdated Show resolved Hide resolved
hitl-setup/dhcpd.conf Outdated Show resolved Hide resolved
@DigitalBrains1
Copy link
Contributor

How do you see this script?

The README says:

Setting up should be done manually, CI should only check if the setup is correct.

So I'm going with option B. I have no clue about NetworkManager, but a systemd service sounds trivial as long as NetworkManager leaves it alone. It can just do the ip link add, set etcetera commands. I would also know how to do it with systemd-networkd and with ifupdown, but both would clash with NetworkManager.

@lmbollen
Copy link
Contributor Author

How do you see this script?

A:

Is it something you intend to run on each CI run within the (docker) container? Then we should consider adding isc-dhcp-server to the Docker image, instead of installing it each time. Also do you even have access to the outside network interfaces to setup a bridge like this? A vanilla docker container definitely does not. But maybe the ones setup by the CI infra are configured differently.

B:

Or is it something run once on a system to set it up for talking ethernet with the FPGAs? Then it should have a more persistent way of making these changes, possibly with a NetworkManager configuration or with some systemd service or something.

The latter. I'm not very familiar with these types of configurations, so this is currently my best effort (within reasonable time)

Comment on lines +26 to +27
WantedBy=multi-user.target
WantedBy=network-online.target
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
WantedBy=multi-user.target
WantedBy=network-online.target
WantedBy=multi-user.target
WantedBy=network-online.target
WantedBy=$whatever-the-github-actions-runner-is

?

Copy link
Contributor

@DigitalBrains1 DigitalBrains1 Sep 2, 2024

Choose a reason for hiding this comment

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

Do you want to prevent the runner from running if this script failed? Because Wanted is not strong enough for that.

If it's just about ordering, that's already taken care of by Before=network.target.

[edit]
Silly me. Wanted doesn't do ordering in the first place. Sorry, please explain what you're trying to accomplish, so we can work out what to do.
[/edit]

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to prevent the runner from running if this script failed?

Yes.

How would you specify this?

Copy link
Contributor

@DigitalBrains1 DigitalBrains1 Sep 2, 2024

Choose a reason for hiding this comment

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

I would add this to the runner:

[Unit]
Requires=fpganet.service
After=fpganet.service

but if you want the runner to be able to run if you systemctl disable fpganet.service, you could add this to fpganet.service:

[Unit]
Before=actions.runner.bittide-bittide-hardware.hoeve-1.service

[Install]
RequiredBy=actions.runner.bittide-bittide-hardware.hoeve-1.service

But this seems weird. Why would it suddenly no longer require the service just because you systemctl disabled it? On the other hand, if you do that, you might have good reason to? I dunno :-).

[edit]
Fixed dumb copy-paste error in latter config snippet.
[/edit]

Copy link
Contributor

Choose a reason for hiding this comment

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

I was under the (possibly naive) impression that just doing:

systemctl restart fpganet.service
systemctl enable fpganet.service
[...]
systemctl add-wants isc-dhcp-server.service fpganet.service
systemctl restart isc-dhcp-server.service
systemctl enable isc-dhcp-server.service

would be good enough to make sure that first the fpganet.service gets started, and then isc-dhcp-server.service, somewhere during boot up of the machine.

If you want to do it "really properly" you should probably add a dependency between the runner and the dhcp server.
Just depending on the setting up of the bridge isn't strictly enough.

Copy link
Contributor

@DigitalBrains1 DigitalBrains1 Sep 16, 2024

Choose a reason for hiding this comment

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

I don't really understand what you mean, but I don't see any ordering in the commands you give. Those commands will certainly not start the fpganet.service before isc-dhcp-server.service.

Note that requirement dependencies do not influence the order in which services are started or stopped. This has to be configured independently with the After= or Before= options. If unit foo.service pulls in unit bar.service as configured with Wants= and no ordering is configured with After= or Before=, then both units will be started simultaneously and without any delay between them if foo.service is activated.

[edit]
Well, it depends on what is taken as a given. As written currently, clearly fpganet.service will run before isc-dhcp-server.service because fpganet.service is Before=network-online.target and isc-dhcp-server.service is After=network-online.target and Wants=network-online.target. But I meant that your commands did not create this ordering; it is a pre-existing ordering, your commands do not establish it.
[/edit]

Copy link
Contributor

Choose a reason for hiding this comment

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

If you want to do it "really properly" you should probably add a dependency between the runner and the dhcp server.

Doing that (if I understand what you mean correctly) means that during bootup, it will wait for the DHCP server to be started before launching the runners. But the DHCP server doesn't need to be available until well into a CI run, not for the runner to be able to start. So I think this is overkill and not really the systemd "we want boot to be quick and ordering dependencies to be few" philosophy. I think we needed fpganet.service to be well and truly before the runner because otherwise Docker would not be able to bind the bridge interface into its runtime; in other words, the runner truly does depend on fpganet.service to be available when it is starting.

But I may be misinterpreting, there are so many ways to interpret "add a dependency" in systemd land.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we needed fpganet.service to be well and truly before the runner because otherwise Docker would not be able to bind the bridge interface into its runtime;

This was an earlier experiment, for the current version, the runner can start fine without the bridge. Obviously CI jobs that need the bridge will fail without it, but the runner starts fine and other jobs run fine.

# SPDX-License-Identifier: CC0-1.0

[Unit]
Description=Set up bridge network with FPGAs
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Description=Set up bridge network with FPGAs
Description=Set up bridge network with FPGAs - used for .......

hitl-setup/setup.sh Outdated Show resolved Hide resolved
Comment on lines 39 to 47
systemctl restart fpganet.service
systemctl enable fpganet.service

cp "$HERE/dhcpd.conf" /etc/dhcp/
echo "INTERFACESv4=\"$BRIDGE_NAME\"" > /etc/default/isc-dhcp-server

systemctl add-wants isc-dhcp-server.service fpganet.service
systemctl restart isc-dhcp-server.service
systemctl enable isc-dhcp-server.service
Copy link
Contributor

Choose a reason for hiding this comment

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

This contains some redundancies:

  • systemctl enable fpganet.service:
    Start fpganet.service on every boot
  • systemctl add-wants isc-dhcp-server.service fpganet.service:
    If isc-dhcp-serve.service starts, also start fpganet.service
    systemctl enable isc-dhcp-server.service:
    Start isc-dhcp-server.service on every boot

Either point makes the other redundant in regard to the starting of fpganet.service. I'll delete the add-wants.

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.

4 participants