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

Do not provision if primary times out while connecting to secon… #1491

Merged
merged 4 commits into from
Dec 16, 2019
Merged

Do not provision if primary times out while connecting to secon… #1491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7c592c0
Less verbose ~ReportQueue()
lbonn Dec 16, 2019
0eecf3d
Set some SotaUptaneClient as const
lbonn Dec 16, 2019
5f68284
Fix "Secoondary" typo
lbonn Dec 13, 2019
dc40f4a
Do not provision partial devices
lbonn Dec 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/aktualizr_primary/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This try/catch was not necessary as this code is already wrapped by the outter try/catch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The exception is ignored if aktualizr.IsRegistered(), I think we cannot rely on the outer try/catch in this case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Primary::initSecondaries(aktualizr, config.uptane.secondary_config_file);
} else {
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";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add e.what() to the log message so we see more details of the secondary provisioning failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, makes sense, it's up to you whether to terminate exceptions in initSecondaries and return boolean or rethrow them.

if (!aktualizr.IsRegistered()) {
LOG_ERROR << "Cannot provision without all secondaries present, exiting...";
return EXIT_FAILURE;
}
}
}

Expand Down
11 changes: 6 additions & 5 deletions src/aktualizr_primary/secondary.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,7 @@ void initSecondaries(Aktualizr& aktualizr, const boost::filesystem::path& config
}
} catch (const std::exception& exc) {
LOG_ERROR << "Failed to initialize a secondary: " << exc.what();
LOG_ERROR << "Continue with initialization of the remaining secondaries, if any left.";
// otherwise rethrow the exception
throw exc;
}
}
}
Expand All @@ -71,7 +70,7 @@ class SecondaryWaiter {
timer_{io_context_},
connected_secondaries_(secondaries) {}

void addSecoondary(const std::string& ip, uint16_t port) { secondaries_to_wait_for_.insert(key(ip, port)); }
void addSecondary(const std::string& ip, uint16_t port) { secondaries_to_wait_for_.insert(key(ip, port)); }

void wait() {
if (secondaries_to_wait_for_.empty()) {
Expand All @@ -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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

As an alternative to these two 'throws' just one throw can be added at the end of createIPSecondaries().

} else {
LOG_ERROR << "Timeout while waiting for secondaries: " << error_code;
LOG_ERROR << "Timeout while waiting for secondary: " << error_code;
throw std::runtime_error("Timeout while waiting for secondary");
}
io_context_.stop();
});
Expand Down Expand Up @@ -149,7 +150,7 @@ static Secondaries createIPSecondaries(const IPSecondariesConfig& config) {
if (sec_creation_res.first) {
result.push_back(sec_creation_res.second);
} else {
sec_waiter.addSecoondary(ip_sec_cfg.ip, ip_sec_cfg.port);
sec_waiter.addSecondary(ip_sec_cfg.ip, ip_sec_cfg.port);
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/libaktualizr/primary/aktualizr.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ void Aktualizr::Initialize() {
api_queue_.run();
}

bool Aktualizr::IsRegistered() const { return storage_->loadEcuRegistered(); }

bool Aktualizr::UptaneCycle() {
result::UpdateCheck update_result = CheckUpdates().get();
if (update_result.updates.empty()) {
Expand Down
5 changes: 5 additions & 0 deletions src/libaktualizr/primary/aktualizr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ class Aktualizr {
*/
void Initialize();

/**
* Returns true if the device has been registered to the backend succesffully.
*/
bool IsRegistered() const;

/**
* Asynchronously run aktualizr indefinitely until Shutdown is called.
* @return Empty std::future object
Expand Down
2 changes: 1 addition & 1 deletion src/libaktualizr/primary/reportqueue.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ ReportQueue::~ReportQueue() {
cv_.notify_all();
thread_.join();

LOG_DEBUG << "Flushing report queue";
LOG_TRACE << "Flushing report queue";
flushQueue();
}

Expand Down
8 changes: 4 additions & 4 deletions src/libaktualizr/primary/sotauptaneclient.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ Json::Value SotaUptaneClient::AssembleManifest() {
return manifest;
}

bool SotaUptaneClient::hasPendingUpdates() { return storage->hasPendingInstall(); }
bool SotaUptaneClient::hasPendingUpdates() const { return storage->hasPendingInstall(); }

void SotaUptaneClient::initialize() {
LOG_DEBUG << "Checking if device is provisioned...";
Expand Down Expand Up @@ -941,12 +941,12 @@ void SotaUptaneClient::campaignPostpone(const std::string &campaign_id) {
report_queue->enqueue(std_::make_unique<CampaignPostponedReport>(campaign_id));
}

bool SotaUptaneClient::isInstallCompletionRequired() {
bool SotaUptaneClient::isInstallCompletionRequired() const {
bool force_install_completion = (hasPendingUpdates() && config.uptane.force_install_completion);
return force_install_completion;
}

void SotaUptaneClient::completeInstall() {
void SotaUptaneClient::completeInstall() const {
if (isInstallCompletionRequired()) {
package_manager_->completeInstall();
}
Expand Down Expand Up @@ -1223,6 +1223,6 @@ std::string SotaUptaneClient::secondaryTreehubCredentials() const {
}
}

Uptane::LazyTargetsList SotaUptaneClient::allTargets() {
Uptane::LazyTargetsList SotaUptaneClient::allTargets() const {
return Uptane::LazyTargetsList(images_repo, storage, uptane_fetcher);
}
13 changes: 7 additions & 6 deletions src/libaktualizr/primary/sotauptaneclient.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,16 +65,17 @@ class SotaUptaneClient {
void campaignAccept(const std::string &campaign_id);
void campaignDecline(const std::string &campaign_id);
void campaignPostpone(const std::string &campaign_id);
bool hasPendingUpdates();
bool isInstallCompletionRequired();
void completeInstall();
Uptane::LazyTargetsList allTargets();
Uptane::Target getCurrent() { return package_manager_->getCurrent(); }

bool hasPendingUpdates() const;
bool isInstallCompletionRequired() const;
void completeInstall() const;
Uptane::LazyTargetsList allTargets() const;
Uptane::Target getCurrent() const { return package_manager_->getCurrent(); }

bool updateImagesMeta(); // TODO: make private once aktualizr has a proper TUF API
bool checkImagesMetaOffline();
data::InstallationResult PackageInstall(const Uptane::Target &target);
TargetStatus VerifyTarget(const Uptane::Target &target) { return package_manager_->verifyTarget(target); }
TargetStatus VerifyTarget(const Uptane::Target &target) const { return package_manager_->verifyTarget(target); }

protected:
void addSecondary(const std::shared_ptr<Uptane::SecondaryInterface> &sec);
Expand Down
7 changes: 6 additions & 1 deletion tests/ipsecondary_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,12 @@ def test_primary_timeout_during_first_run(uptane_repo, secondary, aktualizr, **k
with aktualizr:
aktualizr.wait_for_completion()

return not aktualizr.is_ecu_registered(secondary.id)
info = aktualizr.get_info()
if info is None:
return False
not_provisioned = 'Provisioned on server: no' in info

return not_provisioned and not aktualizr.is_ecu_registered(secondary.id)


@with_uptane_backend()
Expand Down