ostree update on IP secondary (reworked) #1518

mike-sul · 2020-01-15T10:22:08Z

No description provided.

codecov-io · 2020-01-15T11:24:50Z

Codecov Report

Merging #1518 into master will increase coverage by 0.12%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##           master    #1518      +/-   ##
==========================================
+ Coverage   80.74%   80.86%   +0.12%     
==========================================
  Files         184      184              
  Lines       11181    11196      +15     
==========================================
+ Hits         9028     9054      +26     
+ Misses       2153     2142      -11

Impacted Files	Coverage Δ
src/libaktualizr/storage/sqlstorage.h	`50% <ø> (ø)`	⬆️
src/libaktualizr/storage/invstorage.h	`97.56% <ø> (ø)`	⬆️
...tualizr/package_manager/packagemanagerinterface.cc	`94.28% <100%> (+0.16%)`	⬆️
src/libaktualizr/storage/sqlstorage.cc	`77.09% <83.33%> (+0.75%)`	⬆️
src/aktualizr_info/main.cc	`90.23% <0%> (-0.94%)`	⬇️
src/libaktualizr/package_manager/ostreemanager.cc	`78.27% <0%> (-0.75%)`	⬇️
src/libaktualizr/primary/sotauptaneclient.cc	`88.95% <0%> (+0.13%)`	⬆️
src/libaktualizr/storage/sql_utils.h	`86.11% <0%> (+1.38%)`	⬆️
src/libaktualizr/storage/sqlstorage_base.cc	`79.05% <0%> (+4.72%)`	⬆️
... and 1 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 635962f...ed9a0e3. Read the comment docs.

lgtm-com · 2020-01-15T12:45:00Z

This pull request fixes 1 alert when merging de61528 into 21c8595 - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-16T11:09:26Z

This pull request fixes 1 alert when merging 6baf1c2 into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-16T12:47:30Z

This pull request fixes 1 alert when merging e10b080 into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-16T14:12:57Z

This pull request fixes 1 alert when merging 088edc9 into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-16T15:31:36Z

This pull request fixes 1 alert when merging dd01c3a into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

- Each target listed in Director's target file should have corresponding match in ImageRepo's one - Test for the use-case if IP Secondary Director's target file and ImageRepo's one have different number of targets Signed-off-by: Mike Sul <[email protected]>

Signed-off-by: Mike Sul <[email protected]>

lgtm-com · 2020-01-16T17:49:52Z

This pull request fixes 1 alert when merging 3373d03 into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-16T23:49:44Z

This pull request fixes 1 alert when merging d660f55 into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

lgtm-com · 2020-01-17T00:36:44Z

This pull request fixes 1 alert when merging 7c2fa9e into 635962f - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek

Just some small stuff. This looks quite good, though.

I did a diff with the last version and only noticed one thing missing that seems like it should be here: in src/aktualizr_secondary/aktualizr_secondary_config_test.cc, you previously had a change with #ifdef BUILD_OSTREE to control whether the default pacman was kOstree or kNone.

CHANGELOG.md

pattivacek · 2020-01-17T08:40:11Z

src/aktualizr_secondary/aktualizr_secondary.cc

+    storage_->loadInstalledVersions(ecu_serial_.ToString(), nullptr, &pending_target);
+    data::InstallationResult install_res =
+        data::InstallationResult(data::ResultCode::Numeric::kUnknown, "Unknown installation error");
+    LOG_INFO << "There is a pending update, try to apply it, update hash: " << pending_target_.sha256Hash();


Might be worth making this just a bit clearer. "Try to apply it" sounds like the user should take action. Maybe rephrase to "Pending update found; attempting to apply it. Target hash: ".

pattivacek · 2020-01-17T08:42:31Z

src/aktualizr_secondary/aktualizr_secondary.h

+  using Ptr = std::shared_ptr<AktualizrSecondary>;
+
+ public:
+  // TODO: free AktualizrSecondary from dependencies as much as possible, e.g. bootloader


It has now been freed of the bootloader, at least from the perspective of the constructor here. :)

src/aktualizr_secondary/update_agent_file.cc

pattivacek · 2020-01-17T08:50:34Z

src/libaktualizr/primary/sotauptaneclient.cc

+      storage->saveInstalledVersion(f.first.serial.ToString(), f.first.update,
+                                    fut_result == data::ResultCode::Numeric::kOk
+                                        ? InstalledVersionUpdateMode::kCurrent
+                                        : InstalledVersionUpdateMode::kPending);


Not a big deal or blocker here, but I'd probably prefer to have the three-line logic for calculating the result on its own line outside of the function call.

pattivacek · 2020-01-17T08:52:03Z

src/libaktualizr/primary/sotauptaneclient.h

@@ -171,6 +173,7 @@ class SotaUptaneClient {
  // ecu_serial => secondary*
  std::map<Uptane::EcuSerial, std::shared_ptr<Uptane::SecondaryInterface>> secondaries;
  std::mutex download_mutex;
+  mutable Uptane::EcuSerial primary_ecu_serial_;


Why does this need to be mutable?

does not have to be such anymore, leftover of one the previous versions.

Can you still remove the mutable modifier, or did it turn out it was necessary?

sure, just missed it in the first time.

pattivacek · 2020-01-17T08:53:11Z

src/libaktualizr/uptane/isotpsecondary.h


 private:
-  IsoTpSendRecv conn;
+  mutable IsoTpSendRecv conn;


Why is this mutable too?

because most of methods in the secondary interface are 'const' now and isotpsecondary uses IsoTpSendRecv non-const methods from within these const methods implementation.

pattivacek · 2020-01-17T09:08:44Z

src/aktualizr_secondary/uptane_verification_test.cc

-
-extern "C" const char* ostree_deployment_get_csum(OstreeDeployment* ostree_deployment) {
-  return OstreeSecondaryUptaneVerificationTest::curOstreeRootfsRev(ostree_deployment);
-}


What happened to this test suite? In the old version there were a bunch of non-OSTree-specific Uptane tests. Those still seem useful to me.

Update: oh wait, I think you just renamed this, right?

yes, the total number of tests has actually increased, I just split the initial test file into two, one with all non-ostree tests and another with ostree specific.

pattivacek · 2020-01-17T09:51:44Z

src/aktualizr_secondary/aktualizr_secondary.cc

+bool AktualizrSecondary::sendFirmware(const std::string& firmware) {
+  // TODO: how to handle the case when secondary is rebooted after metadata are received
+  if (!pending_target_.IsValid()) {
+    LOG_ERROR << "No any pending target to receive update data/image for";


This message is also a bit confusing. Maybe change to "Aborting image download; no valid target found."

lgtm-com · 2020-01-17T10:07:15Z

This pull request fixes 1 alert when merging 9223c90 into 6112216 - view on LGTM.com

fixed alerts:

1 for FIXME comment

… for IP Secondary Signed-off-by: Mike Sul <[email protected]>

- Decouple the secondary interface implementation (aktualizr_secondary) from the TCP server - Remove redundant wrappers over the aktualizr secondary class/interface Signed-off-by: Mike Sul <[email protected]>

- improve the TCP server implementation - add tests for the TCP server and the RPC (serialization/deserialization) Signed-off-by: Mike Sul <[email protected]>

Signed-off-by: Mike Sul <[email protected]>

mike-sul · 2020-01-17T10:36:22Z

@patrickvacek updated according to the comments

Signed-off-by: Mike Sul <[email protected]>

…on both Primary and Secondary Signed-off-by: Mike Sul <[email protected]>

Signed-off-by: Mike Sul <[email protected]>

…xygen happy Signed-off-by: Mike Sul <[email protected]>

Also include the serial to make it somewhat more useful. Signed-off-by: Patrick Vacek <[email protected]>

lgtm-com · 2020-01-17T11:56:53Z

This pull request fixes 1 alert when merging ed9a0e3 into 462fc83 - view on LGTM.com

fixed alerts:

1 for FIXME comment

pattivacek

Looks great, thanks for fixing all of these things!

eu-siemann

I didn't have time to review thoroughly so far but it looks much better than the previous PR and should it be easy now to revisit some parts later. Great job!

mike-sul added the not-ready label Jan 15, 2020

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from 27db9c8 to 770e25d Compare January 15, 2020 11:30

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from e10b080 to 088edc9 Compare January 16, 2020 13:36

mike-sul removed the not-ready label Jan 16, 2020

mike-sul requested review from pattivacek, lbonn and eu-siemann January 16, 2020 14:55

lbonn changed the title ~~Feat/ota 4174/ostree update on IP secondary~~ ostree update on IP secondary (rebased) Jan 16, 2020

lbonn changed the title ~~ostree update on IP secondary (rebased)~~ ostree update on IP secondary (reworked) Jan 16, 2020

Mike Sul added 2 commits January 16, 2020 18:59

OTA-4174: Remove outdated and useless tests for IP Secondary

599100f

Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from dd01c3a to 3373d03 Compare January 16, 2020 17:11

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch 2 times, most recently from 11681c2 to d660f55 Compare January 16, 2020 23:12

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from d660f55 to 7c2fa9e Compare January 16, 2020 23:59

pattivacek reviewed Jan 17, 2020

View reviewed changes

pattivacek mentioned this pull request Jan 17, 2020

Reduce repetitive logging to debug level. #1516

Merged

pattivacek reviewed Jan 17, 2020

View reviewed changes

pattivacek mentioned this pull request Jan 17, 2020

Changelog: update with recent PRs. #1517

Merged

Mike Sul added 4 commits January 17, 2020 12:18

OTA-4174: Separate the ostree specific tests from the non-ostree one…

1a89b35

… for IP Secondary Signed-off-by: Mike Sul <[email protected]>

OTA-4174: IP Secondary implementation refactoring

1c0b0fd

- Decouple the secondary interface implementation (aktualizr_secondary) from the TCP server - Remove redundant wrappers over the aktualizr secondary class/interface Signed-off-by: Mike Sul <[email protected]>

OTA-4174: IP Secondary TCP server refactoring

d1f3dcd

- improve the TCP server implementation - add tests for the TCP server and the RPC (serialization/deserialization) Signed-off-by: Mike Sul <[email protected]>

OTA-4174: Clean and improve the secondary interface

9c80b9a

Signed-off-by: Mike Sul <[email protected]>

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from 9223c90 to 19527bc Compare January 17, 2020 10:34

Mike Sul and others added 5 commits January 17, 2020 13:05

OTA-4174: Initial implementation of an ostree update on IP Secondary

fead283

Signed-off-by: Mike Sul <[email protected]>

OTA-4174: Use the same manifest generation and signing functionality …

2fc2dfb

…on both Primary and Secondary Signed-off-by: Mike Sul <[email protected]>

OTA-4174: Decrease dependencies in headers and remove sockaddr_io

7e2a08c

Signed-off-by: Mike Sul <[email protected]>

OTA-4174: Move the campaign's functions to its class scope to make do…

36fbcf2

…xygen happy Signed-off-by: Mike Sul <[email protected]>

Reduce repetitive logging to debug level.

ed9a0e3

Also include the serial to make it somewhat more useful. Signed-off-by: Patrick Vacek <[email protected]>

mike-sul force-pushed the feat/OTA-4174/ostree-update-on-IP-secondary branch from 19527bc to ed9a0e3 Compare January 17, 2020 11:08

pattivacek approved these changes Jan 17, 2020

View reviewed changes

This was referenced Jan 17, 2020

feat/ota-4174/direct ostree update of IP Secondary #1500

Closed

Merge pull request #1525 from advancedtelematic/fix/secondary-root-version-bump #1525

Merged

eu-siemann approved these changes Jan 17, 2020

View reviewed changes

pattivacek merged commit da37d45 into master Jan 17, 2020

pattivacek deleted the feat/OTA-4174/ostree-update-on-IP-secondary branch January 17, 2020 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ostree update on IP secondary (reworked) #1518

ostree update on IP secondary (reworked) #1518

mike-sul commented Jan 15, 2020

codecov-io commented Jan 15, 2020 •

edited

Loading

lgtm-com bot commented Jan 15, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 17, 2020

pattivacek left a comment

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020 •

edited

Loading

mike-sul Jan 17, 2020

pattivacek Jan 17, 2020

mike-sul Jan 17, 2020

lgtm-com bot commented Jan 17, 2020

mike-sul commented Jan 17, 2020

lgtm-com bot commented Jan 17, 2020

pattivacek left a comment

eu-siemann left a comment

ostree update on IP secondary (reworked) #1518

ostree update on IP secondary (reworked) #1518

Conversation

mike-sul commented Jan 15, 2020

codecov-io commented Jan 15, 2020 • edited Loading

Codecov Report

lgtm-com bot commented Jan 15, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 16, 2020

lgtm-com bot commented Jan 17, 2020

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

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 Jan 17, 2020 • 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

lgtm-com bot commented Jan 17, 2020

mike-sul commented Jan 17, 2020

lgtm-com bot commented Jan 17, 2020

pattivacek left a comment

Choose a reason for hiding this comment

eu-siemann left a comment

Choose a reason for hiding this comment

codecov-io commented Jan 15, 2020 •

edited

Loading

pattivacek Jan 17, 2020 •

edited

Loading