Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Update xCAT bindings example, to prevent lsdef from gobbling stdin #580

Closed
wants to merge 4 commits into from

Conversation

kcgthb
Copy link
Contributor

@kcgthb kcgthb commented Feb 7, 2025

lsdef consumes stdin by default, so we feed it /dev/null to avoid issues in read loops (useful in scripts!)
See xcat2/xcat-core#6024 for reference.

@thiell thiell added this to the 1.9.4 milestone Feb 7, 2025
@martinetd
Copy link
Collaborator

I'd argue clustershell should not pass stdin to mapping commands in the first place?

diff --git a/lib/ClusterShell/NodeUtils.py b/lib/ClusterShell/NodeUtils.py
index f4a52f639743..ba8ebad4ec5b 100644
--- a/lib/ClusterShell/NodeUtils.py
+++ b/lib/ClusterShell/NodeUtils.py
@@ -44,7 +44,7 @@ import shlex
 import time
 
 from string import Template
-from subprocess import Popen, PIPE
+from subprocess import Popen, PIPE, DEVNULL
 
 try:
     basestring
@@ -202,8 +202,8 @@ class UpcallGroupSource(GroupSource):
         """
         cmdline = Template(self.upcalls[cmdtpl]).safe_substitute(args)
         self.logger.debug("EXEC '%s'", cmdline)
-        proc = Popen(cmdline, stdout=PIPE, shell=True, cwd=self.cfgdir,
-                     universal_newlines=True)
+        proc = Popen(cmdline, stdin=DEVNULL, stdout=PIPE, shell=True,
+                     cwd=self.cfgdir, universal_newlines=True)
         output = proc.communicate()[0].strip()
         self.logger.debug("READ '%s'", output)
         if proc.returncode != 0:

might want to check if we have more like that...

@thiell
Copy link
Collaborator

thiell commented Feb 7, 2025

Very good point, why are we doing that?!

Maybe because subprocess.DEVNULL was only added in Python 3.3?

In clustershell 1.9.x we should just be careful to do it so it doesn't break the compat for py2.7 (I know, it's old, but it's nice to be able to upgrade older clusters)

@martinetd
Copy link
Collaborator

For 2.7 we apparently need to open devnull ourselves...
That's a bit ugly but not impossible?

# compat with python 2.7, import directly once deprecated
try:
    from subprocess import DEVNULL
except ImportError:
    DEVNULL = open(os.devnull, 'r')

@thiell
Copy link
Collaborator

thiell commented Feb 7, 2025

Looks good to me, thank you! @degremont any opinion on this?

@degremont
Copy link
Collaborator

I'm fine with @martinetd proposal, that's the right thing to do here IMO.

@martinetd
Copy link
Collaborator

I can't push to kilian's tree so opened #581
#581

Note the other commit was already merged in #558, so there really only is the stdin bug fix in this PR and this can be closed.

@martinetd martinetd closed this Feb 7, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants