Feat/ota 2282/more failure simulation #1145

lbonn · 2019-03-18T17:20:38Z

Based on #1144.

In a nutshell:

keep a stable ecu order (what's given in StoreEcuSerials)
device installation result code contains a concatenation of ecu_type:error in case of error
added functionality on top of libfiu, uses temporary files as storage for strings

Will add a test for the changes in SotaUptaneClient::computeDeviceInstallationResult when ready

OYTIS · 2019-03-18T18:02:00Z

This pull request introduces 2 alerts when merging ad132cc into 935e0ce - view on LGTM.com

new alerts:

2 for Illegal raise

Comment posted by LGTM.com

codecov-io · 2019-03-19T09:11:42Z

Codecov Report

Merging #1145 into master will increase coverage by 0.09%.
The diff coverage is 94.02%.

@@            Coverage Diff             @@
##           master    #1145      +/-   ##
==========================================
+ Coverage   77.74%   77.83%   +0.09%     
==========================================
  Files         167      167              
  Lines        9947     9994      +47     
==========================================
+ Hits         7733     7779      +46     
- Misses       2214     2215       +1

Impacted Files	Coverage Δ
src/libaktualizr/primary/aktualizr.h	`100% <ø> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.h	`100% <ø> (ø)`	⬆️
...libaktualizr/package_manager/packagemanagerfake.cc	`93.47% <100%> (+0.14%)`	⬆️
src/libaktualizr/storage/sqlstorage.cc	`74.02% <100%> (ø)`	⬆️
src/libaktualizr/primary/sotauptaneclient.cc	`87.04% <80%> (-0.12%)`	⬇️
src/libaktualizr/utilities/types.cc	`68.75% <92.3%> (+2.67%)`	⬆️
src/libaktualizr/utilities/fault_injection.h	`97.14% <97.05%> (-2.86%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.cc	`78.12% <0%> (+2.34%)`	⬆️

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 d91282a...cccfa33. Read the comment docs.

lbonn · 2019-03-21T10:44:45Z

Ok I've cleaned a bit the CMake invocations for running test_fiuinfo.

Last things on my list:

cleaning up the unit test (last commit)
actions.md
changelog

But the bulk of the work is ready for review.

pattivacek

Looks good so far!

pattivacek · 2019-03-21T14:42:27Z

src/libaktualizr/storage/sqlstorage.cc

+    for (auto it = serials.cbegin() + 1; it != serials.cend(); it++) {
+      auto statement = db.prepareStatement<int64_t, std::string, std::string>(
+          "INSERT INTO ecu_serials(id,serial,hardware_id) VALUES (?,?,?);", it - serials.cbegin(), it->first.ToString(),
+          it->second.ToString());


So the ID is just purely for sequencing, and we just trust that what we receive from the server is the order we should maintain? Not a bad idea to finally make it explicit instead of assuming primary is always first.

Yes, the id is for sequencing.

I don't believe the order comes from the server, but the local config:

aktualizr/src/libaktualizr/primary/initializer.cc

Line 68 in a51704b

storage_->storeEcuSerials(ecu_serials);

Hope I haven't overlooked anything but the intent is that any two devices with the same configuration (i.e: with same sets of ecu, deducted from configuration files named the same) would have their ecus in the same order, so that we can safely match them with the same device error code if several ecus fail.

e.g: ecu1:fail1|ecu2:fail2 and never ecu2:fail2|ecu1:fail1

Not a bad idea to finally make it explicit instead of assuming primary is always first.

The api still expects a list of serials that start with the primary.

pattivacek · 2019-03-21T14:42:41Z

src/libaktualizr/primary/aktualizr_test.cc

+
+  Aktualizr aktualizr(conf, storage, http);
+
+  // TODO: was stolen from other test, refactor


uh-oh, a TODO!

ok, refactored a bit

pattivacek · 2019-03-21T14:47:21Z

tests/fiuinfo_test.cc

+TEST(Fiuinfo, PassInfoWithCtrl) {
+  EXPECT_EQ(fiu_fail("failctrl"), 0);
+  Utils::shell(fiu_script + " ctrl -c 'enable name=failctrl,failinfo=test_ctrl' " + std::to_string(getpid()), nullptr);
+  EXPECT_NE(fiu_fail("failctrl"), 0);


As discussed, I think checking that we can unset the failure and recover is worthwhile.

Should be covered now.

lbonn · 2019-03-21T15:05:13Z

Pushed minor changes in the fault injection part, as it was needlessly depending on having unsigned long strictly bigger than 32 bits. Plus, in-line comments to make the inner workings clearer.

pattivacek

Looking even better!

pattivacek · 2019-03-21T15:20:13Z

tests/fiuinfo_test.cc

+
+TEST(Fiuinfo, PassInfoWithRun) {
+  EXPECT_TRUE(fiu_fail("fail"));
+  EXPECT_EQ(fault_injection_last_info(), "test");


Where does "test" come from here?

Here:

aktualizr/tests/CMakeLists.txt

Line 77 in a51704b

add_aktualizr_test(NAME fiuinfo SOURCES fiuinfo_test.cc NO_VALGRIND LAUNCH_CMD ${PROJECT_SOURCE_DIR}/scripts/fiu run -c "enable name=fail,failinfo=test" -- ARGS ${PROJECT_SOURCE_DIR}/scripts/fiu)

Thanks. It could perhaps be a more greppable string, but not a big deal at all.

good point, changed it to something more unique

The new constraints are: primary first and `loadEcuSerials` returns the same order as given in the `storeEcuSerials` vector parameter. Signed-off-by: Laurent Bonnans <[email protected]>

Signed-off-by: Laurent Bonnans <[email protected]>

Put the string result inside quotes, so that it can contain `:`. Useful when the device code reflects the ECUs Signed-off-by: Laurent Bonnans <[email protected]>

+ some refactor Signed-off-by: Laurent Bonnans <[email protected]>

libfiu only supports sending long unsigned, so work around it by storing strings in a binary file Signed-off-by: Laurent Bonnans <[email protected]>

Path is passed through FIU_INFO_FILE, id has the 31th bit set to 1 Signed-off-by: Laurent Bonnans <[email protected]>

Proxy script for fiu-run and fiu-ctrl (`fiu run`, `fiu ctrl`). Syntax is the same, except that it can send strings through `failinfo=`, via an intermediary file. The file is temporary when passed through fiu-run, otherwise it's named after the targets' pid. Signed-off-by: Laurent Bonnans <[email protected]>

And not description, like before Signed-off-by: Laurent Bonnans <[email protected]>

Signed-off-by: Laurent Bonnans <[email protected]>

lbonn added the not-ready label Mar 18, 2019

lbonn requested review from pattivacek, eu-siemann and mike-sul March 18, 2019 17:20

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch from ad132cc to 8ef5496 Compare March 19, 2019 09:11

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch 5 times, most recently from abcfec6 to 989118c Compare March 20, 2019 17:35

lbonn removed the not-ready label Mar 21, 2019

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch from 989118c to fb7ba7e Compare March 21, 2019 10:21

pattivacek reviewed Mar 21, 2019

View reviewed changes

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch from fb7ba7e to a51704b Compare March 21, 2019 15:03

pattivacek reviewed Mar 21, 2019

View reviewed changes

lbonn added 6 commits March 21, 2019 17:28

Keep ecu order in database constant

d288902

The new constraints are: primary first and `loadEcuSerials` returns the same order as given in the `storeEcuSerials` vector parameter. Signed-off-by: Laurent Bonnans <[email protected]>

Concatenate ecu results in device result code

73e5539

Signed-off-by: Laurent Bonnans <[email protected]>

Change in-db representation of installation result codes

be7396c

Put the string result inside quotes, so that it can contain `:`. Useful when the device code reflects the ECUs Signed-off-by: Laurent Bonnans <[email protected]>

Add ability to set a prefix for an aktualizr test

862abf4

+ some refactor Signed-off-by: Laurent Bonnans <[email protected]>

Pass fault injection info through a pid-named file

cea72a1

libfiu only supports sending long unsigned, so work around it by storing strings in a binary file Signed-off-by: Laurent Bonnans <[email protected]>

Fault injection info can come from a fixed file

f79a1e7

Path is passed through FIU_INFO_FILE, id has the 31th bit set to 1 Signed-off-by: Laurent Bonnans <[email protected]>

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch from a51704b to 8f9ac73 Compare March 21, 2019 16:38

lbonn added 4 commits March 21, 2019 17:40

Set result code as fake package manager failure info

f3bf4b1

And not description, like before Signed-off-by: Laurent Bonnans <[email protected]>

Test for device installation result from ecus

84ad1cd

Signed-off-by: Laurent Bonnans <[email protected]>

Update actions and changelog

cccfa33

Signed-off-by: Laurent Bonnans <[email protected]>

lbonn force-pushed the feat/OTA-2282/more-failure-simulation branch from 8f9ac73 to cccfa33 Compare March 21, 2019 16:40

pattivacek approved these changes Mar 22, 2019

View reviewed changes

lbonn merged commit 572b96a into master Mar 22, 2019

lbonn deleted the feat/OTA-2282/more-failure-simulation branch March 22, 2019 09:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/ota 2282/more failure simulation #1145

Feat/ota 2282/more failure simulation #1145

lbonn commented Mar 18, 2019

OYTIS commented Mar 18, 2019

codecov-io commented Mar 19, 2019 •

edited

Loading

lbonn commented Mar 21, 2019

pattivacek left a comment

pattivacek Mar 21, 2019

lbonn Mar 21, 2019 •

edited

Loading

lbonn Mar 21, 2019

pattivacek Mar 21, 2019

lbonn Mar 21, 2019

pattivacek Mar 21, 2019

lbonn Mar 21, 2019

lbonn commented Mar 21, 2019

pattivacek left a comment

pattivacek Mar 21, 2019

lbonn Mar 21, 2019

pattivacek Mar 21, 2019

lbonn Mar 21, 2019


		Aktualizr aktualizr(conf, storage, http);

		// TODO: was stolen from other test, refactor

Feat/ota 2282/more failure simulation #1145

Feat/ota 2282/more failure simulation #1145

Conversation

lbonn commented Mar 18, 2019

OYTIS commented Mar 18, 2019

codecov-io commented Mar 19, 2019 • edited Loading

Codecov Report

lbonn commented Mar 21, 2019

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbonn Mar 21, 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

lbonn commented Mar 21, 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

Choose a reason for hiding this comment

codecov-io commented Mar 19, 2019 •

edited

Loading

lbonn Mar 21, 2019 •

edited

Loading