Skip to content

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

Closed
metux wants to merge 7 commits into
opencontainers:mainfrom
oss-qm:submit/no-systemd-2
Closed

make systemd and its dependencies optional via 'no_systemd' build tag (2nd take)#3959
metux wants to merge 7 commits into
opencontainers:mainfrom
oss-qm:submit/no-systemd-2

Conversation

@metux

@metux metux commented Aug 2, 2023

Copy link
Copy Markdown
Contributor

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 force-pushed the submit/no-systemd-2 branch from 70341b5 to 93bc193 Compare August 2, 2023 17:06
@metux

metux commented Aug 2, 2023

Copy link
Copy Markdown
Contributor Author

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.

}

func NewUnifiedManager(config *configs.Cgroup, path string) (cgroups.Manager, error) {
return nil, errors.New("no lennartix")

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kolyshkin

Copy link
Copy Markdown
Contributor

@opencontainers/runc-maintainers PTAL

@lifubang lifubang left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member

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

@metux
metux force-pushed the submit/no-systemd-2 branch from c7e2881 to cb1899c Compare August 18, 2023 11:33
@metux

metux commented Aug 18, 2023

Copy link
Copy Markdown
Contributor Author

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

done

metux added 7 commits August 25, 2023 15:30
Simplify the branching between systemd and pure cgroups.

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
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>
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
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>
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>
Fix lint-extra err:

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

Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
@metux
metux force-pushed the submit/no-systemd-2 branch from cb1899c to 203096c Compare August 25, 2023 13:30
@kolyshkin

Copy link
Copy Markdown
Contributor

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

@lifubang

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@kolyshkin

Copy link
Copy Markdown
Contributor

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

Better late than never: #4547

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.

3 participants