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

Don't consider opscode-chef-mover service status when checking ha-status #536

Merged
merged 1 commit into from
Sep 21, 2015
Merged

Conversation

poliva83
Copy link
Contributor

Obvious fix for #361

@@ -94,6 +94,9 @@
ha_services = Dir.chdir(running_config['runit']['sv_dir']){Dir.glob('**/keepalive_me').map{|f| File.dirname(f)}}.sort

get_all_services.sort.each do |service_name|
# opscode-chef-mover service is only used during an upgrade, and does not need to be running all of the time
next if service_name.eql? "opscode-chef-mover"
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to use omnibus-ctl's concept of hidden services here. In that case, this check would become:

next if hidden_services.include?(service_name)

@stevendanna
Copy link
Contributor

Thanks! This is a good improvement, I left a minor comment about using the hiddent_services array that omnibus-ctl already provides.

@poliva83
Copy link
Contributor Author

@stevendanna I can change the PR to use hidden_services array. But can you please explain to me how this array becomes available to the ha.rb script?

The only reference to it I found is here:

But to be perfectly honest I didn't really understand how ha.rb script was able to leverage the get_all_services array either.

@stevendanna
Copy link
Contributor

@poliva83 Happy to try to explain.

TL;DR: You have access to any instance method in the Omnibus::Ctl class.

When we load a command definition like ha.rb, an instance of
Omnibus::Ctl eval's the relevant file:

https://github.com/chef/omnibus-ctl/blob/master/lib/omnibus-ctl.rb#L172-L174

The add_command_under_category function used to define new
commands, is actually a methods on the Omnibus::Ctl class. It
uses the block provided to add_command_under_category to define a
new instance method the Omnibus::ctl class:

https://github.com/chef/omnibus-ctl/blob/master/lib/omnibus-ctl.rb#L190

That new instance method, like any other instance method in that
class, has access to all other the other methods already defined
in Omnibus::Ctl.

Both hidden_services and get_all_services are instance methods in
the Omnibus::Ctl class

https://github.com/chef/omnibus-ctl/blob/master/lib/omnibus-ctl.rb#L320
https://github.com/chef/omnibus-ctl/blob/master/lib/omnibus-ctl.rb#L397

which is why you have access to them here.

…when checking ha-status. This is based on assumption the opscode-chef-mover service is only used during an upgrade, and does not need to be running all of the time.
@poliva83
Copy link
Contributor Author

@stevendanna Thanks for explanation, things are more clear now. I tested and updated my pull request. Should be good now.

@stevendanna
Copy link
Contributor

:+1 This looks good to merge.

stevendanna added a commit that referenced this pull request Sep 21, 2015
Don't consider opscode-chef-mover service status when checking ha-status

ChangeLog-Entry: [chef-server-ctl] Don't consider opscode-chef-mover service status when checking ha-status
@stevendanna stevendanna merged commit 729d144 into chef:master Sep 21, 2015
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.

3 participants