Aktualizr secondary auto reboot #1578

lbonn · 2020-02-28T13:54:42Z

Tested manually on qemu.

mike-sul · 2020-02-28T14:15:37Z

src/aktualizr_secondary/secondary_tcp_server.cc

@@ -164,6 +175,12 @@ void SecondaryTcpServer::HandleOneConnection(int socket) {
    } else {
      LOG_DEBUG << "Not sending a response to message " << msg->present();
    }
+


I don't think that there is much reason in calling stop here as it's mainly intended for the server stooping from another thread. Perhaps just returning a bool/int from HandleOneConnection() and handling in the while loop would be slightly better (you could set keep_running_ = false; or just break directly there).
Also, the name may_reboot a bit confusing, it's rather want reboot or require reboot, just minor :)

codecov-io · 2020-02-28T14:23:48Z

Codecov Report

Merging #1578 into master will decrease coverage by 0.02%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master    #1578      +/-   ##
==========================================
- Coverage   82.35%   82.32%   -0.03%     
==========================================
  Files         189      189              
  Lines       11851    11872      +21     
==========================================
+ Hits         9760     9774      +14     
- Misses       2091     2098       +7

Impacted Files	Coverage Δ
...c/aktualizr_secondary/aktualizr_secondary_config.h	`100% <ø> (ø)`	⬆️
src/aktualizr_secondary/secondary_tcp_server.h	`100% <ø> (ø)`	⬆️
src/aktualizr_secondary/aktualizr_secondary.h	`60% <ø> (ø)`	⬆️
src/aktualizr_secondary/update_agent_file.h	`100% <ø> (ø)`	⬆️
src/aktualizr_secondary/update_agent_ostree.h	`100% <ø> (ø)`	⬆️
src/aktualizr_secondary/update_agent_file.cc	`66.66% <0%> (-2.09%)`	⬇️
src/aktualizr_secondary/aktualizr_secondary.cc	`76.74% <100%> (+0.18%)`	⬆️
.../aktualizr_secondary/aktualizr_secondary_config.cc	`95.06% <100%> (+0.12%)`	⬆️
src/libaktualizr/utilities/utils.h	`93.33% <100%> (+0.47%)`	⬆️
src/aktualizr_secondary/update_agent_ostree.cc	`84.31% <100%> (+0.31%)`	⬆️
... and 9 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 3c09519...9689bbb. Read the comment docs.

mike-sul · 2020-02-28T14:31:59Z

src/aktualizr_secondary/main.cc

+    tcp_server.run();
+
+    if (tcp_server.exit_reason() == SecondaryTcpServer::ExitReason::kRebootNeeded) {
+      secondary->completeInstall();


How can we guarantee here that the TCP/IP stack has sent all data from its internal buffers before the reboot is called?
It looks like there is missing connection shutdown in HandleOneConnection() and connection socket closing in run().

I've added a RAII socket in run().

mike-sul

Looks great! I think it's important to make sure that all data from TCP/IP internal buffers are sent through wire before reboot takes place. IMHO, Just shutdown and close a connection socket + wait a bit before reboot.

pattivacek

Thankfully doesn't look quite as bad as we feared. How can we test this?

pattivacek · 2020-02-28T14:52:08Z

src/aktualizr_secondary/secondary_tcp_server.h

-  SecondaryTcpServer(Uptane::SecondaryInterface& secondary, const std::string& primary_ip, in_port_t primary_port,
-                     in_port_t port = 0);
+  enum class ExitReason {
+    kNA,


"NA" for "not applicable"? Minor, but maybe worth spelling out.

I've also renamed Other to Unknown, more consistent with other enums we have.

lbonn · 2020-02-28T15:37:32Z

How can we test this?

There should be something to do with ipsecondary_test.py.

mike-sul · 2020-03-03T07:56:04Z

src/aktualizr_secondary/secondary_tcp_server.cc

+
+    if (need_reboot && reboot_after_install_) {
+      exit_reason_ = ExitReason::kRebootNeeded;
+      return false;


As an option exit_reason_ can be returned from HandleOneConnection to run() and run() in turn can return the exit status to a caller/client so no class member is required to store an exit reason and no additional call for a client to get this exit reason. But it' really minor and not important just an alternative.

mike-sul

LGTM, I just think that it would be great to add some test(s) for this functionality.

It's only used for tests, the equivalent case can be handled in the main one. Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

It was not explicitely closed! Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

"ON" is not the only way to get a true value from CMake Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

lbonn · 2020-03-04T16:15:13Z

Now comes with a test.

pattivacek

LGTM.

pattivacek · 2020-03-05T09:56:52Z

tests/ipsecondary_test.py

+@with_sysroot()
+@with_secondary(start=False, output_logs=False, force_reboot=True)
+@with_aktualizr(start=False, run_mode='once', output_logs=True)
+def test_secondary_ostree_reboot(uptane_repo, secondary, aktualizr, treehub, sysroot, director, **kwargs):


Do we have a negative test for this? (Same goes for the Primary, actually.) Less important, but just curious.

mike-sul · 2020-03-05T10:09:23Z

tests/ipsecondary_test.py

+        with aktualizr:
+            aktualizr.wait_for_completion()
+
+    if not director.get_install_result():


I think it makes sense to add checking if what is currently installed on Secondary is what we expect
something like this `if target_rev != aktualizr.get_current_image_info(secondary.id):

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

mike-sul · 2020-03-05T11:09:08Z

tests/CMakeLists.txt

 add_test(NAME test_ip_secondary
         COMMAND ${PROJECT_SOURCE_DIR}/tests/ipsecondary_test.py
-         --build-dir ${PROJECT_BINARY_DIR} --src-dir ${PROJECT_SOURCE_DIR} --ostree ${BUILD_OSTREE})
+         --build-dir ${PROJECT_BINARY_DIR} --src-dir ${PROJECT_SOURCE_DIR} ${TEST_IPSEC_ARGS})


What ${TEST_IPSEC_ARGS} is for?

It's here to set or not set --ostree as an argument to the script, as using ${BUILD_OSTREE} in the python script would force us to reimplement CMake boolean logic:

True if the constant is 1, ON, YES, TRUE, Y, or a non-zero number. False if the constant is 0, OFF, NO, FALSE, N, IGNORE, NOTFOUND, the empty string, or ends in the suffix -NOTFOUND. Named boolean constants are case-insensitive. If the argument is not one of these specific constants, it is treated as a variable or string and the following signature is used.
https://cmake.org/cmake/help/latest/command/if.html#command:if

I had the problem that the tests would not run because I configured my build directory with -DBUILD_OSTREE=on instead of -DBUILD_OSTREE=ON.

OK, I got it.

lbonn requested review from pattivacek, eu-siemann, mike-sul, xcheng-here and kbushgit February 28, 2020 13:54

lbonn marked this pull request as ready for review February 28, 2020 13:55

mike-sul reviewed Feb 28, 2020

View reviewed changes

pattivacek reviewed Feb 28, 2020

View reviewed changes

lbonn force-pushed the feat/OTA-4524/secondary-auto-reboot branch from ac4f0c1 to 3fda90b Compare February 28, 2020 15:36

mike-sul reviewed Mar 3, 2020

View reviewed changes

lbonn added 4 commits March 4, 2020 12:24

Remove a constructor of SecondaryTcpServer

f491aea

It's only used for tests, the equivalent case can be handled in the main one. Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Optional reboot after update for secondaries

dce95c7

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Use owned socket at return from accept in secondary

273270e

It was not explicitely closed! Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Robustify option parsing in ipsecondary_test.py

83eadff

"ON" is not the only way to get a true value from CMake Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

lbonn force-pushed the feat/OTA-4524/secondary-auto-reboot branch from 3fda90b to 515d858 Compare March 4, 2020 16:14

pattivacek previously approved these changes Mar 5, 2020

View reviewed changes

mike-sul reviewed Mar 5, 2020

View reviewed changes

lbonn added 2 commits March 5, 2020 12:05

Test for secondary auto-reboot

ee173cc

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

Add secondary auto-reboot to changelog

9689bbb

Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>

lbonn dismissed pattivacek’s stale review via 9689bbb March 5, 2020 11:05

lbonn force-pushed the feat/OTA-4524/secondary-auto-reboot branch from 515d858 to 9689bbb Compare March 5, 2020 11:05

mike-sul reviewed Mar 5, 2020

View reviewed changes

pattivacek approved these changes Mar 5, 2020

View reviewed changes

mike-sul approved these changes Mar 5, 2020

View reviewed changes

lbonn merged commit e502843 into master Mar 5, 2020

lbonn deleted the feat/OTA-4524/secondary-auto-reboot branch March 5, 2020 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aktualizr secondary auto reboot #1578

Aktualizr secondary auto reboot #1578

lbonn commented Feb 28, 2020 •

edited

Loading

mike-sul Feb 28, 2020

codecov-io commented Feb 28, 2020 •

edited

Loading

mike-sul Feb 28, 2020

lbonn Feb 28, 2020

mike-sul left a comment •

edited

Loading

pattivacek left a comment

pattivacek Feb 28, 2020

lbonn Feb 28, 2020

lbonn commented Feb 28, 2020

mike-sul Mar 3, 2020

mike-sul left a comment

lbonn commented Mar 4, 2020

pattivacek left a comment

pattivacek Mar 5, 2020

mike-sul Mar 5, 2020

mike-sul Mar 5, 2020

lbonn Mar 5, 2020

mike-sul Mar 5, 2020

Aktualizr secondary auto reboot #1578

Aktualizr secondary auto reboot #1578

Conversation

lbonn commented Feb 28, 2020 • edited Loading

Choose a reason for hiding this comment

codecov-io commented Feb 28, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-sul left a comment • edited Loading

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

lbonn commented Feb 28, 2020

Choose a reason for hiding this comment

mike-sul left a comment

Choose a reason for hiding this comment

lbonn commented Mar 4, 2020

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

lbonn commented Feb 28, 2020 •

edited

Loading

codecov-io commented Feb 28, 2020 •

edited

Loading

mike-sul left a comment •

edited

Loading