Bugfix: Azure Linux 4 AutoByPlat issue observed in Canary(/etc/dnf/automatic.conf absent by design in Linux4) - #368
Bugfix: Azure Linux 4 AutoByPlat issue observed in Canary(/etc/dnf/automatic.conf absent by design in Linux4) #368yashnap wants to merge 13 commits into
Conversation
yashnap
changed the title
yashnap
changed the title
There was a problem hiding this comment.
Pull request overview
Updates the DNF5 auto OS update handling to support Azure Linux 4’s behavior where /etc/dnf/automatic.conf may be absent by design, by separating “default” vs “override” configuration sources and making disable/revert flows resilient to a missing override file.
Changes:
- Treat DNF5 automatic configuration as two layers: default (
/usr/share/.../automatic.conf) + override (/etc/dnf/automatic.conf), and compute effective values accordingly. - Create an override config from the default config when disabling auto OS updates if the override file doesn’t exist; on revert, remove the override file when backup indicates it was absent.
- Update unit tests and legacy env mocks to reflect the new default/override split and dependency simulation changes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/core/src/package_managers/Dnf5PackageManager.py | Implements default+override config handling, new backup keys, and override-file creation/removal during disable/revert. |
| src/core/tests/Test_Dnf5PackageManager.py | Updates tests for new backup schema and config paths; adds coverage for override removal failure. |
| src/core/tests/library/LegacyEnvLayerExtensions.py | Extends command-output mocking for dnf5 install --assumeno --skip-broken openssl used by tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup): | ||
| if self.dnf5_auto_os_update_service in image_default_patch_configuration_backup \ | ||
| and self.dnf5_automatic_download_updates_identifier_text in image_default_patch_configuration_backup[self.dnf5_auto_os_update_service] \ | ||
| and self.dnf5_automatic_apply_updates_identifier_text in image_default_patch_configuration_backup[self.dnf5_auto_os_update_service] \ | ||
| and self.dnf5_automatic_enable_on_reboot_identifier_text in image_default_patch_configuration_backup[self.dnf5_auto_os_update_service] \ | ||
| and self.dnf5_automatic_installation_state_identifier_text in image_default_patch_configuration_backup[self.dnf5_auto_os_update_service]: | ||
| self.composite_logger.log_debug("[DNF5] Extension has a valid backup for default dnf5-automatic configuration settings") | ||
| return True | ||
| else: | ||
| self.composite_logger.log_debug("[DNF5] Extension does not have a valid backup for default dnf5-automatic configuration settings") | ||
| default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key) | ||
| override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key) | ||
|
|
||
| if default_backup_valid and override_backup_valid: |
There was a problem hiding this comment.
Review this
| self.composite_logger.log_debug("[DNF5] Removing override configuration file to restore machine default.[Path={0}]".format(self.os_patch_override_configuration_settings_file_path)) | ||
| code, out = self.env_layer.run_command_output(self.dnf5_automatic_remove_override_configuration_file_cmd, False, False) | ||
|
|
||
| if code != 0: | ||
| error_msg = "[DNF5] Error removing override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out) | ||
| self.composite_logger.log_error(error_msg) | ||
| self.status_handler.add_error_to_status(error_msg, Constants.PatchOperationErrorCodes.OPERATION_FAILED) | ||
| raise Exception(error_msg, "[{0}]".format(Constants.ERROR_ADDED_TO_STATUS)) | ||
|
|
||
| self.composite_logger.log_debug("[DNF5] Removed override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out)) |
There was a problem hiding this comment.
Please review.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #368 +/- ##
==========================================
+ Coverage 94.87% 94.89% +0.01%
==========================================
Files 111 111
Lines 20855 20991 +136
==========================================
+ Hits 19787 19920 +133
- Misses 1068 1071 +3
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:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
src/core/src/package_managers/Dnf5PackageManager.py:642
- The body of is_backup_valid_for_dnf5_automatic is over-indented, which will raise an IndentationError (or change the block structure) at runtime and break the package manager module import.
def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup):
default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key)
override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key)
if default_backup_valid and override_backup_valid:
src/core/tests/Test_Dnf5PackageManager.py:487
- Leftover debug print in the test will add noise to test output and can cause brittle assertions when stdout is captured.
print("packagemanager" ,dir(package_manager))
|
|
||
| def __setup_current_auto_os_update_config(self, package_manager, config_value='', | ||
| config_file_name="automatic.conf"): | ||
| def __setup_current_auto_os_update_config(self, package_manager, config_value='', config_file_name=""): |
There was a problem hiding this comment.
Review this
There was a problem hiding this comment.
This is done. Please check my previous commit : 64846d3
| def __remove_override_configuration_if_exists(self): | ||
| """Removes dnf5-automatic override configuration file if it exists.Missing override file is valid by design, so this method must not throw | ||
| when the file is absent.""" |
There was a problem hiding this comment.
To be corrected.
| self.composite_logger.log_verbose("[DNF5] > Dependency detected: " + dependent_package_name) | ||
| self.composite_logger.log_debug("[DNF5] > Dependency detected: " + dependent_package_name) |
There was a problem hiding this comment.
Does it need to be debug (at scale)? If it's more likely to be needed on a per-customer basis (vs. cross-customer), it's ok to be verbose to improve perf.
|
|
||
| if is_backup_valid: | ||
| self.composite_logger.log_debug("[DNF5] Since extension has a valid backup, no need to log the current settings again. ""[Default Auto OS update settings={0}] [File path={1}]".format(str(image_default_patch_configuration_backup),self.image_default_patch_configuration_backup_path)) | ||
| self.composite_logger.log_debug("[DNF5] Since extension has a valid backup, no need to log the current settings again.[Default Auto OS update settings={0}] [File path={1}]".format(str(image_default_patch_configuration_backup), self.image_default_patch_configuration_backup_path)) |
There was a problem hiding this comment.
Space between "again.[Default" => "again. [Default"
Koshy John (kjohn-msft)
added
bug
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/core/src/package_managers/Dnf5PackageManager.py:641
- The body of is_backup_valid_for_dnf5_automatic is indented one level too deep, which will raise an IndentationError and prevent the module from loading. Align the method body to a single 4-space indent under the def.
def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup):
default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key)
override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key)
if default_backup_valid and override_backup_valid:
self.composite_logger.log_debug("[DNF5] Extension has a valid backup for default and override dnf5-automatic configuration settings")
return True
self.composite_logger.log_debug("[DNF5] Extension does not have a valid backup for default and override dnf5-automatic configuration settings")
return False
src/core/src/package_managers/Dnf5PackageManager.py:721
- Docstring has missing whitespace ("exists.Missing"), which reads like a typo and hurts readability. Add a space after the period.
def __remove_override_configuration_if_exists(self):
"""Removes dnf5-automatic override configuration file if it exists.Missing override file is valid by design, so this method must not throw
when the file is absent."""
src/core/src/package_managers/Dnf5PackageManager.py:732
- Override removal runs a hard-coded rm command (fixed /etc/dnf/automatic.conf). This breaks correctness when os_patch_override_configuration_settings_file_path is overridden (e.g., unit tests) and can remove the wrong file. Build the rm command from the configured override path at call time and use that for logging/error reporting.
self.composite_logger.log_debug("[DNF5] Removing override configuration file to restore machine default.[Path={0}]".format(self.os_patch_override_configuration_settings_file_path))
code, out = self.env_layer.run_command_output(self.dnf5_automatic_remove_override_configuration_file_cmd, False, False)
if code != 0:
error_msg = "[DNF5] Error removing override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out)
src/core/tests/Test_Dnf5PackageManager.py:487
- This debug print will pollute unit test output (and can interfere with any output-capture assertions). Avoid printing from tests unless the test is explicitly validating stdout/stderr; use assertions/logging via the test framework instead.
print("packagemanager" ,dir(package_manager))
src/core/tests/library/LegacyEnvLayerExtensions.py:1121
- This new mock branch matches an
install --skip-brokencommand, but Dnf5PackageManager dependency simulation usesdnf5 upgrade --assumeno ...(see single_package_upgrade_simulation_cmd). As written, this branch is effectively dead and won't support the updated test input. Align the matcher with the actual command shape used by the package manager.
elif cmd.find("sudo dnf5 install --assumeno --skip-broken openssl") > -1:
code = 0
src/core/tests/Test_Dnf5PackageManager.py:632
- This test used to exercise dependency parsing via the mocked
dnf5 upgrade --assumeno ...hyperv-daemons...output. Switching the input toopensslmeans the HappyPath env-layer mock no longer provides a dependency-simulation output for the command, so the test no longer validates dependency extraction behavior.
# Test: get_dependent_list
dependent_list = package_manager.get_dependent_list(["openssl"])
self.assertIsNotNone(dependent_list)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/core/src/package_managers/Dnf5PackageManager.py:636
is_backup_valid_for_dnf5_automatichas an extra indentation level in the method body (12 spaces instead of the usual one level). This is inconsistent with the rest of the file and can fail style/lint checks and makes future diffs harder to read.
def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup):
default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key)
override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key)
if default_backup_valid and override_backup_valid:
src/core/src/package_managers/Dnf5PackageManager.py:721
- Docstring has a missing space after the period ("exists.Missing"), which reads like a typo and reduces readability.
def __remove_override_configuration_if_exists(self):
"""Removes dnf5-automatic override configuration file if it exists.Missing override file is valid by design, so this method must not throw
when the file is absent."""
src/core/src/package_managers/Dnf5PackageManager.py:733
__remove_override_configuration_if_existsremoves a hard-coded path (/etc/dnf/automatic.conf) viadnf5_automatic_remove_override_configuration_file_cmd, but the existence check/logging usesos_patch_override_configuration_settings_file_path. If that path is overridden (e.g., unit tests or future refactors), the method can remove the wrong file.
self.composite_logger.log_debug("[DNF5] Removing override configuration file to restore machine default.[Path={0}]".format(self.os_patch_override_configuration_settings_file_path))
code, out = self.env_layer.run_command_output(self.dnf5_automatic_remove_override_configuration_file_cmd, False, False)
if code != 0:
error_msg = "[DNF5] Error removing override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out)
self.composite_logger.log_error(error_msg)
src/core/tests/Test_Dnf5PackageManager.py:487
- Leftover debug
print()in the test loop will pollute test output and make failures harder to diagnose.
print("packagemanager" ,dir(package_manager))
src/core/src/package_managers/Dnf5PackageManager.py:560
- Log message is missing a space after the period ("again.[Default"), which reads like a typo in diagnostics.
self.composite_logger.log_debug("[DNF5] Since extension has a valid backup, no need to log the current settings again.[Default Auto OS update settings={0}] [File path={1}]".format(str(image_default_patch_configuration_backup), self.image_default_patch_configuration_backup_path))
src/core/src/package_managers/Dnf5PackageManager.py:758
- Log message is missing a space after the period ("onboarding.Removing"), which reads like a typo and makes logs harder to scan.
if override_download_updates == "" and override_apply_updates == "":
self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.")
self.__remove_override_configuration_if_exists()
src/core/tests/Test_Dnf5PackageManager.py:178
- This test writes the override config before calling
disable_auto_os_update(), so it doesn’t exercise the AzL4/DNF5 scenario this PR is addressing (override file absent). Consider not creatingautomatic.confhere and asserting the code creates it from the default config.
self.runtime.write_to_file(default_config_path, dnf5_automatic_os_patch_configuration_settings)
self.runtime.write_to_file(override_config_path, dnf5_automatic_os_patch_configuration_settings)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/core/src/package_managers/Dnf5PackageManager.py:636
- The body of
is_backup_valid_for_dnf5_automaticis indented an extra level compared to the rest of the file (8 spaces instead of 4). This is inconsistent with surrounding methods and makes the code harder to read (and can trip strict style checks).
def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup):
default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key)
override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key)
if default_backup_valid and override_backup_valid:
src/core/src/package_managers/Dnf5PackageManager.py:721
- The docstring text is missing whitespace ("exists.Missing"), which reads like a typo and makes the documentation harder to parse. Consider collapsing this into a single, properly spaced sentence.
def __remove_override_configuration_if_exists(self):
"""Removes dnf5-automatic override configuration file if it exists.Missing override file is valid by design, so this method must not throw
when the file is absent."""
src/core/src/package_managers/Dnf5PackageManager.py:732
__remove_override_configuration_if_existsexecutes a hard-codedrm -f /etc/dnf/automatic.conf, which ignoresos_patch_override_configuration_settings_file_path(tests override this path, and future callers may as well). Build the command fromos_patch_override_configuration_settings_file_pathso the behavior matches the configured override path.
self.composite_logger.log_debug("[DNF5] Removing override configuration file to restore machine default.[Path={0}]".format(self.os_patch_override_configuration_settings_file_path))
code, out = self.env_layer.run_command_output(self.dnf5_automatic_remove_override_configuration_file_cmd, False, False)
if code != 0:
error_msg = "[DNF5] Error removing override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out)
src/core/tests/Test_Dnf5PackageManager.py:487
- Debug output
print("packagemanager", dir(package_manager))will pollute test logs and can make failures harder to read. Tests should avoid unconditional prints.
print("packagemanager" ,dir(package_manager))
src/core/tests/Test_Dnf5PackageManager.py:80
__setup_current_auto_os_update_configtakesconfig_file_namebut never uses it, which is misleading and makes the helper harder to understand/extend. Either use the parameter or remove it from the signature.
def __setup_current_auto_os_update_config(self, package_manager, config_value='', config_file_name=""):
# setup current auto OS update config
default_config_path = os.path.join(self.runtime.execution_config.config_folder, "default_automatic.conf")
override_config_path = os.path.join(self.runtime.execution_config.config_folder, "automatic.conf")
package_manager.dnf5_automatic_default_configuration_file_path = default_config_path
package_manager.dnf5_automatic_override_configuration_file_path = override_config_path
self.runtime.write_to_file(default_config_path, config_value)
self.runtime.write_to_file(override_config_path, config_value)
src/core/src/package_managers/Dnf5PackageManager.py:758
- Log message is missing whitespace ("onboarding.Removing"), which looks like a typo in logs and makes them harder to scan/search.
if override_download_updates == "" and override_apply_updates == "":
self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.")
self.__remove_override_configuration_if_exists()
src/core/src/package_managers/Dnf5PackageManager.py:759
- The new branch that removes the override file when the backup indicates it originally didn’t exist is not validated by unit tests for the successful path (only the failure path is covered). Adding a test that mocks a successful remove and asserts the override file is gone would prevent regressions here.
# Empty values indicate override file did not exist before onboarding.
if override_download_updates == "" and override_apply_updates == "":
self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.")
self.__remove_override_configuration_if_exists()
return
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/core/src/package_managers/Dnf5PackageManager.py:636
is_backup_valid_for_dnf5_automatic’s body is indented more than the surrounding methods, which breaks readability and can trip style/lint checks. Re-indent the method body to match the file’s standard 4-space indentation level inside methods.
def is_backup_valid_for_dnf5_automatic(self, image_default_patch_configuration_backup):
default_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_default_configuration_backup_key)
override_backup_valid = self.__is_backup_valid(image_default_patch_configuration_backup, self.os_patch_override_configuration_backup_key)
if default_backup_valid and override_backup_valid:
src/core/src/package_managers/Dnf5PackageManager.py:721
- Docstring has a missing space after the period ("exists.Missing"), which reads like a typo and makes the documentation harder to read.
def __remove_override_configuration_if_exists(self):
"""Removes dnf5-automatic override configuration file if it exists.Missing override file is valid by design, so this method must not throw
when the file is absent."""
src/core/src/package_managers/Dnf5PackageManager.py:732
- Override config removal runs a hard-coded command (
rm -f /etc/dnf/automatic.conf). This makes the method inconsistent withos_patch_override_configuration_settings_file_pathand breaks unit-testability when the override path is redirected. Build the remove command fromos_patch_override_configuration_settings_file_pathat call time and use it for logging/errors.
self.composite_logger.log_debug("[DNF5] Removing override configuration file to restore machine default.[Path={0}]".format(self.os_patch_override_configuration_settings_file_path))
code, out = self.env_layer.run_command_output(self.dnf5_automatic_remove_override_configuration_file_cmd, False, False)
if code != 0:
error_msg = "[DNF5] Error removing override configuration file. [Command={0}][Code={1}][Output={2}]".format(self.dnf5_automatic_remove_override_configuration_file_cmd, str(code), out)
src/core/tests/Test_Dnf5PackageManager.py:487
- Leftover debug
print(dir(package_manager))will add noise to test output and can make failures harder to read. Remove it.
print("packagemanager" ,dir(package_manager))
src/core/src/package_managers/Dnf5PackageManager.py:757
- Log message is missing a space after the period ("onboarding.Removing"), which reads like a typo.
self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.")
src/core/src/package_managers/Dnf5PackageManager.py:509
- New behavior ensures the override config is created from the default when missing. There is no unit test covering the case where the override file is absent (the main regression scenario described in the PR) to verify: (1) backup captures empty override values, (2) override file gets created, and (3) settings are updated without throwing.
self.composite_logger.log_verbose("[DNF5] Preemptively disabling auto OS updates using dnf5-automatic")
# Check if override.conf file exists, if not copy/create from default config location
self.__ensure_override_configuration_exists()
self.update_os_patch_configuration_sub_setting(self.download_updates_identifier_text, "no", self.dnf5_automatic_config_pattern_match_text)
self.update_os_patch_configuration_sub_setting(self.apply_updates_identifier_text, "no", self.dnf5_automatic_config_pattern_match_text)
src/core/tests/Test_Dnf5PackageManager.py:120
__assert_reverted_automatic_patch_configuration_settingsreads frompackage_manager.dnf5_automatic_default_configuration_file_path, which can point at real system paths (e.g./usr/share/...) in testcases that don’t call__setup_current_auto_os_update_config. This makes the test non-hermetic and can fail depending on the build agent’s installed packages. Prefer asserting against the sandbox config paths underruntime.execution_config.config_folder.
def __assert_reverted_automatic_patch_configuration_settings(self, package_manager, config_exists=True, config_value_expected=''):
reverted_dnf5_automatic_patch_configuration_settings = self.runtime.env_layer.file_system.read_with_retry(
package_manager.dnf5_automatic_default_configuration_file_path, raise_if_not_found=False)
if config_exists:
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/core/tests/Test_Dnf5PackageManager.py:487
- Debug print left in the test loop will pollute test output and can interfere with assertions that capture stdout. It should be removed.
print("packagemanager" ,dir(package_manager))
src/core/tests/Test_Dnf5PackageManager.py:124
- This helper only reads/asserts the default dnf5 config file and never validates the override config file’s existence/content. Since this PR adds restore/remove behavior for the override file, these tests can pass even if override restore/removal is broken. Add assertions for the override file (and pass expected override behavior/values from the testcase).
def __assert_reverted_automatic_patch_configuration_settings(self, package_manager, config_exists=True, config_value_expected=''):
reverted_dnf5_automatic_patch_configuration_settings = self.runtime.env_layer.file_system.read_with_retry(
package_manager.dnf5_automatic_default_configuration_file_path, raise_if_not_found=False)
if config_exists:
self.assertIsNotNone(reverted_dnf5_automatic_patch_configuration_settings)
self.assertEqual(config_value_expected, reverted_dnf5_automatic_patch_configuration_settings)
else:
self.assertIsNone(reverted_dnf5_automatic_patch_configuration_settings)
src/core/src/package_managers/Dnf5PackageManager.py:759
- Log message is missing a space after the period ("onboarding.Removing"), which reads like a typo and makes logs harder to scan/search.
# Empty values indicate override file did not exist before onboarding.
if override_download_updates == "" and override_apply_updates == "":
self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.")
self.__remove_override_configuration_if_exists()
| def __restore_override_configuration_from_backup(self, override_backup): | ||
| """Restore override dnf5-automatic configuration to its backed up state.""" | ||
| override_download_updates = override_backup[self.download_updates_identifier_text] | ||
| override_apply_updates = override_backup[self.apply_updates_identifier_text] | ||
|
|
||
| # Empty values indicate override file did not exist before onboarding. | ||
| if override_download_updates == "" and override_apply_updates == "": | ||
| self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.") | ||
| self.__remove_override_configuration_if_exists() | ||
| return |
There was a problem hiding this comment.
Review this
There was a problem hiding this comment.
Current Behavior
During onboarding, we read the override config file to back up its state:
• If the override file doesn't exist, get_config_values returns ("", "") for download_updates and apply_updates. These empty strings get stored in the backup JSON.
• If the override file exists, we parse it line-by-line looking for download_updates and apply_updates. If found, we store their values. If not found, they remain "" — the same empty strings.
During revert, restore_override_configuration_from_backup checks:
if override_download_updates == "" and override_apply_updates == "":
# "Override file did not exist before onboarding" → delete it
This works correctly when:
• Override file didn't exist → backed up ("", "") → revert deletes the override file the extension created
• Override file existed with both settings defined → backed up ("yes", "no") → revert restores those values
Copilot is complaining about the below use case:
Override file existed but didn't define download_updates/apply_updates → backed up ("", "") → revert thinks the file never existed and deletes it, losing any other user settings in that file
Note: Override file is created by the code and not present by design. The download/apply updates identifier would be present since it is directly copying from default config.
The Question
Is that broken scenario realistic? Customer might change the values (e.g., yes → no or empty -> yes/no) but would be very unlikely to delete those keys entirely from the file.?
This is for my understanding so I can determine whether the fix is needed or not. ( Differentiate between no file at all vs no key -> None) .
There was a problem hiding this comment.
Perhaps we could save the entire state of the override file to restore it to its original state, rather than just those two settings?
There was a problem hiding this comment.
That is one way to do it but we want to keep LPE light-weight and storing full content would bloat the backup unnecessarily. Ther current design of storing only specific settings is consistent across the codebase.
If the scenario I mentioned is indeed a valid/possible scenario then the fix I am suggesting is just store None as their values instead of "". It will be able to differentiate between file exists/content exist without increasing storage space. I am open to thoughts/suggestions from others.
There was a problem hiding this comment.
None vs empty string makes sense in that case. But yeah, I don't know if the scenario is something that is likely. Might be reasonable to guard against it even if we don't expect it to happen
Similar Issue and Fix : #374 Its weird because I see failure in one of my PR run : #368 <img width="987" height="352" alt="UTFIx_1" src="https://github.com/user-attachments/assets/74e2aedf-325f-4ec6-beb7-238ff4152de5" /> and the 2nd one looks fine after the same rebase : #359 The addition of credential sanitizer has started exercising new paths which is reducing the coverage from base branch. - I've removed the test that were marked to skip on GitHub because they kept failing according to this PR : #129 . ( No comment was mentioned about putting it back or reasoning either) - Added sleep time 20s and 30s each for update time use-case which was failing on assertion due to time not getting reflected or writes happening before/around the same time. Dont see any failures at the moment with the github tests that were failing earlier.
|
UT is failing due to sleep timeout not being enough for one of the test that was being enabled. I am fixing this as a part of another PR |
| str(patch_configuration_sub_setting), value)) | ||
| os_patch_configuration_settings = self.env_layer.file_system.read_with_retry(self.os_patch_configuration_settings_file_path) | ||
| if config_file_path is None: | ||
| config_file_path = self.os_patch_override_configuration_settings_file_path |
There was a problem hiding this comment.
so we use the override by default if the config_file_path is not included?
There was a problem hiding this comment.
The idea is that once we create the override file, we only use that until the customers decide to offboard. Meaning we don't touch or update the default file values and only use host specific overrides. Hence the default. This is specifically used when this method is called from disabled_auto_os_update. (Disable OS update use case)
During revert/offboard, we use the proper file path parameter.
There was a problem hiding this comment.
Got it. Thanks for the context
|
|
||
| # Empty values indicate override file did not exist before onboarding. | ||
| if override_download_updates == "" and override_apply_updates == "": | ||
| self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.") |
There was a problem hiding this comment.
Missing a space after period. "onboarding.Removing" -> "onboarding. Removing"
| def __restore_override_configuration_from_backup(self, override_backup): | ||
| """Restore override dnf5-automatic configuration to its backed up state.""" | ||
| override_download_updates = override_backup[self.download_updates_identifier_text] | ||
| override_apply_updates = override_backup[self.apply_updates_identifier_text] | ||
|
|
||
| # Empty values indicate override file did not exist before onboarding. | ||
| if override_download_updates == "" and override_apply_updates == "": | ||
| self.composite_logger.log_debug("[DNF5] Override dnf5-automatic configuration did not exist before onboarding.Removing override configuration file if it exists.") | ||
| self.__remove_override_configuration_if_exists() | ||
| return |
There was a problem hiding this comment.
Perhaps we could save the entire state of the override file to restore it to its original state, rather than just those two settings?
| raise Exception(error_msg, "[{0}]".format(Constants.ERROR_ADDED_TO_STATUS)) | ||
| else: | ||
| self.composite_logger.log_debug("[DNF5] Enabled auto update on reboot. [Command={0}][Code={1}][Output={2}]".format(command, str(code),out)) | ||
| self.composite_logger.log_debug("[DNF5] Enabled auto update on reboot.[Command={0}][Code={1}][Output={2}]".format(command, str(code), out)) |
There was a problem hiding this comment.
Spacing comment here too.
REPRO STEPS:
Provision an Azure Linux 4.0 (AzL4) VM with DNF5
Install or verify DNF5 automatic plugin: rpm -qa | grep dnf5-plugin-automatic
Enable machine default auto updates using timer:
sudo systemctl enable --now dnf5-automatic.timer
Verify timer is enabled:
systemctl is-enabled dnf5-automatic.timer -> ENABLED
No configuration file exist yet :
/etc/dnf/automatic.conf
Run Assess/Install Patch from Azure Portal.
Expected: LPE should read current state which is enabled, disable it, perform the task and leave it in disabled state
Actual: LPE correctly Detects the current state of the system but when trying to disable, it runs into File not found error since /etc/dnf/automatic.conf is not automatically created when service is enabled in Azl4 (DNF5).
SOLUTION
According to their DNF5 doc (Automatic Command — dnf5 documentation), it uses 2 configuration files.
/usr/share/dnf5/dnf5-plugins/automatic.conf contains the default values and should be available once dnf5 is installed
/etc/dnf/automatic.conf contains Host-specific overrides and may not be available/created during dnf5 installation. Has to be explicitly created
They advise using /etc/dnf/automatic.conf to customize configurations for the automatic service.
Sequence of steps for disabling machine default OS updates should be:
3.Check if service is installed
If not, do nothing
If installed, pre-emptively disable it
For this, if the override file does not exist, create a copy from default file and set apply_updates and download_updates to false in the override. DO NOT modify the default file
Now to revert auto OS update to machine default OS update on when AutoPatching is disabled:
If installed, log current state
Read backup file and revert to the state it contains. i.e. if override file did not exist before, remove it. If values in default config were different to what we had logged in backup, modify default config to backup values.
Since override config does not always exist, code does not throw an exception for something that is by design
TESTING
ARM ID : /subscriptions/6acc8a91-e2b0-4041-a069-c2932ab42fd9/resourceGroups/azl4-rg-canary/providers/Microsoft.Compute/virtualMachines/linux4-auto-os
No service Installed
no_service_installed.log
Service instal
service_installed_no_timer.log
led but timer not enabled
Service inst
service_installed_yes_timer.log
alled and timer enabled
Idempotent call checking if backup
revert_idempotent.log
is not overriden/recreated
Revert to Default( Offboarding from GuestPatching) and service installed
revert_service_installed.log
Idempotent call after revert
revert_idempotent.log
Revert back ( Onboard again to Guest Patching)
revert_to_Azure.log