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

Aktualizr secondary auto reboot #1578

Merged
merged 6 commits into from
Mar 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ Our versioning scheme is `YEAR.N` where `N` is incremented whenever a new releas

## [??? (unreleased)]

### Added

- Aktualizr secondaries can now reboot automatically after triggering an update: [PR](https://github.com/advancedtelematic/aktualizr/pull/1578)

## [2020.3] - 2020-02-27

Expand Down
2 changes: 2 additions & 0 deletions src/aktualizr_secondary/aktualizr_secondary.cc
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ data::ResultCode::Numeric AktualizrSecondary::install(const std::string& target_
return install_result;
}

void AktualizrSecondary::completeInstall() { update_agent_->completeInstall(); }

bool AktualizrSecondary::doFullVerification(const Metadata& metadata) {
// 5.4.4.2. Full verification https://uptane.github.io/uptane-standard/uptane-standard.html#metadata_verification

Expand Down
1 change: 1 addition & 0 deletions src/aktualizr_secondary/aktualizr_secondary.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ class AktualizrSecondary : public Uptane::SecondaryInterface {
bool sendFirmware(const std::string& firmware) override;
data::ResultCode::Numeric install(const std::string& target_name) override;

void completeInstall();
bool putMetadata(const Metadata& metadata);

private:
Expand Down
2 changes: 2 additions & 0 deletions src/aktualizr_secondary/aktualizr_secondary_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ void AktualizrSecondaryUptaneConfig::updateFromPropertyTree(const boost::propert
CopyFromConfig(ecu_hardware_id, "ecu_hardware_id", pt);
CopyFromConfig(key_source, "key_source", pt);
CopyFromConfig(key_type, "key_type", pt);
CopyFromConfig(force_install_completion, "force_install_completion", pt);
}

void AktualizrSecondaryUptaneConfig::writeToStream(std::ostream& out_stream) const {
writeOption(out_stream, ecu_serial, "ecu_serial");
writeOption(out_stream, ecu_hardware_id, "ecu_hardware_id");
writeOption(out_stream, key_source, "key_source");
writeOption(out_stream, key_type, "key_type");
writeOption(out_stream, force_install_completion, "force_install_completion");
}

AktualizrSecondaryConfig::AktualizrSecondaryConfig(const boost::program_options::variables_map& cmd) {
Expand Down
1 change: 1 addition & 0 deletions src/aktualizr_secondary/aktualizr_secondary_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ struct AktualizrSecondaryUptaneConfig {
std::string ecu_hardware_id;
CryptoSource key_source{CryptoSource::kFile};
KeyType key_type{KeyType::kRSA2048};
bool force_install_completion{false};

void updateFromPropertyTree(const boost::property_tree::ptree& pt);
void writeToStream(std::ostream& out_stream) const;
Expand Down
9 changes: 8 additions & 1 deletion src/aktualizr_secondary/main.cc
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,14 @@ int main(int argc, char *argv[]) {
LOG_DEBUG << "Current directory: " << boost::filesystem::current_path().string();

auto secondary = AktualizrSecondaryFactory::create(config);
SecondaryTcpServer(*secondary, config.network.primary_ip, config.network.primary_port, config.network.port).run();
SecondaryTcpServer tcp_server(*secondary, config.network.primary_ip, config.network.primary_port,
config.network.port, config.uptane.force_install_completion);

tcp_server.run();

if (tcp_server.exit_reason() == SecondaryTcpServer::ExitReason::kRebootNeeded) {
secondary->completeInstall();
Copy link
Collaborator

Choose a reason for hiding this comment

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

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().

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've added a RAII socket in run().

}

} catch (std::runtime_error &exc) {
LOG_ERROR << "Error: " << exc.what();
Expand Down
40 changes: 31 additions & 9 deletions src/aktualizr_secondary/secondary_tcp_server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,12 @@
#include <netinet/tcp.h>

SecondaryTcpServer::SecondaryTcpServer(Uptane::SecondaryInterface &secondary, const std::string &primary_ip,
in_port_t primary_port, in_port_t port)
: SecondaryTcpServer(secondary, port) {
in_port_t primary_port, in_port_t port, bool reboot_after_install)
: impl_(secondary), listen_socket_(port), keep_running_(true), reboot_after_install_(reboot_after_install) {
if (primary_ip.empty()) {
return;
}

ConnectionSocket conn_socket(primary_ip, primary_port, listen_socket_.port());
if (conn_socket.connect() == 0) {
LOG_INFO << "Connected to Primary, sending info about this secondary...";
Expand All @@ -27,18 +31,22 @@ void SecondaryTcpServer::run() {
LOG_INFO << "Secondary TCP server listens on " << listen_socket_.toString();

while (keep_running_.load()) {
int con_fd;
sockaddr_storage peer_sa{};
socklen_t peer_sa_size = sizeof(sockaddr_storage);

LOG_DEBUG << "Waiting for connection from client...";
if ((con_fd = accept(*listen_socket_, reinterpret_cast<sockaddr *>(&peer_sa), &peer_sa_size)) == -1) {
int con_fd = accept(*listen_socket_, reinterpret_cast<sockaddr *>(&peer_sa), &peer_sa_size);
if (con_fd == -1) {
LOG_INFO << "Socket accept failed. aborting";
break;
}
Socket con_socket(con_fd);
LOG_DEBUG << "Connected...";
HandleOneConnection(con_fd);
bool continue_serving = HandleOneConnection(*con_socket);
LOG_DEBUG << "Client disconnected";
if (!continue_serving) {
break;
}
}
LOG_INFO << "Secondary TCP server exit";
}
Expand All @@ -50,8 +58,9 @@ void SecondaryTcpServer::stop() {
}

in_port_t SecondaryTcpServer::port() const { return listen_socket_.port(); }
SecondaryTcpServer::ExitReason SecondaryTcpServer::exit_reason() const { return exit_reason_; }

void SecondaryTcpServer::HandleOneConnection(int socket) {
bool SecondaryTcpServer::HandleOneConnection(int socket) {
// Outside the message loop, because one recv() may have parts of 2 messages
// Note that one recv() call returning 2+ messages doesn't work at the
// moment. This shouldn't be a problem until we have messages that aren't
Expand All @@ -64,6 +73,8 @@ void SecondaryTcpServer::HandleOneConnection(int socket) {
asn_dec_rval_t res;
asn_codec_ctx_s context{};
ssize_t received;
bool need_reboot = false;

do {
received = recv(socket, buffer.Tail(), buffer.TailSpace(), 0);
LOG_TRACE << "Got " << received << " bytes "
Expand All @@ -76,7 +87,7 @@ void SecondaryTcpServer::HandleOneConnection(int socket) {
Asn1Message::Ptr msg = Asn1Message::FromRaw(&m);

if (res.code != RC_OK) {
return; // Either an error or the client closed the socket
return true; // Either an error or the client closed the socket
}

// Figure out what to do with the message
Expand Down Expand Up @@ -144,10 +155,14 @@ void SecondaryTcpServer::HandleOneConnection(int socket) {
resp->present(AKIpUptaneMes_PR_installResp);
auto response_message = resp->installResp();
response_message->result = static_cast<AKInstallationResultCode_t>(install_result);

if (install_result == data::ResultCode::Numeric::kNeedCompletion) {
need_reboot = true;
}
} break;
default:
LOG_ERROR << "Unrecognised message type:" << msg->present();
return;
return true;
}

// Send the response
Expand All @@ -157,15 +172,22 @@ void SecondaryTcpServer::HandleOneConnection(int socket) {
asn_enc_rval_t encode_result =
der_encode(&asn_DEF_AKIpUptaneMes, &resp->msg_, Asn1SocketWriteCallback, reinterpret_cast<void *>(&socket));
if (encode_result.encoded == -1) {
return; // write error
return true; // write error
}
optval = 1;
setsockopt(socket, IPPROTO_TCP, TCP_NODELAY, &optval, sizeof(int));
} else {
LOG_DEBUG << "Not sending a response to message " << msg->present();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 :)

if (need_reboot && reboot_after_install_) {
exit_reason_ = ExitReason::kRebootNeeded;
return false;
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 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.

}

} // Go back round and read another message

return true;
// Parse error => Shutdown the socket
// write error => Shutdown the socket
// Timeout on write => shutdown
Expand Down
18 changes: 12 additions & 6 deletions src/aktualizr_secondary/secondary_tcp_server.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,14 @@ class SecondaryInterface;
*/
class SecondaryTcpServer {
public:
SecondaryTcpServer(Uptane::SecondaryInterface& secondary, const std::string& primary_ip, in_port_t primary_port,
in_port_t port = 0);
enum class ExitReason {
kNotApplicable,
kRebootNeeded,
kUnkown,
};

SecondaryTcpServer(Uptane::SecondaryInterface& secondary, in_port_t port = 0)
: keep_running_(true), impl_(secondary), listen_socket_(port) {}
SecondaryTcpServer(Uptane::SecondaryInterface& secondary, const std::string& primary_ip, in_port_t primary_port,
in_port_t port = 0, bool reboot_after_install = false);

SecondaryTcpServer(const SecondaryTcpServer&) = delete;
SecondaryTcpServer& operator=(const SecondaryTcpServer&) = delete;
Expand All @@ -31,14 +34,17 @@ class SecondaryTcpServer {
void stop();

in_port_t port() const;
ExitReason exit_reason() const;

private:
void HandleOneConnection(int socket);
bool HandleOneConnection(int socket);

private:
std::atomic<bool> keep_running_;
Uptane::SecondaryInterface& impl_;
ListenSocket listen_socket_;
std::atomic<bool> keep_running_;
bool reboot_after_install_;
ExitReason exit_reason_{ExitReason::kNotApplicable};
};

#endif // AKTUALIZR_SECONDARY_TCP_SERVER_H_
2 changes: 1 addition & 1 deletion src/aktualizr_secondary/secondary_tcp_server_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ TEST(SecondaryTcpServer, TestIpSecondaryRPC) {
PublicKey("pub-key", KeyType::kED25519), Uptane::Manifest());

// create Secondary on Secondary ECU, and run it in a dedicated thread
SecondaryTcpServer secondary_server{secondary};
SecondaryTcpServer secondary_server(secondary, "", 0);
std::thread secondary_server_thread{[&secondary_server]() { secondary_server.run(); }};

// create Secondary on Primary ECU, try it a few times since the secondary thread
Expand Down
1 change: 1 addition & 0 deletions src/aktualizr_secondary/update_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class UpdateAgent {

virtual bool download(const Uptane::Target& target, const std::string& data) = 0;
virtual data::ResultCode::Numeric install(const Uptane::Target& target) = 0;
virtual void completeInstall() = 0;
virtual data::InstallationResult applyPendingInstall(const Uptane::Target& target) = 0;

virtual ~UpdateAgent() = default;
Expand Down
2 changes: 2 additions & 0 deletions src/aktualizr_secondary/update_agent_file.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ data::ResultCode::Numeric FileUpdateAgent::install(const Uptane::Target& target)
return data::ResultCode::Numeric::kOk;
}

void FileUpdateAgent::completeInstall() {}

data::InstallationResult FileUpdateAgent::applyPendingInstall(const Uptane::Target& target) {
(void)target;
return data::InstallationResult(data::ResultCode::Numeric::kInternalError,
Expand Down
1 change: 1 addition & 0 deletions src/aktualizr_secondary/update_agent_file.h
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class FileUpdateAgent : public UpdateAgent {
bool getInstalledImageInfo(Uptane::InstalledImageInfo& installed_image_info) const override;
bool download(const Uptane::Target& target, const std::string& data) override;
data::ResultCode::Numeric install(const Uptane::Target& target) override;
void completeInstall() override;
data::InstallationResult applyPendingInstall(const Uptane::Target& target) override;

private:
Expand Down
2 changes: 2 additions & 0 deletions src/aktualizr_secondary/update_agent_ostree.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,8 @@ data::ResultCode::Numeric OstreeUpdateAgent::install(const Uptane::Target& targe
return (_ostreePackMan->install(target)).result_code.num_code;
}

void OstreeUpdateAgent::completeInstall() { _ostreePackMan->completeInstall(); }

data::InstallationResult OstreeUpdateAgent::applyPendingInstall(const Uptane::Target& target) {
return _ostreePackMan->finalizeInstall(target);
}
Expand Down
1 change: 1 addition & 0 deletions src/aktualizr_secondary/update_agent_ostree.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class OstreeUpdateAgent : public UpdateAgent {
bool getInstalledImageInfo(Uptane::InstalledImageInfo& installed_image_info) const override;
bool download(const Uptane::Target& target, const std::string& data) override;
data::ResultCode::Numeric install(const Uptane::Target& target) override;
void completeInstall() override;
data::InstallationResult applyPendingInstall(const Uptane::Target& target) override;

private:
Expand Down
3 changes: 2 additions & 1 deletion src/libaktualizr/utilities/utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ bool operator<(const sockaddr_storage &left, const sockaddr_storage &right); //
class Socket {
public:
Socket();
virtual ~Socket() = 0;
Socket(int fd) : socket_fd_(fd) {}
virtual ~Socket();

Socket(const Socket &) = delete;
Socket &operator=(const Socket &) = delete;
Expand Down
5 changes: 4 additions & 1 deletion tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,12 @@ add_test(NAME test_log_negative
set_tests_properties(test_log_negative
PROPERTIES PASS_REGULAR_EXPRESSION "Invalid log level")

if (BUILD_OSTREE)
set(TEST_IPSEC_ARGS "--ostree")
endif ()
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})
Copy link
Collaborator

Choose a reason for hiding this comment

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

What ${TEST_IPSEC_ARGS} is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

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 got it.


add_test(NAME test_director_failure
COMMAND ${PROJECT_SOURCE_DIR}/tests/test_director_failure.py
Expand Down
49 changes: 46 additions & 3 deletions tests/ipsecondary_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,46 @@ def test_secondary_ostree_update(uptane_repo, secondary, aktualizr, treehub, sys
return True


@with_treehub()
@with_uptane_backend()
@with_director()
@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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

target_rev = treehub.revision
expected_targetname = uptane_repo.add_ostree_target(secondary.id, target_rev, "GARAGE_TARGET_NAME")

with secondary:
with aktualizr:
aktualizr.wait_for_completion()
secondary.wait_for_completion()

pending_rev = aktualizr.get_current_pending_image_info(secondary.id)

if pending_rev != target_rev:
logger.error("Pending version {} != the target one {}".format(pending_rev, target_rev))
return False

sysroot.update_revision(pending_rev)

with secondary:
with aktualizr:
aktualizr.wait_for_completion()

if not director.get_install_result():
Copy link
Collaborator

Choose a reason for hiding this comment

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

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):

logger.error("Installation result is not successful")
return False

installed_rev = aktualizr.get_current_image_info(secondary.id)

if installed_rev != target_rev:
logger.error("Installed version {} != the target one {}".format(installed_rev, target_rev))
return False

return True


@with_uptane_backend()
@with_director()
@with_secondary(start=False)
Expand Down Expand Up @@ -305,7 +345,7 @@ def test_primary_multiple_secondaries(uptane_repo, secondary, secondary2, aktual
parser = argparse.ArgumentParser(description='Test IP Secondary')
parser.add_argument('-b', '--build-dir', help='build directory', default='build')
parser.add_argument('-s', '--src-dir', help='source directory', default='.')
parser.add_argument('-o', '--ostree', help='ostree support', default='OFF')
parser.add_argument('-o', '--ostree', help='ostree support', action='store_true')

input_params = parser.parse_args()

Expand All @@ -323,8 +363,11 @@ def test_primary_multiple_secondaries(uptane_repo, secondary, secondary2, aktual
test_primary_multiple_secondaries,
]

if input_params.ostree == 'ON':
test_suite.append(test_secondary_ostree_update)
if input_params.ostree:
test_suite += [
test_secondary_ostree_update,
test_secondary_ostree_reboot,
]

test_suite_run_result = TestRunner(test_suite).run()

Expand Down
Loading