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

Problems with watchers when mixing upper and lower cases #927

Closed
cgarciaarano opened this issue Sep 11, 2015 · 7 comments
Closed

Problems with watchers when mixing upper and lower cases #927

cgarciaarano opened this issue Sep 11, 2015 · 7 comments

Comments

@cgarciaarano
Copy link
Contributor

Hi,

in circus 0.10 the default behaviour when stopping a watcher via circusctl was to match the name of the watcher against the name provided in command line using simple string matching. Now in 0.12.1 the default is to match using glob.

That's a good feature, but now, if you give the exact name of the watcher, the stop command (start and restart too) fails with:

error: program WATCHER_NAME not found

That's because the matching is against the lowercase name of the watcher. I would fix it making lowercase both parameters in circus/commands/restart.py, changing the existing code:

            if match == 'glob':
                name = re.compile(fnmatch.translate(props['name']))
            elif match == 'regex':
                name = re.compile(props['name'])

to

            if match == 'glob':
                name = re.compile(fnmatch.translate(props['name'].lower()))
            elif match == 'regex':
                name = re.compile(props['name'].lower())

Regards

@k4nar
Copy link
Contributor

k4nar commented Sep 12, 2015

Thank you for your report :) .

I can reproduce your issue, and I think that your solution would fix it (maybe we should add an intermediary variable though, like watcher_name).

Could you open a Pull-Request ?

@cgarciaarano
Copy link
Contributor Author

One thought, what happens if a user has watchers with the same name but different case? Something like this:

[watcher:some_process]
cmd=sleep 100

[watcher:some_PROCESS]
cmd=ping 8.8.8.8

This is allowed by Circus, so my solution could have undesirable effects, stopping both processes when any of these commands are issued:

circusctl stop some_process
circusctl stop some_PROCESS
circusctl stop soMe_prOcess
...

I'm not really sure what's the best solution. Avoid matching lowercase could be one (remove .lower()), or simply default to simple match. You tell me, guys.

Thanks

@k4nar
Copy link
Contributor

k4nar commented Sep 15, 2015

I don't think that circus has ever been consistent regarding case sensitivity. The current behavior (forcing lowercase when given as an argument) is clearly wrong, but we have three options here:

  1. Always match as lowercase, therefore circusctl stop Foo would stop foo, Foo and FOO.
  2. Making circusctl case sensitive (this could break compatibility)
  3. Match as lowercase, but make sure that two watcher with the same lowercase name cannot be created.

I would go for option 1 as I can't think of any real world example where one would have two watchers with the same name but cased differently.

@scottkmaxwell
Copy link
Contributor

I agree with option #1 with #3 as extra credit. Option #2 definitely seems like asking for trouble, IMO.

k4nar added a commit that referenced this issue Sep 16, 2015
Match lower case when start/stop/restart a watcher
@k4nar
Copy link
Contributor

k4nar commented Sep 16, 2015

Fixed by #929

@k4nar k4nar closed this as completed Sep 16, 2015
@StefanWiechula
Copy link
Contributor

There are still issues with watchers when mixing upper and lower case names. On master at 610195a try the following.

$ cat check_space.ini
[watcher:check space]
cmd = ./echo.sh "check space"

#[watcher:CHECK SPACE]
#cmd = /bin/sh -c 'while true; do echo "this is CHECK SPACE"; sleep 5; done'

[watcher:check_space]
cmd = ./echo.sh "check_space"
$ cat echo.sh
#!/bin/sh

while true
do echo "this is $1"
        sleep 5
done

First:

bin/circusd check_space.ini

Then, in another shell:

bin/circusctl listen ''
  • Note that all messages published by the activities below are on topics starting with 'watcher.check_space.' (this is an unrelated problem caused by the replacement of spaces with underscores introduced in 374e5e0)

In yet another shell:

$ bin/circusctl list
check space,check_space
  • This is correct.
$ bin/circusctl add --start 'CHECK SPACE' './echo.sh "CHECK SPACE"'
ok
$ bin/circusctl list
check space,check_space
  • This is a problem. There are 3 watchers & processes running but only 2 listed.
$ bin/circusctl add --start 'CHECK SPACE' './echo.sh "FUBAR"'
ok
$ bin/circusctl list
check space,check_space
  • Problematic again, now there are 4 watchers running but only 2 listed.
$ bin/circusctl rm 'check space'
ok
$ bin/circusctl list
check_space
  • Logs show 2015-10-27 10:44:33 circus[62938] [INFO] CHECK SPACE stopped
  • I stop seeing 'FUBAR' in the output.
  • I still see output for 'check space', 'check_space' and 'CHECK SPACE'
$ bin/circusctl rm 'check space'
error: program check space not found

$ bin/circusctl stop 'check space'
ok

$ bin/circusctl start 'check space'
ok

$ bin/circusctl list 'check space'
error: program check space not found
  • I can stop and start, but not list or remove the remaining watchers that alias to 'check space'

I think option 3 in k4nar's comment above is necessary, not just extra credit, and recommend:

  • Defining some requirements or constraints on watcher names
  • Coding those up in a normalize_watcher_name function in circus/util.py
  • Using that normalize function everywhere that circus accepts a watcher name as input (from a config file, typed/piped into circusctl, over zeromq)

If others can contribute to the list of normalization requirements, I'm willing to prepare a pull request. Here are the requirements I am aware of so far:

Are there more?

@StefanWiechula
Copy link
Contributor

Alternatively, I see no problem with removing the space/underscore replacement in circus/watcher.py line 207 and have tested it locally for my own use case. Are others relying on this behaviour?

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

No branches or pull requests

4 participants