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

ref: disable dropping director's targets in the install's call #1635

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

kbushgit
Copy link
Contributor

@kbushgit kbushgit commented Apr 8, 2020

Signed-off-by: Kostiantyn Bushko kostiantyn.bushko@yahoo.com

By moving dropping director's targets into the SotaUptaneClient:::AssembleManifest(), means that any failure in the SotaUptaneClient::uptaneInstall(...) call, will lead to drop the director's targets.
Any installation result code, except kOk is considered as a state when we must drop the director's target.

@pattivacek
Copy link
Collaborator

Looks promising! I'm glad none of the tests broke, but I'm still trying to make sure this won't change anything unexpected. Probably not. :)

@kbushgit
Copy link
Contributor Author

We already have a test which cowers case when we process each update(Uptane::Target) separately:

TEST(Aktualizr, SplitUpdates) {

But this test covers the case only with two ECU and with the scenario when both ECU updated successfully, this scenario works fine since we dropping director target only in case of installation failing or completion is needed.

An undefined behavior can occurs in the next scenario:

  • we have 3 or more updates(Uptane::Target)
  • we process each update one by one (download/install)
  • one ECU reports installation failing (important not last one ECU, the first or the second one)

Expected behavior is: the updating process must continue even if one ECU reports failing during installation process.

@kbushgit kbushgit force-pushed the ref/OTAPAD-197/installation-sequence branch from 80ba6ec to db672dd Compare April 13, 2020 13:47
@kbushgit kbushgit marked this pull request as ready for review April 14, 2020 07:50
@pattivacek
Copy link
Collaborator

But this test covers the case only with two ECU and with the scenario when both ECU updated successfully, this scenario works fine since we dropping director target only in case of installation failing or completion is needed.

An undefined behavior can occurs in the next scenario:

* we have 3 or more updates(`Uptane::Target`)

* we process each update one by one (download/install)

* one ECU reports installation failing (important not last one ECU, the first or the second one)

Yep, I agree, we don't test what happens if one of them fails. I never had considered that a requirement before.

Expected behavior is: the updating process must continue even if one ECU reports failing during installation process.

I thought for this use case we actually wanted to abort and possibly even rollback the updates that had succeeded. Are you suggesting we should be able to continue updating ECUs after one reports a failure? I think we might need a new test case that demonstrates the functionality you are expecting.

@kbushgit kbushgit force-pushed the ref/OTAPAD-197/installation-sequence branch from db672dd to 51252bf Compare April 21, 2020 13:37
@pattivacek
Copy link
Collaborator

Just realized this fixes a bug I ran into (because of targets not getting dropped in all failure cases). @kbushgit can you rebase so we can merge this? I'd still like to see the test case we'd discussed here, but it doesn't have to be part of this PR.

Signed-off-by: Kostiantyn Bushko <kostiantyn.bushko@yahoo.com>
@kbushgit kbushgit force-pushed the ref/OTAPAD-197/installation-sequence branch from 51252bf to 7726ebe Compare April 27, 2020 15:17
Copy link
Collaborator

@pattivacek pattivacek left a comment

Choose a reason for hiding this comment

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

Thanks!

@pattivacek pattivacek merged commit e83c64c into master Apr 28, 2020
@pattivacek pattivacek deleted the ref/OTAPAD-197/installation-sequence branch April 28, 2020 08:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants