OTA-3795: Do default verification if a secondary update and an… #1395

mike-sul · 2019-09-27T12:16:14Z

Signed-off-by: Mykhaylo Sul myk.sul@gmail.com

…ase of a secondary update on an ostree-based primary Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

codecov-io · 2019-09-27T13:08:37Z

Codecov Report

Merging #1395 into master will decrease coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1395      +/-   ##
==========================================
- Coverage   80.25%   80.19%   -0.07%     
==========================================
  Files         178      178              
  Lines       10582    10585       +3     
==========================================
- Hits         8493     8489       -4     
- Misses       2089     2096       +7

Impacted Files	Coverage Δ
src/libaktualizr/package_manager/ostreemanager.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/package_manager/ostreemanager.cc	`77.04% <100%> (+0.66%)`	⬆️
src/libaktualizr/storage/sqlstorage.cc	`76.55% <0%> (-0.92%)`	⬇️

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 5e3c1dd...1f3906b. Read the comment docs.

doanac · 2019-09-27T14:51:23Z

src/libaktualizr/package_manager/ostreemanager.cc

    return PackageManagerInterface::fetchTarget(target, fetcher, keys, progress_cb, token);
  }
  return OstreeManager::pull(config.sysroot, config.ostree_server, keys, target, token, progress_cb).success;
 }

 TargetStatus OstreeManager::verifyTarget(const Uptane::Target &target) const {
+  if (!target.IsOstree()) {


I think we may need a similar check in dockerappmanager.cc? In fact I think the current logic isn't quite right there. Its doing verification for all target types and really should just run when the type is OSTree. I think something like this is probably correct:

TargetStatus DockerAppManager::verifyTarget(const Uptane::Target &target) const { TargetStatus status = OstreeManager::verifyTarget(target); if (status != TargetStatus::kGood || !target.IsOstree()) { return status; } auto cb = [this](const std::string &app, const Uptane::Target &app_target) { LOG_INFO << "Verifying " << app << " -> " << app_target; std::stringstream ss; ss << *storage_->openTargetFile(app_target); DockerApp dapp(app, config); return dapp.render(ss.str(), false); }; if (!iterate_apps(target, cb)) { return TargetStatus::kInvalid; } return TargetStatus::kGood; }

If secondaries/multi-ECU update is not required in case of "ostree+docker-app" package manager usage on Primary then the current logic is fine provided that it works now for updating both ostree and docker app on Primary.

My understanding is that the "ostree+docker-app" package manager is intended for supporting of both an ostree-based rootfs update and docker apps configuration update on Primary ? If it's so I am not sure if I understand why auto res = OstreeManager::install(target); is called in DockerAppManager::install(), I suppose, the logic should call the ostree install routine only if the target type is ostree?

What value of target.type_ will be in case of the docker-app update ?

We are planning to redesign approach to package management as new requirements/use cases appeared:

secondaries support

multiple type of update on a single ECU/Primary

providing an integrator with possibility to add a new type of 'package manager' without libaktualizr modification.

@mike-sul - The docker-app manager is confusing right now because I haven't documented it well at all. I think what I should do is put a PR that includes some code comments about the package manager so people can reason about it. Then I can fix it and add some tests to make sure its following the logic you've done here properly.

FYI - here's the blog https://foundries.io/insights/2019/09/23/docker-app/

FYI - here's the blog https://foundries.io/insights/2019/09/23/docker-app/

Thanks for pointing out to it, gonna read it through.

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com> OTA-3795: Force to remove a temporal directory tree containing files OTA-3795: Run aktualizr-info once more if it returns no details about installed nor pending images

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

pattivacek · 2019-10-07T10:28:21Z

tests/test_update.py

+
+    chdir(initial_cwd)
+    exit(0 if test_suite_run_result else 1)
+


This is powerful and cool, but I wonder if it would've been possible to test this at a lower level. This is also becoming quickly redundant with shared_cred_prov_test.py and we should consider merging them at some point.

Also, minor nitpick: the name of this test/file is quite general. It's worth getting a bit more specific next time we mess with this.

Yes, I agree, the name is too generic.

In terms of 'This is also becoming quickly redundant with shared_cred_prov_test.py', I think they are very different, ' shared_cred_prov_test.py' tests just provisioning of non-ostree against the real backend, while 'test_update.py' tests a secondary update against the mocked backend if primary is based on ostree. This is the only test at the moment that can catch the issue described here https://saeljira.it.here.com/browse/OTA-3795, none of our tests could catch the issue before.

In retrospect, I think it makes sense to move this test to the IP secondary test suite (ipsecondary_test.py) as it actually tests the secondary update or replace it with lower level tests (C/C++ based) utilizing a virtual secondary. The later, will require duplication of the boilerplate code around ostree mocking located in aktualizr_fullostree_test.cc or the test logic itself can be added to aktualizr_fullostree_test.cc, what I tried but failed.

In terms of 'This is also becoming quickly redundant with shared_cred_prov_test.py', I think they are very different

Fair enough. Thanks for the explanation!

I think it makes sense to move this test to the IP secondary test suite (ipsecondary_test.py) as it actually tests the secondary update

Not a bad idea.

or replace it with lower level tests (C/C++ based) utilizing a virtual secondary

Yeah, we could do that, but I'd say that isn't a priority, since you already have a nice working version in Python!

mike-sul added the not-ready label Sep 27, 2019

OTA-3795: Apply the default package manager verification routine in c…

16968a5

…ase of a secondary update on an ostree-based primary Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul force-pushed the fix/OTA-3795/secondary-update-on-ostree-primary branch from a54b716 to 16968a5 Compare September 27, 2019 13:08

doanac reviewed Sep 27, 2019

View reviewed changes

OTA-3795: Test secondary update if primary's package manager is ostree

358d51c

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul force-pushed the fix/OTA-3795/secondary-update-on-ostree-primary branch from 10c2b1e to 9f1f54b Compare September 30, 2019 09:51

OTA-3795: Choose a free port for the mock servers dynamically

c6d9e06

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com> OTA-3795: Force to remove a temporal directory tree containing files OTA-3795: Run aktualizr-info once more if it returns no details about installed nor pending images

mike-sul force-pushed the fix/OTA-3795/secondary-update-on-ostree-primary branch from ff883c6 to c6d9e06 Compare September 30, 2019 11:22

OTA-3795: Choose a free port for IP Secondary

1f3906b

Signed-off-by: Mykhaylo Sul <myk.sul@gmail.com>

mike-sul removed the not-ready label Sep 30, 2019

mike-sul mentioned this pull request Sep 30, 2019

Signal handling #1384

Merged

mike-sul requested review from pattivacek and lbonn September 30, 2019 13:41

lbonn approved these changes Sep 30, 2019

View reviewed changes

lbonn changed the title ~~OTA-3795: Do default verification if a secondary update and an ostree-based primary~~ OTA-3795: Do default verification if a secondary update and an… Sep 30, 2019

lbonn merged commit 42f2cc7 into master Sep 30, 2019

lbonn deleted the fix/OTA-3795/secondary-update-on-ostree-primary branch September 30, 2019 13:50

pattivacek reviewed Oct 7, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTA-3795: Do default verification if a secondary update and an… #1395

OTA-3795: Do default verification if a secondary update and an… #1395

mike-sul commented Sep 27, 2019

codecov-io commented Sep 27, 2019 •

edited

Loading

doanac Sep 27, 2019

mike-sul Sep 30, 2019 •

edited

Loading

mike-sul Sep 30, 2019

mike-sul Sep 30, 2019

doanac Sep 30, 2019

doanac Sep 30, 2019

mike-sul Sep 30, 2019

pattivacek Oct 7, 2019

mike-sul Oct 8, 2019

pattivacek Oct 8, 2019

OTA-3795: Do default verification if a secondary update and an… #1395

OTA-3795: Do default verification if a secondary update and an… #1395

Conversation

mike-sul commented Sep 27, 2019

codecov-io commented Sep 27, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mike-sul Sep 30, 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

codecov-io commented Sep 27, 2019 •

edited

Loading

mike-sul Sep 30, 2019 •

edited

Loading