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

Added support for s6, moved some configuration files around #484

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

mazunki
Copy link

@mazunki mazunki commented Aug 13, 2022

I was creating a service for Waydroid for myself, and figured it would be nice if we unified the configuration somewhere.

The s6 service provided here oneshots the network bridge (previously run from waydroid-net.sh) prior to launching its own service. I have considered moving this into its own oneshot service, perhaps that's healthier.

I am sourcing the environment which was previous in waydroid-net.sh from /etc/waydroid/lxc.conf, and I also made it so waydroid-net.sh supports the same file, in case people still use the provided shellscript.

Furthermore, we now have /etc/waydroid/nftables.rules which can be passed on directly to nftables, given that we also pass in LXC_BRIDGE and LXC_NETWORK to it. I didn't change this in the waydroid-net.sh script.

@JamiKettunen , does this sound like what you had in mind?

  • unified config location, created config for lxc so it can be sourced
  • created s6 service, moved systemd into specific directory
  • sourcing configuration instead of hardcoding it
  • moved service
  • split targets apart, created a clean target, added s6 support

@mazunki mazunki changed the title Added support for S6, moved some configuration files around Added support for s6, moved some configuration files around Aug 13, 2022
@mazunki
Copy link
Author

mazunki commented Aug 13, 2022

Also, I should probably say: services_managers/s6/waydroid-srv/data/check currently simply exits successfully.

Ideally, it should only exit 0 when the container is completely up and running. If there's some way to do that, let me know and I'll fix it.

@JamiKettunen
Copy link
Contributor

Please rebase, also:

  • some of these things could be separate PRs that this depends on (just mention that and PR numbers via e.g. #490) to help get things merged faster
  • please don't commit a removal of file X just to add it back one or more commits later in a different path, this makes reviewing commit-by-commit really annoying considering it would just show moved file X if it was all done in a single commit. example which includes 3 files where this is the case 2fa6a70 (service_managers/systemd/waydroid-container.service was just "added" while data/configs/config_{1,2} were just "deleted")
    • I'd also do the same for config/lxc.conf (include sourcing of it from data/scripts/waydroid-net.sh in the same commit, because what's the point of the file otherwise)

@JamiKettunen
Copy link
Contributor

@mazunki Did you see my last comment?

@mazunki
Copy link
Author

mazunki commented Jan 26, 2023

@JamiKettunen , sorry, I hadn't seen it until now, no. I'll rebase and look at the changes sometime next week.

Would you prefer a separate PR for the configuration changes, and the support for s6? Maybe close this PR, and make a separate one for each of those points?

@aleasto
Copy link
Member

aleasto commented Jan 26, 2023

Yes please. This PR is very intrusive. I'd prefer to see minimal changes and their reasoning.

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't these be read by waydroid-net.sh then?

Though there's the issue of dynamically appending ipv6 rules or not

@@ -0,0 +1,17 @@
USE_LXC_BRIDGE="true"
LXC_BRIDGE="${vnic}"
Copy link
Member

Choose a reason for hiding this comment

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

this was made to be renamable so that the network could be configured by the user instead of by waydroid-net.sh, but with this change it's getting configurable already. I'd undo #267 as part of this change (except the part that replaces 192.168.250.0 with 192.168.240.0), as it was never well supported anyways -- waydroid upgrade would overwrite the changes

Comment on lines +171 to +176
if (ready_fd := session_cfg["session"]["ready_fd"]):
if ready_fd.isnumeric():
os.write(int(ready_fd), b"\n")
else:
logging.warning("{readyfd = } is not a valid file descriptor. Ready state skipped")

Copy link
Member

Choose a reason for hiding this comment

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

not super fond of this

command = [tools.config.tools_src +
"/data/scripts/waydroid-net.sh", "start"]
tools.helpers.run.user(args, command, check=False)
if not session_cfg or session_cfg["session"]["autoload_netbridge"] == "1":
Copy link
Member

Choose a reason for hiding this comment

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

this should probably be part of the global config instead (/var/lib/waydroid/waydroid.cfg)

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.

None yet

3 participants