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

salt-minion fails to start, claims ctrl-C received #3412

Closed
madduck opened this Issue Jan 23, 2013 · 12 comments

Comments

Projects
None yet
4 participants
@madduck
Contributor

madduck commented Jan 23, 2013

There's a problem with error handling in salt-minion:

# salt-minion -l debug
[INFO    ] Loaded configuration file: /etc/salt/minion
[WARNING ] Setting up the Salt Minion "sysyphus.madduck.net"
[…]
[DEBUG   ] Minion PUB socket URI: ipc:///var/run/salt/minion/minion_event_b4c17aaa68f70acf6a2d38b23cc0c341_pub.ipc
[DEBUG   ] Minion PULL socket URI: ipc:///var/run/salt/minion/minion_event_b4c17aaa68f70acf6a2d38b23cc0c341_pull.ipc

Exiting on Ctrl-c

However, I never touched ctrl-c!

Using strace, I found that this is due to salt-minion unable to bind to the
minion socket:

bind(19, {sa_family=AF_FILE, path="/var/run/salt/minion/minion_event_b4c17aaa68f70acf6a2d38b23cc0c341_pub.ipc"}, 110) = -1 ENOENT (No such file or directory)
close(19)                               = 0
write(2, "\nExiting on Ctrl-c", 18
Exiting on Ctrl-c)     = 18
write(2, "\n", 1)                       = 1
close(3)                                = 0
rt_sigaction(SIGINT, {SIG_DFL, [], SA_RESTORER, 0x7f5921fcd030}, {0x425842, [], SA_RESTORER, 0x7f5921fcd030}, 8) = 0
exit_group(1)                           = ?

This is due to verify_env==False and /var/run being a tmpfs.

I appreciate that Salt let's me turn verify_env off, because quite frankly,
my systems don't need it as nothing would ever change the permissions of the
configuration directories.

Moreover, the docs say that virtual_env is to "Verify and set permissions on
configuration directories at startup". /var/run/salt/minion is not
a configuration directory, but a dynamic run-time directory. Salt should
always create it if it isn't present.

And Salt should not claim that ctrl-c was hit when it falls over a problem.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 24, 2013

What would be the disadvantage of leaving verify_env at it's default? I also use tmpfs's and clearly it is a problem to combine that with verify_env: off, so I'm thinking one should not do that in this case.

Agreed it would be technically more correct to apply more fine-grained directory/permission enforcement, but without it causing a problem it's hard to justify a change.

ghost commented Jan 24, 2013

What would be the disadvantage of leaving verify_env at it's default? I also use tmpfs's and clearly it is a problem to combine that with verify_env: off, so I'm thinking one should not do that in this case.

Agreed it would be technically more correct to apply more fine-grained directory/permission enforcement, but without it causing a problem it's hard to justify a change.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 24, 2013

Contributor

What would be the disadvantage of leaving verify_env at it's
default?

No disadvantage, really, except it feels like a hack to me, and it
shouldn't be necessary. No configuration setting should exist to
disable the ad-hoc creation of temporary, run-time directories and
files in /var/run because Salt will simply fail to work without
them.

Whether it is necessary to check/enforce/fix permissions on startup
of a software depends on whether it can be expected that something
changes those permissions behind the admin's back. I hope that no
users of Salt are willingly dealing with systems like that and
contend themselves with having verify_env fix it, rather than
solving the root of the problem.

Contributor

madduck commented Jan 24, 2013

What would be the disadvantage of leaving verify_env at it's
default?

No disadvantage, really, except it feels like a hack to me, and it
shouldn't be necessary. No configuration setting should exist to
disable the ad-hoc creation of temporary, run-time directories and
files in /var/run because Salt will simply fail to work without
them.

Whether it is necessary to check/enforce/fix permissions on startup
of a software depends on whether it can be expected that something
changes those permissions behind the admin's back. I hope that no
users of Salt are willingly dealing with systems like that and
contend themselves with having verify_env fix it, rather than
solving the root of the problem.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 24, 2013

Essentially the functionality you seek is already there. Salt does setup it's directories and permissions, and verify_env is a switch we have given the user to disable that. the term 'configuration' in the description of what verify_env does could probably say 'run-time directories' instead to be clearer.

The option was added at a time when Salt was struggling to 'get it right', and there are still some run-time locations being updated. The option should probably be removed from the user interface --that's basically what you are saying isn't it @madduck?

ghost commented Jan 24, 2013

Essentially the functionality you seek is already there. Salt does setup it's directories and permissions, and verify_env is a switch we have given the user to disable that. the term 'configuration' in the description of what verify_env does could probably say 'run-time directories' instead to be clearer.

The option was added at a time when Salt was struggling to 'get it right', and there are still some run-time locations being updated. The option should probably be removed from the user interface --that's basically what you are saying isn't it @madduck?

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 25, 2013

Contributor

Essentially the functionality you seek is already there. Salt does
setup it's directories and permissions, and verify_env is a switch
we have given the user to disable that. the term 'configuration'
in the description of what verify_env does could probably say
'run-time directories' instead to be clearer.

Okay, then let me point at it from a different direction: I do not
want Salt to mess with the permissions in /etc. In my world (and one
of the main reasons for Debian) is that /etc is sysadmin-domain and
nothing there must ever be changed implicitly.

Contributor

madduck commented Jan 25, 2013

Essentially the functionality you seek is already there. Salt does
setup it's directories and permissions, and verify_env is a switch
we have given the user to disable that. the term 'configuration'
in the description of what verify_env does could probably say
'run-time directories' instead to be clearer.

Okay, then let me point at it from a different direction: I do not
want Salt to mess with the permissions in /etc. In my world (and one
of the main reasons for Debian) is that /etc is sysadmin-domain and
nothing there must ever be changed implicitly.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 25, 2013

Maybe we should both 1) remove the option and 2) change what it does so that /etc is only setup when the package is installed (it is in my package)

@thatch45 are we at a point where we can remove /etc from the scope of what verify_env does? The package installation does set that part up. /var is left for Salt to handle at run-time.

ghost commented Jan 25, 2013

Maybe we should both 1) remove the option and 2) change what it does so that /etc is only setup when the package is installed (it is in my package)

@thatch45 are we at a point where we can remove /etc from the scope of what verify_env does? The package installation does set that part up. /var is left for Salt to handle at run-time.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 25, 2013

( pardon me @thatch45 I misread ssiue #3396 )

FWIW it is only the PKI dir that is affected by verify_env in /etc, AFAICS. I sympathize with you about not having that under /var, but I'm glad about that because I also use a tmpfs there.

As a work-around you can set your own pki_dir to live in /var someplace and that should keep verify_env out of /etc for you.

ghost commented Jan 25, 2013

( pardon me @thatch45 I misread ssiue #3396 )

FWIW it is only the PKI dir that is affected by verify_env in /etc, AFAICS. I sympathize with you about not having that under /var, but I'm glad about that because I also use a tmpfs there.

As a work-around you can set your own pki_dir to live in /var someplace and that should keep verify_env out of /etc for you.

@ghost ghost closed this Jan 25, 2013

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 25, 2013

Contributor

Maybe we should both 1) remove the option and 2) change what it
does so that /etc is only setup when the package is installed (it
is in my package)

That sounds good. It is also standard on Unix that software
installed from source requires the admin to set up configuration
manually, while packaging distros do it for you.

Contributor

madduck commented Jan 25, 2013

Maybe we should both 1) remove the option and 2) change what it
does so that /etc is only setup when the package is installed (it
is in my package)

That sounds good. It is also standard on Unix that software
installed from source requires the admin to set up configuration
manually, while packaging distros do it for you.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 25, 2013

Contributor

Good to know about /etc/salt/pki, that wasn't clear to me. Sorry.

I am reopening this issue because I think the error handling needs work.

Contributor

madduck commented Jan 25, 2013

Good to know about /etc/salt/pki, that wasn't clear to me. Sorry.

I am reopening this issue because I think the error handling needs work.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 25, 2013

Contributor

Except I don't know how to reopen on GitHub. Can you please?

Contributor

madduck commented Jan 25, 2013

Except I don't know how to reopen on GitHub. Can you please?

@SEJeff SEJeff reopened this Jan 25, 2013

@thatch45 thatch45 closed this in 917c36a Jan 25, 2013

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 25, 2013

Member

The problem is that @s0undt3ch went finally crazy and this is causing real errors to not float up to the display,making it VERY DIFFICULT to discover the real issue, I am sure there is a very valid exception down here that we need to weed out

Member

thatch45 commented Jan 25, 2013

The problem is that @s0undt3ch went finally crazy and this is causing real errors to not float up to the display,making it VERY DIFFICULT to discover the real issue, I am sure there is a very valid exception down here that we need to weed out

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jan 25, 2013

wow. sorry to shut you out too soon, @madduck --my bad.

ghost commented Jan 25, 2013

wow. sorry to shut you out too soon, @madduck --my bad.

@s0undt3ch

This comment has been minimized.

Show comment
Hide comment
@s0undt3ch

s0undt3ch Jan 25, 2013

Member

/me hides after blushing

Member

s0undt3ch commented Jan 25, 2013

/me hides after blushing

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