Recover from broken key pair

[sayamindu]

We have had instances of damaged/unreadable ssh key files after a forced power off. It may make sense to call sync() to write these and everything else to the disk to ensure that the system does not remain in an unusable state (due to the damaged ssh key file, Sugar refused to start, sending the display manager in a loop).

[sayamindu]

Proposed patch

[dsd]

I disagree with this approach. The system could still crash while writing, after writing but before sync, or during sync. Or the file could be corrupted for another reason (for example the system crashing while another file on the same eraseblock is being rewritten by the SD controller). So the bug will still be present, but just less common.

Moreover, this approach could lead to hundreds of sync() calls scattered throughout the codebase.

Sugar should simply be resilient against all corruption like this. If there is something totally fatal it should present an error message and not crash, but otherwise it should just clean it up, regenerating whatever is needed.

And if there is any data that would be a real pain to lose then it should be explicitly flushed on a per-file basis using fsync(), along with a fat comment explaining why it would be a huge pain to lose this. But sugar still has to be able to cope when this file is corrupt either way.

[sayamindu]

While I agree that calling sync() everywhere is not a nice idea, and fsync() is a better option (I have already proposed that we use fsync() at a couple of places in the datastore), there are certain areas which merit the use of a sync, because a large number of files may be in use, some of which are outside the scope of the sugar codebase (gconf databases, ssh keys, etc). For example, for the following log:

1257402816.022310 ERROR root: Keypair exists, skip generation.
1257402817.445231 ERROR dbus.proxies: Introspect error on org.laptop.Sugar.Presence:/org/laptop/Sugar/Presence: dbus.exceptions.DBusException: org.freedesktop.DBus.Error.Spawn.ChildExited: Launch helper exited with unknown return code 1
1257402818.312790 WARNING sugar.presence.presenceservice: Unable to retrieve local user/owner
                   from presence service: org.freedesktop.DBus.Error.Spawn.ChildExited: Launch helper exited with unknown return code 1
Traceback (most recent call last):
  File "/usr/bin/sugar-session", line 173, in <module>
    main()
  File "/usr/bin/sugar-session", line 145, in main
    start_ui_service()
  File "/usr/bin/sugar-session", line 90, in start_ui_service
    ui_service.start()
  File "/usr/lib/python2.6/site-packages/jarabe/view/service.py", line 63, in start
    owner_model = owner.get_model()
  File "/usr/lib/python2.6/site-packages/jarabe/model/owner.py", line 112, in get_model
    _model = Owner()
  File "/usr/lib/python2.6/site-packages/jarabe/model/owner.py", line 77, in __init__
    self._invites = Invites()
  File "/usr/lib/python2.6/site-packages/jarabe/model/invites.py", line 78, in __init__
    owner = ps.get_owner()
  File "/usr/lib/python2.6/site-packages/sugar/presence/presenceservice.py", line 459, in get_owner
    raise RuntimeError("Could not get owner object.")
RuntimeError: Could not get owner object.

It is very difficult to recover from this gracefully.
To be even more fool-proof, I think we can take a look at the places we can recover gracefully (either by regenerating the data or by using sane defaults), but we can never say for sure that sugar will not break due to filesystem issues.

[dsd]

I think we should work through the bugs on a case-by-case issue and fix them. I don't see why the above issue would be difficult to fix (it may be, but theres nothing in the log that suggests to me that it would be). Sugar should aim to be able to cope with any corruption, or at least gracefully display an "I'm broken" error message.

[tomeu]

Replying to dsd:

I think we should work through the bugs on a case-by-case issue and fix them. I don't see why the above issue would be difficult to fix (it may be, but theres nothing in the log that suggests to me that it would be). Sugar should aim to be able to cope with any corruption, or at least gracefully display an "I'm broken" error message.

This argument sounds good to me in principle, but I think we should be careful and avoid end up shipping less stable releases because we aimed for much we could tackle in the current release cycle.

That said, it may be interesting to see which files are written by sugar and test each of those for corruption and implement fallbacks for all those, instead of waiting for user reports about corruption. Any takers?

[tomeu]

Oh, and we should try to never use sync() as it will cause stuff unrelated to Sugar to be committed, making our performance profile less predictable (apart from the user experience).

[erikos]

I guess the outcome of this discussion is to test through all the files written by Sugar for corruptions. Does the submitter agree and we can remove the r? ?

[tomeu]

removing from the review queue until the submitter comments

[sascha_silbe]

recreate corrupted key pair

[sascha_silbe]

check private key syntax

[sascha_silbe]

I've attached patches to recover from a broken key pair. Please open new tickets for anything that breaks due to some file that's written by Sugar getting corrupted.

[tomeu]

tomeu: unmadindu: what do you think of the patch in http://bugs.sugarlabs.org/ticket/1568 ? do we also need to sync or that's fine with you?
unmadindu: tomeu: I think this is good enough

Patches look good to me, other than the lack of spaces around the + operator and having logging.error("Error parsing public key.") not log a backtrace (I know this is not introduced by this patch).

[erikos]

The sugar-toolkit part looks good to me. We can maybe add the backtrace, like Tomeu suggested.

[sascha_silbe]

Pushed as  bb323a4, thanks!

[sayamindu]

Cherry-picked.

[sascha_silbe]

Reopening because we only reviewed and fixed the sugar-toolkit part, but not sugar. The  updated patch is currently being reviewed on sugar-devel.

[erikos]

has been pushed: 516517a81ecfa0d3d2524f885f30b0cf2b0052c9

Is part of Sugar 0.95.6

[greenfeld]

If the public key or the private key is missing or damaged (in certain manners), we regenerate the SSH keys used by Sugar in OLPC 12.1.0 os11.

However key regeneration also seems to force Sugar to go through it's first boot name/icon color prompt cycle. I do not know if this is intentional.

[sascha_silbe]

Replying to greenfeld:

If the public key or the private key is missing or damaged (in certain manners), we regenerate the SSH keys used by Sugar in OLPC 12.1.0 os11.

However key regeneration also seems to force Sugar to go through it's first boot name/icon color prompt cycle. I do not know if this is intentional.

It's been that way before the patch already. We're showing the intro screen if any of nick name, colour or SSH key is missing (all) or corrupt (SSH key only).

I don't know whether there was a conscious choice before, but it's at least a reasonable thing to do: If the key is corrupted (presumably by the machine crashing early during first Sugar usage), there's a good chance the name and colour settings are corrupted as well.