Serialize downloads #1031

OYTIS · 2018-12-11T10:57:03Z

No description provided.

Parallel writes to SQLite database over-complicate the code for little benefit Signed-off-by: Anton Gerasimov <[email protected]>

Signed-off-by: Anton Gerasimov <[email protected]>

pattivacek · 2018-12-11T11:10:35Z

Before we merge this, we should double-check that no one is relying on something that we will break. I'll look into that. Conceptually, though, I have been convinced that this is the right way forward.

OYTIS · 2018-12-11T11:12:12Z

@patrickvacek What do you mean? Right now we only have sachertorte project as an external user, and I'm pretty sure it's fine.

pattivacek · 2018-12-11T11:20:42Z

@OYTIS I want to make sure that no one else was expecting this mis-feature. :)

Also, could you try adding the feature to implicitly pause when calling shutdown via the API?

codecov-io · 2018-12-11T11:29:19Z

Codecov Report

Merging #1031 into master will increase coverage by 0.02%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #1031      +/-   ##
==========================================
+ Coverage   82.29%   82.31%   +0.02%     
==========================================
  Files         189      189              
  Lines       13655    13663       +8     
==========================================
+ Hits        11237    11247      +10     
+ Misses       2418     2416       -2

Impacted Files	Coverage Δ
src/libaktualizr/primary/sotauptaneclient.cc	`82.33% <100%> (-0.06%)`	⬇️
...libaktualizr/package_manager/debianmanager_test.cc	`100% <100%> (ø)`	⬆️
src/libaktualizr/storage/storage_common_test.cc	`98.18% <100%> (+0.04%)`	⬆️
src/libaktualizr/storage/sqlstorage.cc	`74.96% <63.15%> (-1.22%)`	⬇️
src/libaktualizr/uptane/fetcher.cc	`97.16% <87.5%> (-0.81%)`	⬇️
src/libaktualizr/primary/reportqueue_test.cc	`97.29% <0%> (+1.35%)`	⬆️
src/libaktualizr/package_manager/ostreemanager.cc	`54.7% <0%> (+3.84%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 16b3ecb...c0e57f6. Read the comment docs.

pattivacek · 2018-12-11T13:07:32Z

Do you think it is reasonable to actually measure the download times with and without this change? I believe this should be an improvement, and I also know it will make things easier to maintain, but it's worth knowing what effects this has in practice.

eu-siemann · 2018-12-11T14:35:46Z

@patrickvacek Yes, makes sense to measure. We know the current speed already, it's about 64 kbit/s. When this PR is merged I'll create another one with updated fetcher test, which measures the speed.

That should increase the download speed and reduce the wear load. Signed-off-by: Anton Gerasimov <[email protected]>

lbonn · 2018-12-11T16:31:29Z

Some tests are failing in the debian package manager case (which is pretty obsolete, but still...)

OYTIS · 2018-12-11T16:37:15Z

@lbonn Yes, it uses WHandle directly, fixed that.

OYTIS · 2018-12-11T16:43:49Z

@eu-smirnov Thank you, I was wondering how this should be tested.

pattivacek

Looks fine, and I'm curious to see how it works in action!

pattivacek · 2018-12-12T09:06:11Z

src/libaktualizr/primary/aktualizr_test.cc

  }
  // It is possible for the PutManifestComplete to come before we get the
  // InstallTargetComplete depending on the threading, so check for both.
  if (allcomplete_FullMultipleSecondaries && complete_FullMultipleSecondaries == 2 &&
      manifest_FullMultipleSecondaries) {
+    LOG_ERROR << "SETVALUE";


Is this leftover from testing or intentional?

@patrickvacek That's debug output, thanks, I'll remove it.

eu-siemann · 2018-12-12T13:23:24Z

src/libaktualizr/storage/sqlstorage.cc

  }

  ~SQLTargetWHandle() override {
    if (!closed_) {
      LOG_WARNING << "Handle for file " << target_.filename() << " has not been committed or aborted, forcing abort";
+      db_.commitTransaction();


Didn't notice it before, but why do we call commit here? We want to abort the transaction, right?

Not quite sure actually. My idea was that if we don't abort explicitly then we probably want to save what we've already downloaded.
By the way, maybe we don't need explicit pause from Aktualizr::Shutdown in the end, @patrickvacek's commit confused me somewhat. Just need to add the test that the file really gets committed.

@OYTIS good call, actually! I overlooked the commit in the destructor, which is actually an even better solution than the implicit call to pause.

eu-siemann · 2018-12-12T15:45:24Z

I locally rebased my changes to fetcher/fetcher test to measure the download speed. It certainly got much better, up to 4 Mbit/s from previous 64 kbit/s.
For some reason, speed drops significantly after pause/resume cycle. I'll look into that.

OYTIS · 2018-12-12T15:53:16Z

@eu-smirnov Could you please share the rebased code on some branch? I might try to debug that as well.

Signed-off-by: Anton Gerasimov <[email protected]>

eu-siemann · 2018-12-12T15:57:52Z

@OYTIS Sure, pushed it to fix/fetcher-test.

OYTIS · 2018-12-12T16:02:50Z

@eu-smirnov Thanks!

OYTIS · 2018-12-13T10:46:00Z

OK, as we've found, transactions around sqlite_blob_* API calls are not needed at all. As @eu-smirnov suggested, we'll rewrite it to keep the blob open over wfeed operations, but the database schema needs to be reworked for that.

This PR already improves the speed significantly, but we can do better by keeping the blob open. Let's merge this one (after @eu-smirnov tries it with his #1032), and make the rest of improvements after that.

Blob handler is now kept open between the calls to wfeed Signed-off-by: Anton Gerasimov <[email protected]>

OYTIS · 2018-12-13T11:28:17Z

OK, so it was relatively simple, no schema redesign needed. Let's rebase it on @eu-smirnov's PR and measure the speed now.

pattivacek · 2018-12-13T12:47:39Z

This looks fine to me, but I'd prefer someone with better SQL knowledge to sign off if possible.

I want to make sure that no one else was expecting this mis-feature.

It doesn't sound like anyone will be upset if we remove it, especially since the numbers clearly demonstrate that this is a major improvement. (Or more accurately, this retracts what we now know to have been a downgrade, and it appears to moderately improve upon what we had before that.)

lbonn · 2018-12-13T13:05:06Z

src/libaktualizr/storage/sqlstorage.cc

+      int err = statement.step();
+      if (err != SQLITE_DONE) {
+        LOG_ERROR << "Could not save size in db: " << db_.errmsg();
+        throw StorageTargetWHandle::WriteError("could not update sise of " + target_.filename() + " in sql storage");


typo: "size"

lbonn · 2018-12-13T13:06:00Z

src/libaktualizr/storage/sqlstorage.cc

+    if (blob_ != nullptr) {
+      sqlite3_blob_close(blob_);
+      blob_ = nullptr;
+      auto statement = db_.prepareStatement<int64_t, int64_t>("update target_images SET real_size = ? where rowid = ?;",


really small, but we upper case sql operations everywhere else, would be nice to keep a consistent style for easy grepping etc

Sure, will fix.

lbonn · 2018-12-13T13:14:03Z

Looks good. I don't know if I really have more knowledge of sql but it seems to be consistent with the approached we agreed with.

eu-siemann · 2018-12-13T13:48:27Z

Awesome, now it takes 7 seconds to download a 100 MB file, which is around 120 Mbit/s or ~2000 times faster. Let's merge it, guys :)

pattivacek · 2018-12-13T13:53:50Z

For some reason, speed drops significantly after pause/resume cycle. I'll look into that.

@eu-smirnov Is this still happening? My guess is no, but just wanted to be sure...

eu-siemann · 2018-12-13T14:00:00Z

@patrickvacek nope, that one is also seems to be fixed

Signed-off-by: Anton Gerasimov <[email protected]>

Anton Gerasimov added 2 commits December 11, 2018 11:26

Rollback parallel downloads

568779a

Parallel writes to SQLite database over-complicate the code for little benefit Signed-off-by: Anton Gerasimov <[email protected]>

Remove the sql_mutex as it was only needed for parallel downloads

16312c0

Signed-off-by: Anton Gerasimov <[email protected]>

OYTIS requested review from patriotyk, lbonn, pattivacek and eu-siemann December 11, 2018 10:57

OYTIS force-pushed the bugfix/serializedownloads branch from 8c988a6 to eb2a3a0 Compare December 11, 2018 16:08

Write a target file in a single transaction

fdff394

That should increase the download speed and reduce the wear load. Signed-off-by: Anton Gerasimov <[email protected]>

eu-siemann approved these changes Dec 11, 2018

View reviewed changes

OYTIS force-pushed the bugfix/serializedownloads branch from eb2a3a0 to d810a39 Compare December 11, 2018 16:31

pattivacek reviewed Dec 12, 2018

View reviewed changes

eu-siemann reviewed Dec 12, 2018

View reviewed changes

Support pause/resume in single-transaction downloads

4086bda

Signed-off-by: Anton Gerasimov <[email protected]>

OYTIS force-pushed the bugfix/serializedownloads branch from d810a39 to 4086bda Compare December 12, 2018 16:21

Really get rid of unnecessary transactions

51fd83d

Blob handler is now kept open between the calls to wfeed Signed-off-by: Anton Gerasimov <[email protected]>

OYTIS force-pushed the bugfix/serializedownloads branch from 12ecdd5 to 51fd83d Compare December 13, 2018 11:26

lbonn reviewed Dec 13, 2018

View reviewed changes

Fix typos/style

c0e57f6

Signed-off-by: Anton Gerasimov <[email protected]>

lbonn approved these changes Dec 13, 2018

View reviewed changes

OYTIS merged commit b5d105c into master Dec 13, 2018

lbonn deleted the bugfix/serializedownloads branch December 13, 2018 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize downloads #1031

Serialize downloads #1031

OYTIS commented Dec 11, 2018

pattivacek commented Dec 11, 2018

OYTIS commented Dec 11, 2018

pattivacek commented Dec 11, 2018

codecov-io commented Dec 11, 2018 •

edited

Loading

pattivacek commented Dec 11, 2018

eu-siemann commented Dec 11, 2018

lbonn commented Dec 11, 2018

OYTIS commented Dec 11, 2018

OYTIS commented Dec 11, 2018

pattivacek left a comment

pattivacek Dec 12, 2018

OYTIS Dec 12, 2018

eu-siemann Dec 12, 2018

OYTIS Dec 12, 2018

pattivacek Dec 12, 2018

eu-siemann commented Dec 12, 2018

OYTIS commented Dec 12, 2018

eu-siemann commented Dec 12, 2018

OYTIS commented Dec 12, 2018

OYTIS commented Dec 13, 2018 •

edited

Loading

OYTIS commented Dec 13, 2018

pattivacek commented Dec 13, 2018

lbonn Dec 13, 2018

OYTIS Dec 13, 2018

lbonn Dec 13, 2018

OYTIS Dec 13, 2018

lbonn commented Dec 13, 2018

eu-siemann commented Dec 13, 2018

pattivacek commented Dec 13, 2018

eu-siemann commented Dec 13, 2018

Serialize downloads #1031

Serialize downloads #1031

Conversation

OYTIS commented Dec 11, 2018

pattivacek commented Dec 11, 2018

OYTIS commented Dec 11, 2018

pattivacek commented Dec 11, 2018

codecov-io commented Dec 11, 2018 • edited Loading

Codecov Report

pattivacek commented Dec 11, 2018

eu-siemann commented Dec 11, 2018

lbonn commented Dec 11, 2018

OYTIS commented Dec 11, 2018

OYTIS commented Dec 11, 2018

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eu-siemann commented Dec 12, 2018

OYTIS commented Dec 12, 2018

eu-siemann commented Dec 12, 2018

OYTIS commented Dec 12, 2018

OYTIS commented Dec 13, 2018 • edited Loading

OYTIS commented Dec 13, 2018

pattivacek commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn commented Dec 13, 2018

eu-siemann commented Dec 13, 2018

pattivacek commented Dec 13, 2018

eu-siemann commented Dec 13, 2018

codecov-io commented Dec 11, 2018 •

edited

Loading

OYTIS commented Dec 13, 2018 •

edited

Loading