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

fix issue on Ubuntu with check_running for courier-imap service #21

Merged
merged 1 commit into from Dec 18, 2013

Conversation

Projects
None yet
2 participants
@doc75
Contributor

doc75 commented Dec 15, 2013

When specifying the os_by_host as Ubuntu, the 'check runnning' of courier-imap service (Ubuntu 12.04 tested) is not working.
indeed, the return of service courier-imap status is the following:

  • when running: * Courier IMAP server is running
  • when not running: * Courier IMAP server is not running
@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 16, 2013

Owner

How about service #{escape(service)} status && service #{escape(service)} status | grep 'running' ?

The reason for grepping the string 'running' is that services under upstart return exit status 0 even if they are stopped.

Courier-imap is not under upstart and service courier-imap status returns exit status 1.
So I'd like to use exit status for checking courier-imap.

Owner

mizzy commented Dec 16, 2013

How about service #{escape(service)} status && service #{escape(service)} status | grep 'running' ?

The reason for grepping the string 'running' is that services under upstart return exit status 0 even if they are stopped.

Courier-imap is not under upstart and service courier-imap status returns exit status 1.
So I'd like to use exit status for checking courier-imap.

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 16, 2013

Contributor

OK, I probably misunderstood the way it works then,
Is it too much asking where I can see in the code that you are testing the return code ?
I spent some time to try to understand the mechanic behind specinfra, but I have not enough experience in Ruby to undersand everything.

Contributor

doc75 commented Dec 16, 2013

OK, I probably misunderstood the way it works then,
Is it too much asking where I can see in the code that you are testing the return code ?
I spent some time to try to understand the mechanic behind specinfra, but I have not enough experience in Ruby to undersand everything.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy
Owner

mizzy commented Dec 16, 2013

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 16, 2013

Contributor

Thanks for the info. I now better understand the way it works.

Your proposal is OK and works fine for courier-imap service. I'll push a new version with this fix.

While checking this fix, I found 2 other issues (already existing with current version):
1- For services that do not support status option like courier-authdaemon it is not working well
2- If you mispell the service name and expect it to be running, then it will go through without any problem.

Contributor

doc75 commented Dec 16, 2013

Thanks for the info. I now better understand the way it works.

Your proposal is OK and works fine for courier-imap service. I'll push a new version with this fix.

While checking this fix, I found 2 other issues (already existing with current version):
1- For services that do not support status option like courier-authdaemon it is not working well
2- If you mispell the service name and expect it to be running, then it will go through without any problem.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 17, 2013

Owner

1- For services that do not support status option like courier-authdaemon it is not working well

It's not matter of specinfra. /etc/init.d/courier-authdaemon should support status option.

2- If you mispell the service name and expect it to be running, then it will go through without any problem.

Really?I test service status with the service name that does not exist, it returns exit status 1.

$ service foo status && service foo status|grep running
foo: unrecognized service
exit 1

And this is the test result with serverspec.

$ rake spec
/opt/boxen/rbenv/versions/2.0.0-p247/bin/ruby -S rspec spec/default/httpd_spec.rb
F

Failures:

  1) Service "service-not-exist" should be running
     Failure/Error: it { should be_running   }
       sudo ps aux | grep -w -- service-not-exist | grep -qv grep
       expected Service "service-not-exist" to be running
     # ./spec/default/httpd_spec.rb:4:in `block (2 levels) in <top (required)>'

Finished in 3.73 seconds
1 example, 1 failure

Failed examples:

rspec ./spec/default/httpd_spec.rb:4 # Service "service-not-exist" should be running
/opt/boxen/rbenv/versions/2.0.0-p247/bin/ruby -S rspec spec/default/httpd_spec.rb failed
exit 1

It seems good to me with non-existent service name.

Owner

mizzy commented Dec 17, 2013

1- For services that do not support status option like courier-authdaemon it is not working well

It's not matter of specinfra. /etc/init.d/courier-authdaemon should support status option.

2- If you mispell the service name and expect it to be running, then it will go through without any problem.

Really?I test service status with the service name that does not exist, it returns exit status 1.

$ service foo status && service foo status|grep running
foo: unrecognized service
exit 1

And this is the test result with serverspec.

$ rake spec
/opt/boxen/rbenv/versions/2.0.0-p247/bin/ruby -S rspec spec/default/httpd_spec.rb
F

Failures:

  1) Service "service-not-exist" should be running
     Failure/Error: it { should be_running   }
       sudo ps aux | grep -w -- service-not-exist | grep -qv grep
       expected Service "service-not-exist" to be running
     # ./spec/default/httpd_spec.rb:4:in `block (2 levels) in <top (required)>'

Finished in 3.73 seconds
1 example, 1 failure

Failed examples:

rspec ./spec/default/httpd_spec.rb:4 # Service "service-not-exist" should be running
/opt/boxen/rbenv/versions/2.0.0-p247/bin/ruby -S rspec spec/default/httpd_spec.rb failed
exit 1

It seems good to me with non-existent service name.

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 17, 2013

Contributor

You are right for both point. For #2, my point was to test if a service was not running. But I forgot to test if it is installed (which is going to fail with the mispell).

After some thought I think the patch proposed in this pull request is not correct.
I will re-work on it later on today.

Contributor

doc75 commented Dec 17, 2013

You are right for both point. For #2, my point was to test if a service was not running. But I forgot to test if it is installed (which is going to fail with the mispell).

After some thought I think the patch proposed in this pull request is not correct.
I will re-work on it later on today.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 17, 2013

Owner

I see. Thanks!

Owner

mizzy commented Dec 17, 2013

I see. Thanks!

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 17, 2013

Contributor

Actually the issue mentionned was know in the commit introducing the Ubuntu specific code: 093ecc4

As long as check_running will do this grep on 'running', I do not think we can easily manage the services which are returning a proper return code for their status.

Ideas so far:

  1. put a check in backend/exec.rb (check_running method): return false if ret[:stdout] =~ /is not running/
  2. remove the command/ubuntu.rb override of check_running and consider that output needs to be checks only when return code is zero otherwise we do the check process.

I did not find yet a nice and clean option.

Contributor

doc75 commented Dec 17, 2013

Actually the issue mentionned was know in the commit introducing the Ubuntu specific code: 093ecc4

As long as check_running will do this grep on 'running', I do not think we can easily manage the services which are returning a proper return code for their status.

Ideas so far:

  1. put a check in backend/exec.rb (check_running method): return false if ret[:stdout] =~ /is not running/
  2. remove the command/ubuntu.rb override of check_running and consider that output needs to be checks only when return code is zero otherwise we do the check process.

I did not find yet a nice and clean option.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 18, 2013

Owner

I can't see the point you said.

Current fix seems good to me for checking courier-imap.

Please show me the concrete case that causes the problem with current fix.

Owner

mizzy commented Dec 18, 2013

I can't see the point you said.

Current fix seems good to me for checking courier-imap.

Please show me the concrete case that causes the problem with current fix.

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 18, 2013

Contributor

My mistake again ;-(
I tried several test with courier-imap (returning proper return code) and mysql (returning always zero)
I tested the following matrix:

courier-imap mysql test done result expected result obtained
running running be_running test OK test OK
running running not be_running test KO test KO
stopped stopped be_running test KO test KO
stopped stopped not be_running test OK test OK

I rebased the fix on master.
Thanks for your partience.

Contributor

doc75 commented Dec 18, 2013

My mistake again ;-(
I tried several test with courier-imap (returning proper return code) and mysql (returning always zero)
I tested the following matrix:

courier-imap mysql test done result expected result obtained
running running be_running test OK test OK
running running not be_running test KO test KO
stopped stopped be_running test KO test KO
stopped stopped not be_running test OK test OK

I rebased the fix on master.
Thanks for your partience.

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 18, 2013

Owner

Thanks for the matrix.It's clear.

It seems there are no problem.Can I merge this pull request?

Owner

mizzy commented Dec 18, 2013

Thanks for the matrix.It's clear.

It seems there are no problem.Can I merge this pull request?

@doc75

This comment has been minimized.

Show comment
Hide comment
@doc75

doc75 Dec 18, 2013

Contributor

Yes you can ;-)

Contributor

doc75 commented Dec 18, 2013

Yes you can ;-)

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 18, 2013

Owner

Thanks!

Owner

mizzy commented Dec 18, 2013

Thanks!

mizzy added a commit that referenced this pull request Dec 18, 2013

Merge pull request #21 from doc75/fix_ubuntu
fix issue on Ubuntu with check_running for courier-imap service

@mizzy mizzy merged commit a679e1b into mizzy:master Dec 18, 2013

1 check passed

default The Travis CI build passed
Details

@doc75 doc75 deleted the doc75:fix_ubuntu branch Dec 18, 2013

@mizzy

This comment has been minimized.

Show comment
Hide comment
@mizzy

mizzy Dec 18, 2013

Owner

Released as v0.0.16.

Owner

mizzy commented Dec 18, 2013

Released as v0.0.16.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment