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

MiqServer#recently_active can handle values in the future #22962

Merged
merged 1 commit into from
Mar 28, 2024

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 27, 2024

Overview

When running manageiq-providers-workflow workflows, the api url passed to floe is from remote_ws_url.

This value becomes nil unless MiqServer#heartbeat is run every 10 minutes.
In a server this is automatically handled, but in development (i.e.: rails c and simulate_queue_worker) this task falls upon the developer.

Goal

Make it unnecessary in development to call MiqServer#heartbeat every 10 minutes.

Before

# Setting heartbeat to now

MiqServer.last.update(:last_heartbeat => Time.now.utc)
MiqRegion.my_region.remote_ws_url
# => http://localhost:4000/
sleep(10 * 60) # 10 minutes
MiqRegion.my_region.remote_ws_url
# => nil

# Setting a time in the future

MiqServer.last.update(:last_heartbeat => 1.day.from_now.utc)
MiqServer.recently_active.exist?
# aka MiqServer.where(:last_heartbeat => 10.minutes.ago.utc...Time.now.utc).exist?
# => false
MiqRegion.my_region.remote_ws_url
# => nil

After

# Setting a time in the future

MiqServer.last.update(:last_heartbeat => 1.day.from_now.utc)
MiqServer.recently_active.exist?
# aka MiqServer.where(:last_heartbeat => 10.minutes.ago.utc..).exist? <== definition changed
# => true
MiqRegion.my_region.remote_ws_url
# => http://localhost:4000/
sleep(10 * 60) # 10 minutes
MiqRegion.my_region.remote_ws_url
# => http://localhost:4000/

Discussion

This is not the only solution, but seemed the least invasive.

changes query of recently active from:
```sql
-- MiqServer.where(:last_heartbeat => 10.minutes.ago.utc...Time.now.utc).first

SELECT "miq_servers".*
FROM "miq_servers"
WHERE "miq_servers"."last_heartbeat" >= $1
  AND "miq_servers"."last_heartbeat" < $2
ORDER BY "miq_servers"."id" ASC
LIMIT $3
```

to

```sql
-- MiqServer.where(:last_heartbeat => 10.minutes.ago.utc..).first

SELECT "miq_servers".*
FROM "miq_servers"
WHERE "miq_servers"."last_heartbeat" >= $1
ORDER BY "miq_servers"."id" ASC
LIMIT $2
```

Implications
============

This makes development environments (rails s) easier since we can set the heartbeat in the future.
This is necessary to run workflows with rails console / simulate_queue_worker
because 

```ruby
MiqServer.last.update(:last_heartbeat => 1.day.from_now.utc)

before:

MiqServer.where(:last_heartbeat => 10.minutes.ago.utc...Time.now.utc).exist?
=> false
MiqRegion.my_region.remote_ws_url
=> nil

after:

MiqServer.where(:last_heartbeat => 10.minutes.ago.utc..).exist?
=> true

MiqRegion.my_region.remote_ws_url
=> http://localhost:4000/
```
@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2024

Checked commit kbrock@4a57c46 with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@agrare
Copy link
Member

agrare commented Mar 27, 2024

@kbrock what is your last_heartbeat in that example? Is it nil?

@kbrock
Copy link
Member Author

kbrock commented Mar 28, 2024

@agrare I changed the example to make this more clear.

MiqServer.last.update(:last_heartbeat => 1.day.from_now.utc)

For this example, I'm running in rails console.
I set the heartbeat to tomorrow so I wouldn't have to keep updating the heartbeat every 10 minutes in development.


This also fixes issues in production if clocks on machines are a little out of sync. (even thought this is not the intent/purpose)

@kbrock kbrock changed the title Recently active is active within past 10 minutes MiqServer#recently_active can handle values in the future Mar 28, 2024
@jrafanie
Copy link
Member

@kbrock do we have sufficient tests for this? I'm surprised you didn't need to fix a test for this change.

@kbrock
Copy link
Member Author

kbrock commented Mar 28, 2024

@jrafanie heartbeat sets to Time.now.

The edge case that the time is set in the future (due to time drift or intentional) is not defined behavior.
There are tests for the far past (more than 10 minutes) and recent past (less than 10 minutes) but none for the future.

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

I think this is fine. I think remote_ws_miq_server or remote_ui_miq_server returning no value because one or two servers has their time out of sync is NOT a good place for the problem to manifest itself. I would rather let these things go through and let session fail because it looks to be in the future. I think that's easier to diagnose and handle.

@jrafanie jrafanie self-assigned this Mar 28, 2024
@jrafanie jrafanie merged commit 0a98e44 into ManageIQ:master Mar 28, 2024
8 checks passed
@kbrock kbrock deleted the active_server branch March 28, 2024 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants