Refs #39448 - improve performance on host index page

[sbernhard]

During testing, we found out multiple issues so that the performance of the host index page is really bad.
Before this commit, using '/api/hosts?include_permissions=false&search=' took about 1min 30seconds. Simply returning "false" for "relevant?" decreased the access to 6seconds. With this commit, we are back at 9seconds - which is acceptable hopefully.

Replace call to ".to_status" with ".status" to use the already calculated value of the complianace report.

host_has_hostgroup_policies? is pretty fast because it answers a yes/no question with a cheap EXISTS query instead of loading and deduplicating all applicable policies in Ruby.

[adamruzicka]

I like the general idea, it has some other benefits over just being faster (like search finally working reliably). However this sort of deviates from the pattern we generally use for statuses and makes the whole thing much more prone to showing stale data. For example we don't refresh the status when a policy is assigned/unassigned to the host (or its hostgroup or its ancestors). So either we should keep building the values dynamically or make sure to refresh the sub-status whenever any change that could affect its value happens.

[sbernhard]

For example we don't refresh the status when a policy is assigned/unassigned to the host (or its hostgroup or its ancestors)

In katello, like https://github.com/Katello/katello/blob/master/app/models/katello/errata_status.rb#L25 it is done the same way. Actually, the compliance_status need to be updated if there is a new report. If another policy is set, its "nice" that the status is set ASAP to "not compliant" but it become really meanigful, if there is a report including this policy.

[adamruzicka]

but it become really meanigful, if there is a report including this policy.

I was thinking in slightly different direction - if a host was incompliant with its policy and that policy was replaced with some other, I'd expect the status to turn green immediately, even if (or maybe better "because") it has no reports with the new policy

[jtruestedt]

So if you have a host and a report, which tells the host is compliant/incompliant and you change the assigned policy, you cannot know if it is still compliant/incompliant until a report is there. So from my point of view the status should be "unknown" (warning instead of ok/error) or even empty.
Imagine you have a host which is incompliant, so you change the policy and it will turn to green and on the other side you make sure that no new report is sent - then the host will be compliant forever by changing the policy. Same counts somehow for having to switch to "incompliant" but this shows at least that some action is needed.

Just to add the user-perspective without knowing what the code does.

[sbernhard]

Another thought but not yet verified. The relevant? method checks for, if policies are set. Maybe we should instead check, if there is a report? maybe this is faster.

[ShimShtein]

From looking at the PR, it seems correct. The whole idea is not to recalculate the status on the fly (which can get really expensive).
I do miss in this PR the places when the status should actually be recalculated. I would expect to see a recalculation on status report. Also I would like to see recalculation on host properties change that are relevant for status calculations. For example in rh_cloud we recalculate the status when a certain action happens: https://github.com/theforeman/foreman_rh_cloud/blob/1faeed9fefb7533af906f8d01ac96c69d7a72d85/lib/inventory_sync/async/inventory_full_sync.rb#L46
Notice that if the action is not user-facing, we don't care about performance that much. I have seen the talks about policy change - this is a good example where we can fire off an async action that will invalidate statuses for all the relevant hosts.

[sbernhard]

I do miss in this PR the places when the status should actually be recalculated. I would expect to see a recalculation on status report.

adjusting the compliance status is done by

foreman_openscap/app/controllers/api/v2/compliance/arf_reports_controller.rb

Line 53 in a276f0b

@host.refresh_statuses([HostStatus.find_status_by_humanized_name("compliance")])

, isn't it?

I 'm currently again looking if it would be easily possible to set the status to inconclusive if policies are added/removed.

Regarding backgroup job, without foreman-tasks this might be a challenge, too :)

[sbernhard]

I have changed this to draft again.
I have added e31ed0c - but not yet tested. All of this is required, to make sure, that the compliance status for all hosts is set to 'INCONCLUSIVE' for which a policy was added / removed - no matter if directly to host, hostgroup or parent(s) host group.
Is it worth? @ShimShtein / @adamruzicka / @m-bucher

[ShimShtein]

Did a very high-level pass on the inconclusive commit. Looks quite OK to me. We may be able to improve it a bit, but overall it's OK.
As to whether it's worth it - a good question. It does make our DB much more consistent.

[sbernhard]

Looks quite OK to me. We may be able to improve it a bit, but overall it's OK.

can you let me know, what you want to improve?

As to whether it's worth it - a good question. It does make our DB much more consistent.

I have tested it and it looks like this. From the performance perspecitve, its OK to reset the state.
What I have tested:

• using the old all-hosts page -> set compliance policy for a host -> state is set to INCONCLUSIVE. Run compliance report -> state is set accordingly

• using the old all-hosts page -> unset compliance policy for a host which has a hostgroup policy, too -> state is set to INCONCLUSIVE. Run compliance report -> state is afterward set accordingly

• using the old all-hosts page -> unset compliance policy for a host without hostgroup policy-> state is set to N/A because the host and its hostgroup does not have a policy (=> this is correct)

• Hosts>Compliance>Policies - set policy for a hostgroup -> state is set to INCONCLUSIVE for the hosts of this group and inherited groups. Run compliance report -> state is afterward set accordingly

• Hosts>Compliance>Policies - unset policy for a hostgroup -> state is set to INCONCLUSIVE for all hosts of this group and inherited groups if they have direct host policies or other hostgroup policies. Run compliance report -> state is afterward set accordingly.

• Hosts>Compliance>Policies - unset policy for a hostgroup -> state is set to N/A for the hosts of this group and inherited groups if they don't have direct host policy or other host group policies.

=> this PR works!

Additionally, I want to notice again, that its currently not possible on the new all-hosts page to set policies to hosts - only host group because the bulk actions on the new all hosts page to set / unset compliance policy for a host don't exist:
https://community.theforeman.org/t/deprecating-and-removing-the-legacy-content-hosts-ui/42729/13

Actually, for a plugin like "openscap" it would be super good to have a host details card in which it is possible to a) see which policies are set and b) set a host policy and c) see the compliance status and maybe link to last report - this wouldn't be that hard to implement....

[m-bucher]

This could be a getter-method:

Suggested change

	def reset_changed_assignment_hosts_to_inconclusive
	host_ids = Array(@compliance_status_reset_host_ids).compact.uniq
	def compliance_status_reset_host_ids
	Array(@compliance_status_reset_host_ids).compact.uniq
	end
	def reset_changed_assignment_hosts_to_inconclusive
	host_ids = compliance_status_reset_host_ids

But as long as we only use the value once, it does not really make a difference.