Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No easy way to skip some actors. #823

Open
holser opened this issue Jan 26, 2022 · 10 comments
Open

No easy way to skip some actors. #823

holser opened this issue Jan 26, 2022 · 10 comments

Comments

@holser
Copy link
Contributor

holser commented Jan 26, 2022

Actual behavior
There is no way to easily disable or enable actor. The only way to disable actor is to delete it. However, recent refactoring of leapp broke OpenStack CI as path of actor was changed from

/usr/share/leapp-repository/repositories/system_upgrade/el7toel8/actors/
to
/usr/share/leapp-repository/repositories/system_upgrade/common/actors/

I propose to introduce --skip-actors actor1,actor2 or introduce environment variable to be able to disable some actors for CI purposes. That will allow to use cli option no matter how leapp is organized or refactored underneath.

To Reproduce
Install leapp-0.12 and delete actor
Install leapp-0.13 and delete actor using same script.

Expected behavior
Consistent interface for disabling actors.

@pirat89
Copy link
Member

pirat89 commented Jan 26, 2022

@holser The proposal does not make sense from the design POV. With the option you would be able to skip particular actor, which could lead in various breaks / issues / crashes / unexpected outputs ... basically it is breaking the design and provides more problems than solutions. In the same time, it does not solve your problem, as the name of the actor can be changed as well - which happened several times already, even during the last year IIRC.

First, we are missing examples why some actors needs to be removed.

  • performance - that does not seem so much as a good reason; in case of CI runs, is LEAPP_DEVE_DATABASE_SYNC_OFF specified? usually the majority of time that is consumed by our actors includes operations that cannot be skipped
  • ignore specific inhibitors - that's a different RFE that is on our roadmap and it will require to mark the run as unsupported
  • something is not working as expected in case .... - expected solution is opened PR with the proper fixes, not skipping of actors;

Can you provide reasons why to skip some actors that are not included above?

btw, the suggested feature is supposed to be reported in leapp-repository, as this is the thing of particular repositories (including leapp CLI commands included in them). I will transfer it.

@pirat89 pirat89 transferred this issue from oamg/leapp Jan 26, 2022
@pirat89
Copy link
Member

pirat89 commented Jan 26, 2022

Discussed the possibility to provide LEAPP_DEVEL_SKIP_ACTORS=... possibility instead, that probably makes more sense, however, still there is the question about reasons for such functionality.

@holser
Copy link
Contributor Author

holser commented Jan 26, 2022

The reason is quite simple. The leapp is used in Ecosystem products such as OpenStack or OpenShift to test upgrades. When actor behavior was changed it may act differently which may block CI for a quite a long time. Instead of waiting on new release of leapp a simple workaround may be provided instead.

@holser
Copy link
Contributor Author

holser commented Feb 2, 2022

Another reason is CI. If you have changes in single actor you may disable all other actors to test only particular one.

@vinzenz
Copy link
Member

vinzenz commented Feb 2, 2022 via email

@odyssey4me
Copy link

Here are some examples of actors we have found ourselves removing in some engagements.

persistentnetnamesdisable:

  • we run automation to ensure the network interfaces are properly persisted, but we do not restart the interfaces before running leapp
  • leapp blocks the upgrade because the running names are not right... because it doesn't know the scripts have been rewritten
  • rebooting servers is one of the largest causes of failure, so we try to do it as little as possible
  • rebooting servers takes time, and we have been asked to reduce the time taken

biosdevname:

  • this is oddly specific to dell servers and causes a mismatch in results between different hardware vendors

checkinstalledkernels:

  • this rule is too draconian, forcing a reboot after updating the kernel package
  • it would make more sense to specify some sort of minimum version, rather than forcing the use of the latest
  • we have found that leapp works just fine from RHEL 7.7 with this actor removed

These are decisions made with extensive testing to validate whether they have negative side-effects or not. The point is that it is something that ends up being done, so it'd be better to create an interface to do it which allows the code to be moved around freely while layered products, consulting, documentation, etc can all remain constant.

@holser
Copy link
Contributor Author

holser commented Feb 7, 2022

Also I wanted to mention

checkkerneldrivers:

  • floppy pata_acpi drivers are part of i440FX architecture which comes by default in RHEL 7.9 libvirt image. Firstly, leapp doesn't detect that RHEL runs on virtual machine and makes assumption that these drivers should be removed. Leaving these drivers for virtual environment are fine. It doesn't block leapp to install RHEL 8 and boot up later on.

@vinzenz
Copy link
Member

vinzenz commented Feb 8, 2022

Well, we will keep this in mind, however how things are going right now, there is no way something like this is going to happen in the next release. We will see what we can come up with.

@odyssey4me

... so it'd be better to create an interface to do it which allows the code to be moved...

The problem is that you are asking for an interface where by design was never meant to be an 'interface' and as soon as we would add an interface, we would have to call this 'supported' which we inherently do not want.

There are reason for those actors to be present, and yes there is the possibility that YMMV. If you delete them as a workaround this is on you then to ensure it goes well. Not saying that there aren't any bugs, we can work on those of course and as an interim solution you could delete the actors doing the checks so your CI systems keep working. But it's not a solution for everyone.

Anyway we'll have to have a discussion about this and what we can do to make it maybe 'easier' for you. But it I am pretty sure that the upgrade always will be considered as unsupported.

@holser
Copy link
Contributor Author

holser commented Feb 8, 2022

@vinzenz

There are a lot of different interfaces with different purposes. It's all in our hands how we design and target the interface. As discussed with @pirat89 on IRC we may create LEAPP_DEVEL_SKIP_ACTORS= as interfaces. DEVEL in its name should help end users understand that interface is designed for developers and their needs. Additionally we may specify that in documentation as well.

@pirat89
Copy link
Member

pirat89 commented Feb 8, 2022

@holser but keep in mind that this rfe has super low priority for us and still it has not been decided it will be implemented really. Most of mentioned problems are not good enough reasons for the skipping of actors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants