Feature: Adding Rhel10 Base Support using dnf4 package manager - #359
Feature: Adding Rhel10 Base Support using dnf4 package manager#359yashnap wants to merge 13 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.10% 94.19% +0.08%
==========================================
Files 109 111 +2
Lines 20642 21641 +999
==========================================
+ Hits 19425 20384 +959
- Misses 1217 1257 +40
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()) |
| security_packages, security_package_versions = package_manager.get_security_updates() | ||
| self.assertTrue(5, security_packages) | ||
| self.assertTrue(5, security_package_versions) |
| 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, ) |
michellemcdaniel
left a comment
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)) |
| @@ -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.
| architectures = Constants.SUPPORTED_PACKAGE_ARCH | ||
| for arch in architectures: | ||
| if package_name.endswith(arch): | ||
| return package_name[:-len(arch)], arch |
There was a problem hiding this comment.
earlier, you noted that the package name would be in the form .. Could we not split on . and return split[0], split[1] (assuming a known arch is found)?
There was a problem hiding this comment.
Yes, from the current evidence it looks like packageName.arch and we can split on the first .
However, since this is new to me as well so the question would be if we will have other packages that has multiple dots i.e python-3.10.module.arch in which case the first dot approach would produce incorrect results. (The current one would work regardless)
There was a problem hiding this comment.
Makes sense. I hadn't considered packages that may have extraneous periods in the package name. Might not even be safe to split on period and take the last chunk would it?
There was a problem hiding this comment.
One last thing: I think we might need to do len(arch)+1 here. Can you make sure that we are not accidentally returning "package_name." instead of "package_name" (note the additional period at the end). I did a little test with:
text= "hello.world"
substring = text[:-len("world")]
print(substring)and it print out "hello." So we need to make sure we strip out the trailing period
There was a problem hiding this comment.
@michellemcdaniel The way you are using the example is incorrect. The SUPPORTED_PACKAGE_ARCH = ['.x86_64', '.noarch', '.i686', '.aarch64'] already has a .(dot) in front of the arch name
eg : coreutils.x86_64
So, when the code runs it will take the length of .x86_64 = 7.
Remove that from the packagename from the end so we will be left with just coreutils.
There was a problem hiding this comment.
Ah I see. Makes sense. Thanks for the clarification.
| 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) |
| 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 |
@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
| architectures = Constants.SUPPORTED_PACKAGE_ARCH | ||
| for arch in architectures: | ||
| if package_name.endswith(arch): | ||
| return package_name[:-len(arch)], arch |
There was a problem hiding this comment.
One last thing: I think we might need to do len(arch)+1 here. Can you make sure that we are not accidentally returning "package_name." instead of "package_name" (note the additional period at the end). I did a little test with:
text= "hello.world"
substring = text[:-len("world")]
print(substring)and it print out "hello." So we need to make sure we strip out the trailing period
| 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.
rane-rajasi
left a comment
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 3 comments.
Comments suppressed due to low confidence (6)
src/core/tests/Test_DnfPackageManager.py:586
- test_inclusion_type_other stops and recreates the RuntimeCompositor/container, but continues using the old package_manager instance from the previous container. This can lead to tests operating on a stopped runtime and producing misleading results.
# 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:633
- This test sets env_layer.run_output_command, but the code calls env_layer.run_command_output. The mock won't take effect, so the assertion is not actually testing the intended path.
self.runtime.env_layer.run_output_command = self.mock_run_command_output_no_reboot
src/core/tests/Test_DnfPackageManager.py:641
- The exception-path assertion is incorrect: it passes the result of is_reboot_pending() into assertRaises, and it mocks file_system.write_with_retry even though is_reboot_pending() doesn't write any files. To test exception propagation, mock env_layer.run_command_output to raise and pass the callable to assertRaises.
# Exception Path
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
self.assertIsNotNone(package_manager)
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:686
- assertTrue(5, security_packages) doesn't validate anything (the first arg is treated as the truthy expression). This should assert on the actual lengths/contents. With the current HappyPath DNF mock output, check-update yields 4 packages.
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
- update_os_patch_configuration_sub_setting requires at least the setting key argument, but this test calls it with no arguments, so it will raise TypeError instead of exercising the intended write-failure path. It should also create a config file so the code reaches write_with_retry.
# 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/tools/references/cmd_output_references/dnf4_ouput_expected_formats:3
- The filename appears to have a typo ("ouput" -> "output"), and the content references a
dnf4command while the implementation invokesdnf(v4). Consider renaming the reference file and aligning wording to "dnf (v4)" to reduce confusion.
Example of different output formats for DNF 4 commands:
In DNF4 : Sample Output for the command 'dnf4 install --assumeno git' is as follows(Success use case):
| def mock_run_command_output_check_update(self, cmd, no_output=False, chk_err=True): | ||
| if "check-update" in cmd: | ||
| return 0, "" | ||
| return None |
| self.runtime.write_to_file(package_manager.os_patch_configuration_settings_file_path,dnf_automatic_os_patch_configuration_settings) | ||
| 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) |
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.
Comments suppressed due to low confidence (5)
src/core/tests/Test_DnfPackageManager.py:572
- test_inclusion_type_other() gets package_manager from the old container, then calls self.runtime.stop() and creates a new RuntimeCompositor/container. The test continues using the stale package_manager instance, so it may call into a stopped runtime and behave inconsistently.
self.runtime.set_legacy_test_type('HappyPath')
package_manager = self.container.get('package_manager')
self.assertIsNotNone(package_manager)
self.runtime.stop()
src/core/tests/Test_DnfPackageManager.py:610
- This assertion is effectively asserting that a boolean is not None (always true), so it doesn't validate the file read result.
self.assertIsNotNone(dnf_automatic_os_patch_configuration_settings_file_path_read is not None)
src/core/tests/Test_DnfPackageManager.py:634
- test_is_reboot_pending() assigns to env_layer.run_output_command, but DnfPackageManager.is_reboot_pending() calls env_layer.run_command_output. This mock assignment is a no-op.
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 misuse assertTrue(): the first argument (5) is always truthy, so the test doesn't validate that any security updates were returned (or that versions align).
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 currently triggers a TypeError because update_os_patch_configuration_sub_setting requires arguments. As written, it doesn't actually validate the intended exception-handling path when file writes fail.
"""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, )
| # Exception Path | ||
| self.runtime.set_legacy_test_type('HappyPath') | ||
| package_manager = self.container.get('package_manager') | ||
| self.assertIsNotNone(package_manager) | ||
| self.runtime.env_layer.file_system.write_with_retry = self.mock_write_with_retry_raise_exception | ||
| self.assertRaises(Exception, package_manager.is_reboot_pending()) | ||
|
|
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 - In Progress
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
9. Machine default updates service installed but NOT enabled - In Progress
Machine default updates service NOT installed - In Progress
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
8.core.log
**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.