Feature: Adding Rhel10 Base Support using dnf4 package manager - #359
Feature: Adding Rhel10 Base Support using dnf4 package manager#359yashnap wants to merge 23 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #359 +/- ##
==========================================
+ Coverage 94.87% 94.94% +0.06%
==========================================
Files 111 113 +2
Lines 20855 21830 +975
==========================================
+ Hits 19787 20727 +940
- Misses 1068 1103 +35
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
Adds RHEL 10 support to the LinuxPatchExtension by introducing a DNF4-based package manager implementation and wiring it into package-manager detection and configuration, along with test/mocking updates to cover RHEL10 scenarios.
Changes:
- Introduces
Dnf4PackageManagerwith update discovery, dependency parsing, reboot detection, and auto-OS-update disable/revert logic. - Updates
EnvLayer+ConfigurationFactoryto detect and instantiate the new DNF4 flow on RHEL 10. - Adds/updates unit tests and legacy env-layer command mocks to simulate DNF4/RHEL10 behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/references/cmd_output_references/dnf4_ouput_expected_formats | Adds reference examples of DNF4 output formats used by parsers/tests. |
| src/core/tests/Test_EnvLayer.py | Updates env-layer tests/mocks to reflect RHEL10 detection and DNF version probing. |
| src/core/tests/Test_Dnf4PackageManager.py | Adds a new test suite for DNF4 behavior (repo refresh, dependency simulation, auto OS update config, etc.). |
| src/core/tests/Test_CoreMain.py | Adds an autopatching test covering RHEL10 + DNF4 behavior. |
| src/core/tests/library/LegacyEnvLayerExtensions.py | Extends the legacy command-output mocking to emulate DNF4 outputs and systemctl/rpm behaviors. |
| src/core/src/package_managers/Dnf4PackageManager.py | New package-manager implementation for DNF4/RHEL10. |
| src/core/src/bootstrap/EnvLayer.py | Adds RHEL10 path to select DNF4 based on dnf --version. |
| src/core/src/bootstrap/Constants.py | Adds Constants.DNF4. |
| src/core/src/bootstrap/ConfigurationFactory.py | Wires DNF4 into DI configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'apt_dev_config': self.new_dev_configuration(Constants.APT, AptitudePackageManager), | ||
| 'dnf4_dev_config': self.new_prod_configuration(Constants.DNF4, Dnf4PackageManager), | ||
| 'dnf5_dev_config': self.new_dev_configuration(Constants.DNF5, Dnf5PackageManager), |
| 'apt_test_config': self.new_test_configuration(Constants.APT, AptitudePackageManager), | ||
| 'dnf4_test_config': self.new_prod_configuration(Constants.DNF4, Dnf4PackageManager), | ||
| 'dnf5_test_config': self.new_test_configuration(Constants.DNF5, Dnf5PackageManager), |
| def validate_dnf4_output(self, output): | ||
| for failure_text in self.dnf4_subscription_failure_texts: | ||
| if failure_text in output: | ||
| self.composite_logger.log_error("[DNF4] Subscription/entitlement failure detected. [{0}]".format(failure_text)) | ||
| raise Exception("System is not properly registered with subscription service.") |
| elif cmd.find("systemctl") > -1: | ||
| code = 1 | ||
| output = '' | ||
| elif self.legacy_package_manager_name is Constants.DNF4: |
| self.assertEqual(len(available_updates), 0) | ||
| self.assertEqual(len(package_versions), 0) | ||
|
|
||
| def test_install_package_failure(self): |
| # Restart not required (needs-restarting returns code=0) | ||
| self.runtime.set_legacy_test_type('SadPath') | ||
| self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot | ||
| self.assertFalse(package_manager.is_reboot_pending()) |
Michelle McDaniel (michellemcdaniel)
left a comment
Michelle McDaniel (michellemcdaniel)
left a comment
There was a problem hiding this comment.
As a general note, you have varying spacing in some of your function descriptions, i.e. sometimes including a space or not after """ at the beginning of the description and before the ending """ and sometimes excluding those spaces. Please go through and standardize one way or the other for consistency. Same with function calls and including spaces after commas or not. In that case, please include the space.
|
|
||
| def __get_dnf_version(self): | ||
| code, out = self.run_command_output('dnf --version', False, False) | ||
| # Output : dnf5 version 5.2.18.0/ |
There was a problem hiding this comment.
is the / at the end of this comment intended?
There was a problem hiding this comment.
No, earlier I had dnf5 version 5.2.18.0/4.20.0 but updated later. Removed / from the end
| error_msg = "This distro is not yet supported in your region. Please review https://aka.ms/VMGuestPatchingCompatibility for more information. [Distro={0}][Version={1}][Code={2}]".format(str(os_name), os_version, os_code) | ||
| print("Error: {0}".format(error_msg)) | ||
| if not self.__is_dnf_available(): | ||
| print("Error: Expected package manager dnf not found on this rhel 10 VM.") |
There was a problem hiding this comment.
Would prefer if you matched the original error message formatting.
| if version: | ||
| if version.startswith('4'): | ||
| return Constants.DNF4 | ||
| print("Error: Expected dnf version 4 on this rhel 10 VM. Found: {0}".format(version)) |
There was a problem hiding this comment.
Same comment as above
| @@ -93,8 +108,16 @@ def get_package_manager(self): | |||
|
|
|||
| # Check for unsupported distros | |||
There was a problem hiding this comment.
Update comment. Rhel10 no longer unsupported.
| if not patch_configuration_sub_setting_found_in_file: | ||
| updated_patch_configuration_sub_setting += patch_configuration_sub_setting_to_update + "\n" | ||
|
|
||
| self.env_layer.file_system.write_with_retry(self.os_patch_configuration_settings_file_path,'{0}'.format(updated_patch_configuration_sub_setting.lstrip()),mode='w+') |
There was a problem hiding this comment.
is there something weird about the spacing on this line or is github lying to me? It may be the lack of spaces in the line. Let's add spaces after each comma
There was a problem hiding this comment.
No it's the spacing after comma. I've added it
| apply_updates_value_from_backup = image_default_patch_configuration_backup[self.current_auto_os_update_service][self.apply_updates_identifier_text] | ||
| enable_on_reboot_value_from_backup = image_default_patch_configuration_backup[self.current_auto_os_update_service][self.enable_on_reboot_identifier_text] | ||
|
|
||
| self.update_os_patch_configuration_sub_setting(self.download_updates_identifier_text,download_updates_value_from_backup,self.auto_update_config_pattern_match_text) |
There was a problem hiding this comment.
add spaces after commas
| if str(enable_on_reboot_value_from_backup).lower() == 'true': | ||
| self.enable_auto_update_on_reboot() | ||
| else: | ||
| self.composite_logger.log_debug("[DNF4] Since the backup is invalid or does not exist for current service, we won't be able to revert auto OS patch settings to their system default value. [Service={0}]".format(str(self.current_auto_os_update_service))) |
There was a problem hiding this comment.
Since this is logging that will end up in customer logs, let's reword this to something like "Backup is invalid or does not exist for current service. Unable to revert auto OS patch settings to system default value."
I know that sounds very similar, but the pronouns feel weird in this sort of logging.
There was a problem hiding this comment.
I've updated the message
| def __get_image_default_patch_configuration_backup(self): | ||
| """ Get image_default_patch_configuration_backup file""" | ||
| image_default_patch_configuration_backup = {} | ||
| # read existing backup since it also contains backup from other update services. We need to preserve any existing data within the backup file |
There was a problem hiding this comment.
Capitalize Read
Michelle McDaniel (@michellemcdaniel) I've updated the function descriptions to remove spacing from the """start as well as the end""" to keep it consistent. Also added space after , that was missing at multiple places. Somehow when I copy paste code in Pycharm it automatically adds new lines and when putting it back on one the spaces are missed. I think its good now |
| os_name, os_version, os_code = self.platform.linux_distribution() | ||
|
|
||
| # Check for unsupported distros | ||
| # Check for Rhel 10 ( uses dnf4) |
There was a problem hiding this comment.
nit: Fix the spacing in this comment
| return str() | ||
| code, out, version = self.__get_dnf_version() | ||
| if version: | ||
| if version.startswith('4'): |
There was a problem hiding this comment.
same comment here on the dnf5 PR. Also, I think we may want to split the version comparison into a separate function rather than having a lot of duplicate code. Something like a shared "check_major_version" function
| # Support to get updates and their dependencies | ||
| self.single_package_check_versions = 'sudo dnf4 list --available <PACKAGE-NAME> ' | ||
| self.single_package_check_installed = 'sudo dnf4 list --installed <PACKAGE-NAME> ' | ||
| self.single_package_upgrade_simulation_cmd = 'sudo dnf4 install --assumeno --skip-broken ' |
There was a problem hiding this comment.
Will this have the same issue that dnf5 has that you just changed?
There was a problem hiding this comment.
No, this was specific to dnf5.
dnf/dnf4 works fine with the install command. Please check my testing logs for the same.
Rajasi Rane (rane-rajasi)
left a comment
Rajasi Rane (rane-rajasi)
left a comment
There was a problem hiding this comment.
Does RHEL10 have dnf4 commands or dnf? If you use 'dnf ' what version does it use and how does it work?
The RHEL doc here does not use dnf4: https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/10/pdf/managing_software_with_the_dnf_tool/Red_Hat_Enterprise_Linux-10-Managing_software_with_the_DNF_tool-en-US.pdf
Using a generic dnf makes is ideal to expand to other distros in future rather than implementing a package manager for each version.
AND regarding multi-arch dependencies, their doc does confirm the existence of multiple architectures but does not explicitly state that a package with multiple architectures would not have the same version. Even if we don't find an example today, it is always better to have a fail-safe code than one that would break in future. We should add multi arch dependencies in this implementation similar to what we have currently.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (10)
src/core/tests/Test_DnfPackageManager.py:50
- mock_run_command_output_check_update sometimes returns None, but callers expect run_command_output to always return (code, out). Returning None can cause unpacking errors and makes the test behavior depend on exception swallowing.
def mock_run_command_output_check_update(self, cmd, no_output=False, chk_err=True):
if "check-update" in cmd:
return 0, ""
return None
src/core/tests/Test_DnfPackageManager.py:611
- This assertion checks a boolean expression (
... is not None) rather than the actual file content; it will always pass and won’t catch regressions.
dnf_automatic_os_patch_configuration_settings_file_path_read = self.runtime.env_layer.file_system.read_with_retry(package_manager.os_patch_configuration_settings_file_path)
self.assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)
self.assertIn('apply_updates = yes', dnf_automatic_os_patch_configuration_settings_file_path_read)
src/core/src/package_managers/DnfPackageManager.py:610
- update_os_patch_configuration_sub_setting reads the config with read_with_retry() and immediately calls .strip(); if the file is missing, this raises and contradicts the method docstring (“adds if it doesn't exist”).
def update_os_patch_configuration_sub_setting(self, patch_configuration_sub_setting, value="no", config_pattern_match_text=""):
"""Updates (or adds if it doesn't exist) the given patch_configuration_sub_setting with the given value in os_patch_configuration_settings_file"""
try:
# note: adding space between the patch_configuration_sub_setting and value since, we will have to do that if we have to add a patch_configuration_sub_setting that did not exist before
self.composite_logger.log_debug("[DNF] Updating system configuration settings for auto OS updates. [Patch Configuration Sub Setting={0}] [Value={1}]".format(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)
patch_configuration_sub_setting_to_update = patch_configuration_sub_setting + ' = ' + value
patch_configuration_sub_setting_found_in_file = False
updated_patch_configuration_sub_setting = ""
settings = os_patch_configuration_settings.strip().split('\n')
src/core/src/package_managers/DnfPackageManager.py:258
- The comment references
dnf_output_expected_format.txt, but the repo reference file appears to besrc/tools/references/cmd_output_references/dnf4_ouput_expected_formats. This makes it harder to locate the examples when updating the parser.
def extract_dependencies(self, output, packages):
# Extracts dependent packages from output. Refer dnf_output_expected_format.txt for examples of output formats.
dependencies = []
src/core/src/package_managers/DnfPackageManager.py:114
- validate_dnf_output assumes output is a string; if env_layer returns None,
failure_text in outputwill raise TypeError and mask the real command failure.
def validate_dnf_output(self, output):
for failure_text in self.dnf_subscription_failure_texts:
if failure_text in output:
self.composite_logger.log_error("[DNF] Subscription/entitlement failure detected. [{0}]".format(failure_text))
raise Exception("System is not properly registered with subscription service.")
src/core/src/package_managers/DnfPackageManager.py:383
- dnf_automatic_install_check_cmd has a leading space (
' rpm -qa | grep ...'). It still works, but it’s easy to miss in string comparisons/mocks and is inconsistent with other command strings.
self.dnf_automatic_configuration_file_path = '/etc/dnf/automatic.conf'
self.dnf_automatic_install_check_cmd = ' rpm -qa | grep dnf-automatic'
self.dnf_automatic_enable_on_reboot_check_cmd = 'systemctl is-enabled dnf-automatic.timer'
self.dnf_automatic_disable_on_reboot_cmd = 'systemctl disable --now dnf-automatic.timer'
src/core/tests/Test_DnfPackageManager.py:586
- test_inclusion_type_other recreates RuntimeCompositor but continues using the old package_manager instance from the previous runtime/container, mixing objects from different runtimes.
def test_inclusion_type_other(self):
"""Unit test for dnf package manager with inclusion and Classification = Other. All packages are considered are 'Security' since DNF does not have patch classification"""
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
self.assertIsNotNone(package_manager)
self.runtime.stop()
argument_composer = ArgumentComposer()
argument_composer.classifications_to_include = [Constants.PackageClassification.OTHER]
argument_composer.patches_to_include = ["ssh", "tcpdump"]
argument_composer.patches_to_exclude = ["ssh*", "test"]
self.runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.DNF)
self.container = self.runtime.container
package_filter = self.container.get('package_filter')
self.assertIsNotNone(package_filter)
# test for get_available_updates
available_updates, package_versions = package_manager.get_available_updates(package_filter)
self.assertIsNotNone(available_updates)
src/core/tests/Test_DnfPackageManager.py:635
- The test sets
run_output_command, but the EnvLayer usesrun_command_output. As written, the mock is never used and the ‘no reboot’ branch isn’t actually exercised.
# Restart not required (needs-restarting returns code=0)
self.runtime.set_legacy_test_type('SadPath')
self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot
self.assertFalse(package_manager.is_reboot_pending())
src/core/tests/Test_DnfPackageManager.py:685
- These assertions don’t validate the returned lists:
assertTrue(5, security_packages)always passes because 5 is truthy (second arg is treated as a message). This should assert the expected counts.
def test_get_security_updates(self):
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
self.assertTrue(package_manager)
security_packages, security_package_versions = package_manager.get_security_updates()
self.assertTrue(5, security_packages)
self.assertTrue(5, security_package_versions)
src/core/tests/Test_DnfPackageManager.py:693
- assertRaises calls update_os_patch_configuration_sub_setting without required arguments, so the test is currently validating a TypeError rather than the intended write failure path.
def test_update_os_patch_configuration_sub_setting_exception_handling(self):
"""Test exception handling when override file write fails"""
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.update_os_patch_configuration_sub_setting, )
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (8)
src/core/tests/Test_DnfPackageManager.py:612
assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)is asserting on a boolean expression, which will always be non-None. This doesn't validate the file contents were read.
package_manager.update_os_patch_configuration_sub_setting(package_manager.dnf_automatic_download_updates_identifier_text, "no",package_manager.dnf_automatic_config_pattern_match_text)
dnf_automatic_os_patch_configuration_settings_file_path_read = self.runtime.env_layer.file_system.read_with_retry(package_manager.os_patch_configuration_settings_file_path)
self.assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)
self.assertIn('apply_updates = yes', dnf_automatic_os_patch_configuration_settings_file_path_read)
self.assertIn('download_updates = no', dnf_automatic_os_patch_configuration_settings_file_path_read)
src/core/tests/Test_DnfPackageManager.py:634
- This test assigns the reboot mock to
run_output_command, but the code under test callsenv_layer.run_command_output. As written, the mock is never used, and the assertion can be invalid/flaky.
# Restart not required (needs-restarting returns code=0)
self.runtime.set_legacy_test_type('SadPath')
self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot
self.assertFalse(package_manager.is_reboot_pending())
src/core/tests/Test_DnfPackageManager.py:685
assertTrue(5, security_packages)does not check the package list; it asserts on the constant5(always truthy) and treatssecurity_packagesas the failure message. The test should assert on the list length/content instead.
security_packages, security_package_versions = package_manager.get_security_updates()
self.assertTrue(5, security_packages)
self.assertTrue(5, security_package_versions)
src/core/tests/Test_DnfPackageManager.py:693
assertRaises(..., package_manager.update_os_patch_configuration_sub_setting, )calls the method with no required args, so the test will raiseTypeErrorregardless of the intended write failure path. Add minimal setup and pass the expected arguments so the test actually covers the exception handling inupdate_os_patch_configuration_sub_setting.
def test_update_os_patch_configuration_sub_setting_exception_handling(self):
"""Test exception handling when override file write fails"""
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.update_os_patch_configuration_sub_setting, )
src/core/tests/Test_DnfPackageManager.py:701
backup_image_default_patch_configuration_if_not_exists()is normally called after initializing the auto-update service context; this test calls it directly without initializingcurrent_auto_os_update_service/command fields, so it may exercise an unrealistic code path. Initialize the dnf-automatic context before asserting the exception.
def test_backup_image_default_patch_configuration_if_not_exists_exception_handling(self):
"""Test exception handling when override file write fails"""
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.backup_image_default_patch_configuration_if_not_exists, )
src/core/src/package_managers/DnfPackageManager.py:114
validate_dnf_output()assumesoutputis a string; ifrun_command_outputreturnsNone(or another falsey value),failure_text in outputwill raise aTypeErrorand mask the real failure. Coerce to an empty string before scanning.
def validate_dnf_output(self, output):
for failure_text in self.dnf_subscription_failure_texts:
if failure_text in output:
self.composite_logger.log_error("[DNF] Subscription/entitlement failure detected. [{0}]".format(failure_text))
raise Exception("System is not properly registered with subscription service.")
src/core/src/package_managers/DnfPackageManager.py:610
update_os_patch_configuration_sub_setting()reads the config file withraise_if_not_found=True, but the docstring says it will add the setting if it doesn't exist. If the config file itself is missing, this will throw before it can create/update it. Read withraise_if_not_found=Falseand treat missing content as empty.
self.composite_logger.log_debug("[DNF] Updating system configuration settings for auto OS updates. [Patch Configuration Sub Setting={0}] [Value={1}]".format(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)
patch_configuration_sub_setting_to_update = patch_configuration_sub_setting + ' = ' + value
patch_configuration_sub_setting_found_in_file = False
updated_patch_configuration_sub_setting = ""
src/core/tests/Test_EnvLayer.py:358
- The "wrong_version" test case currently uses
mock_run_command_for_dnf_wrong_version, which (per the existing mock) returns a DNF 4.x major version. SinceEnvLayer.__get_dnf_version()only checks the major version, this case doesn't actually validate the "dnf major != 4" path. Use the existingmock_run_command_for_dnf5(returns major 5) for this row so the test covers the intended failure case.
test_input_output_table = [
[self.mock_run_command_for_dnf4, "dnf"],
[self.mock_run_command_for_dnf_not_found, str()],
[self.mock_run_command_for_dnf_wrong_version, str()],
[self.mock_run_command_for_dnf_version_command_failure, str()]
| def mock_run_command_for_dnf4(self, cmd, no_output=False, chk_err=False): | ||
| if "which dnf" in cmd: | ||
| return 0, '/usr/bin/dnf' | ||
| if "dnf --version" in cmd: | ||
| return 0, '4.20.0' | ||
| return -1, '' | ||
|
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (11)
src/core/tests/Test_DnfPackageManager.py:50
- mock_run_command_output_check_update returns None for commands other than check-update, but callers expect a (code, out) tuple (e.g., refresh_repo also runs clean expire-cache). Returning None will raise during tuple-unpacking.
def mock_run_command_output_check_update(self, cmd, no_output=False, chk_err=True):
if "check-update" in cmd:
return 0, ""
return None
src/core/tests/Test_DnfPackageManager.py:610
- This assertion is checking a boolean expression (
... is not None) rather than the actual file contents variable, so it can pass even when the read result is None.
self.assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)
src/core/tests/Test_DnfPackageManager.py:641
- assertRaises is currently invoked with
package_manager.is_reboot_pending()(executed immediately) and the mocked failure is applied to file_system.write_with_retry, which is not used by is_reboot_pending(). This will not test the exception path as intended.
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.is_reboot_pending())
src/core/tests/Test_DnfPackageManager.py:701
- assertRaises has a trailing comma, and passes the function object without actually asserting the intended exception behavior cleanly. Use the callable directly (no trailing comma).
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.backup_image_default_patch_configuration_if_not_exists, )
src/core/src/package_managers/DnfPackageManager.py:115
- validate_dnf_output assumes output is a string; if run_command_output returns None,
failure_text in outputwill raise a TypeError and mask the real error. Guard against empty/None output before searching for subscription failure strings.
def validate_dnf_output(self, output):
for failure_text in self.dnf_subscription_failure_texts:
if failure_text in output:
self.composite_logger.log_error("[DNF] Subscription/entitlement failure detected. [{0}]".format(failure_text))
raise Exception("System is not properly registered with subscription service.")
src/core/src/package_managers/DnfPackageManager.py:257
- The comment references
dnf_output_expected_format.txt, but the repo reference file added for DNF4 output formats is under src/tools/references/cmd_output_references/dnf4_ouput_expected_formats. This mismatch makes it harder to find the examples.
# Extracts dependent packages from output. Refer dnf_output_expected_format.txt for examples of output formats.
src/core/tests/Test_DnfPackageManager.py:634
- test_is_reboot_pending sets env_layer.run_output_command, but the code under test calls env_layer.run_command_output. As a result, the mock isn't used and the test won't exercise the intended path.
# Restart not required (needs-restarting returns code=0)
self.runtime.set_legacy_test_type('SadPath')
self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot
self.assertFalse(package_manager.is_reboot_pending())
src/core/tests/Test_DnfPackageManager.py:685
- These assertions use assertTrue with two arguments, which doesn't validate the expected counts and can mask failures. Also, the expected number of security updates is mock-dependent, so asserting exact counts is brittle.
security_packages, security_package_versions = package_manager.get_security_updates()
self.assertTrue(5, security_packages)
self.assertTrue(5, security_package_versions)
src/core/tests/Test_DnfPackageManager.py:693
- assertRaises is called without required arguments to update_os_patch_configuration_sub_setting, so the test will raise TypeError instead of validating the intended write failure handling.
This issue also appears on line 699 of the same file.
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.update_os_patch_configuration_sub_setting, )
src/core/tests/library/LegacyEnvLayerExtensions.py:694
- String constants should be compared with ==, not
is. Using identity comparison can be flaky across Python implementations and can cause the DNF-specific mock branches to be skipped unexpectedly.
elif self.legacy_package_manager_name is Constants.DNF:
src/core/tests/Test_DnfPackageManager.py:585
- test_inclusion_type_other recreates RuntimeCompositor/container, but continues using the package_manager instance created before self.runtime.stop(). That instance is tied to the stopped runtime and can produce incorrect behavior or failures.
self.runtime = RuntimeCompositor(argument_composer.get_composed_arguments(), True, Constants.DNF)
self.container = self.runtime.container
package_filter = self.container.get('package_filter')
self.assertIsNotNone(package_filter)
# test for get_available_updates
available_updates, package_versions = package_manager.get_available_updates(package_filter)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (10)
src/core/tests/Test_DnfPackageManager.py:161
- test_disable_auto_os_update_failure currently does not set up any failing condition, so disable_auto_os_update() is unlikely to raise in the default HappyPath runtime. The test should explicitly force a failure in a later step (e.g., systemctl disable) so the method raises while still creating the backup.
def test_disable_auto_os_update_failure(self):
package_manager = self.container.get('package_manager')
self.assertRaises(Exception, package_manager.disable_auto_os_update)
self.assertTrue(package_manager.image_default_patch_configuration_backup_exists())
src/core/tests/Test_DnfPackageManager.py:585
- test_inclusion_type_other() reinitializes RuntimeCompositor/container, but still calls get_available_updates() on the old package_manager instance from the previous container (the runtime was stopped). This can make the test pass/fail for the wrong reason or crash depending on implementation details.
# test for get_available_updates
available_updates, package_versions = package_manager.get_available_updates(package_filter)
src/core/tests/Test_DnfPackageManager.py:610
- This assertion is incorrect:
assertIsNotNone(expr is not None)always receives a boolean and will always pass. It should assert the actual file contents are not None.
self.assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)
src/core/tests/Test_DnfPackageManager.py:633
- The test sets
run_output_command, but the environment layer method used elsewhere isrun_command_output. As written, the mock is not applied and the 'no reboot required' branch won't be tested reliably.
self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot
src/core/tests/Test_DnfPackageManager.py:685
- These assertions misuse assertTrue(): the first argument becomes the condition (always truthy: 5), so the test doesn't validate anything about the returned updates. It should assert on list lengths (and ideally that package/version counts match).
security_packages, security_package_versions = package_manager.get_security_updates()
self.assertTrue(5, security_packages)
self.assertTrue(5, security_package_versions)
src/core/tests/Test_DnfPackageManager.py:693
- This assertRaises call is incomplete: update_os_patch_configuration_sub_setting requires arguments and the test doesn't create a config file / set os_patch_configuration_settings_file_path. As written it will raise due to bad invocation rather than exercising the intended write-failure path.
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.update_os_patch_configuration_sub_setting, )
src/core/tests/Test_DnfPackageManager.py:701
- This assertRaises call is incomplete and likely tests the wrong thing: backup_image_default_patch_configuration_if_not_exists depends on current_auto_os_update_service/os_patch_configuration_settings_file_path being initialized (via get_current_auto_os_patch_state()/__init_auto_update_for_dnf_automatic) and on the config file existing. Without setup, failures won't reflect the intended write-failure path.
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
# Mock file_system.write_with_retry to raise exception
self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception
self.assertRaises(Exception, package_manager.backup_image_default_patch_configuration_if_not_exists, )
src/core/src/package_managers/DnfPackageManager.py:114
- validate_dnf_output() assumes output is a string; when run_command_output returns None,
failure_text in outputraises a TypeError and masks the real failure. Normalize output to an empty string before searching for subscription failure texts.
def validate_dnf_output(self, output):
for failure_text in self.dnf_subscription_failure_texts:
if failure_text in output:
self.composite_logger.log_error("[DNF] Subscription/entitlement failure detected. [{0}]".format(failure_text))
raise Exception("System is not properly registered with subscription service.")
src/core/src/package_managers/DnfPackageManager.py:664
- revert_auto_os_update_to_system_default_for_dnf_automatic() only enables the timer when the backup says enable_on_reboot=true, but it never disables the timer when the backup says false. This means revert may leave dnf-automatic enabled on reboot even when the system default was disabled.
self.update_os_patch_configuration_sub_setting(self.download_updates_identifier_text, download_updates_value_from_backup, self.auto_update_config_pattern_match_text)
self.update_os_patch_configuration_sub_setting(self.apply_updates_identifier_text, apply_updates_value_from_backup, self.auto_update_config_pattern_match_text)
if str(enable_on_reboot_value_from_backup).lower() == 'true':
self.enable_auto_update_on_reboot()
src/core/tests/Test_DnfPackageManager.py:119
- Unit tests should not print debug output. Here the test prints the refresh result instead of asserting behavior, which adds noise to test logs and can hide failures.
This issue also appears in the following locations of the same file:
- line 157
- line 584
- line 633
- line 683
- line 689
- ...and 1 more
# When no updates available and exit code 0
self.runtime.env_layer.run_command_output = self.mock_run_command_output_check_update
result = package_manager.refresh_repo_safely()
print("DEBUG:", result)
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.
| def is_distro_rhel(self, distro_name): | ||
| # type: (str) -> bool | ||
| """ Checks if the current distro is RHEL 10 """ | ||
| """ Checks if the current distro is RHEL 10. Can be expanded backwards in future""" | ||
| return self.__is_matching_distro_and_version(distro_name, Constants.RED_HAT, version_to_match=10) |
There was a problem hiding this comment.
While you can expand in the future, the function today checks RHEL 10, in the future it may not check all RHEL, it may only be specific versions other than 10. Thus the function rename is premature (and has the potential to mislead if the definition is not read).
There was a problem hiding this comment.
I've updated it back to what it is. i.e rhel10 specific wit the comment
Koshy John (kjohn-msft)
left a comment
Koshy John (kjohn-msft)
left a comment
There was a problem hiding this comment.
One comment inline
|
UT failure is flaky. I am looking into it. Disregard the Test_ExtOutputStatusHandler.py change. I will be removing that once I verify the sleep time |
Rajasi Rane (rane-rajasi)
left a comment
Rajasi Rane (rane-rajasi)
left a comment
There was a problem hiding this comment.
What are the differences between DNF4 and DNF5?
I see a lot of code duplicated between these versions. If the commands and outputs are mostly the same, it makes sense to have a single package manager (single source of code) and address any differences between versions as special cases.
I'm not convinced with the design of having version specific package managers. It will likely need us to implement a package manager for every new version along with an increase in the size of code/build that gets installed on a VM.
I'm thinking this could use a structure similar to how TdnfPackageManager and AzL3TdnfPackageManager are implemented. TdnfPkgMgr is the base/parent with all common code to be used for Mariner (i.e. AzL2) and AzL3 distros, while AzL3TdnfPkgMgr only includes the special cases needed for AzL3
Koshy John (@kjohn-msft), what are your thoughts on this?
| 'tdnf_test_config': self.new_test_configuration(Constants.TDNF, AzL3TdnfPackageManager), | ||
| 'dnf_test_config': self.new_test_configuration(Constants.DNF, DnfPackageManager), | ||
| 'dnf5_test_config': self.new_test_configuration(Constants.DNF5, Dnf5PackageManager), | ||
| 'tdnf_test_config': self.new_test_configuration(Constants.TDNF, AzL3TdnfPackageManager), |
There was a problem hiding this comment.
nit: Indentations are still not consistent in all configs
| def __get_dnf_version(self): | ||
| """ | ||
| This method currently checks for dnf versions on | ||
| azure linux 4 ad rhel10 system. Both outputs differ in styles. |
There was a problem hiding this comment.
nit: This method currently checks for DNF versions on Azure Linux 4 and RHEL 10 distros. Output formats differ between versions.
| """Returns dependent List for the list of packages""" | ||
| package_names = " ".join(packages) | ||
| cmd = self.single_package_upgrade_simulation_cmd + package_names | ||
| code, output = self.env_layer.run_command_output(cmd, False, False) |
There was a problem hiding this comment.
An ideal implementation of invoke_package_manager_advanced() would not need run_command_output to be called here, thereby not needing errored output processing in each function
| self.assertEqual(prev_modified_time, modified_time) | ||
|
|
||
| time.sleep(0.03) # ensure filesystem mtime granularity is exceeded | ||
| time.sleep(0.1) # ensure filesystem mtime granularity is exceeded |
There was a problem hiding this comment.
Why is this time increase needed?
| self.assertEqual(code, -1) | ||
|
|
||
| code, out = self.mock_run_command_for_dnf5('which not-dnf') | ||
| code, out = self.mock_run_command_for_dnf4('which not-dnf') |
There was a problem hiding this comment.
why is 'which not-dnf' tested when that command is never run in code?
There was a problem hiding this comment.
These lines are uncovered branches in the test mocks themselves. I added fallback-path tests to execute the return -1 branches. The commands are intentionally invalid because the mocks return -1 for any unrecognized command.
| self.assertEqual(code, -1) | ||
|
|
||
| code, out = self.mock_run_command_for_dnf_version_command('dnf --v', "wrong_version") | ||
| code, out = self.mock_run_command_for_dnf_wrong_version('dnf --v') |
There was a problem hiding this comment.
Again, this command is never executed by the main code, why is it then tested in UTs?
| self.assertEqual(code, -1) | ||
|
|
||
| code, out = self.mock_run_command_for_dnf_version_command('dnf --v', "version_command_failure") | ||
| code, out = self.mock_run_command_for_dnf_version_command_failure('dnf --v') |
There was a problem hiding this comment.
Same question as above
Rajasi Rane (@rane-rajasi) Koshy John (@kjohn-msft) However, I'd propose doing this refactor as a separate follow-up PR rather than in the current one, for these reasons:
I think it would be safer to land the RHEL 10 support first then take the consolidation as a focused follow-up. I can create a follow-up work item to consolidate DnfPackageManager and Dnf5PackageManager into a shared base class with version-specific overrides. |
|
Failure is related to UT flakines that was introduced in https://github.com/Azure/LinuxPatchExtension/pull/376/changes . I will update the sleep time accordingly. |
Agreed with this. I was also noting a lot of overlap, though it is possible that there is some formatting/other stuff that are quite a bit different? I do think I would rather us have a base DnfPkgMgr, with all the shared code, and then child classes that inherit and override where needed. The less duplicated code we have, the less we have to maintain. |
Ok to keep it out of this PR. |
Implemented Dnf4PackageManager by extending PackageManager.
Implemented changes:
TESTS
On demand Assessment (ConfigurePatching can be validated within Assess or Install Patches run)
4.core.log
On demand Installation, Classification: [Critical, Security, Other]
"classificationsToInclude": ["Security","Other","Critical"]
5.core.log
Auto assessment, recurring on schedule -
2.aa.core.log
2.core.log
3.json
Only Package inclusions installed
7.core.log
Included : python3-perf ( Only installed)
With package exclusions i.e. excluded packages are not installed
Exclude list [xxd, openssl ] - Both not installed
4.core.log
With Dependent packages i.e. dependent packages identified and installed
6.core.log
Included : coreutils (It installed dependent packages coreutils-common etc)
Excluding a package because its dependency needs to be excluded : I
5.core.log
Included: fprintd , Excluded : fprintd-pam
Auto Patching request with only security and critical updates in request, which should install all classifications
"classificationsToInclude": ["Security", "Critical"]
9.core.log
Logs for disabling auto OS (machine default) updates
Machine default updates service installed but NOT enabled - 4.core.log
Machine default updates service NOT installed -
autoOS_notInstalled_.log
Machine default updates service installed and enabled - ConfigurePatching reads and logs that auto OS updates are installed and enabled and disables them. Auto OS updates are disabled
**Note on Redhat machine not being able to get updates with the below message: **
Unable to read consumer identity This system is not registered with an entitlement server. You can use "rhc" or "subscription-manager" to register.Add Multi_arch_dependencies:
Evaluated whether DNF4 needs add_arch_dependencies() logic. Verified that DNF4 already expands transactions automatically during dependency resolution.
Could not find any package in RHEL10 repos that exists with:
same package name
same version
different architecture
No evidence found that DNF4 requires manual architecture sibling expansion.
Validation Performed
Ran:
dnf4 update glibc.x86_64 --assumenoDNF4 automatically added:
glibc-common
glibc-gconv-extra
glibc-langpack-en
Ran:
dnf4 update kernel.x86_64 --assumenoDNF4 automatically added:
kernel-core
kernel-modules
kernel-modules-core
Checked multilib configuration:
multilib_policy = best
Searched for packages available in multiple architectures.
Verified versions of those packages. Architectures existed, but versions were different.
Example:
cockpit-bridge.noarch 356.2-1.el10_2
cockpit-bridge.x86_64 334.1-1.el10_0
Performed repository-wide scan for same package name, same version and multiple architectures but no matches found
Thoughts:
DNF4 already performs dependency/transaction expansion internally.
Could not reproduce the exact scenario that add_arch_dependencies() was designed for
No evidence found that DNF4 requires additional architecture expansion logic at this time.