canonical / mir

The Mir compositor
GNU General Public License v2.0
627 stars 100 forks source link

StaticDisplayConfig: `unrecognized content` prevents Mir starting up #3628

Closed Saviq closed 4 days ago

Saviq commented 1 week ago

When the display file is not a valid yaml, or does not have the right contents, we should still start up with the default configs.

Not rewrite the file, like we do when empty - but still start up.

tarek-y-ismail commented 5 days ago

Small note: When the YAML file is empty, we don't write anything to it, we just throw ERROR: in display configuration file: 'path/to/file' : unrecognized content.

Saviq commented 5 days ago

OK looks like snap set… to empty tricked me, then. We should start up with defaults anyway. And continue to not paint over the empty file.

AlanGriffiths commented 4 days ago

When the display file is not a valid yaml, or does not have the right contents, we should still start up with the default configs.

Should we? That is contrary to our established strategy of reporting configuration errors noisily so that they get fixed.

The effect of this would be that setting an invalid display configuration gives unclear feedback: it leaves the system in an apparently working state. It may not be obvious that something is wrong.

Saviq commented 4 days ago

Should we? That is contrary to our established strategy of reporting configuration errors noisily so that they get fixed.

The effect of this would be that setting an invalid display configuration gives unclear feedback: it leaves the system in an apparently working state. It may not be obvious that something is wrong.

I can get convinced that we should only treat empty as if the file wasn't there…

OTOH, there are cases where starting up with a, possibly, not optimal configuration can be considered better than not starting at all (any kind of kiosk / point of sale / digital signage comes to mind). If we report an error, it's then up to the operator to monitor the logs for problems.

We could also use the diagnostic feature to (for a time?) signify a problem to whoever's at the display.

AlanGriffiths commented 4 days ago

We could also use the diagnostic feature to (for a time?) signify a problem to whoever's at the display.

That I like (for Frame). That could be generalised to any startup/initialization error reporting: we would just need to reexec with a "safe" config and error text.

I can get convinced that we should only treat empty as if the file wasn't there…

Me too, but I'd like to know how we get to that condition

AlanGriffiths commented 4 days ago

After a bit of discussion elsewhere I have a theory about what is happening. It sounds like an error in the configuration reloading code.

I found and fixed this in the ReloadingConfigFile code I "stole" from the ReloadingYamlFileDisplayConfig implementation. But haven't yet backported the corrected logic.

The problem is that Mir attempts to read a file when it is created, not when closed after write. At that point the file will appear empty.

Saviq commented 4 days ago

The problem is that Mir attempts to read a file when it is created, not when closed after write. At that point the file will appear empty.

In the case this issue stemmed from, the file did indeed end up empty somewhere between the equivalent of snap set ubuntu-frame display="…" and Frame starting up. It's then in a restart loop:

ubuntu-frame.daemon[4362]: + exec /snap/ubuntu-frame/9120/bin/graphics-core22-wrapper /snap/ubuntu-frame/9120/usr/local/bin/frame
ubuntu-frame.daemon[4362]: ERROR: in display configuration file: '/var/snap/ubuntu-frame/9120/frame.display' : unrecognized content
systemd[1]: snap.ubuntu-frame.daemon.service: Main process exited, code=exited, status=1/FAILURE
systemd[1]: snap.ubuntu-frame.daemon.service: Failed with result 'exit-code'.
AlanGriffiths commented 4 days ago

The problem is that Mir attempts to read a file when it is created, not when closed after write. At that point the file will appear empty.

While I'm thinking about it: Here's the fix I mention:

https://github.com/canonical/mir/pull/3636

Saviq commented 4 days ago

Closing in favour of #3637