WIP: Initial version of golang port #41

alexlarsson · 2022-09-15T14:16:03Z

This is not necessarily meant to go in this repo, but this is an initial cut at porting all of quadlet into golang, for eventual integration with the podman project.

You can build it with go build github.com/containers/quadlet/cmd/quadlet and then run the tests from the "tests" subdirectory with: ./testcase-runner.py cases/ /path/to/golang/quadlet

rhatdan · 2022-09-16T10:41:21Z

@vrothberg PTAL

cmd/quadlet/main.go

pkg/quadlet/podmancmdline.go

pkg/quadlet/quadlet.go

alexlarsson · 2022-09-23T14:19:46Z

Fixed the comments from @ygalblum and rebased

vrothberg · 2022-09-26T12:03:44Z

I just ran golangci-lint run (https://golangci-lint.run/) before reading the code and it reports a number of issues which need to be fixed before we can move it over to containers/podman. The linter is a strict enforcement in podman.

vrothberg

First round of reviews. Need to stop here due to time constraints but will continue reviewing podmancmdline.go ff tomorrow.

vrothberg · 2022-09-26T12:09:00Z

cmd/quadlet/main.go

+	files, err := os.ReadDir(sourcePath)
+	if err != nil {
+		if !errors.Is(err, os.ErrNotExist) {
+			Log("Can't read \"%s\": %s", sourcePath, err)


I recommend returning errors and let the callers take care of logging or considering it fatal. Only logging errors quickly turns into spaghetti code.

Applies to other parts of the code as well.

What do you mean by callers? This is the main.go file of a systemd generator. There is no caller of this code. And it has to log, directly to /dev/kmsg, because it is running in an extremely limited environment during early boot where there is no other way to communicate to anything at all.

I was referring to code hygiene and being more Go idiomatic.

if err != nil { Log(err) } return

should be

if err != nil { return err }

and let the callers of this function do the logging. It certainly works as is but it is very uncommon in Go - it loves throwing its errors around.

Maybe that makes sense for this error, but later in this function we log parse error for each .container file, but then continue loading the others. So, for that case we need to log directly.

vrothberg · 2022-09-26T12:14:00Z

cmd/quadlet/main.go

+	}
+}
+
+func getUnitDirs(user bool) []string {


I'd appreciate a verbose comment on this function (and many others) to be able to match intent vs. implementation.

Other container tools such as Podman et al. will read system configurations (/etc/containers/) even when being run as a ordinary rootless user. man containers.conf, for example, will read /etc/containers/containers.conf unless there is a $HOME/.config/containers/containers.conf.

I do not have an opinion on how Quadlet should handle that but I think should be documented in the code.

The goal of this function is to figure out where to read the *.container files from. This is triggered by whether the quadlet process is started as /usr/lib/systemd/system-generators/quadlet-system-generator, or /usr/lib/systemd/system-generators/quadlet-user-generator.

In the earlier case it will always return /etc/containers/systemd and /usr/share/containers/systemd (actually these are build time options in the C version), and in the user case it is $XDG_CONFIG_HOME/containers/systemd. It additionally respects an env var, but that is only used to drive the CI.

We can't do things like read /etc/containers.conf during the generators. It can reference all sort of stuff that are not available yet. See here for some basic rules: https://www.freedesktop.org/software/systemd/man/systemd.generator.html#Notes%20about%20writing%20generators

In addition, the generator phase completely blocks the boot of the entire system, and until all generators are done systemd won't even be trying to parse any unit files. Any preformance problem here significantly drags down the boot time of the entire system, which is very bad. Just going from C to golang is likely to make quadlet the slowest generator (due to binary size).

vrothberg · 2022-09-26T12:14:56Z

cmd/quadlet/main.go

+var isUser bool
+
+var noKmsg bool = false
+var kmsgFile *os.File = nil


Consider moving that into a

var ( )

along with comments

vrothberg · 2022-09-26T12:20:47Z

cmd/quadlet/main.go

+
+	for _, file := range files {
+		name := file.Name()
+		if units[name] == nil &&


I think Quadlet should log when units[name] != nil (i.e., a unit-name clash across multiple directories). It may be a simple oversight of the user who may be tricked into believing "it just works". Debugging that scenario can be time intensive as there will be a service running with the expected name but with an unexpected workload.

This is how all systemd overriding works though. If you have a /usr/ version of the .container file you can override that as sysadmin by adding one in /etc.

vrothberg · 2022-09-26T12:22:54Z

cmd/quadlet/main.go

+
+	defer f.Close()
+
+	err = service.Write(f)


Note to self: This seems to be point where Quadlet and podman generate systemd can share logic. (didn't read the entire code yet)

vrothberg · 2022-09-26T12:25:19Z

cmd/quadlet/main.go

+	return nil
+}
+
+func enableServiceFile(outputPath string, service *systemdparser.UnitFile) {


Note to self: I do not understand what this function does but did not inspect the callees yet.

This implements the [Install] section in the .container unit file. This is a section that is otherwise implemented by the "systemctl enable" call. See #20 for the origin of this.

vrothberg · 2022-09-26T12:33:29Z

cmd/quadlet/main.go

+	prgname := path.Base(os.Args[0])
+	isUser = strings.Index(prgname, "user") != -1
+
+	flag.Parse()


I suggest moving the var decls, the flag initializing and main() closer. It's a bit scattered at the moment and wont' fit on one screen.

vrothberg · 2022-09-26T12:34:53Z

cmd/quadlet/main.go

+			continue
+		}
+
+		if service == nil {


Suggested change

if service == nil {

if err != nil {

go idiomatic

vrothberg · 2022-09-26T12:36:12Z

cmd/quadlet/main.go

+
+	os.MkdirAll(outputPath, os.ModePerm)
+
+	for name, unit := range units {


Consider turning the body into a function that returns an error.

vrothberg · 2022-09-26T12:37:17Z

go.mod

@@ -0,0 +1,3 @@
+module github.com/containers/quadlet
+
+go 1.18


I love this go.mod file. No dependencies are good dependencies.

vrothberg · 2022-09-27T10:59:15Z

pkg/quadlet/quadlet.go

+
+const (
+	UnitDirAdmin  = "/etc/containers/systemd"
+	UnitDirDistro = "/usr/share/containers/systemd"


Did not read the rest of the code below, but I'd expect a UnitDirUser in $HOME such that a user can configure their own services without root privileges.

Yeah, that is this code from main:

if user { if configDir, err := os.UserConfigDir(); err == nil { dirs = append(dirs, path.Join(configDir, "containers/systemd")) } } else {

Hard to do as a const though.

Can be a var -> https://github.com/containers/image/blob/main/pkg/sysregistriesv2/system_registries_v2.go#L72-L76

I'm not sure why that would be better though. That just means everything that links to this computes this at init time whether used or not. (This over-reliance on running random code during init is imho a problem with golang in general.) Better would be to have a function to get it. But maybe we should wait until there is an actual external user and see what is best.

The get free out of jail card is to mark Quadlet as "tech preview" until we have figured such things out.

vrothberg · 2022-09-27T11:01:44Z

pkg/quadlet/quadlet.go

+
+var validPortRange = regexp.MustCompile(`\d+(-\d+)?(/udp|/tcp)?$`)
+
+var supportedContainerKeys = map[string]bool{


All the keys can be consts and shared with other locations using the strings.

vrothberg · 2022-09-27T11:16:26Z

pkg/systemdparser/unitfile.go

+	return f
+}
+
+func ParseUnitFile(pathName string) (*UnitFile, error) {


I wonder if we can simplify the parser. Couldn't we require the .container files to start with the [container] table? This way, Quadlet only had to parse the [container] table and leave the rest untouched (i.e., by parsing until EOF or the next table).

It seems to be possible to specify the same table multiple times, so I imagine:

[container] ... [unit from user]

can be turned into

[unit from user] [unit from quadlet] [service from quadlet]

I think this could simplify the code quite a bit.

But we modify the service section (and others), as well as parse out details from other sections (like install). I don't see how this would help anything.

I mean, i guess I see how we can e.g. allow the user to say:

[service] Restart=always

And then when can set the exec line of the service section in a separate copy of it. However, we still do things like read the install section. And anyway, if we can parse one section, we could parse any? Is it really easier to parse one?

Signed-off-by: Alexander Larsson <alexl@redhat.com>

Based on the initial port in containers/quadlet#41 Signed-off-by: Alexander Larsson <alexl@redhat.com>

Based on the initial port in containers/quadlet#41 This contains the unit tests and the testcases from the C code as well as modification to the podman spec file based on what the quadlet spec file looks like, producing a podman-quadlet subpackage. Signed-off-by: Alexander Larsson <alexl@redhat.com>

rhatdan · 2022-12-04T12:07:15Z

Since this has been merged into Podman, closing PR.

alexlarsson mentioned this pull request Sep 15, 2022

Better settings for running podman containers under systemd containers/podman#15686

Closed

LorbusChris mentioned this pull request Sep 18, 2022

Include new podman with quadlet functionality coreos/fedora-coreos-tracker#998

Closed

ygalblum reviewed Sep 18, 2022

View reviewed changes

alexlarsson force-pushed the golang-version branch from 5123ccd to 8c665f2 Compare September 23, 2022 14:19

alexlarsson force-pushed the golang-version branch from 8c665f2 to 06ba235 Compare September 23, 2022 14:20

vrothberg reviewed Sep 26, 2022

View reviewed changes

vrothberg reviewed Sep 27, 2022

View reviewed changes

Initial version of golang port

ac914de

Signed-off-by: Alexander Larsson <alexl@redhat.com>

alexlarsson force-pushed the golang-version branch from 06ba235 to ac914de Compare October 3, 2022 09:47

alexlarsson added a commit to alexlarsson/podman that referenced this pull request Oct 3, 2022

Initial quadlet version integrated in golang

71636ff

Based on the initial port in containers/quadlet#41 Signed-off-by: Alexander Larsson <alexl@redhat.com>

alexlarsson mentioned this pull request Oct 3, 2022

Initial quadlet version integrated in golang containers/podman#16035

Merged

alexlarsson added a commit to alexlarsson/podman that referenced this pull request Oct 4, 2022

Initial quadlet version integrated in golang

aaf81c7

Based on the initial port in containers/quadlet#41 Signed-off-by: Alexander Larsson <alexl@redhat.com>

alexlarsson added a commit to alexlarsson/podman that referenced this pull request Oct 5, 2022

Initial quadlet version integrated in golang

162bef7

Based on the initial port in containers/quadlet#41 Signed-off-by: Alexander Larsson <alexl@redhat.com>

alexlarsson added a commit to alexlarsson/podman that referenced this pull request Oct 14, 2022

Initial quadlet version integrated in golang

93c4775

Based on the initial port in containers/quadlet#41 Signed-off-by: Alexander Larsson <alexl@redhat.com>

rhatdan closed this Dec 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Initial version of golang port #41

WIP: Initial version of golang port #41

alexlarsson commented Sep 15, 2022

rhatdan commented Sep 16, 2022

alexlarsson commented Sep 23, 2022

vrothberg commented Sep 26, 2022

vrothberg left a comment •

edited

Loading

vrothberg Sep 26, 2022

alexlarsson Sep 29, 2022

vrothberg Sep 29, 2022

alexlarsson Sep 29, 2022

vrothberg Sep 26, 2022

alexlarsson Sep 29, 2022

vrothberg Sep 26, 2022

vrothberg Sep 26, 2022

alexlarsson Oct 3, 2022

vrothberg Sep 26, 2022

vrothberg Sep 26, 2022

alexlarsson Sep 27, 2022

vrothberg Sep 26, 2022

vrothberg Sep 26, 2022

vrothberg Sep 26, 2022

vrothberg Sep 26, 2022

vrothberg Sep 27, 2022

alexlarsson Sep 27, 2022

vrothberg Sep 28, 2022

alexlarsson Sep 29, 2022

vrothberg Sep 29, 2022

vrothberg Sep 29, 2022

vrothberg Sep 27, 2022

vrothberg Sep 27, 2022

alexlarsson Sep 27, 2022

alexlarsson Sep 27, 2022

rhatdan commented Dec 4, 2022


		os.MkdirAll(outputPath, os.ModePerm)

		for name, unit := range units {


		var validPortRange = regexp.MustCompile(`\d+(-\d+)?(/udp\|/tcp)?$`)

		var supportedContainerKeys = map[string]bool{

WIP: Initial version of golang port #41

WIP: Initial version of golang port #41

Conversation

alexlarsson commented Sep 15, 2022

rhatdan commented Sep 16, 2022

alexlarsson commented Sep 23, 2022

vrothberg commented Sep 26, 2022

vrothberg left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Dec 4, 2022

vrothberg left a comment •

edited

Loading