Low: ping: Avoid temp files in fping_check (bsc#987348)

[krig]

Unfortunately the previous attempt to avoid this issue (commit
7eb958c) didn't work since the problem happens only when the
output buffer is flushed but the write fails. Since there is no
mechanism in the shell to check if a pipe write is successful,
this silently fails.

In ping_check, there are no temporary files created. It's
possible to avoid the temporaries for fping too, but I had to
resort to using python.

[kgaillot]

You didn't want to rewrite the whole thing in python? :-)

[krig]

Hah, well. Sure! I would definitely prefer if it was all python, yes. That's a bit more work than I have time for right now, though. Plus, I feel like if I'm going to start writing resource agents in python, there should be a common library like for the fence agents etc. Suddenly it's a LOT more work. ;)

[jnpkrn]

I'd say this macaronic programming is rather counter-productive and should be avoided if possible (not to speak about introducing new dependencies; note that Requires: python for proper pacemaker package in the spec file is very rough overapproximation that should be eventually fixed). Which is likely a case here, just by combining stdout and stderr and acting on that that line by line in read loop.

Beside that, there are some latent issues in the parsing logic, such as when host_list contains alive or unreachable (literal hostnames) machines to ping...

[jnpkrn]

(in fact, that python dependency is currently justified with docker-wrapper, which is currently something I have no clear idea about)

[kgaillot]

I see this as a first step toward full pythonization of the agent, which would be ideal (the agent is too complex for a shell script to be readable and maintainable).

I do think it would be a good idea to have a python library in resource-agents, but unfortunately that wouldn't help here -- pacemaker agents don't use resource-agents libraries because we don't want to depend on a particular version.

I hadn't noticed that the main pacemaker package depends on python; I didn't think it did. Probably -libs-devel should have the dependency instead. In any case, I don't think we need a strict dependency on python for this particular code, because the agent is not required for a cluster, and rpm doesn't have a "recommends"-style optional dependency, which would be appropriate here. I could see one day pythonizing some of the command-line tools and making -cli depend on python, but I wouldn't want to do that unless we were python 3-compatible.

[kgaillot]

(in fact, that python dependency is currently justified with docker-wrapper, which is currently something I have no clear idea about)

See lib/pengine/complex.c:handle_rsc_isolation(). It's only used by pacemaker itself, as part of the undocumented resource isolation feature.

[jnpkrn]

On 15/08/16 08:30 -0700, Ken Gaillot wrote:

I see this as a first step toward full pythonization of the agent,
which would be ideal (the agent is too complex for a shell script to
be readable and maintainable).

It's a fair deal if this is a midstep in that direction, supposing
nothing worthy will get sacrificed just for the comfort's sake.
After all, it was my voice slighty bashing (pun intended) shell
language for not being as error-proof (handy) as other alternatives:

http://oss.clusterlabs.org/pipermail/users/2015-May/000403.html
http://oss.clusterlabs.org/pipermail/developers/2015-May/000023.html
http://lists.community.tummy.com/pipermail/ocf/2015-May/001439.html

Anyway, what is applicable in general incl. this PR and, I think,
would be desirable:

1/ python invocation should be accompanied with at least -Es options
(we don't want to be affected by the environment, do we?)

and just for this PR (but the principle should be kept in mind
for other possible pythonifications):

2/ use_fping function should check for python executable as well

I do think it would be a good idea to have a python library in
resource-agents, but unfortunately that wouldn't help here --
pacemaker agents don't use resource-agents libraries because we
don't want to depend on a particular version.

There's also a danger of a circular dependency, but that's nothing
fatal or even noteworthy, unlike unsatisfied version :-)

I hadn't noticed that the main pacemaker package depends on python;
I didn't think it did. Probably -libs-devel should have the
dependency instead. In any case, I don't think we need a strict
dependency on python for this particular code, because the agent is
not required for a cluster,

true, but mentioned change 2/ should be made so as to prevent corner
case issues when python command is not present (perhaps just
python3 if any, but this should be figured out at the configure
phase, shouldn't it?)

and rpm doesn't have a "recommends"-style optional dependency, which
would be appropriate here.

No longer true:
http://rpm.org/wiki/Releases/4.12.0#Generalbugfixesandenhancements
http://rpm.org/wiki/PackagerDocs/Dependencies#Weakdependencies

but nothing that could be used instantly, procedurally, technically
(version propagation into practical use and distros as such),
and policy-wise (e.g.,
https://fedoraproject.org/wiki/Packaging:Guidelines#Rich.2FBoolean_dependencies)

I could see one day pythonizing some of the command-line tools
and making -cli depend on python, but I wouldn't want to do that
unless we were python 3-compatible.

Reasonable.

Jan (Poki)

[krig]

Haha, macaronic programming is a great description. I don't disagree with the arguments against mixing in python like this. I'll look at rewriting it in python 2+3 completely. At least that way, the prerequisites will be clear.

[jnpkrn]

On 15/08/16 20:43 +0200, Jan Pokorný wrote:

On 15/08/16 08:30 -0700, Ken Gaillot wrote:

I do think it would be a good idea to have a python library in
resource-agents, but unfortunately that wouldn't help here --
pacemaker agents don't use resource-agents libraries because we
don't want to depend on a particular version.

There's also a danger of a circular dependency, but that's nothing
fatal or even noteworthy, unlike unsatisfied version :-)

scratch this out, I guess I had the rpm query wrong

Jan (Poki)

[kgaillot]

Haha, macaronic programming is a great description. I don't disagree with the arguments against mixing in python like this. I'll look at rewriting it in python 2+3 completely. At least that way, the prerequisites will be clear.

Just merged our first-ever Python coding guidelines: https://github.com/ClusterLabs/pacemaker/blob/master/doc/Pacemaker_Development/en-US/Ch-Python.txt

It includes our intended approach to 2+3 compatibility. It's very much a work in progress, so feel free to recommend changes/additions.