Feature: [Ubuntu, no-CVM] Adding reboot, hibernate checks and aligning all pre-reqs together#363
Open
rane-rajasi wants to merge 3 commits into
Open
Feature: [Ubuntu, no-CVM] Adding reboot, hibernate checks and aligning all pre-reqs together#363rane-rajasi wants to merge 3 commits into
rane-rajasi wants to merge 3 commits into
Conversation
…g all pre-reqs together
rane-rajasi
requested review from
a team,
kjohn-msft,
michellemcdaniel and
najams
and removed request for
a team
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #363 +/- ##
==========================================
- Coverage 94.08% 94.05% -0.04%
==========================================
Files 109 109
Lines 20099 20351 +252
==========================================
+ Hits 18911 19141 +230
- Misses 1188 1210 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the UEFI certificate update flow (used during default patching) by enforcing additional pre-requisites (CVM exclusion, hibernation-off, latest-certs check, and uptime-based reboot handling) and updating unit tests accordingly.
Changes:
- Adds prerequisite gating in
PatchInstallerbefore attempting UEFI certificate updates, including reboot/hibernate/latest-certs checks. - Adds Apt-specific implementations for uptime and hibernation detection to support the new prerequisites.
- Expands test coverage to validate prerequisite branching and status/error recording behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/src/core_logic/PatchInstaller.py | Adds certificate-update prerequisite checks and records status/errors for prerequisite failures and update failures. |
| src/core/src/package_managers/AptitudePackageManager.py | Implements uptime-based reboot check and hibernation-state detection for cert-update prerequisites. |
| src/core/src/package_managers/PackageManager.py | Refactors certificate update entrypoints and adds shared mokutil availability gating for cert checks. |
| src/core/src/package_managers/YumPackageManager.py | Adds stub implementations for new certificate prerequisite APIs. |
| src/core/src/package_managers/TdnfPackageManager.py | Adds stub implementations for new certificate prerequisite APIs. |
| src/core/src/package_managers/ZypperPackageManager.py | Adds stub implementations for new certificate prerequisite APIs. |
| src/core/src/package_managers/Dnf5PackageManager.py | Adds stub implementations for new certificate prerequisite APIs. |
| src/core/tests/Test_PatchInstaller.py | Adds/updates tests to validate prerequisite ordering and installation-status error recording. |
| src/core/tests/Test_AptitudePackageManager.py | Adds/updates tests for mokutil/cert-check behavior and uptime-based reboot decisioning. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.status_handler.add_error_to_status(error_msg, Constants.PatchOperationErrorCodes.CERTIFICATE_UPDATE) | ||
| raise Exception(error_msg, "[{0}]".format(Constants.ERROR_ADDED_TO_STATUS)) | ||
| def ensure_mokutil_available_for_cert_checks(self): | ||
| # type: (bool) -> bool |
Comment on lines
531
to
+534
| def are_latest_certs_present(self): | ||
| if not self.ensure_mokutil_available_for_cert_checks(): | ||
| return False | ||
|
|
Comment on lines
+984
to
+986
| if code != self.apt_exitcode_ok: | ||
| self.composite_logger.log_debug('[APM][Certs] Unable to determine hibernation state for cert update. Assuming disabled. [Code={0}][Output={1}]'.format(str(code), out)) | ||
| return False |
| @@ -1130,5 +1130,13 @@ def try_install_mokutil(self): | |||
| def try_update_certs(self): | |||
| """ Attempts to update certificate status """ | |||
| pass | |||
| @@ -773,5 +773,13 @@ def try_install_mokutil(self): | |||
| def try_update_certs(self): | |||
| """ Attempts to update certificate status """ | |||
| pass | |||
| @@ -882,5 +882,13 @@ def try_install_mokutil(self): | |||
| def try_update_certs(self): | |||
| """ Attempts to update certificate status """ | |||
| pass | |||
| @@ -718,4 +718,12 @@ def try_install_mokutil(self): | |||
| def try_update_certs(self): | |||
| """ Attempts to update certificate status """ | |||
| pass | |||
kjohn-msft
reviewed
Jul 10, 2026
| try: | ||
| is_confidential_vm, detection_details = self.env_layer.detect_confidential_vm() | ||
| except Exception as e: | ||
| self.composite_logger.log_warning("Unable to determine whether the VM is a Confidential VM before attempting the UEFI certificate update. Continuing with patch installation... [Error: {0}]".format(str(e))) |
Collaborator
There was a problem hiding this comment.
Code works, but log is ambiguous (true, when non-determinable).
And the function name thus becomes misleading. What you can only know for sure then is is_definitely_not_cvm.
If it's cvm, false, if non-determinable, false. And log for reason why it's false should be clear.
Calling function then handles the additional negation appropriately.
kjohn-msft
reviewed
Jul 10, 2026
Comment on lines
+105
to
+107
| self.get_uptime_seconds_cmd = "cat /proc/uptime" | ||
| self.get_hibernation_state_cmd = "cat /sys/power/disk" | ||
| self.pre_cert_reboot_uptime_threshold_seconds = 7 * 24 * 60 * 60 |
Collaborator
There was a problem hiding this comment.
Not for this PR - these aren't specific to this class. Ignore for the PR but note on what needs to be done later.
kjohn-msft
reviewed
Jul 10, 2026
Comment on lines
+963
to
+974
| def should_reboot_before_cert_update(self): | ||
| # type: () -> bool | ||
| """Return True when certificate update should be preceded by a reboot.""" | ||
| uptime_seconds = self.__get_vm_uptime_seconds() | ||
| if uptime_seconds is None: | ||
| # Best-effort check: do not block cert flow if uptime cannot be determined. | ||
| return False | ||
|
|
||
| requires_reboot = uptime_seconds > self.pre_cert_reboot_uptime_threshold_seconds | ||
| self.composite_logger.log_debug("[APM][Certs] Pre-cert reboot check. [UptimeInSeconds={0}][ThresholdInSeconds={1}][RequiresReboot={2}]" | ||
| .format(str(uptime_seconds), str(self.pre_cert_reboot_uptime_threshold_seconds), str(requires_reboot))) | ||
| return requires_reboot |
Collaborator
There was a problem hiding this comment.
There is a specific reason why this function exists - the nuance for why is lost in comments (or absence thereof).
|
|
||
| def can_continue_cert_update_after_reboot_check(self): | ||
| # type: () -> bool | ||
| """Attempts pre-cert reboot when required. |
There was a problem hiding this comment.
I'm not sure if it's the naming of this function or something else, but it feels like this function is doing too many things: saying it's ok to do the cert update and also doing the reboot. I'd prefer if it was either made more clear in the function name that this will attempt to perform an update if required or to have the reboot side effect moved to a different function.
kjohn-msft
requested changes
Jul 10, 2026
kjohn-msft
left a comment
kjohn-msft
left a comment
Collaborator
There was a problem hiding this comment.
Review code coverage + minor comments
| return self.package_install_expected_avg_time_in_seconds | ||
|
|
||
| # region Update certificates in factory defaults | ||
| def should_reboot_before_cert_update(self): |
There was a problem hiding this comment.
Wondering if this could be renamed to "is_reboot_required_before_cert_update"
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| if cmd.find(self.runtime.package_manager.fwupd_refresh_cmd) > -1: | ||
| return 1, "Error" | ||
| return 0, "" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.fwupd_refresh_cmd) > -1) |
| return True | ||
|
|
||
| def mock_run_command_output_uptime_above_threshold(self, cmd, no_output=False, chk_err=True): | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_uptime_seconds_cmd) > -1) |
| return 0, "700000.12 123.45" | ||
|
|
||
| def mock_run_command_output_uptime_below_threshold(self, cmd, no_output=False, chk_err=True): | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_uptime_seconds_cmd) > -1) |
|
|
||
| def mock_run_command_output_uptime_cmd_fails(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate uptime command failure""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_uptime_seconds_cmd) > -1) |
|
|
||
| def mock_run_command_output_uptime_empty_output(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate uptime command returning empty output""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_uptime_seconds_cmd) > -1) |
|
|
||
| def mock_run_command_output_uptime_invalid_output(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate uptime command returning unparseable output""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_uptime_seconds_cmd) > -1) |
|
|
||
| def mock_run_command_output_hibernation_enabled(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate hibernation enabled state""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_hibernation_state_cmd) > -1) |
|
|
||
| def mock_run_command_output_hibernation_disabled(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate hibernation disabled state""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_hibernation_state_cmd) > -1) |
|
|
||
| def mock_run_command_output_hibernation_cmd_fails(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate hibernation check command failure""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_hibernation_state_cmd) > -1) |
|
|
||
| def mock_run_command_output_hibernation_empty_output(self, cmd, no_output=False, chk_err=True): | ||
| """Mock run_command_output to simulate empty output for hibernation check""" | ||
| self.assertTrue(cmd.find(self.runtime.package_manager.get_hibernation_state_cmd) > -1) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR defines all pre-requisites for updating certificates in Canonical VMs, some of which were already existing and are now re-aligned in appropriate sections and some are added as new.
The pre-reqs for certificate updates are:
Pending: