Skip to content

Feature: Adding Rhel10 Base Support using dnf4 package manager - #359

Open
yashnap wants to merge 29 commits into
masterfrom
dnf4_package_manager_rhel10
Open

Feature: Adding Rhel10 Base Support using dnf4 package manager#359
yashnap wants to merge 29 commits into
masterfrom
dnf4_package_manager_rhel10

Conversation

@yashnap

@yashnap yashnap commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Implemented Dnf4PackageManager by extending PackageManager.

Implemented changes:

  • DNF5 package manager
  • ConfigurePatching operation
  • Auto Assessment operation
  • Assessment operation
  • Installation operation
  • UTs
  • Disabling Auto OS updates

TESTS

  1. On demand Assessment (ConfigurePatching can be validated within Assess or Install Patches run)
    4.core.log

  2. On demand Installation, Classification: [Critical, Security, Other]
    "classificationsToInclude": ["Security","Other","Critical"]
    5.core.log

  3. Auto assessment, recurring on schedule -
    2.aa.core.log
    2.core.log
    3.json

  4. Only Package inclusions installed
    7.core.log
    Included : python3-perf ( Only installed)

  5. With package exclusions i.e. excluded packages are not installed
    Exclude list [xxd, openssl ] - Both not installed
    4.core.log

  6. With Dependent packages i.e. dependent packages identified and installed
    6.core.log
    Included : coreutils (It installed dependent packages coreutils-common etc)

  7. Excluding a package because its dependency needs to be excluded : I
    5.core.log
    Included: fprintd , Excluded : fprintd-pam

  8. 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

  1. Machine default updates service installed but NOT enabled - 4.core.log

  2. Machine default updates service NOT installed -
    autoOS_notInstalled_.log

  3. 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](https://github.com/user-attachments/files/29564822/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.

  • This message shows up on the VM when any command is run. Error message is being thrown so the customer is aware of what is happening.
redhat_systemerror

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 --assumeno
DNF4 automatically added:
glibc-common
glibc-gconv-extra
glibc-langpack-en

Ran:
dnf4 update kernel.x86_64 --assumeno
DNF4 automatically added:
kernel-core
kernel-modules
kernel-modules-core

Checked multilib configuration:
multilib_policy = best

Searched for packages available in multiple architectures.

Found examples:
cockpit-bridge.noarch
cockpit-bridge.x86_64
osbuild.noarch
osbuild.x86_64
clang-analyzer.noarch
clang-analyzer.x86_64

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.

E2E Scenarios Testing(After code updates) - September 9th, 2026

  1. On demand Assessment (ConfigurePatching can be validated within Assess or Install Patches run)
    8.core.on-demand-assess_config.log

  2. On demand Installation, Classification: [Critical, Security, Other]
    "classificationsToInclude": ["Security","Other","Critical"]
    Install 1 packages: [sos.noarch]
    10.core.install.1package.log

  3. Auto assessment, recurring on schedule -
    7.aa.core.log

  4. Only Package inclusions installed
    Included : tzdata.noarch
    Excluded : tiwilink-firmware.noarch
    11.core.include_exclude.log

  5. With package exclusions i.e. excluded packages are not installed
    Included : tzdata.noarch
    Excluded : tiwilink-firmware.noarch
    11.core.include_exclude.log

  6. With Dependent packages i.e. dependent packages identified and installed
    Included : insights-core
    Installed both insights-core and insights-core-selinux.noarch
    13.core.insights-core.log

  7. Excluding a package because its dependency needs to be excluded : I
    Included: fprintd , Excluded : fprintd-pam
    15.core.fpam.working.log

Included : selinux-policy, Excluded : selinux-policy-targeted.
17.core.selinuxworking.log

  1. Auto Patching request with only security and critical updates in request, which should install all classifications
    "classificationsToInclude": ["Security", "Critical"]
    9.core.security.install.log

Logs for disabling auto OS (machine default) updates

  1. Machine default updates service installed but NOT enabled
    23.core.service.notimer.log

  2. Machine default updates service NOT installed -
    24.core.noservice.log

  3. 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
    22.core.autoos_enabled_install.log

ARM : /subscriptions/6acc8a91-e2b0-4041-a069-c2932ab42fd9/resourceGroups/rhel10-yashna-rg/providers/Microsoft.Compute/virtualMachines/yashna-rhel10-vm

Exclude Use-case issue
Problem Statement.pdf

Updated code ( same as YumPackageManager)
exclude_dependencies

17.core.selinuxworking.log
18.core.fprintworking1.log

Copilot AI lite review requested due to automatic review settings July 1, 2026 20:45
Comment thread src/core/tests/Test_DnfPackageManager.py Fixed
Comment thread src/core/tests/Test_Dnf4PackageManager.py Fixed
Comment thread src/core/tests/Test_Dnf4PackageManager.py Fixed
Comment thread src/core/tests/Test_Dnf4PackageManager.py Fixed
@codecov

codecov Bot commented Jul 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.14243% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.98%. Comparing base (cbd9ac9) to head (105cd95).

Files with missing lines Patch % Lines
src/core/src/package_managers/DnfPackageManager.py 93.02% 32 Missing ⚠️
src/core/tests/Test_DnfPackageManager.py 98.68% 5 Missing ⚠️
src/core/src/bootstrap/EnvLayer.py 95.23% 1 Missing ⚠️
src/core/tests/Test_EnvLayer.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #359      +/-   ##
==========================================
+ Coverage   94.93%   94.98%   +0.04%     
==========================================
  Files         111      113       +2     
  Lines       20883    21844     +961     
==========================================
+ Hits        19826    20749     +923     
- Misses       1057     1095      +38     
Flag Coverage Δ
python27 94.96% <96.14%> (+0.04%) ⬆️
python312 94.95% <96.14%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Dnf4PackageManager with update discovery, dependency parsing, reboot detection, and auto-OS-update disable/revert logic.
  • Updates EnvLayer + ConfigurationFactory to 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.

Comment on lines 81 to 83
'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),
Comment on lines 88 to 90
'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),
Comment on lines +110 to +114
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.")
Comment on lines +897 to +900
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):
Comment on lines +639 to +642
# 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())
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
Comment thread src/tools/references/cmd_output_references/dnf4_ouput_expected_formats Outdated
Comment thread src/core/tests/Test_Dnf4PackageManager.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/core/src/bootstrap/ConfigurationFactory.py Outdated
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated

def __get_dnf_version(self):
code, out = self.run_command_output('dnf --version', False, False)
# Output : dnf5 version 5.2.18.0/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the / at the end of this comment intended?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, earlier I had dnf5 version 5.2.18.0/4.20.0 but updated later. Removed / from the end

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
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.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer if you matched the original error message formatting.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
if version:
if version.startswith('4'):
return Constants.DNF4
print("Error: Expected dnf version 4 on this rhel 10 VM. Found: {0}".format(version))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
@@ -93,8 +108,16 @@ def get_package_manager(self):

# Check for unsupported distros

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update comment. Rhel10 no longer unsupported.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment thread src/core/src/package_managers/DnfPackageManager.py
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+')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add spaces after commas

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

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)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Capitalize Read

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@yashnap

yashnap commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

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.

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

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
os_name, os_version, os_code = self.platform.linux_distribution()

# Check for unsupported distros
# Check for Rhel 10 ( uses dnf4)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Fix the spacing in this comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/core/src/package_managers/DnfPackageManager.py
@kjohn-msft Koshy John (kjohn-msft) added the feature New feature or request label Jul 10, 2026
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
return str()
code, out, version = self.__get_dnf_version()
if version:
if version.startswith('4'):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working on this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this

# 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 '

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this have the same issue that dnf5 has that you just changed?

@yashnap yashnap Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this was specific to dnf5.
dnf/dnf4 works fine with the install command. Please check my testing logs for the same.

@rane-rajasi Rajasi Rane (rane-rajasi) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yashnap

yashnap commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

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.

  • Both dnf --version and dnf --version gives the same output. I can rename all commands to use dnf instead of dnf4.
Screenshot 2026-07-27 121440
  • I'll update the code to have multi-arch depedencies code in.

Copilot AI review requested due to automatic review settings July 27, 2026 19:51
Copilot AI review requested due to automatic review settings August 7, 2026 14:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 is run_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 output raises 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)

Koshy John (kjohn-msft) pushed a commit that referenced this pull request Aug 7, 2026
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.
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
Comment on lines 79 to 82
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated it back to what it is. i.e rhel10 specific wit the comment

@kjohn-msft Koshy John (kjohn-msft) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment inline

@yashnap

yashnap commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

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

@rane-rajasi Rajasi Rane (rane-rajasi) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment thread src/core/tests/Test_DnfPackageManager.py
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
Comment thread src/core/src/bootstrap/ConfigurationFactory.py
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
def __get_dnf_version(self):
"""
This method currently checks for dnf versions on
azure linux 4 ad rhel10 system. Both outputs differ in styles.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This method currently checks for DNF versions on Azure Linux 4 and RHEL 10 distros. Output formats differ between versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

"""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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this time increase needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer : #359 (comment)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I take this is still pending?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been removed from the recent commit. New PR is out to address it : #380

Comment thread src/core/tests/Test_EnvLayer.py Outdated
Comment thread src/core/tests/Test_EnvLayer.py Outdated
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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this command is never executed by the main code, why is it then tested in UTs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #359 (comment)

Comment thread src/core/tests/Test_EnvLayer.py Outdated
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')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #359 (comment)

@yashnap

yashnap commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

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 (Koshy John (@kjohn-msft)), what are your thoughts on this?

Rajasi Rane (@rane-rajasi) Koshy John (@kjohn-msft)
Agreed — the duplication is a valid concern, and a shared base class (similar to TdnfPackageManager/AzL3TdnfPackageManager) is the right long-term approach.

However, I'd propose doing this refactor as a separate follow-up PR rather than in the current one, for these reasons:

  1. Scope — Refactoring requires changes across both package managers and their associated classes
  2. Regression risk — It would need full re-testing across both RHEL 10 and Linux 4 distros to ensure nothing breaks
  3. Review overhead — A combined feature + refactor PR would be significantly larger and take longer to review, potentially delaying the RHEL 10 feature release

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.
Let me know your thoughts.

@yashnap

yashnap commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

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.

@michellemcdaniel

Michelle McDaniel (michellemcdaniel) commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

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 (Koshy John (@kjohn-msft)), what are your thoughts on this?

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.

@kjohn-msft

Copy link
Copy Markdown
Collaborator

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 (Koshy John (Koshy John (@kjohn-msft))), what are your thoughts on this?

Rajasi Rane (Rajasi Rane (@rane-rajasi)) Koshy John (Koshy John (@kjohn-msft)) Agreed — the duplication is a valid concern, and a shared base class (similar to TdnfPackageManager/AzL3TdnfPackageManager) is the right long-term approach.

However, I'd propose doing this refactor as a separate follow-up PR rather than in the current one, for these reasons:

  1. Scope — Refactoring requires changes across both package managers and their associated classes
  2. Regression risk — It would need full re-testing across both RHEL 10 and Linux 4 distros to ensure nothing breaks
  3. Review overhead — A combined feature + refactor PR would be significantly larger and take longer to review, potentially delaying the RHEL 10 feature release

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. Let me know your thoughts.

Ok to keep it out of this PR.

Comment thread src/core/src/bootstrap/EnvLayer.py
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
Comment thread src/core/src/package_managers/DnfPackageManager.py Outdated
self.single_package_upgrade_simulation_cmd = 'sudo dnf install --assumeno --skip-broken '

# Install update
self.single_package_upgrade_cmd = 'sudo dnf -y install '

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Responded in the other thread

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.")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You do not need a separate handler for this error type. If all unexpected return codes are marked as failures with it being reported in error objects, surfacing this error to customers is already done. There is no need for a separate code to handle this. We already surface this as a customer error in Yum. You can search for "subscription-manager" errors in LPE telemetry and see how they are marked.

self.validate_dnf_output(out)
is_valid_not_installed = (self.dnf_list_installed_command_patterns in command and code == self.dnf_not_installed_exit_code and self.dnf_not_installed_text in (out or ""))

if code in self.dnf_exitcode_ok or is_valid_not_installed:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How best to handle this would also depend on the other comments of install vs upgrade, how to handle the missing subscription registration error, etc. Overall comment, mark all special case handlings with code comments (either inline or in their respective functions) explaining the need and keep the function implementation clean and easy to decipher

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I take this is still pending?

@rane-rajasi Rajasi Rane (rane-rajasi) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments inline and some responses in previous iterations reviews

Comment thread src/core/src/bootstrap/EnvLayer.py Outdated
Comment thread src/core/src/package_managers/DnfPackageManager.py Outdated
Comment thread src/core/src/package_managers/DnfPackageManager.py Outdated
Comment thread src/core/src/package_managers/DnfPackageManager.py Outdated
Comment thread src/core/src/package_managers/DnfPackageManager.py Outdated
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
Comment thread src/core/tests/Test_DnfPackageManager.py Outdated
package_manager = self.container.get('package_manager')
self.assertIsNotNone(package_manager)
deduped_packages, deduped_package_versions = package_manager.dedupe_update_packages_to_get_latest_versions(
packages, package_versions)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: single line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment thread src/core/tests/Test_EnvLayer.py Outdated
Comment thread src/core/tests/Test_EnvLayer.py Outdated

@rane-rajasi Rajasi Rane (rane-rajasi) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments inline.

Re-run all the validation tests against the latest code as there have been functional changes since they were run last.

Image

I will approve once the validations are re-run and successful

"""Get all missing updates"""
self.composite_logger.log_verbose("[DNF] Discovering all packages...")
if cached and not len(self.all_updates_cached) == 0:
self.composite_logger.log_debug("[DNF] Get all updates : [Cached={0}][PackagesCount={1}]]".format(str(cached), len(self.all_updates_cached)))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "[DNF] Get all updates: [Cached={0}][PackagesCount={1}]]"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def __assert_reverted_automatic_patch_configuration_settings(self, package_manager, config_exists=True, config_value_expected=''):
if config_exists:
reverted_dnf_automatic_patch_configuration_settings = self.runtime.env_layer.file_system.read_with_retry(
package_manager.dnf_automatic_configuration_file_path)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: single line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


# Create backup with service marked as not installed
package_manager.image_default_patch_configuration_backup_path = os.path.join(
self.runtime.execution_config.config_folder, Constants.IMAGE_DEFAULT_PATCH_CONFIGURATION_BACKUP_PATH)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: single line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

package_versions = ['3.12.3-1.azl3', '102-7.azl3 ', '2.11.5-1.azl3', '3.0-16.azl3', '3.12.9-2.azl3',
'3.12.9-1.azl3', '3.12.3-4.azl3', '6.6.78.1-1.azl3', '3.12.3-5.azl3', '3.12.3-5.azl3']
deduped_packages, deduped_package_versions = package_manager.dedupe_update_packages_to_get_latest_versions(
packages, package_versions)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: single line

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants