-
Notifications
You must be signed in to change notification settings - Fork 571
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 check for /boot/cmdline.txt #206
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this works for Raspbian/Raspberry Pi OS but not for vanilla Debian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an observation but I don't understand all the extra complexity. Going back one step, the code introduced by PR201 is straightforward:
The problem is the implied assumption that
/etc/default/grub
always exists, with the consequence thatgrep
fails when it doesn't.Why not just augment the
if
test to protect thegrep
? Something like:I realise that has the same effect as the first part of your proposal. I suppose the bit I don't get is the stuff with
cmdline.txt
. It seems to me that that's not needed if either of the following is true:/etc/default/grub
doesn't exist; or/etc/default/grub
exists and containssystemd.unified_cgroup_hierarchy=false
Am I missing the point entirely?
I keep looking at that "not" in
! grep
and wondering if it is somehow misleading my basic analysis?Here is what I think is going on. First, a test:
Starting point = /etc/default/grub does not exist
Create /etc/default/grub as an empty file (ie the file will exist but doesn't contain the magic incantation):
Now add the magic incantation:
Don't those three tests reflect the desired behaviour? The conditional code should only execute on a system that actually knows about grub, and where the expected file does not contain the magic incantation?
Then, if the system does not know about grub, why be adding
systemd.unified_cgroup_hierarchy=false
to cmdline? I'm really confused. Apologies if I wind up confusing everyone else too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I understood is that we're trying to solve issue home-assistant/supervisor#3444 where
cgroup2
is the default on Debian and Home Assistant requirescgroup1
. This is not related to grub, but grub is used to set the correct version ofcgroup
. Raspberry Pi doesn't support grub even on Debian Bullseye, so even if the test you mention is correct, the issue wouldn't be fixed.So, we still need to insert the appropriate line of code into a file that controls the version of
cgroup
on non-grub systems, which in the case of Raspberry Pi is/boot/firmware/cmdline.txt
for vanilla Debian Bullseye and/boot/cmdline.txt
for Raspberry Pi OS (which is not supported and hence the edit).I don't expect all non-grub devices to be supported, but the Raspberry Pi is extremely popular and regularly used to run Home Assistant, so I would expect this device to be supported (still running vanilla Debian Bullseye as required).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the clarification. I came into this via a different path - a person reporting
cp: cannot stat '/etc/default/grub': No such file or directory
so I was just looking at the symptoms.I don't know if it will help or hinder your investigations but I just did a fresh build of current Raspberry Pi OS 64-bit (which describes itself as Debian). My cmdline.txt has:
As I understand it,
cgroup_memory=1 cgroup_enable=memory
enables cgroups, save that I think of it as being needed fordocker stats
rather than HA.When I run:
So, all other things being equal, this should be incompatible with HA and trigger the #3444 problem - yes?
All I did before installing HA was:
After installing HA:
Collecting all the container logs (attached) and looking for mentions of "cgroup" yields silence.
Searching
/var/log/syslog
gets a few mentions but nothing saying "this ain't working":Other stuff:
Bottom line: it looks to me like HA is working without needing to add anything like
systemd.unified_cgroup_hierarchy=false
to/boot/cmdline.txt
.Anywhere else I should be looking for signs of this problem?
collected-home-assistant-logs.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is originally reported as happening when using RaspberryMatic add-on in Home Assistant. I do not use this add-on, but if you want to check for errors, I suggest you install it. Here's a guide, which actually has a warning regarding the original problem with
cgroup
versions, and even mentions what to do to fix it.Thanks for the info, but unless Home Assistant decides to support other OSs (even if they are based on Debian), only Debian Bullseye is supported; they are very explicit about this in their documentation. If you want to re-do your tests (including RaspberryMatic), I suggest you download Debian v11 (Bullseye) for your hardware from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you switch all the references from
/boot/cmdline.txt
to/boot/firmware/cmdline.txt
as suggested? I have vanilla debian on an rpi4 right now and am hitting this. Can't test your branch because this file also doesn't exist.@pvizeli after this change I would suggest we merge this. I don't believe this is an unsupported OS issue. I am hitting this right now on a freshly flashed vanilla image from here on an RPI4 which should be supported (it's in our list of supported machine types and running vanilla debian 11)