Feat/ota 4984/secondary replacement #1686

pattivacek · 2020-06-08T15:13:11Z

Allow ECU re-registration to support adding, removing, and replacing Secondaries. Tested manually and with unit tests for virtual and IP Secondaries.

codecov-commenter · 2020-06-08T15:37:40Z

Codecov Report

Merging #1686 into master will decrease coverage by 13.22%.
The diff coverage is 56.25%.

@@             Coverage Diff             @@
##           master    #1686       +/-   ##
===========================================
- Coverage   78.26%   65.03%   -13.23%     
===========================================
  Files         190      190               
  Lines       12756    15346     +2590     
===========================================
- Hits         9983     9980        -3     
- Misses       2773     5366     +2593

Impacted Files	Coverage Δ
src/aktualizr_secondary/aktualizr_secondary.cc	`63.58% <0.00%> (-22.36%)`	⬇️
...aktualizr_secondary/aktualizr_secondary_factory.cc	`48.14% <ø> (-13.76%)`	⬇️
src/libaktualizr/primary/aktualizr.cc	`58.88% <ø> (-37.02%)`	⬇️
src/libaktualizr/primary/aktualizr.h	`75.00% <ø> (-25.00%)`	⬇️
src/libaktualizr/primary/sotauptaneclient.h	`57.89% <0.00%> (-17.11%)`	⬇️
src/libaktualizr/storage/invstorage.h	`24.35% <0.00%> (-7.85%)`	⬇️
src/libaktualizr/storage/sqlstorage.h	`33.33% <ø> (ø)`
src/libaktualizr/primary/sotauptaneclient.cc	`50.73% <33.33%> (-36.26%)`	⬇️
src/libaktualizr/primary/initializer.cc	`53.72% <51.76%> (-36.03%)`	⬇️
src/libaktualizr-posix/ipuptanesecondary.cc	`88.98% <58.82%> (-0.31%)`	⬇️
... and 56 more

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 889ba11...d943cd9. Read the comment docs.

pattivacek · 2020-06-09T06:43:37Z

The LGTM failure looks pretty bogus to me; it failed because of a timeout fetching from the Ubuntu servers. I can't figure out how to retry it, though.

lbonn · 2020-06-09T07:35:51Z

The LGTM failure looks pretty bogus to me; it failed because of a timeout fetching from the Ubuntu servers. I can't figure out how to retry it, though.

If you log in, they have a "retry analysis" button. Just clicked it.

pattivacek · 2020-06-09T07:41:34Z

If you log in, they have a "retry analysis" button. Just clicked it.

Thanks. I logged in and looked around but couldn't find it. Anyway, I just pushed another tiny commit, so it probably got retriggered by that, too.

lbonn · 2020-06-09T09:46:47Z

src/libaktualizr/storage/sqlstorage.cc

-      auto statement = db.prepareStatement<std::string, std::string, int>(
-          "INSERT INTO misconfigured_ecus VALUES (?,?,?);", it->serial.ToString(), it->hardware_id.ToString(),
-          static_cast<int>(it->state));
+  auto statement = db.prepareStatement<std::string>("SELECT count(*) FROM misconfigured_ecus WHERE serial = ?;",


We could get rid of the count and use INSERT OR REPLACE here. serial would be better off being a primary key instead of just "unique" but it should also work.

We could get rid of the count and use INSERT OR REPLACE here.

Good point.

serial would be better off being a primary key instead of just "unique" but it should also work.

True, but then I have to add a migration. Is that worth it?

I think it should work without so probably not.

lbonn · 2020-06-16T09:32:02Z

src/libaktualizr/primary/initializer.cc

+                 << it->second->getHwId() << " not found in storage.";
+        register_ecus = true;
+      } else {
+        found[static_cast<size_t>(std::distance(stored_ecu_serials.cbegin(), store_it))] = true;


Suggested change

found[static_cast<size_t>(std::distance(stored_ecu_serials.cbegin(), store_it))] = true;

found[store_it - stored_ecu_serials.cbegin()] = true;

Ah this code was just moved...

Indeed, but I think I also wrote it originally. Can't remember why exactly I used std::distance here. Do you see a good reason to prefer one over the other?

oh oops I thought I answered earlier, must have missed the button.

So after reading some docs, I can't really see the difference but the substraction is shorter and seems clearer to me.

src/libaktualizr/primary/initializer.cc

lbonn · 2020-06-16T09:45:04Z

src/aktualizr_primary/primary_secondary_registration_test.cc

@@ -65,53 +64,11 @@ TEST(PrimarySecondaryReg, SecondariesMigration) {
    EXPECT_EQ(secs_info[0].type, "IP");
    EXPECT_EQ(secs_info[0].extra, R"({"ip":"127.0.0.1","port":9061})");
  }
-


What's the reason to remove this part of the test?

I knew you were going to ask about that! Well, I spent a lot of time trying to get it to work, and then I realized it was testing something that we never tried to fix: it's migrating two Secondaries from the old storage layout to the new storage. In the actual migration code for that (in src/aktualizr_primary/secondary.cc), it explicitly only does the migration for one Secondary. Previously, this test was doing something kind of strange. Now, this isn't allowed: we just reprovision the device with two Secondaries in this case. Migration doesn't really make sense here.

Thanks! That's a satisfying reason.

pattivacek · 2020-06-17T08:17:41Z

I've added a fix for a somewhat obscure issue with trying to reregister Secondaries while an update is active. The backend will reject the registration, and the only resolution is to restore the previous Secodnary configuration, which was complicated by the fact that we'd already stored the new Secondary information. Now we wait to store it until registration is succesful.

pattivacek · 2020-06-17T09:14:01Z

@lbonn Sorry, just blasted your review because I realized I'd broken the networking test without realizing it. Thankfully the fix was easy. :)

pattivacek · 2020-06-17T09:54:28Z

Finally fixed all the tests.

eu-siemann · 2020-06-17T11:24:35Z

src/libaktualizr/storage/sqlstorage.cc

+  auto statement = db.prepareStatement<std::string, std::string, int>(
+      "INSERT OR REPLACE INTO misconfigured_ecus VALUES (?,?,?);", ecu.serial.ToString(), ecu.hardware_id.ToString(),
+      static_cast<int>(ecu.state));
+  if (statement.step() != SQLITE_DONE || sqlite3_changes(db.get()) != 1) {


I'm not sure it's worth checking sqlite3_changes value here. My understanding was that it is only useful when you have WHERE in your statement or something similar.

Oops, that was lazy copying on my part, didn't even think about it. I'll double check but will probably remove it.

Fixed. Not is it not necessary, but according to https://www.sqlite.org/c3ref/changes.html it doesn't even do anything for REPLACE, so it would've caused a bug.

That was my first impression as well, but it looks like while REPLACE doesn't increment the counter, the subsequent INSERT does. As I understood, REPLACE does only the deleting part.

Oh, I see. Well, I removed it anyway. :) Seems unnecessary.

Works for Virtual Secondaries, but still a work in progress for IP Secondaries. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Now they cover adding, remove, and replacing Secondary ECUs. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

The logic for matching addresses to ECUs is not trivial, so it's worth being extra careful. Also test replacing an ECU that reuses the same address and port and that we correctly track an ECU that changes its address or port. Also note some the timeouts (and some ordering issues) that were necessary to get some of these tests to pass. These are more reasons why a proper connection manager would be nice to have; things like this really shouldn't be necessary. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

We no longer track unregistered ECUs, since that is no longer a problem. Still dump anything in the database with that flag, just in case, but it shouldn't happen. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

There's no real problem, just an ugly log message that will go away eventually on its own. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

For device credential provisioning, it is still a fatal error if the cert is unavailable. For shared cred prov, we revert to the old behavior of generating a random pet name if the cert is unavailable. This is generally only observable if you attempt to replace a Primary ECU, which requires reusing the same device certificate. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

The backend does not accept empty filenames, but we have to say something. This comes up if you wipe a Secondary's database but keep the image file. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

If registration fails (due to the server complaining about an update in progress, for example), we want to make it easier to undo the changes and prevent re-registration. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek requested review from simao, lbonn, eu-siemann, Raigi, kbushgit and zabbal June 8, 2020 15:13

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from 81e1837 to c890cf7 Compare June 9, 2020 07:39

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from c890cf7 to 70df3f1 Compare June 9, 2020 07:53

lbonn reviewed Jun 9, 2020

View reviewed changes

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch 3 times, most recently from 0cbc6bc to 5201bf4 Compare June 15, 2020 12:22

lbonn reviewed Jun 16, 2020

View reviewed changes

src/libaktualizr/primary/initializer.cc Outdated Show resolved Hide resolved

lbonn reviewed Jun 16, 2020

View reviewed changes

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from 5201bf4 to b73bde3 Compare June 17, 2020 08:15

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from b73bde3 to 0e688c5 Compare June 17, 2020 08:54

lbonn previously approved these changes Jun 17, 2020

View reviewed changes

pattivacek dismissed lbonn’s stale review via 6b43af9 June 17, 2020 09:13

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from 0e688c5 to 6b43af9 Compare June 17, 2020 09:13

lbonn previously approved these changes Jun 17, 2020

View reviewed changes

eu-siemann reviewed Jun 17, 2020

View reviewed changes

Allow ECU re-registration to support changing Secondaries.

66b2c85

Works for Virtual Secondaries, but still a work in progress for IP Secondaries. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek added 9 commits June 17, 2020 13:37

Fix IP Secondary re-registration and some broken tests.

1ed4d38

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Expand Secondary registration tests.

57673c8

Now they cover adding, remove, and replacing Secondary ECUs. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Fix a bogus message about unregistered ECUs.

b11a785

We no longer track unregistered ECUs, since that is no longer a problem. Still dump anything in the database with that flag, just in case, but it shouldn't happen. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Add a comment about a semi-obscure almost-bug.

e473c86

There's no real problem, just an ugly log message that will go away eventually on its own. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Fall back to "unknown" image name instead of an empty string.

64aac55

The backend does not accept empty filenames, but we have to say something. This comes up if you wipe a Secondary's database but keep the image file. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

uptane_serial_test: Get rid of unused input parameter.

3ca4e32

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek dismissed lbonn’s stale review via d943cd9 June 17, 2020 11:37

pattivacek force-pushed the feat/OTA-4984/secondary-replacement branch from 6b43af9 to d943cd9 Compare June 17, 2020 11:37

eu-siemann self-requested a review June 17, 2020 12:03

eu-siemann approved these changes Jun 17, 2020

View reviewed changes

pattivacek merged commit f868186 into master Jun 17, 2020

pattivacek deleted the feat/OTA-4984/secondary-replacement branch June 17, 2020 12:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ota 4984/secondary replacement #1686

Feat/ota 4984/secondary replacement #1686

pattivacek commented Jun 8, 2020

codecov-commenter commented Jun 8, 2020 •

edited

Loading

pattivacek commented Jun 9, 2020

lbonn commented Jun 9, 2020

pattivacek commented Jun 9, 2020

lbonn Jun 9, 2020

pattivacek Jun 9, 2020

lbonn Jun 9, 2020

lbonn Jun 16, 2020

lbonn Jun 16, 2020

pattivacek Jun 16, 2020

lbonn Jun 16, 2020

pattivacek Jun 17, 2020

lbonn Jun 16, 2020

pattivacek Jun 16, 2020

lbonn Jun 16, 2020

pattivacek commented Jun 17, 2020

pattivacek commented Jun 17, 2020

pattivacek commented Jun 17, 2020

eu-siemann Jun 17, 2020

pattivacek Jun 17, 2020

pattivacek Jun 17, 2020

eu-siemann Jun 17, 2020

pattivacek Jun 17, 2020

	found[static_cast<size_t>(std::distance(stored_ecu_serials.cbegin(), store_it))] = true;
	found[store_it - stored_ecu_serials.cbegin()] = true;

Feat/ota 4984/secondary replacement #1686

Feat/ota 4984/secondary replacement #1686

Conversation

pattivacek commented Jun 8, 2020

codecov-commenter commented Jun 8, 2020 • edited Loading

Codecov Report

pattivacek commented Jun 9, 2020

lbonn commented Jun 9, 2020

pattivacek commented Jun 9, 2020

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

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

pattivacek commented Jun 17, 2020

pattivacek commented Jun 17, 2020

pattivacek commented Jun 17, 2020

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

codecov-commenter commented Jun 8, 2020 •

edited

Loading