Undeterministic propagation of --dry option in tmt run

[adiosnb]

Recently I found an interesting behavior of the --dry option if it is used only for the provision step. It also sets the dry option of Plan objects.

Plan.opt('dry') == True when $ tmt run --all --remove provision --how virtual --dry
Plan.opt('dry') == None when $ tmt run --all --remove provision --how virtual --dry plan

This causes a bug in the imported plan feature, which is fixed by a workaround 47b9250.

Do you have any idea why it happens? Is it intentional for some reason? Or well a hidden bug?

[adiosnb]

Do you think it is possible that these three lines

tmt/tmt/base.py

Line 2660 in 47b9250

tmt.steps.prepare.Prepare._options["dry"] = True

may cause the problem?

A PDB tells me that these steps and Plan shares the same _options dictionary from tmt.utils.Common.

id(Common._options)
Out[21]: 140548690376256
id(Plan._options)
Out[22]: 140548690376256
id(tmt.steps.finish.Finish._options)
Out[23]: 140548690376256
id(tmt.steps.execute.Execute._options)
Out[24]: 140548690376256

[psss]

That might be it!

[adiosnb]

The problem is that all classes inheriting from Common share the same dict unless it is overridden.

# Plan class uses Common._options
$ tmt run --all --remove provision --how virtual --dry 

# Plan class receiver default context from click subcontext *
$ tmt run --all --remove provision --how virtual --dry plan 

Plan context override

tmt/tmt/cli.py

Line 309 in 47b9250

tmt.base.Plan._save_context(context)

I have tried to fix this using a metaclass that creates a copy of _options dict for each child here: #1983

@happz How would you solve this issue? Is it possible to get default context from click even if plan is not specified on CLI?

[happz]

The problem is that all classes inheriting from Common share the same dict unless it is overridden.

# Plan class uses Common._options
$ tmt run --all --remove provision --how virtual --dry 

# Plan class receiver default context from click subcontext *
$ tmt run --all --remove provision --how virtual --dry plan 

Plan context override

tmt/tmt/cli.py

Line 309 in 47b9250

tmt.base.Plan._save_context(context)

I have tried to fix this using a metaclass that creates a copy of _options dict for each child here: #1983

@happz How would you solve this issue? Is it possible to get default context from click even if plan is not specified on CLI?

I don't have any proposal, as of now, but I've been poking this particular set of attributes for some time. To support multiple phases on the command line (prepare -h shell --script ... prepare -h ansible --playbook ...), we will have to get rid of the singular aspect of _options and _context anyway. Now all prepare steps share the same one, as class variables, and the latter overwrites the former... So, I'm in the area, but I don't have the proposal ready - it should play nicely with steps where this approach is also used, see https://github.com/teemtee/tmt/blob/main/tmt/steps/prepare/__init__.py#L64. I'm experimenting with instances of these step classes.

Your metaclass might also work, although I did not try this option yet. I'm afraid it would work only for cases where classes based on Common are instantiated, but _context and _options are also used as class variables... So, no helpful advice from my side, yet, all I have at the moment is the bigger picture :(

[adiosnb]

The current problem caused by the issues has a workaround here 47b9250. The question is, whether we will live with the workaround and be aware of such behavior until you refactor these options, or the metaclass from #1983 and possibly revert the workaround.

I am OK with both.

@psss: What do you think?

[adiosnb]

I am trying to fix this issue with a simple fix #2050, which focuses only on the Plan._options dictionary. The generic solution with the metaclass from #1983 has an error when running in python3.6, and the fix would require a lot of code modifications.

As @happz plans to change the _options and the _context propagation in the future, I propose temporarily using this fix #2050 instead of a generic solution from #1983.

@psss @happz What do you think?

[happz]

I am trying to fix this issue with a simple fix #2050, which focuses only on the Plan._options dictionary. The generic solution with the metaclass from #1983 has an error when running in python3.6, and the fix would require a lot of code modifications.

As @happz plans to change the _options and the _context propagation in the future, I propose temporarily using this fix #2050 instead of a generic solution from #1983.

@psss @happz What do you think?

+1 from me. _options and _context will need some changes if we ever want to support multiple phases on a command line via repetition of commands (provision -h beaker ... provision -h container ...).