The beakerlib package is missing for the upgrade step

[martinhoyer]

When adding an upgrade step which uses beakerlib framework, it seems beakerlib is not being installed and the upgrade tasks fail with ./task.sh: line 2: /usr/share/beakerlib/beakerlib.sh: No such file or directory

In the upgrade tasks main.fmf:

test: ./task.sh
framework: beakerlib

Example report (login-logout-stress is not using beakerlib framework):

[lukaszachy]

Looks like we ignore require/recommend of upgrade tasks as well, don't we?

[lukaszachy]

Hopefully small enough reproducer which runs fast as it doesn't upgrade :)
Run as tmt run -avvv plan -n parent
Currently /check if beakerlib installed fails on old and new as beakerlib isn't installed and the task-beakerlib runs inside 'perform the system upgrade' errors as beakerlib isn't installed.

tmt init

cat > parent.fmf <<EOF
discover:
    how: shell
    tests:
    - name: check if beakerlib installed
      test: test -e /usr/share/beakerlib/beakerlib.sh

provision:
    how: container      
execute:
    how: upgrade
    path: upgrade-path
EOF

cat > task-beakerlib.fmf <<EOF
framework: beakerlib
test: |
    . /usr/share/beakerlib/beakerlib.sh || exit 1
    rlJournalStart
       rlPhaseStartTest 
            rlPass "yes"
      rlPhaseEnd
    rlJournalEnd
EOF

cat > upgrade-path.fmf <<EOF
discover:
    how: fmf
execute:
    how: tmt
EOF

[martinhoyer]

So if I understand it correctly (as a total noob), the Upgrade being Execute step and having it's own tasks discovery, resolving the framework/requires/recommends of the tasks would need one of:

• Modifying the Discovery step to check for upgrade step, do the custom discovery and then pass the data to the upgrade step somehow.
• Having a custom prepare step being run before the Upgrade. I believe it would be better to have all dependencies resolved before the first test execution, in order to have the system is in a same state for 'old' and 'new' runs.
• Declaring (and informing a user) that upgrade tasks are not 'tests' and do not support these keywords (and adding beakerlib/other frameworks as dependency of upgrade plugin).

[lukaszachy]

Having a custom prepare step being run before the Upgrade. I believe it would be better to have all dependencies resolved before the first test execution, in order to have the system is in a same state for 'old' and 'new' runs.

I'm not sure about this one. At one hand it makes it easier to compare old/new as just "upgrade" happen.

On the other hand I think it could be more useful to have 'Upgrade' run also prepare/finish and as such it can add repos which make dependencies available (they are not available for the OLD and if finish step removes them then neither for NEW).

[lukaszachy]

So I'd advocate to change 'ExecuteUpgrade' to use provisioned guests of the parent run and run discover & prepare (including install require/recommend) & execute & finish.
Finish might be a feat for the future as currently this steps does also guest cleanup which should be moved to its own, new, step

[happz]

So I'd advocate to change 'ExecuteUpgrade' to use provisioned guests of the parent run and run discover & prepare (including install require/recommend) & execute & finish. Finish might be a feat for the future as currently this steps does also guest cleanup which should be moved to its own, new, step

Do you mean by calling self.step.plan.prepare.go(), or something more complicated? I did try your reproducer, and it worked very well, and I was thinking whether it'd be possible to somehow reconnect upgrade to the right flow of tmt pipeline. It feels very exceptional right now... But, not have enough time there yet, may change my mind & trash it completely :)

[lukaszachy]

Currently upgrade does this: https://github.com/teemtee/tmt/blob/main/tmt/steps/execute/upgrade.py#L288
So instead of calling just _discover_upgrade we should call also at least prepare.

I'm not sure how much work would be to change current implementation to just call prepare.go() after feeding it with relevant data.

[martinhoyer]

Regardless of how much work would it be, I'd like to humbly suggest that it might be a good idea to take a step back and decide what the "upgrade" is actually for and how it should be implemented, because what you're describing sounds like a full tmt run within a tmt run, just with extra custom steps :)

[lukaszachy]

because what you're describing sounds like a full tmt run within a tmt run, just with extra custom steps :)

With few _less _steps :) but yes. IMHO this is what upgrade plan is and I see no issue why it shouldn't be it.

@psss Where would be best place to sync a bit about it? This was implemented by LEAPP, right?

[martinhoyer]

see no issue why it shouldn't be it.

All I'm saying is that conceptually there should be only one prepare step.

[happz]

It might be a good opportunity to improve ways in which a plugin could invoke other steps and plugins when needed. It's not trivial today, upgrade needs to shuffle discover and execute a bit to make it work, now we need to run prepare properly...

[lukaszachy]

All I'm saying is that conceptually there should be only one prepare step.

What is 'step' in this case? Step as a implementation or Step as a time when it runs? OK for implementation, opt-out for time.

[martinhoyer]

As in resolving all dependencies and preparations before the first test(s) are being run.

[lukaszachy]

As in resolving all dependencies and preparations before the first test(s) are being run.

Then I beg to differ that "upgrade dependencies and preparations" should be run as part of the upgrade itself or at least have it configurable.

[martinhoyer]

Well we can't all have the same opinions on everything :)
@psss This might need some discussion and consensus. Can you help please?

[lukaszachy]

I think that 'upgrade' part of the execute upgrade plugin should behave as a nested tmt run with own discover/prepare/execute/finish (once guest cleanup is not part of the finish) steps and thus dependencies of upgrade tasks should be installed in this nested tmt run. Or at least have the option to work it this way.

But it isn't something I treat as a priority, so if it is faster/easier and makes more sense for others to have upgrade gather its dependencies and install as part of the 'parent' tmt run, let's to it that way.

[lukaszachy]

Not directly related but #2784 might change how/when tmt gathers require/recommend.

[psss]

Summary from today's sync with @martinhoyer and @mmacura311: Placing the required installation right before the upgrade path execution seems to be the best approach and should not break any essential upgrade scenario. When the patch is ready @mmacura311 is willing to verify against their scenarios to ensure that everything's fine.

Regarding the code, it seems the upgrade task requires could be gathered once the upgrade discovery is done, somewhere around here:

tmt/tmt/steps/execute/upgrade.py

Lines 251 to 253 in 22731b9

	try:
	self._discover_upgrade.wake()
	self._discover_upgrade.go()

Packages gathered from require and recommend keys of upgrade tasks would be used to create a new PrepareInstall instances similar as here:

tmt/tmt/steps/prepare/__init__.py

Lines 305 to 315 in 22731b9

	data: _RawPrepareStepData = {
	'how': 'install',
	'name': 'requires',
	'summary': 'Install required packages',
	'order': tmt.utils.DEFAULT_PLUGIN_ORDER_REQUIRES,
	'where': [guest.name for guest in collection.guests],
	'package': [
	dependency.to_spec()
	for dependency in collection.dependencies
	]}
	self._phases.append(PreparePlugin.delegate(self, raw_data=data))

Created instances would be applied to all guests, as the execute step does not support the where key. As the upgrade scenarios are not primarily targeting multihost testing I'd say it's not a hard requirement to do the guest preparation in parallel. If needed we can improve later in a separate pull request.

@happz, @lukaszachy, any additional thoughts on this?