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

checks if docker is running based on process name #29

Merged
merged 2 commits into from May 26, 2015

Conversation

tiadobatima
Copy link
Contributor

No description provided.

@puneetk
Copy link
Contributor

puneetk commented May 26, 2015

I understand it doesnt work this way currently, but could you please put the sig path in the map.jinja, so that if there is an alternate location for it in an OS or someones setup, they can update the map.jinja.

thanks

@tiadobatima
Copy link
Contributor Author

Done. Reused pkg map key so we don't have too keys scattered all over.

puneetk added a commit that referenced this pull request May 26, 2015
checks if docker is running based on process name
@puneetk puneetk merged commit bd93fd6 into saltstack-formulas:master May 26, 2015
@ticosax
Copy link
Contributor

ticosax commented May 26, 2015

I'm not sure it is the right thing to do here.
why it works for some people but not for you ?
@puneetk I believe this kind of change deserve to be seen and approved by more peers when the submitted issue is not completely clear.

@puneetk
Copy link
Contributor

puneetk commented May 26, 2015

@ticosax sounds good.

@tiadobatima
Copy link
Contributor Author

@ticosax, @puneetk: I'm suspicious this is working for some people... At least on ubuntu, docker's init script returns 1:

root@22df5f0994b5:~/git/salt# ps -ef | grep docker    
root      6920     1  0 19:54 ?        00:00:01 /usr/bin/docker -d -p /var/run/docker.pid
root      6999     1  0 21:47 ?        00:00:00 grep --color=auto docker
root@22df5f0994b5:~/git/salt# /etc/init.d/docker start
 * Starting Docker: docker                                                                                        root@22df5f0994b5:~/git/salt# echo $?
1

Without this using the process signature the state breaks:

----------
          ID: docker-service
    Function: service.running
        Name: docker
      Result: False
     Comment: Service docker is already enabled, and is dead
     Started: 21:54:10.778322
    Duration: 107.402 ms
     Changes:   
              ----------
              docker:
                  False
----------
          ID: test/my_test_image
    Function: docker.pulled
      Result: False
     Comment: One or more requisite failed: docker.docker-service
     Started: 
    Duration: 
     Changes:   

But I'm not sure why the formula is trying to start docker even though the status shows it's running:

[INFO    ] Running state [python-apt] at time 21:54:10.737877
[INFO    ] Executing state pkg.installed for python-apt
[INFO    ] Package python-apt is already installed.
[INFO    ] Completed state [python-apt] at time 21:54:10.738849
[INFO    ] Running state [docker-dependencies] at time 21:54:10.739073
[INFO    ] Executing state pkg.installed for docker-dependencies
[INFO    ] All specified packages are already installed.
[INFO    ] Completed state [docker-dependencies] at time 21:54:10.740002
[DEBUG   ] Could not LazyLoad pkgrepo.mod_init
[INFO    ] Running state [deb https://get.docker.com/ubuntu docker main] at time 21:54:10.741989
[INFO    ] Executing state pkgrepo.managed for deb https://get.docker.com/ubuntu docker main
[INFO    ] Package repo 'deb https://get.docker.com/ubuntu docker main' already configured
[INFO    ] Completed state [deb https://get.docker.com/ubuntu docker main] at time 21:54:10.773988
[INFO    ] Running state [lxc-docker] at time 21:54:10.775244
[INFO    ] Executing state pkg.installed for lxc-docker
[INFO    ] Package lxc-docker is already installed.
[INFO    ] Completed state [lxc-docker] at time 21:54:10.776195
[DEBUG   ] Could not LazyLoad service.mod_init
[INFO    ] Running state [docker] at time 21:54:10.778322
[INFO    ] Executing state service.running for docker
[INFO    ] Executing command ['service', 'docker', 'status'] in directory '/root'
[DEBUG   ] output:  * Docker is running
[INFO    ] Executing command ['service', 'docker', 'start'] in directory '/root'
[ERROR   ] Command ['service', 'docker', 'start'] failed with return code: 1
[ERROR   ] output:  * Starting Docker: docker
[ERROR   ] {'docker': False}
[INFO    ] Completed state [docker] at time 21:54:10.885724

Any ideas?

@ticosax
Copy link
Contributor

ticosax commented May 27, 2015

@tiadobatima on ubuntu I believe you should use:

$ service docker start

And not use directly the /etc/init.d/docker executable

@tiadobatima
Copy link
Contributor Author

Right @ticosax, @puneetk: I was actually using "service" in all my tests. In this particular case, it doesn't matter which is being used because "service" is just executing the sysv init script anyways. And after a long time tracking this weird behaviour, I think understand why now:

  • docker package installs both upstart and sysv init script on Ubuntu
  • "service" on Ubuntu handles both upstart and init scripts
  • if upstart is not running (ie if you want to run salt inside a docker container), service will seamlessly bypass any upstart configs and just try sysv scripts.
  • but salt's upstart.py module puts some extra logic on top of service status(), by checking the output "service docker status" if the upstart config exists (which will fail if upstart is not running) instead of checking for the return code.

I see the comment in upstart.py about why it's done this way, but I'm not totally convinced salt should use the output instead of the return code to handle status. I think the command should fail it it's meant to do so. Notice that it doesn't behave like that in the other actions like start(), stop(), restart()...

As for this formula in particular, until (and if) salt's behaviour changes I believe we have two choices:

  • Always use process signature. This will work regardless the OS, and upstart. This is what we have now after my previous pull request
  • Make the use of the signature configurable with pillar. This adds a bit of extra complexity, the need to document, and most likely questions raised by the users, but it'll try to do the "right thing" by default.

What do you guys think?

@ticosax
Copy link
Contributor

ticosax commented Jun 1, 2015

I think running salt-master and docker within a docker container is too experimental (If I understand your use case correctly) to be the default behaviour. I'm more in favour of making it opt-in.

@tiadobatima
Copy link
Contributor Author

The use case is automated testing @ticosax . Quite handy.

@ticosax
Copy link
Contributor

ticosax commented Jun 2, 2015

I personally consider testing salt integration within a container a bad practice since you can't reproduce the same behaviour if you run ubuntu either on bare-metal or in the container. The environments are too diverging to be a trust-able integration test.
Feel free to ignore my opinion, I just wanted to explain why I consider it experimental.

I will update my fork to current master and give it a try. So let's keep it this way.

@tiadobatima
Copy link
Contributor Author

Hi @ticosax.... Just adding some food for thought on the experimental nature of containers, without wanting to get too religious :)

Container support in Linux is not really beta quality anymore, though, one could argue Docker is. It's just a different "platform". By nature, containers will never "catch up" to bare metal or even VMs. Similarly, VMs do behave differently than bare metal. If one understands the limitations of containers, it's an amazing tool for fast iterations during testing, or even for some production loads. Considering an IBM server can take up to 10 minutes just to start booting, and starting an EC2 instance up to a few minutes, one could run hundreds of tests with containers on that time (depending on the tests of course).

Also, it's surprisingly not that complex to design software and config management deployments to be platform aware, though of course, depending on what we want to do, containers (or even VMs) just won't cut it. We just gotta know what we're doing. Some stuff just can't be done on containers, the same way, but to a lesser extent, some stuff can't be done on VMs.

Anyways... Thanks for checking the the PR!! :)

@ticosax
Copy link
Contributor

ticosax commented Jun 3, 2015

Thank for sharing your point of view. It was interesting to read. I understand better why you choose to go that way. I can be convinced there is good things to run those tests in a container knowing their limitations.
Cheers.

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

3 participants