Wait for secondaries before installing

[lbonn]

This is quite big and will need more tests.

[mike-sul]

The following comment is not exactly related to this PR and is not blocker for its merging, just simply general comment to the overall data model of aktualizr.

I think, it makes sense to have a notion of ECU in the data model (not ecu_serial) and corresponding table in the DB (ECU table), so manifest, public_key, etc are attributes of ECU not ecu_serials entity.

[lbonn]

Yes, I've also wondered if it should just be another table.

As we've added fields to ecu_serials it has effectively become what you describe and would probably more aptly named ecus.

[mike-sul]

This dependency of the IP Secondary factory on Aktualizr is questionable. Actually, it's me who implicitly introduced this dependency by putting createIPSecondaries() implementation in this file :) , it should be under libaktualizr-posix folder, IMHO. Anyway, this is not exactly comment to this PR, just thing that we might need to consider at some point.

[mike-sul]

Can the secondaries that have been added to the aktualizr instance be 'not registered', if not then perhaps renaming of GetRegisteredSecondaries() to just GetSecondaries() would make sense. Or, as an option, storage_ can be passed to createIPSecondaries instead of aktualizr so we can call storage_->loadSecondaryInfo(&info); here and replace dependency on Aktualizr with dependency on Storage which looks better since it reduces cross-dependencies, IMHO. Or even more better, just pass exactly what is required to do the job here, which is std::vector<SecondaryInfo>.

[lbonn]

GetSecondaries() would probably work.

My first version was also passing the storage around, the problem I had is that call originates from src/aktualizr_primary/main.cc directly where the underlying storage object is not accessible.

Passing std::vector<SecondaryInfo> might be fine or maybe too specific? Also, because of the previous point, the aktualizr method would still be needed in the current state.

[mike-sul]

perhaps, IP and port could be stored in the DB too, so an order that secondaries are listed in the conf won't matter?

[lbonn]

In the same vein as the previous comment, it is made harder by the fact we've abstracted the ip secondary concept from SotaUptaneClient and the storage.

One version I've explored was to add another free form string column to the ecu_serials table and SecondaryInfo that might have to be re-parsed.

[pattivacek]

We'll want to avoid tables with secondary type-specific columns, so the free-form string might be the best option. Tedious but might be the most future-proof.

[mike-sul]

I agree that adding secondary specific columns into a generic table (ecu_serials here) is a bad idea. I see a few options here:

1. extend the existing ecu_serials table with an opaque column (e.g. sec_data, sec_info, etc) intended for serialized secondary specific data required for its instantiation;

2. don't alter the existing ecu_serials table. Introduce another table ecu with the generic fields + the column containing serialized secondary specific data;

3. Introduce another table ip_secondary that contains the ip secondary specific info. This table is managed solely by the IP secondary factory object/method/function. In this case we can avoid dependency on Aktualizr object at all, what is needed just a path to the DB if we would like to store this table in the same DB file.

4. Introduce two new tables ECU containing generic columns + the ip_secondary specific table.

[mike-sul]

sec_it->second->getManifest() sets up a TCP connection with Secondary, a timeout of the setting up TCP connection really depends on multiple factors and configuration of TCP stack at a kernel level. The same is true for write/read to/from socket, so it won't be 10 minutes for sure and actually will significantly differs in a case of a local host and a non-local one (http://willbryant.net/overriding_the_default_linux_kernel_20_second_tcp_socket_connect_timeout)

[lbonn]

Yes that's true, I will rework something.

[mike-sul]

BTW, why not just do certain amount of retries within getManifest() (I mean Asn1Rpc call) or keep retrying until certain time if we need to wait at least some period of time.

[codecov-io]

Codecov Report

Merging #1533 into master will decrease coverage by 0.08%.
The diff coverage is 84.4%.

@@            Coverage Diff            @@
##           master   #1533      +/-   ##
=========================================
- Coverage   82.19%   82.1%   -0.09%     
=========================================
  Files         186     187       +1     
  Lines       11494   11744     +250     
=========================================
+ Hits         9447    9642     +195     
- Misses       2047    2102      +55

Impacted Files	Coverage Δ
src/libaktualizr/primary/aktualizr.h	100% <ø> (ø)	⬆️
src/libaktualizr/primary/initializer.h	100% <ø> (ø)	⬆️
src/libaktualizr/storage/sqlstorage.h	50% <ø> (ø)	⬆️
src/libaktualizr/uptane/manifest.h	100% <ø> (ø)	⬆️
...c/virtual_secondary/partialverificationsecondary.h	63.63% <0%> (-14.15%)	⬇️
src/aktualizr_secondary/aktualizr_secondary.h	60% <0%> (-40%)	⬇️
src/libaktualizr/uptane/isotpsecondary.h	0% <0%> (ø)
src/aktualizr_primary/main.cc	83.03% <100%> (-0.16%)	⬇️
src/libaktualizr-posix/ipuptanesecondary.h	100% <100%> (ø)	⬆️
src/libaktualizr/storage/invstorage.h	97.95% <100%> (+0.39%)	⬆️
... and 20 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 ce42c5d...ba40f33. Read the comment docs.

[lbonn]

Looks like I've overlooked something, e2e62c6 breaks Uptane.UptaneSecondaryMisconfigured

[pattivacek]

Looks like I've overlooked something, e2e62c6 breaks Uptane.UptaneSecondaryMisconfigured

Not necessarily unexpected, right? That test might need some reworking to still be meaningful.

[mike-sul]

This 'manifest caching' functionality looks like IP Secondary specific functionality, so I would 'hide' it behind the secondary interface and implemented it in IpUptaneSecondary::getManifest().

[lbonn]

I would like that also, but there should then be a way to access the storage from there. To evaluate

[mike-sul]

Maybe dedicated table in sql.db or even DB that is initialized in IP secondary factory (preferable) or ctor.

[mike-sul]

I am not sure that this functionality belongs here, IMHO, this is secondary specific functionality and should not be in a generic code. Why we wait just here before the sending metatada but not wait before sending actual images, secondary can be available at the first step but becomes not available before the second (installation) step ?
This rhetorical question is another argument for having this wait or retry policy withing the IP secondary implementation, specifically, within IpUptaneSecondary implementation.

[pattivacek]

I think it's fine. We're likely to need a generic way to wait for Secondaries regardless of protocol. Obviously for something like virtual Secondaries it can just return true. And if we can improve this later, that's fine too.

[mike-sul]

I think, this Type() as well as the logic of passing around of the registered secondaries can be eliminated if we persist required information about IP secondaries within the IP secondary builder/factory, e.g. in createIPSecondaries under if (provision) { branch. Therefore, we won't need aktualizr instance here and in its else we won't need to secondaries_info.erase(, we will just load the info from the DB.

Actually, I think, even provision parameter is not required here. In createIPSecondaries, we just check if the info about the secondary is persisted in the DB if so, just `Uptane::IpUptaneSecondary::connectAndCheck() if not connectAndCreate() + persist the info into the DB. What we need here, is just an instance of the object that can operate over the DB, BTW, I think, it's not necessary has to be the same instance that the sotauptaneclient owns as we can have IP secondary specific table.

[lbonn]

Passing the provision around was indeed needlessly complicated, the last version is a bit more simple.

I'll continue the investigations about the Type() method and dedicated storage.

[mike-sul]

Regardless of the solution/PR design, I think, it makes sense to add test(s) into ipsecondary_test.py testing the given use-case.

[lbonn]

Here is another version with two tables ecus and secondary_ecus, it uses some more complicated sql.

[lbonn]

The version currently pushed only caches the manifest in memory and it is indeed simpler. But I am gonna revert to put it in the storage somehow because it doesn't fit the bill: if the secondary never gets up between the primary start and the attempted install, the backend marks the install as failed before it can start as the manifest is incomplete. And we clearly want to support this case.

[mike-sul]

IMHO, these two tables should not know anything about each other. libaktualizr should not know details of any secondary implementation. In ideal world the IP secondary should have its own dedicated persistent layer.

[lbonn]

Well the real world is all about compromises.

[pattivacek]

LGTM. Can you still document somewhere (maybe in the ticket?) what might still require some followup?

[pattivacek]

This is all happy-path, with everything more or less starting simultaneously, right? Can we extended this for the cases when one or more Secondaries don't come up immediately?