Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

Fix/ota 831/retry download #1001

Merged
merged 4 commits into from
Nov 19, 2018
Merged

Fix/ota 831/retry download #1001

merged 4 commits into from
Nov 19, 2018

Conversation

patriotyk
Copy link
Contributor

No description provided.

@patriotyk patriotyk force-pushed the fix/OTA-831/retry-download branch 3 times, most recently from 1300e67 to 97ccbbb Compare November 12, 2018 13:50
@codecov-io
Copy link

codecov-io commented Nov 12, 2018

Codecov Report

Merging #1001 into master will increase coverage by 0.09%.
The diff coverage is 98%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1001      +/-   ##
==========================================
+ Coverage   81.95%   82.04%   +0.09%     
==========================================
  Files         188      188              
  Lines       13202    13219      +17     
==========================================
+ Hits        10820    10846      +26     
+ Misses       2382     2373       -9
Impacted Files Coverage Δ
src/libaktualizr/utilities/utils.h 90.47% <ø> (ø) ⬆️
src/libaktualizr/primary/sotauptaneclient.h 83.33% <ø> (ø) ⬆️
src/libaktualizr/primary/sotauptaneclient.cc 81.25% <100%> (+0.28%) ⬆️
src/libaktualizr/primary/reportqueue.cc 96% <100%> (ø) ⬆️
src/libaktualizr/uptane/uptane_network_test.cc 100% <100%> (ø) ⬆️
src/libaktualizr/http/httpclient.cc 91.15% <100%> (+3.49%) ⬆️
src/libaktualizr/uptane/fetcher.cc 93.87% <100%> (+1.95%) ⬆️
src/libaktualizr/uptane/uptane_test.cc 98.63% <100%> (-0.03%) ⬇️
src/libaktualizr/utilities/utils.cc 86% <80%> (-0.09%) ⬇️
src/libaktualizr/storage/invstorage.cc 90.3% <0%> (-0.05%) ⬇️
... and 2 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 a4c1f05...17e75b6. Read the comment docs.

@patriotyk patriotyk force-pushed the fix/OTA-831/retry-download branch from 3ea3457 to b550165 Compare November 12, 2018 17:45
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks about right. The Jenkins failure is from the hsm_prov test; it's probably transient.

conf.storage.path = temp_dir.Path();
conf.provision.expiry_days = "noerrors";
conf.provision.primary_ecu_serial = "noerrors";
conf.provision.primary_ecu_hardware_id = "download_failure";
Copy link
Collaborator

@pattivacek pattivacek Nov 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a significance to "download_failure" here? I think I created it with the intention that it would be used by the fake server to trigger the download failures.

@@ -8,7 +8,24 @@

class Handler(BaseHTTPRequestHandler):
def do_GET(self):
pass
if self.path.startswith("/repo/targets/noerrors"):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this really be for "noerrors"? Won't this get triggered for all the network tests?

@pattivacek
Copy link
Collaborator

I merged my large PR, so this needs to be rebased. I fixed one of the flaky tests, though, so at least that's one less thing to worry about.

@patriotyk patriotyk force-pushed the fix/OTA-831/retry-download branch from b550165 to ea698fa Compare November 15, 2018 09:20
@patriotyk
Copy link
Contributor Author

I can't look in to the Jenkins

@pattivacek
Copy link
Collaborator

I can't look in to the Jenkins

Sorry. It's hsm_prov again. I don't understand why it is failing, but here's the log:

114/116 Test #102: hsm_prov_test .......................................***Failed   64.63 sec
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_primary/aktualizr --config /tmp/tmpgh_d1_b3/conf.d --loglevel 0 --running-mode once
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
Device has not yet provisioned (as expected). Running aktualizr-cert-provider.
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/cert_provider/aktualizr-cert-provider -c **** -l / -r -s -u -g /tmp/tmpgh_d1_b3/conf.d/20-sota_hsm_prov.toml
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/scripts/export_to_hsm.sh
> Running pkcs11-tool --module=/usr/lib/softhsm/libsofthsm2.so -O --login --pin 1234
> Running softhsm2-util --show-slots
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_primary/aktualizr --config /tmp/tmpgh_d1_b3/conf.d --loglevel 0 --running-mode once
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
> Running /home/laurent/jenkins-slave/jenkins-home/workspace/aktualizr_PR-1001-7RE67M5RGS372BWYW464TROUSU44ZFUJ23LZBJSSS6WN527WU2YA@3/build-coverage/src/aktualizr_info/aktualizr-info --config /tmp/tmpgh_d1_b3/conf.d
Provisioning failed: Reading config: "/tmp/tmpgh_d1_b3/conf.d/20-sota_hsm_prov.toml"
Use existing SQL storage: "/tmp/tmpgh_d1_b3/sql.db"
Storage backend: Sqlite
Device ID: ornate-kuerbiskernbrot-619
Primary ecu serial ID: 1dd24927a4a01d29edea6eb731bb22aef856b5df036df7211cd027738ae4e018
Primary ecu hardware ID: 074b1a25f796
Provisioned on server: no
Fetched metadata: no

I restarted it in case it is just transient, but since it's happened two or three times now, it probably isn't.

@pattivacek pattivacek force-pushed the fix/OTA-831/retry-download branch 3 times, most recently from b80af8f to 19f2d6b Compare November 16, 2018 10:41
@pattivacek
Copy link
Collaborator

pattivacek commented Nov 16, 2018

I've narrowed the problem down to the changes to httpclient.cc in last commit about fixing valgrind complaints. Before that change, hsm provisioning works. @patriotyk Why was that change necessary?

It may also need a fix for reboot detection. (FYI @lbonn) At least when I test locally, I have to add something like this to my config:

[bootloader]
reboot_sentinel_dir = "/home/patrick/Code/aktualizr/tests/hsm_test/"

Otherwise it complains with boost::filesystem::status: Permission denied: "/var/run/aktualizr-session/need_reboot".

@lbonn
Copy link
Contributor

lbonn commented Nov 16, 2018

It looks like it is the same curl bug that @OYTIS initially bumped into (see TODO in httpclient.cc).

It's actually not an unknown bug anymore, I tracked it down to curl/curl#2829.

For current distributions, the workaround is still required but can probably be shortened to:

if (pkcs11_key) {
    curlEasySetoptWrapper(curl_put, CURLOPT_SSLENGINE, "pkcs11");
}

This line should be enough in all cases and I suggest we'd put that in an internal wrapper for cloning curl handles to refactor it.

@pattivacek
Copy link
Collaborator

It looks like it is the same curl bug that @OYTIS initially bumped into (see TODO in httpclient.cc).

Agreed. I'll update this and try to take care of it since i'm already in the middle of it.

pattivacek and others added 3 commits November 16, 2018 17:25
Signed-off-by: Patrick Vacek <[email protected]>
Signed-off-by: Serhiy Stetskovych <[email protected]>
Signed-off-by: Serhiy Stetskovych <[email protected]>
Signed-off-by: Patrick Vacek <[email protected]>
Signed-off-by: Serhiy Stetskovych <[email protected]>
Signed-off-by: Patrick Vacek <[email protected]>
@pattivacek pattivacek force-pushed the fix/OTA-831/retry-download branch from 19f2d6b to 1c1236f Compare November 16, 2018 16:25
CURL* curl_post = Utils::curlDupHandleWrapper(curl, pkcs11_key);
curlEasySetoptWrapper(curl_post, CURLOPT_URL, url.c_str());
curlEasySetoptWrapper(curl_post, CURLOPT_POST, 1);
if (pkcs11_key) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be unneeded here (in Utils::curlDupHandleWrapper)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that was a mistake. Fixed.

@pattivacek pattivacek force-pushed the fix/OTA-831/retry-download branch from 1c1236f to 7d01357 Compare November 16, 2018 16:48
Now wrapped in a convenience function to make life easier.

Signed-off-by: Patrick Vacek <[email protected]>
@pattivacek pattivacek force-pushed the fix/OTA-831/retry-download branch from 7d01357 to 17e75b6 Compare November 16, 2018 16:50
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is done now. Any further thoughts from anyone before we merge it?

@patriotyk
Copy link
Contributor Author

Looks good for me

@pattivacek pattivacek merged commit 67dca3f into master Nov 19, 2018
@pattivacek pattivacek deleted the fix/OTA-831/retry-download branch November 19, 2018 10:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants