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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #368 +/- ##
==========================================
+ Coverage 94.98% 94.99% +0.01%
==========================================
Files 113 113
Lines 21955 22139 +184
==========================================
+ Hits 20855 21032 +177
- Misses 1100 1107 +7
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))
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_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.
These are the possible scenarios when a customer onboards to AutoByPlat :
- Override config does not exist -> we create one with apply and download updates set to 'no'
- Override config exists with apply and/or download updates -> we update these values in it to "no"
- Override config exists but does not contain apply and download updates -> we create a new override config and write apply and download updates in it.
3 is a problematic scenario, where even during onboarding we have overridden a customer's config and have no way to recover it later.
Please re-evaluate and consider this in your design
There was a problem hiding this comment.
Make sense, Thanks for the explanation.
I've implemented a solution to cover all the use-cases mentioned above:
Fix: Added an explicit override_file_exists boolean to the override backup block (captured before onboarding modifies anything). Revert now uses this flag plus per-key logic, so it never deletes a pre-existing file. Also trimmed the redundant enable_on_reboot/installation_state from the override block (leaner schema), with is_backup_valid updated to validate each block's own keys.
| # | Onboarding Scenario | What Revert Does | End Result |
|---|---|---|---|
| 1 | Override file did not exist (extension created one with download_updates=no and apply_updates=no) |
override_file_exists=false → remove the override file created by the extension |
Override file deleted. Machine returns to default state with no override file present. |
| 2 | Override file already existed and contained both download_updates and apply_updates keys (extension modified their values) |
override_file_exists=true and backed-up values exist → restore each key to its original backed-up value |
Override file preserved. Original download_updates and apply_updates values restored. |
| 3 | Override file already existed, but download_updates and apply_updates keys were absent while other customer configuration was present (extension appended the two keys) |
override_file_exists=true and backed-up values are empty ("") → remove only the two keys appended by the extension |
Override file preserved. Customer's existing configuration remains intact, and only the extension-added keys are removed. |
Overall: A pre-existing customer override file is never deleted; it's always returned to its exact pre-onboarding state (restored values or removed appended keys), and only extension-created files are removed. Covered by 3 unit tests in Test_Dnf5PackageManager.py
There was a problem hiding this comment.
The distinction comes from the backup file.
1.When file present and identifier present, download/apply updates will have yes/no value
2. When file present and identifier absent, download/apply values will be ""
3. When backup file exist, copy from default and updates the values to no
There was a problem hiding this comment.
yashnap For use case 1, when the override file did not exist before and LPE created it, deletion still could be an extreme step with consequences. What if the customer added some other configs to the override file while the VM was onboarded to AutoByPlat? Upon reversion when we delete the file it still leads to us deleting customer config which we were never a part of.
We should only be responsible for the configs we use and nothing else. Simply setting download-updates and apply-updates values to no should work in for both usecase 1 and 3. I believe that is what we do in other package managers, do verify
There was a problem hiding this comment.
Okay, it that use-case is possible then yes it make sense to keep the file even if it wasnt present and just remove our added keys.
I've updated the code to have both use case 1 and 3 to have values "no". Use case 2 stays intact. Now it will just revert to the file as it was or the keys values without deleting it
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 |
| 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?
Rajasi Rane (rane-rajasi)
left a comment
Rajasi Rane (rane-rajasi)
left a comment
There was a problem hiding this comment.
Comments inline
| and self.dnf5_automatic_enable_on_reboot_identifier_text in image_default_patch_configuration_backup[backup_key] | ||
| and self.dnf5_automatic_installation_state_identifier_text in image_default_patch_configuration_backup[backup_key]) | ||
|
|
||
| def update_os_patch_configuration_sub_setting(self, patch_configuration_sub_setting, value="no", config_pattern_match_text="", config_file_path=None): |
There was a problem hiding this comment.
This is a method override of one declared in PackageManager.py (parent). While it works here because config_file_path has a default value, be careful and use new parameters in overridden methods only when necessary
There was a problem hiding this comment.
Review once if this is the only way or if there is an alternative available
There was a problem hiding this comment.
I've updated the code to keep the override method intact(no new parameters). Added a new private method instead that it calls internally to pass the config value and updated code to use the new private method where we were passing the config file directly.
| self.download_updates_identifier_text: override_download_updates_value, | ||
| self.apply_updates_identifier_text: override_apply_updates_value, | ||
| self.enable_on_reboot_identifier_text: enable_on_reboot_value, | ||
| self.installation_state_identifier_text: is_service_installed |
There was a problem hiding this comment.
Do the values for enable on reboot and installation state differ between default and configuration files? If no, there could maybe be a different solution to use here. This could also be used to solve the issue of unwanted removal of override config when download and apply updates values are "": https://github.com/Azure/LinuxPatchExtension/pull/368/changes#r4020532629
There was a problem hiding this comment.
No, they are the same. Please refer to my latest code changes. (i.e #368 (comment) )
| 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.
yashnap The concern raised by Co-pilot is a valid and possible scenario. On override file does not exist today, in a VM you are creating from say a default template, but this environment won't always be true for all customers. Customers have their own configurations and keep changing those too. So,
- it is possible for an override file to already exist when the customer onboards to our service for the very first time.
- It is also possible for a pre-existing override config to have "" as a value for both apply updates and download updates.
- This config is not restricted to only have the 2 configs: apply updates and download updates. It can contain other configurations.
In a scenario where all of these 3 points apply, this code will log "" as override apply update and download updates in our backup file. And on reversion, it will delete the override config because these values being "" makes the code assume that an override file did not exist. This will be a bug where we interfere with a customer environment that did not have prior approval
| 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.
These are the possible scenarios when a customer onboards to AutoByPlat :
- Override config does not exist -> we create one with apply and download updates set to 'no'
- Override config exists with apply and/or download updates -> we update these values in it to "no"
- Override config exists but does not contain apply and download updates -> we create a new override config and write apply and download updates in it.
3 is a problematic scenario, where even during onboarding we have overridden a customer's config and have no way to recover it later.
Please re-evaluate and consider this in your design
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| default_backup = image_default_patch_configuration_backup[self.os_patch_default_configuration_backup_key] | ||
| override_backup = image_default_patch_configuration_backup[self.os_patch_override_configuration_backup_key] | ||
|
|
||
| self.__restore_default_configuration_from_backup(default_backup) |
There was a problem hiding this comment.
If we are never making any changes to the default configuration file, what is the need to restore it here?
There was a problem hiding this comment.
Right, since we only read from that file, we dont need this restore. Removed it
|
|
||
| config = self.__read_override_config(override_path) | ||
| if not config: | ||
| self.composite_logger.log_warning("[DNF5] Could not read existing override configuration; skipping restore to avoid overwriting the file. [Path={0}]".format(override_path)) |
There was a problem hiding this comment.
This should at the very least be logged as an error in status blob since this is LPE failing in it's contract of returning the machine's state to what it was before
| self.composite_logger.log_warning("[DNF5] Could not read existing override configuration; skipping restore to avoid overwriting the file. [Path={0}]".format(override_path)) | ||
| return | ||
|
|
||
| self.__apply_override_setting(config, self.download_updates_identifier_text, override_download_updates) |
There was a problem hiding this comment.
Why not use update_os_patch_configuration_sub_setting() here?
There was a problem hiding this comment.
Since, we are calling this method twice for each key and iterating it multiple times, the new mthods were added to have it single pass.
To address this comment : #368 (comment)
| 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.
yashnap For use case 1, when the override file did not exist before and LPE created it, deletion still could be an extreme step with consequences. What if the customer added some other configs to the override file while the VM was onboarded to AutoByPlat? Upon reversion when we delete the file it still leads to us deleting customer config which we were never a part of.
We should only be responsible for the configs we use and nothing else. Simply setting download-updates and apply-updates values to no should work in for both usecase 1 and 3. I believe that is what we do in other package managers, do verify
| self.composite_logger.log_debug("[DNF5] Restoring default dnf5-automatic configuration values from backup.[Path={0}][download_updates={1}][apply_updates={2}]" | ||
| .format(self.os_patch_default_configuration_settings_file_path, str(default_download_updates), str(default_apply_updates))) | ||
|
|
||
| self.__update_os_patch_configuration_sub_setting(self.download_updates_identifier_text, default_download_updates, self.auto_update_config_pattern_match_text, self.os_patch_default_configuration_settings_file_path) |
There was a problem hiding this comment.
This could be a potential risk since __update_os_patch_configuration_sub_setting() adds the config if it doesn't exist in file, which means we end up updating default config (tying to my other comment on default config)
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
NEW LOGS AFTER CODE CHANGES( SEPT 21st, 2026)
No service installed
9.core_no_service_installed.log
Service installed but no timer enabled
10.core_service_installed_no_timer.log
Service installed + timer enabled
13.core_service_timer_enabled.log
REVERT USE CASES
Initial state
Patch Orchestration : Changed from ImageDefault -> Azure Managed
Override file : absent
33.core.nofile_guestPatch_log.txt
Patch Orchestration Updated : ImageDefault
Result : Since Override file didnt exist when customer onboarded, it was deleted.
35.core.reverted_file_deleted_log.txt
Initial State : Changed from ImageDefault -> Azure Managed
Override file : Present with download_updates/apple_updates values present ( Manually created file before onboarding to azure)
21.core.fileexiists_values_present.log
Patch Orchestration updates : ImageDefault
Result : Override file not deleted. Only the values that were updated were reverted back to its original values
24.core.revert_override_values_yes.log
Initial State : Changed from ImageDefault -> Azure Managed
Override file : Present but no download_updates/apply_updates identifier. ( Manually created file before onboarding to azure)
Code appends download_updates/ apple_updates =no
26.core.overide_yes_identifer_no_onboard.log
Patch orchestration updates : Image Default
Results : Overrride file not deleted. Only the newly appended 2 values were removed to restore original state.
28.core.reverted_26.log
Logs after the change : September 24th
44.core.log
Logs after refactor ( October 2nd, 2026)
Patch Orchestration : Changed from ImageDefault -> Azure Managed
Override file : absent
[48.core.no_override_file_exists.onboard.log](https://github.com/user-attachments/files/32972405/48.core.no_override_file_exists.onboard.
After deboarding: File is intact ( no deletion)
49.core.deboard_48.log
log)
Override file ; present with both download/apply updates identifier and values
50.core.override_with_values.log
Deboard : File is intact, values restored to original from backup
51.core.revert.51.log
Overrride_file : Present but download/apply updates keys not present
52.core.overrde_with_no_keys.log
Deboard: File is intact, with both identifier deleted
54.core.revert.52.log