tmt clean (virtual)guests doesn't work well with --workdir-root cli opt

[skycastlelily]

If users specify a workdir-root different from TMT_WORKDIR_ROOT ,clean guests will fail,
that's because virtual plugin doesn't accept and apply cli_opts

dev) lnie@fedora:~/tmt$ echo $TMT_WORKDIR_ROOT
/var/tmp/tmt

(dev) lnie@fedora:~/tmt$  tmt clean guests -vvdd --workdir-root /var/tmp/test

clean
guests
Using tree '/home/lnie/tmt'.
Workdir '/var/tmp/test/run-001' already exists.
Read file '/var/tmp/test/run-001/run.yaml'.
Workdir '/var/tmp/test/run-001/plans/testcloud' already exists.
Create the data directory '/var/tmp/test/run-001/plans/testcloud/data'.
Create the environment file '/var/tmp/test/run-001/plans/testcloud/data/variables.env'.
Workdir '/var/tmp/test/run-001/plans/testcloud/discover' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/discover/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.discover.shell' already imported.
status: done
Read file '/var/tmp/test/run-001/plans/testcloud/discover/tests.yaml'.
Workdir '/var/tmp/test/run-001/plans/testcloud/provision' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/provision/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.provision.testcloud' already imported.
status: done
Read file '/var/tmp/test/run-001/plans/testcloud/provision/guests.yaml'.
Module 'tmt.steps.provision.testcloud' already imported.
Workdir '/var/tmp/test/run-001/plans/testcloud/prepare' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/prepare/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.prepare.install' already imported.
Module 'tmt.steps.prepare.shell' already imported.
status: todo
Workdir '/var/tmp/test/run-001/plans/testcloud/execute' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/execute/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.execute.internal' already imported.
status: todo
Read file '/var/tmp/test/run-001/plans/testcloud/execute/results.yaml'.
Workdir '/var/tmp/test/run-001/plans/testcloud/report' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/report/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.report.display' already imported.
status: todo
Workdir '/var/tmp/test/run-001/plans/testcloud/finish' already exists.
Read file '/var/tmp/test/run-001/plans/testcloud/finish/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.finish.shell' already imported.
status: todo
Read file '/var/tmp/test/run-001/plans/testcloud/provision/step.yaml'.
Successfully loaded step data.
Module 'tmt.steps.provision.testcloud' already imported.
Read file '/var/tmp/test/run-001/plans/testcloud/provision/guests.yaml'.
Module 'tmt.steps.provision.testcloud' already imported.
Step is done, not touching its data.
Using the 'ProvisionTestcloud' plugin for the 'virtual' method.
step is done, not overwriting plugin data
Waking up testcloud instance 'tmt-001-uduSmlAv'.
testcloud version: 0.9.12
Provision wake up complete (already done before).
Stopping guests in run '/var/tmp/test/run-001' plan '/plans/testcloud'.
finish
workdir: /var/tmp/test/run-001/plans/testcloud/finish

        Stopping testcloud instance 'tmt-001-uduSmlAv'.
        guest: stopped
        Removing testcloud instance 'tmt-001-uduSmlAv'.

warn: Could not stop guest in run '/var/tmp/test/run-001': Failed to remove testcloud instance: [Errno 2] No such file or directory: '/var/tmp/tmt/testcloud/instances/tmt-001-uduSmlAv'.

       

[happz]

testcloud plugin should probably follow the suite and start using effective_workdir_root()... Or, better, it shouldn't touch WORKDIR_ROOT, and it should be told what the workdir root is. Otherwise, we would need a global name for the current workdir root, resolved and set by the current command. But shared global state is a PITA in general, so I'd like to avoid that. I wonder whether we could replace tmt.steps.testcloud.TESTCLOUD_DATA with something like self.plan.workdir.parent or something like that, i.e. derive it from the plan and run itself - they should already be aware of their workdir and its root.

[skycastlelily]

Sorry for a little late reply,got high fever from my vacation🥲

testcloud plugin should probably follow the suite and start using

effective_workdir_root()... That is what I was thinking,just didn't get enough energy to work out a merge request due to the high fever 🥴

Or, better, it shouldn't touch WORKDIR_ROOT, and it should be told what

the workdir root is. Nope, we need pass workdir-root to ProvisionPlugin which will affect other plugins than testcloud. Here is the merge request:#2838 for this issue^^

…

On Mon, Apr 8, 2024 at 6:37 PM Miloš Prchlík ***@***.***> wrote: testcloud plugin should probably follow the suite and start using effective_workdir_root()... Or, better, it shouldn't touch WORKDIR_ROOT, and it should be told what the workdir root is. Otherwise, we would need a global name for the current workdir root, resolved and set by the current command. But shared global state is a PITA in general, so I'd like to avoid that. I wonder whether we could replace tmt.steps.testcloud.TESTCLOUD_DATA with something like self.plan.workdir.parent or something like that, i.e. derive it from the plan and run itself - they should already be aware of their workdir and its root. — Reply to this email directly, view it on GitHub <#2834 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AKFR23COZMFXBMUGQTDOZBLY4JXPRAVCNFSM6AAAAABF4NI4P6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBSGQYTGMJXGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[happz]

Or, better, it shouldn't touch WORKDIR_ROOT, and it should be told what
the workdir root is.

Nope, we need pass workdir-root to ProvisionPlugin which will affect other plugins than testcloud. Here is the merge request:#2838 for this issue^^

I disagree, that's exactly what we should not do. By the time testcloud is running, the workdir root has been already established. All inputs we taken, considered, and tmt decided which path is a workdir root. Passing workdir-root to testcloud implies all this work would be replicated there once again. Instead, what testcloud and any plugin ever interested in workdir root should do is to reach out to a place where this workdir root path would be saved. We don't have one right now, but apparently, we should add one, because re-running all the guessing in various plugins is just asking for trouble - you need to propagate all the relevant inputs to all plugins, you need to have the right code, and so on. All that when the workdir root is already known... Something took all the inputs, decided what the workdir root is, and used it to create self.workdir.

So you have two issues for the price of one:

• clean not honoring --workdir-root,
• the actual workdir root is not saved anywhere - we have get_effective_workdir_root() which is called two times, but the result is not saved. If any part of code needs workdir root, it needs to compute it once more.

So to me, adding this computation to testcloud would not be a good solution, it spreads the guessing to plugins, leaks command option to plugins, we want the workdir root to be saved somewhere instead, once and for all code to read. testcloud needs not to worry about --workdir-root and other inputs - the workdir root is already known, let's materialize it somewhere, and use it. My proposal would therefore to be to 1. add workdir_root where workdir is initialized, and use it in testcloud plugin to construct paths, and 2. fix clean to actually honor --workdir-root as needed, by passing the option to where the workdir root is being established.

[happz]

Basically, I'd go for the following:

• tmt.base.Clean is not using get_effective_root(), and get_effective_root() does not accept CLI input. Amending those two would be the start, Clean would then got the right workdir root the right way: https:/teemtee/tmt/blob/main/tmt/base.py#L3847
• with --last, a Run is then constructed, disconnected from this workdir root. It runs get_effective_root(), and without CLI, may easily get different input. Instead of this guessing, we should tell Run what the workdir root is - Clean knows, Run does not. Clean owns the information.
• similarly for cleaning guests, Run is constructed, but disconnected from what Clean established as workdir root - it needs to be told rather than guess on its own. Common accepts workdir, but not workdir_root - and I think it should, instead of running get_effective_workdir() on its own.
• and it should be saved, e.g. at https:/teemtee/tmt/blob/main/tmt/utils.py#L1540, maybe as a _workdir_root attribute, with workdir_root being a property which would return either self._workdir_root or self.parent.workdir_root if self.parent is not None, or.eventually, call get_effective_workdir_root().

That way, we would control the flow of workdir root info in tmt, from places that have all necessary info to decide what the workdir root should be to mere users, like testcloud. Various top-level objects like Clean, Status, Run - objects that represent actual commands what accept --workdir-root options - would take the info, decide what the workdir root is, and let their subordinates know. No guessing down the stream: this is the workdir root, it's been decided, obey.

Propagating --workdir-root to plugins would lead to an outcome where multiple layers would be deciding what the workdir is, often without all relevant input, and, sooner or later, some of them would be getting wrong answers. All that when top-level commands like run or clean already have the answer.

[skycastlelily]

Here is the first merge request:#2850 ^^