Do not provision if primary times out while connecting to secon… #1491
Conversation
Move the flush report log to trace. Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
If a secondary connection times out before the device was provisioned, exit early so that partially up devices are not provisioned in an invalid state. Already provisioned devices will still start even if some secondaries are missing. Signed-off-by: Laurent Bonnans <laurent.bonnans@here.com>
lbonn
requested review from
pattivacek,
eu-siemann,
mike-sul,
Zee314159,
xcheng-here and
kbushgit
|
Unfortunately, an update of doxygen seems to have broken the docs generation. I'll merge anyway because this was already broken. We'll have to fix that in another PR quite soon. |
lbonn
changed the title
| @@ -122,11 +122,14 @@ int main(int argc, char *argv[]) { | |||
| conn = aktualizr.SetSignalHandler(f_cb); | |||
|
|
|||
| if (!config.uptane.secondary_config_file.empty()) { | |||
| if (boost::filesystem::exists(config.uptane.secondary_config_file)) { | |||
| try { | |||
There was a problem hiding this comment.
This try/catch was not necessary as this code is already wrapped by the outter try/catch.
There was a problem hiding this comment.
The exception is ignored if aktualizr.IsRegistered(), I think we cannot rely on the outer try/catch in this case.
There was a problem hiding this comment.
ok. I see, this is for the use case when Primary and Secondaries are trying to connect to each other after a device has been already provisioned, e.g. just a device and/or aktualizr reboot. I think, it might be not an optimal strategy to exit the connection procedure on a first failure if a device has been already registered, but, I suppose, it's not so important, adding secondaries dynamically is more important or not trying to connect to them on a startup at all if a device already registered.
There was a problem hiding this comment.
I think, it might be not an optimal strategy to exit the connection procedure on a first failure if a device has been already registered, but, I suppose, it's not so important, adding secondaries dynamically is more important or not trying to connect to them on a startup at all if a device already registered.
That's a good point indeed... But I remember I chose this simple solution at first because right now, we can't really do better than what the IP secondaries config parser gives us and it handles all IP secondaries at once.
| LOG_WARNING << "The specified secondary config file does not exist: " << config.uptane.secondary_config_file | ||
| << "\nProceed further without secondary(ies)"; | ||
| } catch (const std::exception &e) { | ||
| LOG_ERROR << "Secondary initialization failed"; |
There was a problem hiding this comment.
I would add e.what() to the log message so we see more details of the secondary provisioning failure.
There was a problem hiding this comment.
In the current version, e.what() is logged in initSecondaries() but I agree this is maybe not really clear.
initSecondaries() could also log these errors and return a boolean, to simplify the call site in main.cc.
There was a problem hiding this comment.
I see, makes sense, it's up to you whether to terminate exceptions in initSecondaries and return boolean or rethrow them.
| @@ -82,8 +81,10 @@ class SecondaryWaiter { | |||
| timer_.async_wait([&](const boost::system::error_code& error_code) { | |||
| if (!!error_code) { | |||
| LOG_ERROR << "Wait for secondaries has failed: " << error_code; | |||
| throw std::runtime_error("Error while waiting for secondary"); | |||
There was a problem hiding this comment.
As an alternative to these two 'throws' just one throw can be added at the end of createIPSecondaries().
No description provided.