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

Add systemd-devel to the buildenv fedora image #1493

Merged
merged 1 commit into from
Feb 2, 2022

Conversation

paulpopelka
Copy link
Contributor

@paulpopelka paulpopelka commented Feb 1, 2022

We are going to build krun with systemd support compiled in.
This needs the systemd-devel package in the buildenv image.

Once this change is merged, I'm going to push the new buildenv-image to our image repository.
Then I will submit another PR that changes the krun build to make krun that uses systemd and change km to use the krun that is systemd aware.
Correct me if this is the wrong procedure.

We are going to build krun with systemd support compiled in.
This needs the systemd-devel package in the buildenv image.
@paulpopelka paulpopelka self-assigned this Feb 1, 2022
@paulpopelka paulpopelka changed the title Add systemd-delvel to the buildenv fedora image Add systemd-devel to the buildenv fedora image Feb 1, 2022
@johnmuth81
Copy link
Contributor

Do we have a writeup explaining the motivation behind this decision? I'm not against it, but what's motivating it?

@paulpopelka
Copy link
Contributor Author

Do we have a writeup explaining the motivation behind this decision? I'm not against it, but what's motivating it?

No writeup exists, but I'll summarize the steps that got us here.
This is a result of a moderately long chain of events explained below:
The initial problem was crun was using oci image-spec 1.0.1 go code which apparently has a flaw that I thought github had discovered in crun and complained about.
It turned out github was complaining about tools/faktory/go.mod requiring image-spec 1.0.1.
That is fixed in another PR.
I don't think github had any idea that crun was using image-spec 1.0.1 code.
Since image-spec 1.0.1 is still problematic, we still need to change crun to use image-spec 1.0.2
So, we moved our krun changes on top of crun 1.4 to get image-spec 1.0.2
Testing krun 1.4 exposed a problem with krun trying to use systemd to setup cgroups when krun is built without systemd support.
Apparently krun in the past could deal with the --systemd-cgroup flag on its command line when the systemd support is not compiled into krun. With 1.4 it no longer can.
So, I changed our krun build to include systemd which requires the systemd-devel package.
This exposed yet another bug in crun where the cgroup CPUshares attribute is getting an invalid value from the config.json supplied to crun by docker/containerd. Turns out the "invalid" value is zero and docker folks decided that means use the default CPUshares value even though the oci-runtime specification doesn't mention any of this.
So, the crun folks fixed this problem and to get the fix I moved our krun changes on top of crun 1.4.2 and we still need to compile krun with systemd support built in.

Apparently some people think this is fun. It's just more clown car software engineering in action.

@paulpopelka
Copy link
Contributor Author

paulpopelka commented Feb 2, 2022

I checked https://opencontainers.org/release-notices/overview/ to see if there is a newer version of the oci-runtime spec that says a cpu share attribute with value 0 means use the default.
I am looking at runtime spec 1.0.2 and it says:

CPU
cpu(object, OPTIONAL) represents the cgroup subsystemscpuandcpusets.
For more information, see the kernel cgroups documentation about cpusets.
The following parameters can be specified to set up the controller:

•shares(uint64, OPTIONAL)- specifies a relative share of CPU time available to the tasks in a cgroup
.......

@paulpopelka paulpopelka merged commit 6f4957e into master Feb 2, 2022
@paulpopelka paulpopelka deleted the paulp/add-systemd-devel-to-buildenv branch February 2, 2022 19:47
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.

3 participants