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

Allow running multiple orchestrators in active-passive mode #335

Merged
merged 2 commits into from Sep 3, 2019

Conversation

adamruzicka
Copy link
Contributor

@adamruzicka adamruzicka commented Aug 14, 2019

Upon startup, the orchestrator checks if dynflow_orchestrator_uuid key is unset in Redis. If so, it acquires an orchestrator lock by associating the uuid of the orchestrator's world with the key. However this lock has limited time to live and needs to be periodically
refreshed.

If the key is set, the orchestrator goes into passive mode and periodically polls the lock's status. If it manages to acquire the lock, it becomes the active orchestrator.

Active orchestrator periodically updates the record in Redis to ensure its freshness.

Redmine: https://projects.theforeman.org/issues/27412

@adamruzicka
Copy link
Contributor Author

Rubocop should be fixed by #334

Upon startup, the orchestrator checks if dynflow_orchestrator_uuid key
is unset in Redis. If so, it acquires an orchestrator lock by
associating the uuid of the orchestrator's world with the key. However
this lock has limited time to live and needs to be periodically
refreshed.

If the key is set, the orchestrator goes into passive mode and
periodically polls the lock's status. If it manages to acquire the lock,
it becomes the active orchestrator.

Active orchestrator periodically updates the record in Redis to ensure
its freshness.
Copy link
Contributor

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Can't we use https://github.com/kenn/redis-mutex ? I just generally do not like to make something what is out there already 🤔

mode = nil
loop do
active = ::Sidekiq.redis do |conn|
conn.set(REDIS_LOCK_KEY, @world.id, :ex => REDIS_LOCK_TTL, :nx => true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the set operation on redis atomic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

@adamruzicka
Copy link
Contributor Author

I looked at it briefly and I didn't exactly set our use case. We need to take the lock, make it expire after a while, but keep refreshing it as long as we hold it. As far as I can tell, the refreshing part is not implemented there. But I'd be happy to be proven wrong :)

@ezr-ondrej
Copy link
Contributor

no I haven't seen the decay introduced there. It's effective only for a period of time, and thus can be probably forced to way we want, but it wouldn't feel right.
I'm just thinking if this actually make sense to run more orchestrators just to make the takeover faster opposed to restart of the orchestrator?
On the other hand it probably makes sense to lock the redis to prevent issues if multiple executors are ran by accident, right?

@adamruzicka
Copy link
Contributor Author

This was meant mainly as a safeguard to prevent accidentally running multiple orchestrators. I could change it to only prevent another orchestrator from starting if some is already active, but imo this gives a better user experience.

Copy link
Contributor

@ezr-ondrej ezr-ondrej 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 as a start, we can change the behaviour upon user feedback.
Thanks @adamruzicka 👍

@ezr-ondrej ezr-ondrej merged commit 209a674 into Dynflow:master Sep 3, 2019
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.

None yet

2 participants