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

manifest: Add irqbalance #158

Merged
merged 1 commit into from
Sep 15, 2019
Merged

Conversation

cgwalters
Copy link
Member

Pulled from RHCOS, but with the correct architecture conditionals
here, as well as some background links.

Pulled from RHCOS, but with the correct architecture conditionals
here, as well as some background links.
@bgilbert
Copy link
Contributor

bgilbert commented Sep 6, 2019

Content LGTM. Is this what we want to do? Current presets would have this enabled by default.

@cgwalters
Copy link
Member Author

I'm not an expert on irqbalance, but...yeah. For a long time dating back to the original Red Hat Linux, the system was defined via "comps", and the first reference I see to irqbalance is in this commit. No rationale though.

Heh, some searching around on this turns up interesting threads and perhaps more relevantly, this Debian bug.

@cgwalters
Copy link
Member Author

Another relevant bit:

Irqbalance/irqbalance#54

Since it's being actively developed, I'd lean towards shipping it. I think the people who had problems didn't end up reporting them upstream.

@lucab
Copy link
Contributor

lucab commented Sep 9, 2019

I'm not against this. But since this is coming from OpenShift, a couple of questions:

  • isn't this under the realm of the node-tuning team/operator?
  • is it preferred to have this running on nodes OS instead of orchestrated in a container (e.g. a daemonset)?

Additionally, this introducing a new etc-syconfig file is not exciting.

@cgwalters
Copy link
Member Author

cgwalters commented Sep 9, 2019

I guess the questions here are:

  • Is this something we want to share with FCOS by default?
  • Does this apply to "single node/non-Kubernetes cases" that are just running podman would want?
  • Are there "bootstrap" concerns, i.e. this is something often desirable Before=kubelet.service?

Additionally, this introducing a new etc-syconfig file is not exciting.

Yeah, more Red Hat Linux legacy 😦
That said a patch to have the service directly read /etc/irqbalance.conf with the same format would likely be trivial, and we could then change the package to drop the sysconfig file by default.

@dustymabe
Copy link
Member

Content LGTM.

LGTM too

Is this what we want to do? Current presets would have this enabled by default.

Looks like the service has WantedBy=multi-user.target. If I rpm-ostree install it and then reboot the service is running.

@bgilbert
Copy link
Contributor

Looks like the service has WantedBy=multi-user.target. If I rpm-ostree install it and then reboot the service is running.

Right, that was my point. 🙂

Container Linux doesn't run irqbalance, but Fedora Server does. I don't see much point in overriding the kernel here, but I guess I'm okay sticking with the Fedora default.

@cgwalters
Copy link
Member Author

Tangentially related to "userspace making policy/optimization decisions for the kernel" is systemd/systemd#13321 (comment)

On the other hand, I don't think the "bootstrap issue" applies as much to irqbalance as it does to block I/O scheduling.

@dustymabe
Copy link
Member

Right, that was my point. slightly_smiling_face

fail on my part. I read "Current presets wouldn't have this enabled by default." Where you actually said would.

@dustymabe
Copy link
Member

I'm not seeing any obvious objections.. Going once.. twice...

@lucab
Copy link
Contributor

lucab commented Sep 13, 2019

Digging a bit, I think we got an implicit request for this in CL once coreos/bugs#2135.

@cgwalters
Copy link
Member Author

@lucab Nice find!

@nhorman
Copy link

nhorman commented Sep 13, 2019

FWIW, irqbalance in almost all cases is a pretty significant component to use in just about every linux system. Its purpose is to codify irq handling policy across cpus. By default, that policy is to ensure that all cpus (to the best of their ability) share cpu load from generated interrupts. For systems that have large multiqueue nics, this results in more even irq loadbalancing, and (as a result), more consistent latency times, and reduced latency jitter. There are cases in which that policy should be modified, and irqbalance provides facilities to permit that. It should certainly be included in RHCOS builds. If its not, it will be likely that irq load will be primarily handled in cpu order (that is to say, cpu0 will be pegged with all the load generated by all interrupts on a system, with spillover being handled by cpu1, then cpu2, etc). Depending on timing and scheduling conflicts, you can see high latency on applications/containers that happen to get scheduled at the same time on those cpus, which results in poor user experiences.

As for configuration changes, I'm not especially open to dropping the /etc/sysconfig configuration mechanism. I say that because irqbalance is used on a huge range of platforms, including those that continue to run on sysvinit, which expects configuration to live there. Given that it pulls in that configuration on systemd driven system via the EnvironmentFile directive, I'm hard pressed to see what the advantage is to moving the configuration setup. If you would like to make an argument in its favor however, I'm happy to consider it.

@dustymabe dustymabe merged commit 6063ce4 into coreos:testing-devel Sep 15, 2019
@lucab
Copy link
Contributor

lucab commented Sep 16, 2019

@nhorman thanks for the feedback! I think we could move the remaining sysconf discussion to a packaging ticket, if you are ok with that.

Without breaking compatibility, I think there is still some wiggle room to move the defaults to a read-only location and leave users full control of content in /etc. A rough sketch of the plan would go as follows:

  • move the current default configuration sample out of /etc (as a naïf example, let's say to /usr/lib/irqbalance/defaults.env)
  • re-arrange the systemd unit to source multiple environment files, with the /etc one being optional, like this:
EnvironmentFile=/usr/lib/irqbalance/defaults.env
EnvironmentFile=-/etc/sysconfig/irqbalance

This should allow us to ship (and own) the default env-file as part of the OS immutable content, while allowing the user to create (and own) their own editable env-file under /etc (which could re-use the sysconfig path, if you prefer so).

@nhorman
Copy link

nhorman commented Sep 16, 2019

I'd be fine with augmenting the unit file to allow multiple optional environment files, but I'd would want to make them both optional. I assert that because, in the RHEL case, the defaults are to have no overrides in the environment file, and so I would like to avoid having to carry a second environment file there.

@lucab
Copy link
Contributor

lucab commented Sep 16, 2019

Sure. I'm not particularly attached to my suggestion; anything that helps splitting disto-defaults vs user-customizations is fine and helpful to me here. I've moved this to https://bugzilla.redhat.com/show_bug.cgi?id=1752520 if you'd like to chime in there directly.

@nhorman
Copy link

nhorman commented Sep 16, 2019

Can you open an issue upstream, and attach it to the above bz please? I handle the upstream project (here https://github.com/Irqbalance/irqbalance) , but Petr maintains it in RHEL these days. If you can do that, I'll fix it upstream, and petr can just backport it.

dustymabe pushed a commit to jbtrystram/fedora-coreos-config that referenced this pull request Apr 19, 2024
This updates the FAQ entry for locksmith/CLUO, pointing to their
morphing into Zincati and MCO.
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.

5 participants