Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix failing registration test #17346

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shubhamsg199
Copy link
Contributor

Problem Statement

test_positive_force_register_twice is failing as we are asserting rhel_contenthost.hostname but we are changing the hostname in the test

Solution

Update the test to assert the updated hostname

@shubhamsg199 shubhamsg199 added No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master labels Jan 16, 2025
@shubhamsg199 shubhamsg199 self-assigned this Jan 16, 2025
@shubhamsg199 shubhamsg199 requested a review from a team as a code owner January 16, 2025 10:17
@@ -221,12 +221,13 @@ def test_positive_force_register_twice(module_ak_with_cv, module_org, rhel_conte
assert result.status == 0
assert rhel_contenthost.subscribed
assert f'Unregistering from: {target_sat.hostname}' in str(result.stdout)
assert f'The registered system name is: {rhel_contenthost.hostname}' in str(result.stdout)
hostname = rhel_contenthost.execute('hostname').stdout.strip()
Copy link
Contributor

@shweta83 shweta83 Jan 16, 2025

Choose a reason for hiding this comment

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

It looks like the unregistering of the host is not done properly and RHEL10 is only impacted here. I am guessing it an issue of RHEL10 rather than automation.

Copy link
Contributor Author

@shubhamsg199 shubhamsg199 Jan 16, 2025

Choose a reason for hiding this comment

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

Yeah, that was my initial guess but looking at the logs, it says unregistered.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shubhamsg199 @shweta83 Looking at the test failure in CI, when hostnamectl set-hostname {name} it never worked on < RHEL 10 since we use containers for this test and especially for RHEL10 we don't have any container images created yet it and it default checkout a VM where set-hostname works and this started to fail.

2025-01-16 04:41:22 - gw0 - broker - DEBUG - a33bb7c2198e executing command: hostnamectl set-hostname ZugqfcdVhq.example.com
2025-01-16 04:41:22 - gw0 - broker - DEBUG - a33bb7c2198e command result:
    stdout:
    
    stderr:
    System has not been booted with systemd as init system (PID 1). Can't operate.
    Failed to connect to bus: Host is down
    
    status: 1

IMO this is expected behaviour, so if we're considering to fix it this way then we should consider using no_containers for this test, but before that I'd like to understand the rational behind changing the hostname step if is that really required for this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the explanation @Gauravtalreja1 , I think we should update the test and use no_containers
The reason behind changing the hostname step is because of the BZ attached which is the test requirement

Copy link
Contributor

Choose a reason for hiding this comment

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

The test scenario of the bug was testing case-sensitiveness of the hostname. But as @Gauravtalreja1 said, the scenario was never tested due to containers.
The problem here is we are asserting rhel_contenthost.hostname with hostname which will always fail. We need to fix it and run the test on VM.

reg_id_new = re.search(reg_id_pattern, result.stdout).group(1)
assert f'The system has been registered with ID: {reg_id_new}' in str(result.stdout)
assert reg_id_new != reg_id_old
assert (
target_sat.cli.Host.info({'name': rhel_contenthost.hostname}, output_format='json')[
target_sat.cli.Host.info({'name': hostname.lower()}, output_format='json')[
Copy link
Collaborator

Choose a reason for hiding this comment

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

If hostname is in uppercase before registration, then after registration does it get changed to lowercase ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so.

Copy link
Collaborator

Choose a reason for hiding this comment

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

then we don't need .lower()

Copy link
Contributor

@synkd synkd left a comment

Choose a reason for hiding this comment

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

Out of curiousity, have we evaluated whether this is still a valid test? BZ 1361309 is very old, and the code that was changed to fix it [1] is no longer present in the same module upstream [2]. I was unable to find it elsewhere in the Katello codebase either.

[1] https://github.com/Katello/katello/pull/6214/files
[2] https://github.com/Katello/katello/blob/master/app/models/katello/host/subscription_facet.rb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No-CherryPick PR doesnt need CherryPick to previous branches Stream Introduced in or relating directly to Satellite Stream/Master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants