Add ecu_version_report counter for primary #1451

xcheng-here · 2019-11-19T08:12:52Z

According to the standard of Uptane, add counter for
ECU version report. Currently, only added for primary.
It's name in the manifest is "report_counter", in the
payload of primary ecu version report.

Signed-off-by: cheng.xiang ext-cheng.xiang@here.com

xcheng-here · 2019-11-19T10:06:43Z

Ah, I missed scripts for database migration and rollback. And I didn't test all of the test cases before push it, only tested the case I modified. So stupid.

pattivacek

On the whole, looks like it's on the right track to me. You will need to add forward and backwards migrations, though.

A few tiny issues:

For future reference, the github branch name could be formatted a bit more nicely. See https://github.com/advancedtelematic/aktualizr/blob/master/CONTRIBUTING.md#making-a-pull-request. Including the ticket name is useful, but since no one outside of Here knows what those are, it's useful to specify fix/feat/etc. and a brief useful name, e.g. feat/OTA-3864/version-report-counter. No need to rename this PR, though.
Always feel free to select the team as reviewers of your PRs, although you might not get much feedback while the tests are failing.
One of the tests is failing due to metadata expiry, but Laurent has fixed it in his PR I just merged. A simple rebase on master should fix that.

pattivacek · 2019-11-19T10:02:48Z

src/libaktualizr/primary/initializer.cc

+
+  storage_->saveEcuReportCounter(Uptane::EcuSerial(ecu_serials[0].first.ToString()), 0);
+  return true;
+}
 // Postcondition: "ECUs registered" flag set in the storage


Tiny quibble, but an empty line here between the functions would be good.

pattivacek · 2019-11-19T10:06:41Z

src/libaktualizr/storage/sqlstorage.cc

+  // keep the same order as in ecu_serials (start with primary)
+  auto statement = db.prepareStatement(
+      "SELECT ecu_serial, counter FROM ecu_report_counter INNER JOIN ecu_serials ON "
+      "ecu_serials.serial = ecu_serial AND ecu_serials.is_primary = 1 ORDER BY ecu_serials.id;");


Why the inner join? Is that just to get the Primary first?

Yes, I want this sql help me filter the Primary for now. There is already a loop several lines below, it will put all of the ecu_serial in the vector. So, when I need to implement this feature for all of the ECUs, in this file I only need to modify this line of sql. I'm not very familiar with sql, just learned a few in 15 minutes. Do you think it's not good to use it in this way ?

Now I get it. I still think the ecu_serials.serial = ecu_serial AND ecu_serials.is_primary = 1 is unnecessary, though. You might as well get everything, and ordering by ecu_serials.id should mean the Primary is always first. You can see a similar example in SQLStorage::loadEcuInstallationResults() that does about the same thing.

It seems the SQLStorage::loadEcuInstallationResults() is also using "INNER JOIN". I think the reason might be that we only have "id" column in ecu_serial table, so if we want to order the result by "id", we still have to inner join with ecu_serial table. I think "ORDER BY" might be better than default order of "select".

Sorry, I misquoted what I thought was unnecessary. I think the INNER JOIN and ORDER BY are fine, it's just the AND ecu_serials.is_primary = 1 that I think is unnecessary.

pattivacek · 2019-11-19T10:07:43Z

src/libaktualizr/storage/sqlstorage.cc

+  auto statement = db.prepareStatement<std::string, int64_t>(
+      "INSERT OR REPLACE INTO ecu_report_counter (ecu_serial, counter) VALUES "
+      "(?,?);",
+      ecu_serial.ToString(), counter);


I'm not sure INSERT OR REPLACE is right here. We should enforce one counter per ECU, so won't we need a WHERE clause? The counter itself doesn't need to be unique.

I didn't use WHERE because I also use this function in initializer, when it's an empty table. After initialization, every call of this function should use an existing ecu_serial from database, so there will be only one line for each ecu in this ecu_report_counter table.

Okay, I think this was my mistake. I'm out of practice with SQL! Looking at other examples, this looks fine.

pattivacek · 2019-11-19T10:12:52Z

config/sql/schema.sql

