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

OLPSUP-9046: Handle expired Director's target metadata #1548

Merged
merged 2 commits into from
Feb 4, 2020

Conversation

mike-sul
Copy link
Collaborator

@mike-sul mike-sul commented Feb 3, 2020

  • Apply pending update even if its metadata are expired provided that it
    was installed before the expiration
  • Add tests for the target metadata expiration use-case

Signed-off-by: Mike Sul [email protected]

@mike-sul
Copy link
Collaborator Author

mike-sul commented Feb 3, 2020

@lbonn Please, see if it makes sense to you, I am just not sure if this is the right fix for https://saeljira.it.here.com/browse/OLPSUP-9046, as an alternative we could "cancel" a pending update if its metadata are expired.

@mike-sul mike-sul force-pushed the fix/OLPSUP-9046/handle-expired-metadata branch from e1ac39e to 748d6e8 Compare February 3, 2020 13:29
@@ -63,6 +63,7 @@ bool DirectorRepository::checkMetaOffline(INvStorage& storage) {
}

if (targetsExpired()) {
last_exception = Uptane::ExpiredMetadata(type.toString(), Role::Targets().ToString());
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

return;
}

LOG_INFO << "Device has been rebooted after an update";
LOG_INFO << "Check if there is any pending update for Primary ECU to be applied.";
Copy link
Contributor

Choose a reason for hiding this comment

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

This text reads like a suggestion to the user, whereas it's a description of the current operation.

Suggested change
LOG_INFO << "Check if there is any pending update for Primary ECU to be applied.";
LOG_INFO << "Checking for pending update for primary ECU to apply";

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, will update accordingly

@@ -35,47 +35,24 @@ enum class TargetStatus {

class PackageManagerInterface {
public:
PackageManagerInterface(PackageConfig pconfig, BootloaderConfig bconfig, std::shared_ptr<INvStorage> storage,
Copy link
Contributor

Choose a reason for hiding this comment

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

As you mentioned in your comment, this would break #1498. Do you then have an alternative design to propose? Also, the exception in this PR is the Debian package manager which we don't really care about.

Maybe an intermediate solution would just be to pass a const reference to the full config here, so that implementations can pick what's needed.

In any case, I would prefer if we kept this change in another PR since it's independent from the other changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not independent, the interface cleanup depends on the fix implementation.

I thought that this is what Eugene intended eventually to do with moving the bootloader instance under the ostree package manager.

I will remove the pack man interface cleanup from this PR and create another PR, although the later will depend on this PR.

@mike-sul mike-sul force-pushed the fix/OLPSUP-9046/handle-expired-metadata branch from 6fffd1e to 470a297 Compare February 3, 2020 17:05
- Apply pending update even if its metadata are expired provided that it
was installed before the expiration
- Add tests for the target metadata expiration use-case

Signed-off-by: Mike Sul <[email protected]>
@mike-sul mike-sul force-pushed the fix/OLPSUP-9046/handle-expired-metadata branch from 470a297 to 230c905 Compare February 3, 2020 17:28
@codecov-io
Copy link

codecov-io commented Feb 3, 2020

Codecov Report

Merging #1548 into master will increase coverage by 0.15%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
+ Coverage   81.89%   82.05%   +0.15%     
==========================================
  Files         186      186              
  Lines       11424    11451      +27     
==========================================
+ Hits         9356     9396      +40     
+ Misses       2068     2055      -13
Impacted Files Coverage Δ
src/uptane_generator/director_repo.h 100% <ø> (ø) ⬆️
.../libaktualizr/package_manager/packagemanagerfake.h 100% <ø> (ø) ⬆️
src/libaktualizr/package_manager/ostreemanager.h 100% <ø> (ø) ⬆️
src/libaktualizr/utilities/types.h 83.92% <ø> (ø) ⬆️
src/uptane_generator/uptane_repo.h 100% <ø> (ø) ⬆️
...ktualizr/package_manager/packagemanagerinterface.h 80% <ø> (-10%) ⬇️
src/libaktualizr/package_manager/debianmanager.h 66.66% <0%> (ø) ⬆️
...libaktualizr/package_manager/packagemanagerfake.cc 91.52% <100%> (+0.61%) ⬆️
src/aktualizr_secondary/aktualizr_secondary.cc 76.56% <100%> (ø) ⬆️
src/uptane_generator/uptane_repo.cc 100% <100%> (ø) ⬆️
... and 19 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 05fee03...461f7b6. Read the comment docs.

@mike-sul mike-sul force-pushed the fix/OLPSUP-9046/handle-expired-metadata branch 2 times, most recently from c45055f to 2d66e5a Compare February 3, 2020 18:58
@mike-sul mike-sul removed the not-ready label Feb 3, 2020
// e.g. the pacman will reset/clear the flag by itself
_ostreePackMan->rebootFlagClear();
return install_result;
return _ostreePackMan->finalizeInstall(target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR really but according to good sources, identifiers starting with underscores should be avoided as they are reserved for "C++ implementations": https://stackoverflow.com/a/20496955/1931271

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It actually contradicts the google coding guidelines that we supposed to follow, I found out about it just after this change was merged. I will change this underscore naming to make it consistent with our coding style in another PR.

But personally I prefer/like this code guideline https://www.appinf.com/download/CppCodingStyleGuide.pdf which says the following in regard of underscore usage:

"

Rule 30
Global names must not begin with one or two underscores (“_” or “__”).
This is forbidden by the C++ standard as such names are reserved for use by
the compiler.

Rule 31
With the exceptions of constants and enumeration values, there are no
underscores in names (after the first character of the name). 2
"

Rule 33
The name of a private or protected member variable begins with a single
underscore.3
 This is the only underscore in such a name (see Rule 31).


Copy link
Contributor

Choose a reason for hiding this comment

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

Ah cool, this guide might have interesting insights as well.

In this case, however, even the C++ standard has something to say (2.10):

In addition, some identifiers are reserved for use by C++implementations and shall not be used otherwise;no diagnostic is required.
— Each identifier that contains a double underscore__or begins with an underscore followed by an uppercase letter is reserved to the implementation for any use.
— Each identifier that begins with an underscore is reserved to the implementation for use as a name int he global namespace.

It doesn't exactly conflict as these are not in the global namespace and do not follow the underscore by an uppercase letter but that's quite close.

@@ -65,6 +65,7 @@ class OstreeManager : public PackageManagerInterface {

private:
TargetStatus verifyTargetInternal(const Uptane::Target &target) const;
data::InstallationResult applyInstall(const Uptane::Target &target);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this new method? We would have install, completeInstall, finalizeInstall and applyInstall. This is starting to get a bit confusing. Or maybe it's missing some docs and/or comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a private helper method and is not part of the class interface as the other listed methods are, but it, indeed is not really useful now and can be removed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated

@lbonn
Copy link
Contributor

lbonn commented Feb 4, 2020

Looks good, just had some minor comments.
The tests are also nice 👍.

@mike-sul mike-sul force-pushed the fix/OLPSUP-9046/handle-expired-metadata branch from 2d66e5a to 461f7b6 Compare February 4, 2020 12:18
@mike-sul
Copy link
Collaborator Author

mike-sul commented Feb 4, 2020

@lbonn Updated

@lbonn lbonn merged commit c196f1f into master Feb 4, 2020
@lbonn lbonn deleted the fix/OLPSUP-9046/handle-expired-metadata branch February 4, 2020 12:53
@simao
Copy link

simao commented Feb 5, 2020

I am not sure we should merge this. The backend is returning expired metadata, which is wrong, there is fix in sit right now for this

@mike-sul
Copy link
Collaborator Author

mike-sul commented Feb 5, 2020

I am not sure we should merge this. The backend is returning expired metadata, which is wrong, there is fix in sit right now for this

Without this fix the issue will persists even if the backend returns correctly refreshed metadata (not expired date stamp and incremented version). I emulated the correct back-end behavior by using uptane-generator utility and could reproduce the issue, changes in this PR fixes this issue. I actually managed to get exactly the same error log sequence that Carmeq reported against aktualizr version that they use and this error log sequence is kind of unique and can happen only in one specific scenario - locally stored targets metadata are expired after installation but before a device is rebooted.
This error logs sequence is (important to notice 'check' which makes it unique):

Failed to check director metadata: The targets metadata was expired. Invalid UPTANE metadata in storage.

@lbonn
Copy link
Contributor

lbonn commented Feb 5, 2020

This is a different issue than the backend one.

The scenario is:

  • aktualizr deploys a tree from valid metadata but does not reboot
  • targets metadata used for the install expire
  • the device reboots, see that some install was ongoing, check the metadata stored on the device (no interaction with backend here). Since the metadata is expired, aktualizr panics.

@simao
Copy link

simao commented Feb 5, 2020

@lbonn ah ok, my mistake.

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.

4 participants