Add --wait-until-provisioned option to aktualizr-info #1253

Zee314159 · 2019-07-11T02:10:40Z

Draft code.

pattivacek

Looks good so far!

pattivacek · 2019-07-11T07:32:54Z

src/aktualizr_info/main.cc

    std::shared_ptr<INvStorage> storage = INvStorage::newStorage(config.storage, readonly);
    bool cmd_trigger = false;
+    bool provision_trigger = true;


Might be simpler to initialize this to false and just use a while(!provisioned) loop.

pattivacek · 2019-07-11T07:36:05Z

src/aktualizr_info/main.cc

+      if (provision_trigger) {
+        break;
+      } else {
+        sleep(5);


I think a much smaller sleep time would be better. 1 second is probably plenty. However, that will result in a lot of garbage printed to the screen, so we might want to any output until the loop finishes.

Does anyone else have opinions on what they'd like to see printed while waiting for provisioning to complete? Perhaps most ideal would be to only print the successful parts as they happen.

I am just wondering if there is/are condition(s) under which this 'for' loop can last forever, e.g. device hasn't been registered at the server for some reason.

I might have the same question as Mike said, under what condition(s) would the loop last forever? Now I just assume the conditions are :device ID and ECU serials could be loaded, device has been registered, metadata is available. I actually am not sure about it. Is provisioned means those four conditions are met? @patrickvacek

You can't register without device ID and ECU serials, and you can't fetch metadata without provisioning, so the only conditions that need to be checked are registration and metadata successfully fetched. It is true that this loop could last forever if something goes wrong. We've discussed a possible timeout feature, but for a first go of this feature, I don't think we need that yet.

It probably shouldn't output anything until it's provisioned, so that the output can be parsed without errors / duplicates.

The body of the while loop looks quite big as well. Is it possible to have something like that?

bool provisioned = false; while (wait_provisioning && !provisioned) { // some simple condition, no output } // regular aktualizr-info body

For the timeout parameter, as it's usually called as a subprocess by other tools (in python for example), we can use a timeout there for the moment.

Thanks. Much clear now！

For the timeout parameter, as it's usually called as a subprocess by other tools (in python for example), we can use a timeout there for the moment.

So, how does the other tools（in python for example） outside aktualizr-info main function body break this loop？

For example, in Python:

https://docs.python.org/3/library/subprocess.html#subprocess.run

The timeout argument is passed to Popen.communicate(). If the timeout expires, the child process will be killed and waited for. The TimeoutExpired exception will be re-raised after the child process has terminated.

Ah, thanks!

pattivacek · 2019-07-11T07:36:57Z

src/aktualizr_info/main.cc

+      std::cout << "Provisioned on server: " << (registered ? "yes" : "no") << std::endl;
+      std::cout << "Fetched metadata: " << (has_metadata ? "yes" : "no") << std::endl;
+      provision_trigger = (provision_trigger && registered);
+      provision_trigger = (provision_trigger && has_metadata);


These can be merged into one line: provisioned = (registered && has_metadata);

codecov-io · 2019-07-12T04:37:29Z

Codecov Report

Merging #1253 into master will decrease coverage by 0.03%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master    #1253      +/-   ##
==========================================
- Coverage   79.32%   79.28%   -0.04%     
==========================================
  Files         171      171              
  Lines       10158    10164       +6     
==========================================
+ Hits         8058     8059       +1     
- Misses       2100     2105       +5

Impacted Files	Coverage Δ
src/aktualizr_info/main.cc	`91.58% <66.66%> (-2.17%)`	⬇️
src/libaktualizr-posix/ipuptanesecondary.cc	`82.66% <0%> (-4%)`	⬇️
src/libaktualizr/package_manager/ostreemanager.cc	`75.42% <0%> (-1.7%)`	⬇️
src/aktualizr_primary/secondary.cc	`82.89% <0%> (-1.32%)`	⬇️
src/libaktualizr/primary/sotauptaneclient.cc	`91.45% <0%> (-0.31%)`	⬇️
src/libaktualizr/http/httpclient.cc	`91.92% <0%> (-0.2%)`	⬇️
src/libaktualizr/storage/sqlstorage.cc	`76.19% <0%> (+0.9%)`	⬆️
src/libaktualizr/storage/sqlstorage_base.cc	`80.4% <0%> (+2.02%)`	⬆️

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 0cbc6f3...24c5e59. Read the comment docs.

pattivacek · 2019-07-12T07:43:43Z

src/aktualizr_info/main.cc

-      if (vm.count("name-only") != 0u) {
-        std::cout << device_id << std::endl;
-        return EXIT_SUCCESS;
+    while (!provisioned && wait_provisioning) {


This will break anything that doesn't use the --wait-until-provisioned flag since the loop will get skipped.

Yes, I realized that too. I'm fixing it right now.

pattivacek

Seems cool, looks quite close! Thanks.

@rdanitz @balajirrao @Raigi Is this useful for your needs?

pattivacek · 2019-07-16T11:31:02Z

src/aktualizr_info/main.cc

+    while (wait_provisioning && !provisioned) {
+      registered = storage->loadEcuRegistered();
+      has_metadata = storage->loadLatestRoot(&director_root, Uptane::RepositoryType::Director());
+      provisioned = (registered && has_metadata);


I'm not sure this provisioned variable is doing much anymore. Also, currently storage->loadEcuRegistered() and storage->loadLatestRoot() will always run at least twice. If the while loop's condition directly used has_metadata and registered, you wouldn't have that problem.

Sure, thanks!

Seems cool, looks quite close! Thanks.

@rdanitz @balajirrao @Raigi Is this useful for your needs?

Currently I have a loop that will check aktualizr-info multiple times for provisioned: yes and metadata: yes if it received false. But it might be faster and better with aktualizr-info-wait-until-provisioned when I am creating like 50 devices.

Cool, that's exactly the type of thing we're hoping this will help optimize.

pattivacek · 2019-07-17T07:13:14Z

src/aktualizr_info/main.cc

+      registered = storage->loadEcuRegistered();
+      has_metadata = storage->loadLatestRoot(&director_root, Uptane::RepositoryType::Director());
+
+      sleep(1);


This will result in sleeping even if registration and loading the metadata is successful, which is undesirable. I think the previous way you had it where you called those two load calls outside of the loop first was actually fine.

Ok, I've changed it back and pushed it here. By the way, I didn't change anywhere else but sometimes the CI failed sometimes it passed. It's confusing. I'll check the output again after the CI process is done.

About the CI, we've had problem with Travis timing out but it should be much better after some recent changes. If you rebase your PR on master, it should go much faster and fail less often.

About the CI, we've had problem with Travis timing out but it should be much better after some recent changes. If you rebase your PR on master, it should go much faster and fail less often.

It's not the Travis timing out issue. The confusing part is sometimes the prosess will pass. I'll check the output later.

CI is passing this time.

When it failed the message for Travis might be:
The command "if [[ $RELEASE_NAME = ubuntu_16.04 ]]; then ./scripts/test_aktualizr_deb_ubuntu.sh /persistent "${INSTALL_DOCKERFILE}"; fi" exited with 1.
If GitLab CI failed , it's mostly with 'coverage'.

I don't know if this infomation is useful. I'm sure I'll run into the same situation again. Then I can dig deeper.

Sounds like it might be a race condition with the deb generation, probably not related to your PR. If you do see it again, please do let the rest of the team know, though.

Print messages until the loop finishes Optimized the logic of loop condition Add timeout feature Optimized the loop condition Signed-off-by: Zee314159 <252806294@qq.com>

pattivacek

Looks good, let's merge it!

Zee314159 force-pushed the aktualizr-info-wait-until-provisioned branch 2 times, most recently from 4c0923b to 529af68 Compare July 11, 2019 06:20

pattivacek reviewed Jul 11, 2019

View reviewed changes

Zee314159 force-pushed the aktualizr-info-wait-until-provisioned branch from 2beae00 to 7bb47e7 Compare July 12, 2019 04:39

pattivacek reviewed Jul 12, 2019

View reviewed changes

Zee314159 force-pushed the aktualizr-info-wait-until-provisioned branch 4 times, most recently from 74617f8 to 82ba09b Compare July 16, 2019 01:58

pattivacek reviewed Jul 16, 2019

View reviewed changes

Zee314159 force-pushed the aktualizr-info-wait-until-provisioned branch 3 times, most recently from 556700f to 24c5e59 Compare July 17, 2019 02:19

pattivacek reviewed Jul 17, 2019

View reviewed changes

Add wait-until-provisioned to aktualizr-info

78d32b6

Print messages until the loop finishes Optimized the logic of loop condition Add timeout feature Optimized the loop condition Signed-off-by: Zee314159 <252806294@qq.com>

Zee314159 force-pushed the aktualizr-info-wait-until-provisioned branch from 24c5e59 to 78d32b6 Compare July 17, 2019 07:32

pattivacek approved these changes Jul 17, 2019

View reviewed changes

pattivacek merged commit 953386c into master Jul 17, 2019

pattivacek deleted the aktualizr-info-wait-until-provisioned branch July 17, 2019 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --wait-until-provisioned option to aktualizr-info #1253

Add --wait-until-provisioned option to aktualizr-info #1253

Zee314159 commented Jul 11, 2019

pattivacek left a comment

pattivacek Jul 11, 2019

pattivacek Jul 11, 2019

mike-sul Jul 11, 2019

Zee314159 Jul 12, 2019 •

edited

Loading

pattivacek Jul 12, 2019

lbonn Jul 12, 2019

Zee314159 Jul 12, 2019

Zee314159 Jul 12, 2019

lbonn Jul 12, 2019

Zee314159 Jul 12, 2019

pattivacek Jul 11, 2019

codecov-io commented Jul 12, 2019 •

edited

Loading

pattivacek Jul 12, 2019

Zee314159 Jul 12, 2019

pattivacek left a comment

pattivacek Jul 16, 2019

Zee314159 Jul 17, 2019

Raigi Jul 17, 2019

pattivacek Jul 17, 2019

pattivacek Jul 17, 2019

Zee314159 Jul 17, 2019

lbonn Jul 17, 2019

Zee314159 Jul 17, 2019

Zee314159 Jul 17, 2019

pattivacek Jul 17, 2019

pattivacek left a comment

Add --wait-until-provisioned option to aktualizr-info #1253

Add --wait-until-provisioned option to aktualizr-info #1253

Conversation

Zee314159 commented Jul 11, 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

Zee314159 Jul 12, 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

codecov-io commented Jul 12, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

pattivacek left a comment

Choose a reason for hiding this comment

Zee314159 Jul 12, 2019 •

edited

Loading

codecov-io commented Jul 12, 2019 •

edited

Loading