Skip to content

Refs #39068 - Check for persisted statuses when building global status#10993

Open
synkd wants to merge 1 commit into
theforeman:developfrom
synkd:sat_42220_qa_failure
Open

Refs #39068 - Check for persisted statuses when building global status#10993
synkd wants to merge 1 commit into
theforeman:developfrom
synkd:sat_42220_qa_failure

Conversation

@synkd
Copy link
Copy Markdown
Contributor

@synkd synkd commented May 21, 2026

After #10962 was merged, a failure was found downstream in which a host showed a WARN status on the All Hosts page immediately after registration, although the same host showed OK for all statuses on its host details page. The cause of this appears to be that, at registration time, the host has a nil value for its execution status. When the get_status() method in app/models/host/managed.rb reads this nil status, it calls host_statuses.new() for the status. Because the host has no config reports, its status is eventually returned as status type WARN by the to_global() method in app/models/host_status/configuration_status.rb.

This PR addresses this issue by checking whether all relevant statuses are also persisted when building global status for a host.

Testing Steps:

  1. Register a host to a Foreman server with the remote execution plugin enabled.
  2. On the All Hosts page, verify that the status icon for the host is OK (green).

@synkd synkd force-pushed the sat_42220_qa_failure branch 2 times, most recently from 54db0fd to fbdc419 Compare May 21, 2026 20:51
After theforeman#10962 was merged, a failure was found downstream in which a host
showed a WARN status on the All Hosts page immediately after
registration, although the same host showed OK for all statuses on its
host details page. The cause of this appears to be that, at registration
time, the host has a nil value for its execution status. When the
`get_status()` method in `app/models/host/managed.rb` reads this nil status,
it calls `host_statuses.new()` for the status. Because the host has no
config reports, its status is eventually returned as status type `WARN` by
the `to_global()` method in `app/models/host_status/configuration_status.rb`.

This PR addresses this issue by checking whether all relevant statuses
are also persisted when building global status for a host.

Testing Steps:
1. Register a host to a Foreman server with the remote execution plugin
   enabled.
2. On the All Hosts page, verify that the status icon for the host is OK
   (green).
@synkd synkd force-pushed the sat_42220_qa_failure branch from fbdc419 to c8b47d1 Compare May 26, 2026 19:22
Copy link
Copy Markdown
Contributor

@LadislavVasina1 LadislavVasina1 left a comment

Choose a reason for hiding this comment

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

Looks good to me, automation made for this feature is passing with this patch.

Copy link
Copy Markdown
Contributor

@LadislavVasina1 LadislavVasina1 left a comment

Choose a reason for hiding this comment

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

Sorry, dismissing my previous ACK, I accidentally checked the automation results against the incorrect branch.

@LadislavVasina1
Copy link
Copy Markdown
Contributor

Reacking, can confirm the test is passing on the correct branch.

Copy link
Copy Markdown
Contributor

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Thanks @synkd

The fix is working as expected. Without this change, my newly-registered host shows a WARN global status because of an "Unknown execution status," but with the PR checked out, the host global status shows OK for both All Hosts list and host details.

There is still room for improvement in one place, however - The tooltip on the All Hosts page lists status text for irrelevant statuses (Execution & Inventory in this example):

Image

Whereas host details correctly just shows "N/A" for the status text of irrelevant statuses:

image

I think it would also be worth it to fix that here.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants