Using discover step names containing spaces as a directory name causes troubles #1520

kkaarreell · 2022-09-19T12:04:56Z

Having a discover step name like

  discover:
    - name: Configure system with SW TPM and IMA
      how: fmf
      url: https://github.com/RedHat-SP-Security/keylime-tests
      ref: rhel-9-main

the step name will be used as a directory name, e.g.
/var/tmp/tmt/run-002/plans/keylime-with-swtpm/discover/Configure system with SW TPM and IMA/tests/setup/configure_tpm_emulator
Spaces in such a directory can cause troubles for tests and tooling that is not working with spaces properly.
It would be safer to replace spaces e.g. with underscores.

The text was updated successfully, but these errors were encountered:

psss · 2022-09-19T13:20:48Z

Yes, makes sense and should be easy to address. The fix should cover all steps, not just the discover step.

idorax · 2022-09-20T14:28:56Z

@kkaarreell and @psss, does it make sense if we use MD5 (128-bit) checksum or UUID to convert the name to name used in the directory? e.g.

>>> import uuid
>>> name1 = 'Configure system with SW TPM and IMA'
>>> str(uuid.uuid5(uuid.NAMESPACE_DNS, name1))
'07ec9773-ac81-58c4-9636-a817ad34646c'
>>>

psss · 2022-09-20T14:39:59Z

I don't think so. Very often it's useful to investigate run/plan/step directories and it's useful to pick directory by name.

kkaarreell · 2022-09-20T15:14:14Z

Exactly, I enter that dir when troubleshooting and run test manually. MD5 sum would be almost unusable

idorax · 2022-09-22T11:26:45Z

OK， will replace the spaces with underscores.

idorax · 2022-09-22T14:14:57Z

If name is specified in discover phase, _workdir is set after invoking tmt/steps/init.py#L909

# https://github.com/teemtee/tmt/blob/main/tmt/steps/__init__.py#L909
   906	    def go_prolog(self) -> None:
   907	        """ Perform actions shared among plugins when beginning their tasks """
   908	        # Show the method
   909	        self.info('how', self.get('how'), 'magenta')

Here is the output from debugging, which is helpful to know where _workdir is set, but we need further debugging to find out where it is exactly from.

(tmt) huanli@kvm-01-guest02:foo$ tree -a
.
├── .fmf
│   └── version
├── plans
│   └── foo.fmf
└── tests
    └── foo
        ├── main.fmf
        └── test.sh

4 directories, 4 files
(tmt) huanli@kvm-01-guest02:foo$ cat plans/foo.fmf 
summary: https://github.com/teemtee/tmt/issues/1520 
discover:
    name: A B C D e f g
    how: fmf
execute:
    how: tmt
(tmt) huanli@kvm-01-guest02:foo$ pt run discover
> /home/huanli/.virtualenvs/tmt/bin/tmt(3)<module>()
-> import click
(Pdb) b tmt/steps/__init__.py:909
Breakpoint 1 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py:909
(Pdb) b tmt/steps/__init__.py:911
Breakpoint 2 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py:911
(Pdb) c
/var/tmp/tmt/run-014

/plans/foo
    discover
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py(909)go_prolog()
-> self.info('how', self.get('how'), 'magenta')
(Pdb) pp self
<tmt.steps.discover.fmf.DiscoverFmf object at 0x7f1066c93430>
(Pdb) pp self.__dict__
{'_relative_indent': 1,
 'data': DiscoverFmfStepData(name='A B C D e f g',
                             how='fmf',
                             order=50,
                             summary=None,
                             dist_git_source=False,
                             dist_git_type=None,
                             url=None,
                             ref=None,
                             path=None,
                             test=[],
                             link=[],
                             filter=[],
                             modified_only=False,
                             modified_url=None,
                             modified_ref=None,
                             dist_git_init=False,
                             dist_git_remove_fmf_root=False,
                             dist_git_merge=False,
                             dist_git_extract=None,
                             fmf_id=False,
                             exclude=[],
                             repository=None,
                             revision=None),
 'name': 'A B C D e f g',
 'order': 50,
 'parent': <tmt.steps.discover.Discover object at 0x7f1066dd7370>,
 'step': <tmt.steps.discover.Discover object at 0x7f1066dd7370>}
(Pdb) pp self._workdir  # <<<< NOTE: it's None
None
(Pdb) l
904  	    # Therefore we need a different name, and a way how not to forget to call this
905  	    # method from child classes.
906  	    def go_prolog(self) -> None:
907  	        """ Perform actions shared among plugins when beginning their tasks """
908  	        # Show the method
909 B->	        self.info('how', self.get('how'), 'magenta')
910  	        # Give summary if provided
911 B	        if self.get('summary'):
912  	            self.info('summary', self.get('summary'), 'magenta')
913  	        # Show name only if it's not the default one
914  	        if not self.name.startswith(tmt.utils.DEFAULT_NAME):
(Pdb) c
        how: fmf
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py(911)go_prolog()
-> if self.get('summary'):
(Pdb) pp self._workdir  # <<<< NOTE: it's not None any more
'/var/tmp/tmt/run-014/plans/foo/discover/A B C D e f g'
(Pdb) p self.get
<bound method BasePlugin.get of <tmt.steps.discover.fmf.DiscoverFmf object at 0x7f1066c93430>>

idorax · 2022-09-23T13:33:24Z

To find out where the path of self._workdir is produced, let's just find all os.makedirs() because self._workdir will be created. Once we find out when the directory path (e.g. /var/tmp/tmt/run-014/plans/foo/discover/A B C D e f g) is created, we can easily get the function which helps to create the directory path.

01 - All lines having os.makedirs()

$ find tmt -type f | xargs grep -in 'os.makedirs' | \
  awk -F':' '{print $1":"$2}' | sort -u 
tmt/base.py:896
tmt/base.py:916
tmt/steps/discover/__init__.py:118
tmt/steps/execute/__init__.py:191
tmt/steps/prepare/install.py:165
tmt/steps/provision/__init__.py:740
tmt/steps/provision/testcloud.py:516
tmt/steps/provision/testcloud.py:517
tmt/utils.py:1683
tmt/utils.py:204
tmt/utils.py:729
tmt/utils.py:744

02 - Patch to tmt/steps/discover/fmf.py to help debugging

diff --git a/tmt/steps/discover/fmf.py b/tmt/steps/discover/fmf.py
index 0611461..413db4e 100644
--- a/tmt/steps/discover/fmf.py
+++ b/tmt/steps/discover/fmf.py
@@ -219,7 +219,9 @@ class DiscoverFmf(tmt.steps.discover.DiscoverPlugin):
 
     def go(self) -> None:
         """ Discover available tests """
+        print('XXX-001', self._workdir)
         super(DiscoverFmf, self).go()
+        print('XXX-002', self._workdir)
 
         # Check url and path, prepare test directory
         url = self.get('url')

03 - Find out the function which helps to create directory path

(tmt) huanli@kvm-01-guest02:foo$ python3 -m pdb ~/.virtualenvs/tmt/bin/tmt run discover
> /home/huanli/.virtualenvs/tmt/bin/tmt(3)<module>()
-> import click
(Pdb) b tmt/base.py:896
Breakpoint 1 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/base.py:896
(Pdb) b tmt/base.py:916
Breakpoint 2 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/base.py:916
(Pdb) b tmt/steps/discover/__init__.py:118
Breakpoint 3 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/discover/__init__.py:118
(Pdb) b tmt/steps/execute/__init__.py:191
Breakpoint 4 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/execute/__init__.py:191
(Pdb) b tmt/steps/prepare/install.py:165
Breakpoint 5 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/prepare/install.py:165
(Pdb) b tmt/steps/provision/__init__.py:740
Breakpoint 6 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/provision/__init__.py:740
(Pdb) b tmt/steps/provision/testcloud.py:516
Breakpoint 7 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/provision/testcloud.py:516
(Pdb) b tmt/steps/provision/testcloud.py:517
Breakpoint 8 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/provision/testcloud.py:517
(Pdb) b tmt/utils.py:1683
Breakpoint 9 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py:1683
(Pdb) b tmt/utils.py:204
Breakpoint 10 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py:204
(Pdb) b tmt/utils.py:729
Breakpoint 11 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py:729
(Pdb) b tmt/utils.py:744
Breakpoint 12 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py:744
(Pdb) 
Breakpoint 13 at /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py:744
(Pdb) c
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(744)_workdir_init()
-> os.makedirs(workdir)
(Pdb) c
/var/tmp/tmt/run-001
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/base.py(916)_initialize_data_directory()
-> os.makedirs(self.data_directory, exist_ok=True)
(Pdb) 

/plans/foo
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) 
    discover
XXX-001 None
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) w
  /usr/lib64/python3.10/bdb.py(597)run()
-> exec(cmd, globals, locals)
  <string>(1)<module>()
...<snip>...
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/base.py(2050)go()
-> plan.go()
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/base.py(1211)go()
-> step.go()
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/discover/__init__.py(268)go()
-> phase.go()
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/discover/fmf.py(223)go()
-> super(DiscoverFmf, self).go()
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py(930)go()
-> self.go_prolog()
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/steps/__init__.py(909)go_prolog()
-> self.info('how', self.get('how'), 'magenta')
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(483)info()
-> self._log(self._indent(key, value, color=None, shift=shift))
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(453)_log()
-> if self.workdir is None:
  /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(800)workdir()
-> create_directory(self._workdir, 'workdir', quiet=True)
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(1683)create_directory()
-> os.makedirs(path, exist_ok=True)
(Pdb) p path
'/var/tmp/tmt/run-001/plans/foo/discover/A B C D e f g'
(Pdb) up
> /home/huanli/.virtualenvs/tmt/lib/python3.10/site-packages/tmt/utils.py(800)workdir()
-> create_directory(self._workdir, 'workdir', quiet=True)
(Pdb) ll
791  	    @property
792  	    def workdir(self) -> Optional[str]:
793  	        """ Get the workdir, create if does not exist """
794  	        if self._workdir is None:
795  	            self._workdir = self._workdir_name()
796  	            # Workdir not enabled, even parent does not have one
797  	            if self._workdir is None:
798  	                return None
799  	            # Create a child workdir under the parent workdir
800  ->	            create_directory(self._workdir, 'workdir', quiet=True)
801  	        return self._workdir
(Pdb) p self._workdir
'/var/tmp/tmt/run-001/plans/foo/discover/A B C D e f g'
(Pdb) p self._workdir_name()
'/var/tmp/tmt/run-001/plans/foo/discover/A B C D e f g'
# ^^^^^ The function _workdir_name() helps to create the directory path !!!

Finally, we found that the function which helps to create directory path (e.g. /var/tmp/tmt/run-001/plans/foo/discover/A B C D e f g) is Common._workdir_name().

https://github.com/teemtee/tmt/blob/main/tmt/utils.py#L763

 763	    def _workdir_name(self) -> Optional[str]:
 764	        """ Construct work directory name from parent workdir """
 765	        # Need the parent workdir
 766	        if self.parent is None or self.parent.workdir is None:
 767	            return None
 768	        # Join parent name with self
 769	        return os.path.join(self.parent.workdir, self.name.lstrip('/'))

idorax · 2022-09-23T13:47:10Z

Since we have known that the root cause is Common._workdir_name() doesn't consider name having spaces,

https://github.com/teemtee/tmt/blob/main/tmt/utils.py#L769

 769	        return os.path.join(self.parent.workdir, self.name.lstrip('/'))

the fix would be pretty simple, e.g.

diff --git a/tmt/utils.py b/tmt/utils.py
index 6aac821..9979269 100644
--- a/tmt/utils.py
+++ b/tmt/utils.py
@@ -766,7 +766,8 @@ class Common:
       if self.parent is None or self.parent.workdir is None:
           return None
       # Join parent name with self
-        return os.path.join(self.parent.workdir, self.name.lstrip('/'))
+        base_name = re.sub(r'\s+', '_', self.name).lstrip('/')
+        return os.path.join(self.parent.workdir, base_name)

idorax · 2022-09-26T13:14:20Z

The patching in the following introduced a new bug which caused tests/core/spaces to fail.

diff --git a/tmt/utils.py b/tmt/utils.py
index 6aac821..1671048 100644
...<snip>...
-        return os.path.join(self.parent.workdir, self.name.lstrip('/'))
+        base_name = re.sub(r'\s+', '_', self.name).lstrip('/')
+        return os.path.join(self.parent.workdir, base_name)

The failures look like:

(tmt) huanli@kvm-01-guest01:data$ tmt run -arvvv provision -h local
/var/tmp/tmt/run-001
Found 1 plan.

/a directory/some plans/a plan
    discover
        how: fmf
        name: some discover
        order: 50
        directory: /home/huanli/dev/1520/dev/tests/core/spaces/data
        summary: 1 test selected
            /a directory/some tests/a shell test
    provision
        how: local
        order: 50
        kernel: 5.19.10-200.fc36.x86_64
        summary: 1 guest provisioned
    prepare
        summary: 0 preparations applied
    execute
        how: tmt
        order: 50
        exit-first: False
            test: A simple shell test with spaces
    finish
        summary: 0 tasks completed
The working directory '/var/tmp/tmt/run-001/a_directory/some_plans/a_plan/discover/some discover/tests/a directory/some tests/a shell test' does not exist.

It is because phase.name = some discover, it was updated to be some_discover when invoking os.makedirs(), but it is used later as some discover to access the workdir.

https://github.com/teemtee/tmt/blob/main/tmt/steps/discover/__init__.py#L275

 270	                # Prefix test name only if multiple plugins configured
 271	                prefix = f'/{phase.name}' if len(self.phases()) > 1 else ''
 272	                # Check discovered tests, modify test name/path
 273	                for test in phase.tests():
 274	                    test.name = f"{prefix}{test.name}"
 275	                    test.path = f"/{phase.name}{test.path}"
 276	                    # Use the default test framework if not defined in L1
 277	                    # FIXME remove when we drop the old execution methods
 278	                    if not test.framework:
 279	                        test.framework = self.plan.execute._framework
 280	                    # Update test environment with plan environment
 281	                    test.environment.update(self.plan.environment)
 282	                    self._tests.append(test)

Hence, it doesn't work if we update name in Common._workdir_name() roughly. Maybe we have to introduce a shadow name which is just an alias of the raw name, and the shadow name is used to create directory path and should be saved for accessing the directory path later.

happz · 2022-09-26T13:31:47Z

The patching in the following introduced a new bug which caused tests/core/spaces to fail.

diff --git a/tmt/utils.py b/tmt/utils.py
index 6aac821..1671048 100644
...<snip>...
-        return os.path.join(self.parent.workdir, self.name.lstrip('/'))
+        base_name = re.sub(r'\s+', '_', self.name).lstrip('/')
+        return os.path.join(self.parent.workdir, base_name)

The failures look like:

(tmt) huanli@kvm-01-guest01:data$ tmt run -arvvv provision -h local
/var/tmp/tmt/run-001
Found 1 plan.

/a directory/some plans/a plan
    discover
        how: fmf
        name: some discover
        order: 50
        directory: /home/huanli/dev/1520/dev/tests/core/spaces/data
        summary: 1 test selected
            /a directory/some tests/a shell test
    provision
        how: local
        order: 50
        kernel: 5.19.10-200.fc36.x86_64
        summary: 1 guest provisioned
    prepare
        summary: 0 preparations applied
    execute
        how: tmt
        order: 50
        exit-first: False
            test: A simple shell test with spaces
    finish
        summary: 0 tasks completed
The working directory '/var/tmp/tmt/run-001/a_directory/some_plans/a_plan/discover/some discover/tests/a directory/some tests/a shell test' does not exist.

It is because phase.name = some discover, it was updated to be some_discover when invoking os.makedirs(), but it is used later as some discover to access the workdir.

https://github.com/teemtee/tmt/blob/main/tmt/steps/discover/__init__.py#L275

 270	                # Prefix test name only if multiple plugins configured
 271	                prefix = f'/{phase.name}' if len(self.phases()) > 1 else ''
 272	                # Check discovered tests, modify test name/path
 273	                for test in phase.tests():
 274	                    test.name = f"{prefix}{test.name}"
 275	                    test.path = f"/{phase.name}{test.path}"
 276	                    # Use the default test framework if not defined in L1
 277	                    # FIXME remove when we drop the old execution methods
 278	                    if not test.framework:
 279	                        test.framework = self.plan.execute._framework
 280	                    # Update test environment with plan environment
 281	                    test.environment.update(self.plan.environment)
 282	                    self._tests.append(test)

Hence, it doesn't work if we update name in Common._workdir_name() roughly. Maybe we have to introduce a shadow name which is just an alias of the raw name, and the shadow name is used to create directory path and should be saved when accessing the directory path.

I like your approach, "hiding" the change into _workdir_name, feels like the right place for it. Feels correct. IIUIC, the issue here lies in phase.name being used as part of a filesystem path (test.path = f"/{phase.name}{test.path}"). I'd propose adding a new property, name-but-one-you-should-use-for-filesystem-paths, e.g. fs_name, something along those lines - based on Common.name, but for filesystem. It'd be implemented in Common, then it would be basically replacing base_namein your patch. And the code yu quote below should use it as well when constructingtest.path. It's similar to your suggestion of "shadow name", but it does not have to be saved anywhere, it can be derived from namewhen needed. The hard part is to fix all places that usenameand should rather use this newfs_name` - luckily, failing test already identified one place where this is happening.

idorax · 2022-09-28T13:36:54Z

@happz，thank you for your comment. I have used fs_name as the shadow name, please help to take a look at the latest patch in PR 1527, thanks!

happz · 2022-09-29T07:22:06Z

@happz，thank you for your comment. I have used fs_name as the shadow name, please help to take a look at the latest patch in PR 1527, thanks!

Looks very good to me, nice!

psss added the good first issue Good for newcomers label Sep 19, 2022

idorax self-assigned this Sep 20, 2022

idorax mentioned this issue Sep 23, 2022

Replace spaces with underscores if making workdir #1527

Merged

psss added this to the 1.18 milestone Sep 29, 2022

psss closed this as completed in #1527 Sep 30, 2022

kkaarreell mentioned this issue Jan 17, 2023

cat: /var/tmp/tmt/run-075/plans/package_update-swtpm/execute/data/Setup: No such file or directory beakerlib/beakerlib#148

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using discover step names containing spaces as a directory name causes troubles #1520

Using discover step names containing spaces as a directory name causes troubles #1520

kkaarreell commented Sep 19, 2022

psss commented Sep 19, 2022

idorax commented Sep 20, 2022

psss commented Sep 20, 2022

kkaarreell commented Sep 20, 2022

idorax commented Sep 22, 2022

idorax commented Sep 22, 2022

idorax commented Sep 23, 2022 •

edited

Loading

idorax commented Sep 23, 2022 •

edited

Loading

idorax commented Sep 26, 2022 •

edited

Loading

happz commented Sep 26, 2022

idorax commented Sep 28, 2022

happz commented Sep 29, 2022

Using discover step names containing spaces as a directory name causes troubles #1520

Using discover step names containing spaces as a directory name causes troubles #1520

Comments

kkaarreell commented Sep 19, 2022

psss commented Sep 19, 2022

idorax commented Sep 20, 2022

psss commented Sep 20, 2022

kkaarreell commented Sep 20, 2022

idorax commented Sep 22, 2022

idorax commented Sep 22, 2022

idorax commented Sep 23, 2022 • edited Loading

idorax commented Sep 23, 2022 • edited Loading

idorax commented Sep 26, 2022 • edited Loading

happz commented Sep 26, 2022

idorax commented Sep 28, 2022

happz commented Sep 29, 2022

idorax commented Sep 23, 2022 •

edited

Loading

idorax commented Sep 23, 2022 •

edited

Loading

idorax commented Sep 26, 2022 •

edited

Loading