Skip to content

Conversation

metux
Copy link
Contributor

@metux metux commented Jun 2, 2021

Running under systemd requires lots of special code that contributes
to ca. 10 percent (ca. 1 MB) to the binary size. This is only needed on
targets that might run systemd - there're dozens of distros, let alone
embedded/edge devices or special images (eg. cluster worker nodes) that
do not and never will run systemd, thus do not need that code at all.

It's not just about reducing memory consumption, but also having over
10.000 lines of code less to audit.

In order not to change default behaviour, introducing an inverse build tag,
'no_systemd', for explicitly opting out from systemd special handlings.

Signed-off-by: Enrico Weigelt, metux IT consult info@metux.net

// SystemdProps are any additional properties for systemd,
// derived from org.systemd.property.xxx annotations.
// Ignored unless systemd is used for managing cgroups.
SystemdProps []systemdDbus.Property `json:"-"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of duplicating the whole structure, you need to embed a sub-structure which can be defined to be empty in case no_systemd build tag is set.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed that in recent push

@cyphar
Copy link
Member

cyphar commented Jun 3, 2021

@AkihiroSuda @kolyshkin Does cgroupv2 still require systemd, or is systemd support only required if you're using cgroupv2 on a systemd system? (From memory the early design required it, but I haven't closely followed how it's evolved since. Since we have fs2 I guess you can technically run cgroupv2 without systemd -- as long as you're not running with systemd, because of the obvious conflicts.)

@metux This needs new CI jobs to make sure that no_systemd builds actually work and pass the unit/integration tests.

@AkihiroSuda
Copy link
Member

Does cgroupv2 still require systemd, or is systemd support only required if you're using cgroupv2 on a systemd system?

Never required on non-systemd hosts.

Even on systemd hosts, the systemd driver was never strictly required in practice, but future version of systemd may strictly require it, in theory.

@AkihiroSuda
Copy link
Member

This needs new CI jobs to make sure that no_systemd builds actually work and pass the unit/integration tests.

Yes, and README (the build tags section) needs to be updated too.

@cyphar
Copy link
Member

cyphar commented Jun 3, 2021

Even on systemd hosts, the systemd driver was never strictly required in practice, but future version of systemd may strictly require it, in theory.

"Required" may have been too strong of a word to use. Though you can't set Delegate=yes if you don't have systemd support I guess? (And given the history of systemd futzing around with our cgroups, I'm a bit skeptical that it's not at the very least a recommended setup?) In any case this doesn't really block the PR then.

@kolyshkin
Copy link
Contributor

Though you can't set Delegate=yes if you don't have systemd support I guess?

I think we can implement "minimal" systemd support, adding something like

if IsRunningSystemd() {
    err := os.Exec("systemctl", "set-property", unit, "Delegate=yes").Run()
    ...
}

This code won't add any new dependencies or impact the binary size much. In fact, ideally this should be done with

Thinking about this, an "alternative" systemd driver can be added to runc, which will call systemctl upon container start/stop/update. This has its own pros and cons, of course.

@metux metux force-pushed the submit/no-systemd branch 2 times, most recently from eb4038a to 699e479 Compare June 10, 2021 09:06
@metux
Copy link
Contributor Author

metux commented Jun 10, 2021

Though you can't set Delegate=yes if you don't have systemd support I guess?

I think we can implement "minimal" systemd support, adding something like

if IsRunningSystemd() {
    err := os.Exec("systemctl", "set-property", unit, "Delegate=yes").Run()
    ...
}

This code won't add any new dependencies or impact the binary size much. In fact, ideally this should be done with

Where does IsRunningSystemd() come from when speaking to systemd and thats dependencies are off ?
I wonder what the purpose if that shall be when systemd support is switched off anyways.

OTOH, I feel very unhappy with such arbitrary command calls, especially in case when systemd stuff had been disabled explicitly. Could end up calling a completely different program. Actually, I once used to write one with that name, long before systemd even existed.

@metux metux force-pushed the submit/no-systemd branch 4 times, most recently from 956794d to 19758b8 Compare June 10, 2021 10:58
@kolyshkin kolyshkin modified the milestones: post-1.0, 1.1.0 Jun 11, 2021
@kolyshkin
Copy link
Contributor

I think we should have it in 1.1. Set the milestone accordingly.

Running under systemd requires lots of special code that contributes
to ca. 10 percent (ca. 1 MB) to the binary size. This is only needed on
targets that might run systemd - there're dozens of distros, let alone
embedded/edge devices or special images (eg. cluster worker nodes) that
do not and never will run systemd, thus do not need that code at all.

It's not just about reducing memory consumption, but also having over
10.000 lines of code less to audit.

In order not to change default behaviour, introducing an inverse build tag,
'no_systemd', for explicitly opting out from systemd special handlings.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux metux force-pushed the submit/no-systemd branch from 19758b8 to 5bc5a96 Compare June 16, 2021 14:08
@kolyshkin
Copy link
Contributor

Though you can't set Delegate=yes if you don't have systemd support I guess?

I think we can implement "minimal" systemd support, adding something like

if IsRunningSystemd() {
    err := os.Exec("systemctl", "set-property", unit, "Delegate=yes").Run()
    ...
}

This code won't add any new dependencies or impact the binary size much. In fact, ideally this should be done with

Where does IsRunningSystemd() come from when speaking to systemd and thats dependencies are off ?
I wonder what the purpose if that shall be when systemd support is switched off anyways.

OTOH, I feel very unhappy with such arbitrary command calls, especially in case when systemd stuff had been disabled explicitly. Could end up calling a completely different program. Actually, I once used to write one with that name, long before systemd even existed.

The idea is to make it possible to use runc compiled without systemd support on a systemd-enabled system.

The IsRunningSystemd function does a simple stat(2), and keeping it even when building with no_systemd won't affect the binary size much, nor will it bring any new dependencies.

But I don't think that calling systemctl will work as I thought it would -- first of all, we don't have a unit to set properties for.

So we can just assume that system won't screw up whatever we create in /sys/fs/cgroup.

@kolyshkin
Copy link
Contributor

Can we somehow limit the number of jobs? 60 is too much :)

Say, only test with non-default tags on 1 go version. Or something similar.

@kolyshkin
Copy link
Contributor

@metux can you please address #2987 (comment), and also do a rebase?

@AkihiroSuda AkihiroSuda removed this from the 1.1.0 milestone Sep 2, 2021
@kolyshkin
Copy link
Contributor

@metux please let us know if you want to keep working on this. This needs a rebase, and #2987 (comment) also needs to be addressed.

@metux
Copy link
Contributor Author

metux commented Aug 2, 2023

obsoleted by rewrite: #3959

@cyphar cyphar closed this Aug 4, 2023
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.

4 participants