diff --git a/docs/overview.rst b/docs/overview.rst index 87917126e8..cfe4515ff6 100644 --- a/docs/overview.rst +++ b/docs/overview.rst @@ -662,6 +662,9 @@ TMT_PLAN_ENVIRONMENT_FILE **not** allowed, use ``TMT_PLAN_SOURCE_SCRIPT`` instead to include other bash commands. + Note that this is not shared between guests of the given plan, each + guest has its own dedicated file. + Example of the file content:: COUNT=1 diff --git a/docs/releases/pending/4772.fmf b/docs/releases/pending/4772.fmf new file mode 100644 index 0000000000..57107487a5 --- /dev/null +++ b/docs/releases/pending/4772.fmf @@ -0,0 +1,7 @@ +description: | + The :ref:`plan environment file ` is now + guest-specific, it is no longer shared by all guests. Being shared + made the file open to race conditions, as tmt would fetch it from + all guests, saving all input files under the same name - the last + one saved wins. The filename is now guest specific, files do not + overwrite each other. diff --git a/tests/unit/test_guest.py b/tests/unit/test_guest.py index ab9c1f62be..67b02d104d 100644 --- a/tests/unit/test_guest.py +++ b/tests/unit/test_guest.py @@ -154,6 +154,11 @@ def test_execute_no_connection_closed( logger=root_logger, parent=step, name='foo', data=GuestSshData(primary_address='bar') ) + monkeypatch.setattr( + guest, + '_prepare_environment', + MagicMock(return_value=Environment()), + ) monkeypatch.setattr( guest, '_run_guest_command', diff --git a/tmt/base/plan.py b/tmt/base/plan.py index f231a3ffc8..eb1181db60 100644 --- a/tmt/base/plan.py +++ b/tmt/base/plan.py @@ -471,7 +471,6 @@ def _environment_from_intrinsics(self) -> Environment: if self.my_run: environment['TMT_PLAN_DATA'] = EnvVarValue(self.data_directory) - environment['TMT_PLAN_ENVIRONMENT_FILE'] = EnvVarValue(self.plan_environment_file) environment['TMT_PLAN_SOURCE_SCRIPT'] = EnvVarValue(self.plan_source_script) return environment @@ -504,25 +503,6 @@ def _environment_from_cli(self) -> Environment: logger=self._logger, ) - @property - def _environment_from_plan_environment_file(self) -> Environment: - """ - Environment sourced from the :ref:`plan environment file `. - """ - - if ( - self.my_run - and self.plan_environment_file.exists() - and self.plan_environment_file.stat().st_size > 0 - ): - return tmt.utils.Environment.from_file( - filename=self.plan_environment_file.name, - root=self.plan_environment_file.parent, - logger=self._logger, - ) - - return Environment() - @property def environment(self) -> Environment: """ @@ -531,7 +511,6 @@ def environment(self) -> Environment: Contains all environment variables collected from multiple sources (in the following order): - * :ref:`plan environment file `, * plan's ``environment`` and ``environment-file`` keys, * importing plan's environment, * ``--environment`` and ``--environment-file`` options, @@ -542,7 +521,6 @@ def environment(self) -> Environment: if self.my_run: return Environment( { - **self._environment_from_plan_environment_file, **self._environment_from_fmf, **self._environment_from_importing, **self._environment_from_cli, @@ -711,17 +689,6 @@ def _initialize_data_directory(self) -> None: self.debug(f"Create the data directory '{self.data_directory}'.", level=2) self.data_directory.mkdir(exist_ok=True, parents=True) - @functools.cached_property - def plan_environment_file(self) -> Path: - assert self.data_directory is not None # narrow type - - plan_environment_file_path = self.data_directory / "variables.env" - plan_environment_file_path.touch(exist_ok=True) - - self.debug(f"Create the environment file '{plan_environment_file_path}'.", level=2) - - return plan_environment_file_path - @functools.cached_property def plan_source_script(self) -> Path: assert self.data_directory is not None # narrow type diff --git a/tmt/guest/__init__.py b/tmt/guest/__init__.py index fa636c4772..9016eda396 100644 --- a/tmt/guest/__init__.py +++ b/tmt/guest/__init__.py @@ -58,6 +58,8 @@ ) from tmt.utils import ( Command, + Environment, + EnvVarValue, GeneralError, OnProcessEndCallback, OnProcessStartCallback, @@ -78,6 +80,9 @@ T = TypeVar('T') +#: Name of the :ref:`plan environment file `. +PLAN_ENVIRONMENT_FILENAME = 'variables.env' + #: How many seconds to wait for a connection to succeed after guest boot. #: This is the default value tmt would use unless told otherwise. DEFAULT_CONNECT_TIMEOUT = 2 * 60 @@ -1823,6 +1828,41 @@ def scripts_path(self) -> Path: else tmt.steps.scripts.DEFAULT_SCRIPTS_DEST_DIR ) + @functools.cached_property + def plan_environment_path(self) -> Optional[Path]: + """ + A path to the :ref:`plan environment file ` file. + """ + + if not isinstance(self.parent, tmt.steps.provision.Provision): + return None + + path = self.parent.plan.data_directory / f'{PLAN_ENVIRONMENT_FILENAME}-{self.safe_name}' + path.touch(exist_ok=True) + + self.debug(f"Create the environment file '{path}'.", level=2) + + return path + + @property + def plan_environment(self) -> Environment: + """ + Environment sourced from the :ref:`plan environment file `. + """ + + if ( + self.plan_environment_path + and self.plan_environment_path.exists() + and self.plan_environment_path.stat().st_size > 0 + ): + return tmt.utils.Environment.from_file( + filename=self.plan_environment_path.name, + root=self.plan_environment_path.parent, + logger=self._logger, + ) + + return Environment() + @classmethod def options(cls, how: Optional[str] = None) -> list[tmt.options.ClickOptionDecoratorType]: """ @@ -2139,15 +2179,19 @@ def _prepare_command_environment( """ if environment is None: - # narrow type - assert isinstance(self.parent, tmt.steps.Step) - environment = tmt.utils.Environment() - environment.update( - self.environment, - self.parent.plan.environment, - ) + environment.update(self.environment) + + if isinstance(self.parent, tmt.steps.Step): + environment.update(self.parent.plan.environment) + + # TODO: this was owned by plan, but at wrong position, and it will + # be owned by plan again once the dust of environment untangling + # settles. Follow https://github.com/teemtee/tmt/issues/4241 for + # more. + if self.plan_environment_path: + environment['TMT_PLAN_ENVIRONMENT_FILE'] = EnvVarValue(self.plan_environment_path) else: # Create a copy of given environment - this prevents any diff --git a/tmt/steps/execute/__init__.py b/tmt/steps/execute/__init__.py index f80fa791ac..9c341aad73 100644 --- a/tmt/steps/execute/__init__.py +++ b/tmt/steps/execute/__init__.py @@ -365,6 +365,7 @@ def environment(self) -> Environment: environment.update( self.guest.environment, self.test.environment, + self.guest.plan_environment, self.phase.parent.plan.environment, ) @@ -384,6 +385,12 @@ def environment(self) -> Environment: else: environment = self._environment + # TODO: this was owned by plan, but at wrong position, and it will + # be owned by plan again once the dust of environment untangling + # settles. Follow https://github.com/teemtee/tmt/issues/4241 for + # more. + environment['TMT_PLAN_ENVIRONMENT_FILE'] = EnvVarValue(self.guest.plan_environment_path) + environment.update( # Add variables from invocation contexts self.abort, diff --git a/tmt/steps/provision/local.py b/tmt/steps/provision/local.py index 502879a10a..2b7334c229 100644 --- a/tmt/steps/provision/local.py +++ b/tmt/steps/provision/local.py @@ -132,10 +132,7 @@ def execute( sourced_files = sourced_files or [] # Prepare the environment (plan/cli variables override) - environment = tmt.utils.Environment() - environment.update(env or {}) - if self.parent: - environment.update(self.parent.plan.environment) + environment = self._prepare_command_environment(environment=env) if tty: self.warn("Ignoring requested tty, not supported by the 'local' provision plugin.")