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

Replace the ps ax | grep ... call with pkill #16

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vrachnis
Copy link

Using the pipes has the nasty side effect of killing the service command
itself. Having a guest called openbsd0 these are the processes killed:

    root@i5:~ # ps ax | grep openbsd0
    33916  2  Is+      0:00.01 csh -c /bin/sh /usr/local/etc/rc.d/vm onerun openbsd0
    33920  2  I+       0:00.01 /bin/sh /usr/local/etc/rc.d/vm onerun openbsd0
    33955  2  S+       0:13.20 bhyve: openbsd0 (bhyve)
    34114  3  S+       0:00.00 grep --color openbsd0

Killing the grep process stops the execution of the f_stop function
before cleaning up /dev/vmm and the tap interfaces.

   root@i5:~ # tmux ls
   openbsd0: 1 windows (created Fri Oct 31 01:14:20 2014) [159x69]
   root@i5:~ # service vm onestop openbsd0

   Stopping openbsd0

   Sending ACPI shutdown to openbsd0
   Terminated
   root@i5:~ # pgrep -lf bhyve
   root@i5:~ # ls /dev/vmm
   openbsd0

This issue was not present when running the stop directive without an
argument (service vm onestop).

After this patch, and using the pkill command I didn't notice any
leftovers after the stop command, and it didn't change the behavior of
service vm onestop, though I only have one guest on this host, and I
didn't try the loop.

Using the pipes has the nasty side effect of killing the service command
itself. Having a guest called openbsd0 these are the processes killed:

        root@i5:~ # ps ax | grep openbsd0
        33916  2  Is+      0:00.01 csh -c /bin/sh /usr/local/etc/rc.d/vm onerun openbsd0
        33920  2  I+       0:00.01 /bin/sh /usr/local/etc/rc.d/vm onerun openbsd0
        33955  2  S+       0:13.20 bhyve: openbsd0 (bhyve)
        34114  3  S+       0:00.00 grep --color openbsd0

Killing the grep process stops the execution of the f_stop function
before cleaning up /dev/vmm and the tap interfaces.

       root@i5:~ # tmux ls
       openbsd0: 1 windows (created Fri Oct 31 01:14:20 2014) [159x69]
       root@i5:~ # service vm onestop openbsd0

       Stopping openbsd0

       Sending ACPI shutdown to openbsd0
       Terminated
       root@i5:~ # pgrep -lf bhyve
       root@i5:~ # ls /dev/vmm
       openbsd0

This issue was not present when running the stop directive without an
argument (service vm onestop).

After this patch, and using the pkill command I didn't notice any
leftovers after the stop command, and it didn't change the behavior of
`service vm onestop`, though I only have one guest on this host, and I
didn't try the loop.
@vrachnis
Copy link
Author

By the way, maybe I should use pkill -TERM -f "bhyve: $1" if that is better. That way the vm script won't be killed either.

@michaeldexter
Copy link
Owner

On 10/31/14 9:51 AM, vrachnis wrote:

By the way, maybe I should use pkill -TERM -f "bhyve: $1" if that
is better. That way the vm script won't be killed either.

There was definitely some risky placeholder code. I have added new
routines this last two days and have some additional ones commented out.
I welcome your feedback on these.

They are in the snapshot at: http://vmrc.bsd.lv/vmrc-snapshot.tar and
should be easy to find in 'vm' as there are few instances of 'kill'.

Thank you for testing this and suggesting ideas.

Michael

@vrachnis
Copy link
Author

vrachnis commented Nov 6, 2014

Indeed, pkill -f is equivalent to grep | grep -v as Wikipedia notes.

On the other hand, I was also thinking about a completely different approach. Right now, the stop call will attempt to terminate all the guests with the given name. If it is acceptable to assume that the only bhyve guests running in the system are managed by vmrc, maybe the bhyve pid could be stored in some place like /usr/local/vmrc/vm0/vm0.pid and then use that file with the kill command to send the ACPI signal.

@michaeldexter
Copy link
Owner

Are you willing to pull and try this again? I'm not dead, it's just been stable enough for production use.

@vrachnis
Copy link
Author

vrachnis commented Jun 3, 2018

I had actually forgotten about this PR. Since vm_pid is now detected by running ps axwww | grep -v grep | grep "bhyve: $vmname (bhyve)" | awk '{ print $1 }' , the issue that I was trying to address is now gone.

In theory, we could replace all those pipes with pgrep -f "bhyve: $vmname (bhyve)", but that's a preferential and extremely minor change. So, as far as I am concerned, we can close this PR.

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.

2 participants