Opened 7 years ago

Last modified 6 years ago

#4662 reopened defect

DataStore loses metadata between reboots

Reported by: uwog Owned by: tomeu
Priority: high Milestone: 9.1.0-cancelled
Component: sugar-datastore Version:
Keywords: 8.2.0:- 9.1.0:? Cc: bcsaller, tomeu, jg, paulswartz, philipmac, paulproteus, pascal, gregorio, morgs
Blocked By: Blocking:
Deployments affected: Action Needed: code
Verified: no

Description

The datastore seems to loose the 'source' metadata property for ds objects (and possibly other metadata).

See the example code, which adds an HTML file to the DS with having the 'source' metadata property set to true.

With the current jhbuild, when you resume such an entry from the journal, Write will correctly open it showing the contents of the file (ie. not with any markup, just raw data).

This will keep working, unless the DS is restarted. Then the 'source' metadata property of the journal object seems to be vanished.

This makes http://dev.laptop.org/search?q=4022 rather useless.

Attachments (3)

dtest.py (254 bytes) - added by uwog 7 years ago.
Sample python test program
ds.patch (342 bytes) - added by uwog 7 years ago.
Patch that fixes the issue (from marcopg)
test_ds.py (381 bytes) - added by tomeu 7 years ago.
another test script that queries entries with the source property

Download all attachments as: .zip

Change History (48)

Changed 7 years ago by uwog

Sample python test program

comment:1 Changed 7 years ago by marco

source is not in the model, so the problem might be limited to those properties. It should probably be added to the model but we want to solve the problem with non-model properties too, if that's the case.

Changed 7 years ago by uwog

Patch that fixes the issue (from marcopg)

comment:2 Changed 7 years ago by uwog

Note that that patch will not work for 'non-model' properties

comment:3 Changed 7 years ago by jg

  • Milestone changed from Never Assigned to Update.1

OK for Update.1

comment:4 Changed 7 years ago by tomeu

I don't think it's wise to restore support for non-model properties for Update1. I'd add the property 'source' to the model and log an error when a property not in the model is submitted.

comment:5 Changed 7 years ago by marco

I'm really really unhappy that we didn't get this one right.
But if you say it's not safe to change it at this point we will have to deal with it. So +1 on the plan from me.

comment:6 Changed 7 years ago by tomeu

  • Cc bcsaller added

Unless Ben can provide a solution that is not too invasive for Update1.

comment:7 Changed 7 years ago by bert

Does this mean any custom meta-data properties are not stored? That's ... very unfortunate. What is the definitive list of preserved properties then?

Changed 7 years ago by tomeu

another test script that queries entries with the source property

comment:9 Changed 7 years ago by tomeu

  • Owner changed from tomeu to erikos

With the atatched script, I see the 'source' property persisted across reboots. Suspecting of a bug in the activity code, I'm reassigning it to Simon.

comment:10 follow-up: Changed 7 years ago by marco

Tomeu, what is going here?

I thought you said custom metadata properties are not stored. Does your test script contradict that? Does the script restart the DS?

comment:11 in reply to: ↑ 10 Changed 7 years ago by tomeu

  • Cc tomeu added

Replying to marco:

Tomeu, what is going here?

Don't really know. I suspect of some error in the activity code.

I thought you said custom metadata properties are not stored.

This code made me think that:
http://dev.laptop.org/git?p=projects/datastore;a=blob;f=src/olpc/datastore/xapianindex.py;h=06041aa5770554f9739c0ccaa726aae92d0ad65e;hb=HEAD#l369

But after reading more, I saw that this other code will add the properties missing in the model to the secore config:

http://dev.laptop.org/git?p=projects/datastore;a=blob;f=src/olpc/datastore/xapianindex.py;h=06041aa5770554f9739c0ccaa726aae92d0ad65e;hb=HEAD#l310

Does your test script contradict that?

Yeah, you can run it and one entry with the source property will be created at every run. If you close sugar, restart and run it again, the script will show you that the entries created earlier still have the source property.

Does the script restart the DS?

No, you need to manually restart the DS when you want to check if the files still have the source property.

comment:12 Changed 7 years ago by tomeu

  • Owner changed from erikos to ApprovalForUpdate

This patch should fix the issue of non-model properties getting lost:

diff --git a/src/olpc/datastore/xapianindex.py b/src/olpc/datastore/xapianindex.py
index 06041aa..dfef53c 100644
--- a/src/olpc/datastore/xapianindex.py
+++ b/src/olpc/datastore/xapianindex.py
@@ -104,6 +104,12 @@ class IndexManager(object):
         datamodel = kwargs.get('model', model.defaultModel)
         datamodel.apply(self)
 
+        # configure the model according to the database
+        for field_name in self.write_index._field_actions:
+            if field_name not in datamodel.fields:
+                datamodel.addField(field_name, 'string')
+            self.fields.add(field_name)
+
         # store a reference
         self.datamodel = datamodel
         
@@ -308,6 +314,7 @@ class IndexManager(object):
         d = {}
         add_anything = False
         for k,v in props.iteritems():
+            k = str(k)
             p, added = self.datamodel.fromstring(k, v,
                                                  allowAddition=True)
             if added is True:

comment:13 Changed 7 years ago by jg

OK for update.1.

comment:14 Changed 7 years ago by tomeu

  • Owner changed from ApprovalForUpdate to marco

Pushed. Marco, can you do an rpm? Thanks!

comment:15 Changed 7 years ago by marco

  • Owner changed from marco to cscott

Built as sugar-datastore-0.3.
Reassigning to cscott to tag it.

comment:16 Changed 7 years ago by cscott

  • Owner changed from cscott to tomeu

Tagged for update1. Please verify that this is in update1-638 (or later) and close the bug.

comment:17 follow-up: Changed 7 years ago by cscott

  • Summary changed from DataStore looses metadata between reboots to DataStore loses metadata between reboots

Datastore is crashing in update1 after 637. Reverting to previous sugar-datastore-0.2 for update1-641. kim will attach crash logs.

comment:18 Changed 7 years ago by marco

#5072 has useful logs

comment:19 in reply to: ↑ 17 Changed 7 years ago by yani

Replying to cscott:

Datastore is crashing in update1 after 637. Reverting to previous sugar-datastore-0.2 for update1-641. kim will attach crash logs.

It seems to be the reason that many activities cant load in 640 : http://dev.laptop.org/ticket/5072

crash logs are available there

comment:20 Changed 7 years ago by tomeu

Patch attached to #5072.

comment:21 Changed 7 years ago by marco

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #5072

comment:22 Changed 7 years ago by tomeu

  • Milestone changed from Update.1 to Retriage, Please!

I think we should keep this one open until we have a fix in the builds.

#5072 is about activities crashing because of an incompatible upgrade between development versions.

Marking #5120 as a duplicate of this one.

Marking #5253 as depending on this one.

If we fix #5254 for Update.1, we could postpone this one to Update.2.

comment:23 Changed 7 years ago by tomeu

  • Resolution duplicate deleted
  • Status changed from closed to reopened

comment:24 Changed 7 years ago by jg

tomeu, what is your recommendation on this?

comment:25 Changed 7 years ago by tomeu

#5254 is already in joyride and I see no reason why it would not get into update.1.

So, this one should be deferred until update.2.

comment:26 follow-up: Changed 7 years ago by jg

  • Cc jg added

Tomeu, I'm confused as to what is to be deferred...

comment:27 in reply to: ↑ 26 Changed 7 years ago by tomeu

Replying to jg:

Tomeu, I'm confused as to what is to be deferred...

I think we should fix custom properties support for update.2 because it would be a quite invasive change that could render the laptops unusable. So by defer I meant to retarget for update.2.

comment:28 Changed 7 years ago by jg

  • Milestone changed from Retriage, Please! to Update.2

comment:29 Changed 7 years ago by paulswartz

  • Cc paulswartz added

comment:30 Changed 7 years ago by philipmac

  • Cc philipmac added

comment:31 Changed 7 years ago by paulproteus

  • Cc paulproteus added

comment:32 Changed 7 years ago by marco

What's the status of this one?

comment:33 Changed 6 years ago by tomeu

  • Keywords 8.2.0:? added

If we changed the datastore to read the json files from the fs instead of extracting the metadata from the xapian index, this could be solved. Performance and scalability may improve, as well.

comment:34 Changed 6 years ago by tomeu

  • Action Needed set to never set
  • Cc pascal added

Pascal has asked me about this issue, so here comes a brain dump:

The problem is that the DS stores info about properties in three places: in an in-memory dictionary, in model.py and in a xapian config file.

When we push a yet unknown property to the DS:

  • this property gets added to the in-memory dictionary,
  • xapian (or rather the xapian bindings) tracks it and updates its config file.

When we later retrieve an entry with that property, xapian will map it to the name we know and the DS will check in its in-memory dict if the property is known and which data type it has, etc.

Now the problem happens when the DS stops and starts again, because the in-memory dictionary gets recreated from model.py, so it doesn't know anything about properties not there. So that property gets dropped with the results we know.

One solution would be to store the custom properties in one more config file, another would be to fallback to some defaults when xapian hands us an unknown property.

comment:35 Changed 6 years ago by Eben

  • Action Needed changed from never set to code
  • Priority changed from normal to high

comment:36 Changed 6 years ago by gregorio

  • Cc gregorio added
  • Keywords 8.2.0:- 9.1.0? added; 8.2.0:? removed
  • Milestone changed from 8.2.0 (was Update.2) to 9.1.0

comment:37 Changed 6 years ago by tomeu

Any solutions to this look quite high risk to me. If anyone wants to provide a patch, we can think again if we should include it in 8.2.0.

comment:38 Changed 6 years ago by mstone

  • Keywords 9.1.0:? added; 9.1.0? removed

comment:39 Changed 6 years ago by morgs

  • Cc morgs added

comment:40 Changed 6 years ago by gregorio

Hi Guys,

I have heard from several deployments recently that students and teachers lose data (aka their work is not in the journal when they go to look for it later). Could this bug the cause of that?

That's a major concern and I want to address it as much as possible in this release.

I know its late in the game to add "high risk" code but I want to make progress on this soon so let me know if we have a design proposal and a scoping on what it would take to get this resolved.

Thanks,

Greg S

comment:41 follow-up: Changed 6 years ago by marco

No, this only apply to custom metadata which I don't we are using in activities yet.

comment:42 in reply to: ↑ 41 Changed 6 years ago by morgs

Replying to marco:

No, this only apply to custom metadata which I don't we are using in activities yet.

Read uses this to store the current page number.

I think Gregorio's scenario is #6014, solved by the session manager in 8.2.0. Without that, shutting down when you have activities open doesn't trigger a save.

comment:43 Changed 6 years ago by marco

Yeah that's probably the most likely cause. I wouldn't exclude other kind of dataloss though. We know that the datastore is every fragile.

comment:44 follow-up: Changed 6 years ago by gnu

Does Tomeu's new datastore implementation fix this bug? I.e. can an activity store whatever property names (and corresponding values) that it wants, and will the datastore retain them forever?

comment:45 in reply to: ↑ 44 Changed 6 years ago by tomeu

Replying to gnu:

Does Tomeu's new datastore implementation fix this bug? I.e. can an activity store whatever property names (and corresponding values) that it wants, and will the datastore retain them forever?

Yup!

Note: See TracTickets for help on using tickets.