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.

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

Jump to bottom

make systemd and its dependencies optional via 'no_systemd' build tag (2nd take) #3959

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

make systemd and its dependencies optional via 'no_systemd' build tag (2nd take) #3959

wants to merge 7 commits into from

Conversation

metux
Copy link
Contributor

@metux metux commented Aug 2, 2023

Hello folks,

this is my second take for making systemd stuff build-time optional. (obsoletes #2987)

It's been quite a while since the last time, and lots of things changed.

These patches first factors out systemd specific stuff in easily digestable steps, bevor adding the actual build flag.
Several functions are dummies on no_systemd flag (that shouldn't be actually called) for sake of simpler code flow.

@metux metux force-pushed the submit/no-systemd-2 branch 4 times, most recently from 45d2baa to 70341b5 Compare August 2, 2023 16:04
@metux metux mentioned this pull request Aug 2, 2023
Closed
@metux
Copy link
Contributor Author

metux commented Aug 2, 2023

No idea how to fix the lint error - running gofumpt didn't seem to be sufficient.

Maybe the problem is the vendor dir, which isn't at all gofumpt'ed.

kolyshkin
kolyshkin reviewed Aug 2, 2023
View reviewed changes
libcontainer/cgroups/systemd/nosystemd.go Outdated
}

func NewUnifiedManager(config *configs.Cgroup, path string) (cgroups.Manager, error) {
return nil, errors.New("no lennartix")
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 that the term used in this error message will be very confusing to the majority of users.

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.

@kolyshkin
Copy link
Contributor

Overall LGTM, just a few minor issues:

  • I would rename *nosystemd.go files to *no_systemd.go or *systemd_stub.go for readability;
  • files that end with _linux.go should still end with _linux.go even when you rename/split those;
  • change the error messages to be less confusing and refer to systemd.

No idea how to fix the lint error - running gofumpt didn't seem to be sufficient.

The gofumpt errors in validate / lint are there because you're using old-style +build tags in the new code. Please see commit d8da003 for more info on that (and if your gofumpt doesn't show/change anything, you need to update it).

@kolyshkin
Copy link
Contributor

  • files that end with _linux.go should still end with _linux.go even when you rename/split those;

I was talking about utils_linux.go file. The (better, I guess) alternative to the above is to drop the _linux part of the file name and use //go:build linux annotation instead (which I think you already do).

@metux metux force-pushed the submit/no-systemd-2 branch 4 times, most recently from 51a3899 to 9167c11 Compare August 3, 2023 12:50
kolyshkin
kolyshkin approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@kolyshkin
Copy link
Contributor

@opencontainers/runc-maintainers PTAL

lifubang
lifubang approved these changes Aug 13, 2023
View reviewed changes
Copy link
Member

@lifubang lifubang left a comment

Choose a reason for hiding this comment

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

LGTM

  1. This will reduce the size of runc binary file by 700k+.
    It is useful for my daily use, but I think upstream projects(containerd/docker/k8s) may not use this feature.
  2. This PR will make the code more clear and readable.

@lifubang
Copy link
Member

@metux Please use git rebase main to catch up with the latest change.

@metux
Copy link
Contributor Author

metux commented Aug 18, 2023

@metux Please use git rebase main to catch up with the latest change.

done

@metux
specconv: CreateCgroupConfig(): simplify code flow
d0d45eb 
Simplify the branching between systemd and pure cgroups.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
specconv: spec_linux: factor out systemd specific cgroup init
3306a08 
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
libcontainer: factor out systemdDbus.Properties references to an alias
3f1a6ed 
Factor out all references to Lennartix-specific dbus property struct into
some alias. That's helpful in making it possible to build it w/o any systemd
dependencies (which would make the binary at least 10% smaller.)

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
cgroups: devices: factor out systemd specific init to own function
62cb27b 
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
libcontainer: specconv: factor out lennartix specific functions
6c66d9d 
Factor out an init function specific to Lennartix platform, so subsequent
patches can easily make building systemd support build-time optional.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
make systemd and its dependencies optional via 'no_systemd' build tag
23ddbc0 
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
cgroups: rename addCpuQuota() to addCPUQuota()
203096c 
Fix lint-extra err:

    Warning: var-naming: func addCpuQuota should be addCPUQuota (revive)

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@kolyshkin
Copy link
Contributor

@metux needs a rebase. Guess we can merge it right after.

@lifubang
Copy link
Member

@metux Are you still working on this PR? Please rebase, I think we can merge this one, and in the future release we can also provide a binary without systemd.

@kolyshkin
Copy link
Contributor

If @metux can no longer work on this, perhaps I can carry this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kolyshkin kolyshkin kolyshkin approved these changes

@lifubang lifubang lifubang approved these changes

Assignees
No one assigned
Labels
area/systemd status/4-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@metux @kolyshkin @lifubang