Coverage around bz#1649961

[mirekdlugosz]

It would be nice to have some coverage around bz#1649961. Let's discuss how we can approach the problem. I'm willing to create PR after discussion, but I will need some code pointers.

Key facts:

1. Issue manifested on data that must have been created early in Satellite 6.2 cycle. To reproduce it, I had to install 6.2.1 and upgrade all the way to 6.4.1 (6.2.1 -> 6.2.16 -> 6.3.5 -> 6.4.1). During upgrade path, I had to upgrade RHEL (7.2 -> 7.6) and puppet (3 -> 4).
2. It affects only roles with permissions that changed resources between upgrades. Between June 2016 and February 2018 there were only 3 cases like that in Satellite.
3. After upgrade, issue can be reproduced on web UI and hammer. I haven't tried, but I assume it can be reproduced using API as well.
4. Fix is applied in foreman-maintain - specific pre-upgrade check is run and if it fails (discovers that roles are in inconsistent state), it will ask user if it should recover automatically.
5. We do expect things to change after upgrade. If things are unchanged after foreman-maintain upgrade run, it means there is a bug somewhere.
6. In Bugzilla, there is migration script attached. It can be used to break the system - introduce issue on any version of Satellite (provided specific data was entered first).

Approach 1

Basic scenario, where we enter some roles, upgrade Satellite and verify that roles are intact.

If we don't have much coverage around roles and filters, this is good idea anyway. But it is very unlikely to catch this specific issue in the future.

Approach 2

Use/extend existing coverage around roles to catch filters that changed resources between versions.

This is good info to have, so if we don't gather it automatically, we should. But it can only guide further testing, not act as check on itself - most of resource changes are expected.

Approach 3

Use migration script to break Satellite between entering data and running foreman-maintain.

It currently breaks view_arf_reports permission. I would probably want to randomize it a bit.

What I dislike, though, is that fix for the issue applied in foreman-maintain targets environment created by this migration (dev didn't try to follow upgrade path). If we repeatedly break environment and run foreman-maintain with fix targeted at this specific breakage, we won't discover if foreman-maintain is unable to handle slight variations of that bug. We will only catch case when fix in foreman-maintain breaks - but is it even likely? And if we worry about it, wouldn't it be better to increase coverage in foreman-maintain test suite?

Thoughts, opinions and ideas are more than welcome.

[sghai]

Approach 1

Basic scenario, where we enter some roles, upgrade Satellite and verify that roles are intact.

Some time back, devs requested to increase the coverage around roles and filters. Now bz components have been assigned so I'm leaving this up to component owner. However, having one test case where if "you create a custom role with some filters, it should be available and usable after upgrade" would be helpful here IMO.

If we don't have much coverage around roles and filters, this is good idea anyway. But it is very unlikely to catch this specific issue in the future.

Approach 2

Use/extend existing coverage around roles to catch filters that changed resources between versions.

This is good info to have, so if we don't gather it automatically, we should. But it can only guide further testing, not act as check on itself - most of resource changes are expected.

Approach 3

Use migration script to break Satellite between entering data and running foreman-maintain.

It currently breaks view_arf_reports permission. I would probably want to randomize it a bit.

What I dislike, though, is that fix for the issue applied in foreman-maintain targets environment created by this migration (dev didn't try to follow upgrade path). If we repeatedly break environment and run foreman-maintain with fix targeted at this specific breakage, we won't discover if foreman-maintain is unable to handle slight variations of that bug. We will only catch case when fix in foreman-maintain breaks - but is it even likely? And if we worry about it, wouldn't it be better to increase coverage in foreman-maintain test suite?

Thoughts, opinions and ideas are more than welcome.

Since the fix is in foreman-maintain that identify inconsistent roles and ask for auto recovery then I think we should cover this scenario at foreman-maintain level. what do you think @ntkathole @jameerpathan111

[jameerpathan111]

Since the fix is in foreman-maintain that identify inconsistent roles and ask for auto recovery then I think we should cover this scenario at foreman-maintain level. what do you think @ntkathole @jameerpathan111

sure, will add coverage around it in Testfm

[jyejare]

@Mirzal, Does that conversation solves your problem ?