@@ -23,3 +23,4 @@ CREATE TABLE ecu_installation_results(ecu_serial TEXT NOT NULL PRIMARY KEY, succ
 CREATE TABLE need_reboot(unique_mark INTEGER PRIMARY KEY CHECK (unique_mark = 0), flag INTEGER NOT NULL DEFAULT 0);
 CREATE TABLE rollback_migrations(version_from INT PRIMARY KEY, migration TEXT NOT NULL);
 CREATE TABLE delegations(meta BLOB NOT NULL, role_name TEXT NOT NULL, UNIQUE(role_name));
+CREATE TABLE ecu_report_counter(ecu_serial TEXT NOT NULL PRIMARY KEY, counter INTEGER NOT NULL DEFAULT 0);


Good call to include the ecu_serial already. Once this ticket is good and merged, I think part two will be adding secondary-specific counters, so it'll be nice not to have to do another migration.

pattivacek · 2019-11-19T10:15:47Z

src/libaktualizr/primary/sotauptaneclient.cc

+  if (!storage->loadEcuReportCounter(&ecu_cnt) || (ecu_cnt.size() == 0)) {
+    LOG_ERROR << "No ECU version report counter, please check the database!";
+  } else {
+    primary_ecu_version["report_counter"] = std::to_string(ecu_cnt[0].second + 1);


@simao @jochenschneider If we add the ECU version report counter here, does that look good to you? You can maybe see it better in the test below (https://github.com/advancedtelematic/aktualizr/pull/1451/files#diff-81426acc9fdba515977c8106aaa0eec4R159).

Is that field documented somewhere? It is hard to see the context here.

Is https://saeljira.it.here.com/browse/OTA-3864 enough context?

But actually, either way it makes sense to make this more obvious. @xcheng-here can you post an example of what a manifest with the counter would look like? You can get it from the logs while running manually or by debugging the test case that uses it.

Yes, I'd love to have an example.

Sure, I uploaded a manifest.txt to the jira case, it's at line 94.

codecov-io · 2019-11-20T12:02:41Z

Codecov Report

Merging #1451 into master will decrease coverage by 0.14%.
The diff coverage is 86.84%.

@@            Coverage Diff             @@
##           master    #1451      +/-   ##
==========================================
- Coverage   80.45%   80.31%   -0.15%     
==========================================
  Files         183      183              
  Lines       11010    11047      +37     
==========================================
+ Hits         8858     8872      +14     
- Misses       2152     2175      +23

Impacted Files	Coverage Δ
src/libaktualizr/storage/invstorage.h	`97.56% <ø> (ø)`	⬆️
src/libaktualizr/primary/initializer.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/storage/sqlstorage.h	`50% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`88.93% <100%> (+0.08%)`	⬆️
src/libaktualizr/primary/initializer.cc	`80.64% <81.81%> (-0.05%)`	⬇️
src/libaktualizr/storage/sqlstorage.cc	`76.45% <86.36%> (-1.36%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.cc	`78.37% <0%> (-2.03%)`	⬇️
src/libaktualizr/storage/sql_utils.h	`84.5% <0%> (-1.41%)`	⬇️

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 662aa23...3e6bb34. Read the comment docs.

According to the standard of Uptane, add counter for ECU version report. Currently, only added for primary. It's name in the manifest is "report_counter", in the payload of primary ecu version report. Signed-off-by: cheng.xiang <ext-cheng.xiang@here.com>

pattivacek

Looks good, thanks! The Travis failure is bogus; the tests passed.

@jochenschneider @simao how hard would it be to start checking for this counter?

jochenschneider · 2019-11-22T09:03:18Z

You mean checking that it increases? That wouldn't be rocket science. It's just that we are still working on the new version of the director and would preferably implement it there.

pattivacek · 2019-11-22T09:31:33Z

You mean checking that it increases? That wouldn't be rocket science. It's just that we are still working on the new version of the director and would preferably implement it there.

Sounds fine. I'd like to also implement this counter for secondaries as well, but I'd like to make sure we have this figured out with the backend side of things before we get too far ahead of ourselves on the embedded side. We can be patient for now, though. :)

pattivacek · 2019-11-25T08:29:24Z

It's just that we are still working on the new version of the director and would preferably implement it there.

Should I make a ticket for the BE to capture that work?

jochenschneider · 2019-11-25T09:03:28Z

That sounds like a good idea. Thanks!

pattivacek reviewed Nov 19, 2019

View reviewed changes

xcheng-here force-pushed the OTA-3864 branch 2 times, most recently from 8d72990 to ab445d4 Compare November 20, 2019 12:02

xcheng-here force-pushed the OTA-3864 branch from ab445d4 to 3e6bb34 Compare November 21, 2019 03:41

pattivacek approved these changes Nov 22, 2019

View reviewed changes

pattivacek merged commit 5b09647 into master Nov 22, 2019

pattivacek deleted the OTA-3864 branch November 22, 2019 08:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ecu_version_report counter for primary #1451

Add ecu_version_report counter for primary #1451

xcheng-here commented Nov 19, 2019

xcheng-here commented Nov 19, 2019

pattivacek left a comment

pattivacek Nov 19, 2019

pattivacek Nov 19, 2019

xcheng-here Nov 19, 2019

pattivacek Nov 19, 2019 •

edited

Loading

xcheng-here Nov 20, 2019

pattivacek Nov 20, 2019

pattivacek Nov 19, 2019

xcheng-here Nov 19, 2019

pattivacek Nov 19, 2019

pattivacek Nov 19, 2019

pattivacek Nov 19, 2019

jochenschneider Nov 19, 2019

pattivacek Nov 19, 2019

jochenschneider Nov 19, 2019

xcheng-here Nov 20, 2019

codecov-io commented Nov 20, 2019 •

edited

Loading

pattivacek left a comment

jochenschneider commented Nov 22, 2019 •

edited

Loading

pattivacek commented Nov 22, 2019

pattivacek commented Nov 25, 2019

jochenschneider commented Nov 25, 2019

Add ecu_version_report counter for primary #1451

Add ecu_version_report counter for primary #1451

Conversation

xcheng-here commented Nov 19, 2019

xcheng-here commented Nov 19, 2019

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

pattivacek Nov 19, 2019 • edited Loading

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

Choose a reason for hiding this comment

codecov-io commented Nov 20, 2019 • edited Loading

Codecov Report

pattivacek left a comment

Choose a reason for hiding this comment

jochenschneider commented Nov 22, 2019 • edited Loading

pattivacek commented Nov 22, 2019

pattivacek commented Nov 25, 2019

jochenschneider commented Nov 25, 2019

pattivacek Nov 19, 2019 •

edited

Loading

codecov-io commented Nov 20, 2019 •

edited

Loading

jochenschneider commented Nov 22, 2019 •

edited

Loading