slapgrid: fix invocation of bootstrapBuildout

This commit reverts ddd77222.

The goal is to fix nexedi/slapos.buildout!25 (comment 148861)

os.environ is a special mapping object that transparently modifies the
actual environment and we should never make os.environ make point to
something else (see
nexedi/slapos.buildout!25 (comment 149333)
and https://docs.python.org/3/library/os.html#os.environ)

Instead change the environment only when calling SlapPopen. We will see
if the problems fixed by ddd77222 appear
again...

slapos/grid/SlapObject.py

@@ -266,9 +266,6 @@ class Software(object):
     """ Fetches buildout configuration from the server, run buildout with
     it. If it fails, we notify the server.
     """
-    root_stat = os.stat(self.software_root)
-    os.environ = getCleanEnvironment(logger=self.logger,
-                                     home_path=pwd.getpwuid(root_stat.st_uid).pw_dir)
     try:
       os.mkdir(self.software_path)
     except OSError as e:
@@ -604,8 +601,6 @@ class Partition(object):
                                  'permissions are: 0%o, wanted are 0%o' %
                                  (self.instance_path, permission,
                                   REQUIRED_COMPUTER_PARTITION_PERMISSION))
-    os.environ = getCleanEnvironment(logger=self.logger,
-                                     home_path=pwd.getpwuid(instance_stat_info.st_uid).pw_dir)
 
     # Check that Software Release directory is present
     if not os.path.exists(self.software_path):

slapos/grid/utils.py

@@ -341,6 +341,8 @@ def bootstrapBuildout(path, logger, buildout=None,
     process_handler = SlapPopen(invocation_list,
                                 preexec_fn=lambda: dropPrivileges(uid, gid, logger=logger),
                                 cwd=path,
+                                env=getCleanEnvironment(logger,
+                                     home_path=pwd.getpwuid(os.stat(path).st_uid).pw_dir),
                                 logger=logger)
     if process_handler.returncode is None or process_handler.returncode != 0:
       message = 'Failed to run buildout profile in directory %r' % path

[Thomas Gambier]

This commit broke the slapos.core unit tests

@jerome @jm @kirr do you have an idea what could be wrong? The unit tests are OK outside of Nexedi test environment.

I suppose something is setting $HOME to a tmp directory but this directory doesn't exist when we call os.stat(os.environ['HOME']) and the error was masked because of the os.environ = lines I removed in this commit.

[Jérome Perrin]

@jerome @jm @kirr do you have an idea what could be wrong? The unit tests are OK outside of Nexedi test environment.

In the past when I debugged a "it works on my machine but not on testnodes" problem with slapos.core test, it was actually a problem happening when running all the tests, not just by running the failing test. The test was only failing when it was ran after another test which was changing something.

One cheap way to get the test back might be to assign os.environ['HOME'] to a variable in the global namespace, then it will be evaluated at import time, not at test run time - so it will be the $HOME at the beginning of the tests, even if other tests alter it.

For a more robust fix, my idea was to change getCleanEnvironment to be a context manager that would restore the state on exit. Here we just replaced the global state and expect that the next code path which needs a clean environment will also replace the global state. This pattern was generally error prone, this problem with /root/.slapos_cached_get was also a consequence of this dangerous approach. That's what I had in mind when I sad I will work on this, but let's be honest, I don't think I'll have time to work on this any time soon.

[Jérome Perrin]

There are tests changing os.environ['HOME'] and never resetting back:

• https://lab.nexedi.com/nexedi/slapos.core/blob/7f45fa10b12cff84f6bddc1ec9e933a069d6860f/slapos/tests/test_configure_local.py#L49
• https://lab.nexedi.com/nexedi/slapos.core/blob/72c1088be26326b33fffbc07bb27b0b772755ac5/slapos/tests/test_collect.py#L493
• https://lab.nexedi.com/nexedi/slapos.core/blob/72c1088be26326b33fffbc07bb27b0b772755ac5/slapos/tests/test_collect.py#L641
• https://lab.nexedi.com/nexedi/slapos.core/blob/72c1088be26326b33fffbc07bb27b0b772755ac5/slapos/tests/test_collect.py#L853

pytest has nice support for this, but we don't use pytest here.

would you mind trying a patch like this ? the same cleanup will be needed in this four places

diff --git a/slapos/tests/test_collect.py b/slapos/tests/test_collect.py
index 4f444ebea..a0651383d 100644
--- a/slapos/tests/test_collect.py
+++ b/slapos/tests/test_collect.py
@@ -490,6 +490,7 @@ class TestCollectSnapshot(unittest.TestCase):
         self.slap = slapos.slap.slap()
         self.app = SlapOSApp()
         self.temp_dir = tempfile.mkdtemp()
+        self.addCleanup(os.environ.__setitem__, "HOME", os.environ["HOME"])
         os.environ["HOME"] = self.temp_dir
         self.instance_root = tempfile.mkdtemp()
         self.software_root = tempfile.mkdtemp()

[Thomas Gambier]

Thanks a lot @jerome I did 392a993b and it worked fine. See https://erp5js.nexedi.net/#/test_result_module/20220322-1B8DC3C35/10