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

StaticDisplayConfig: unrecognized content prevents Mir starting up #3628

Closed
Saviq opened this issue Oct 11, 2024 · 9 comments
Closed

StaticDisplayConfig: unrecognized content prevents Mir starting up #3628

Saviq opened this issue Oct 11, 2024 · 9 comments
Assignees

Comments

@Saviq
Copy link
Collaborator

Saviq commented Oct 11, 2024

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 tarek-y-ismail self-assigned this Oct 15, 2024
@tarek-y-ismail tarek-y-ismail added the triaged Triage into JIRA to plan it in label Oct 15, 2024
@tarek-y-ismail
Copy link
Contributor

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
Copy link
Collaborator Author

Saviq commented Oct 15, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator Author

Saviq commented Oct 16, 2024

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
Copy link
Collaborator

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
Copy link
Collaborator

AlanGriffiths commented Oct 16, 2024

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
Copy link
Collaborator Author

Saviq commented Oct 16, 2024

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
Copy link
Collaborator

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:

#3636

@Saviq
Copy link
Collaborator Author

Saviq commented Oct 16, 2024

Closing in favour of #3637

@Saviq Saviq closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants