Log only unique servers id used to process miq_request

[yrudman]

ISSUE:
In some situations miq_request may start re-queuing :
in log:

FIX:
Log only unique servers id

this PR is follow-up to #17451

@miq-bot add-label core, hammer/yes, changelog/yes, technical debt

[yrudman]

\cc @lfu @d-m-u

[d-m-u]

Honestly this had nothing to do with me at all and I don't know why I'm involved. But this looks better than it was.

[jrafanie]

Is there value to keep the same server multiple times?

[d-m-u]

I don't think so but Lucy should answer this.

[yrudman]

I think order is important

[lfu]

This is from previous discussion: #17451 (comment).

If we want to record all the servers in the order the tasks have been executed and limit the array size, maybe we can do this:

 options[:executed_on_servers] << MiqServer.my_server.id unless options[:executed_on_servers].last == MiqServer.my_server.id

cc @gmcculloug

[yrudman]

@lfu good suggestion, but it may not help if there are more than one server which may process request.
We could either sacrifice order and collect unique entries or sacrifice full history and limit size, I am not sure how we can do both.
It is a corner case, I am not sure what is the correct solution here and what limit should be in case we will decide to limit array size

[yrudman]

I think order will be gone if we will use Set:
entry [1,2,1] will be [1,2] and last server will be shown as 2

[jrafanie]

I think order will be gone if we will use Set:
entry [1,2,1] will be [1,2]

Only if duplicates are have some value... I have no idea, so I'm just asking.

[yrudman]

me too :) I am not sure what is correct solution here

[gmcculloug]

Review the comments from @lfu on the thread she listed above #17451 (comment):

This array is to record the id of servers where the automate task has been processed. So all the logs from these servers would be collected in case of troubleshooting. The order of the ids in the array is not important in this case.

and

If a long array is not a concern, we can get rid of calling uniq

So uniq still gives us the ability to see all the places it ran, which was the main purpose, and if we go this way I would skip limiting the array for now so we know where to collect logs from. This is based on my assumption that most environments have a reasonable number of workers per-zone so the list would be contained by that. If you feel this is incorrect than we should limit it.

This is a bit crazy, but since I worked on it I thought I would include it for fun.
If we want to keep a little sense of order the following logic clears out the current server record from the list and then appends it to the end and ensures we only return the last LOG_SERVERS_LIMIT elements. Again, with removing dups we could probably drop this limit here as well.

def last_test_servers(server_list, server_id)
  ((server_list -= [server_id]) << server_id).last(LOG_SERVERS_LIMIT)
end

or without limits:

def last_test_servers(server_list, server_id)
  (server_list -= [server_id]) << server_id
end

[yrudman]

👍 It looks like good compromise

[miq-bot]

Checked commits yrudman/manageiq@98d7bfd~...b7e8fa3 with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍪