CA-390759: Revert removal of PBIS (PowerBroker Identity Service) for future backports.

bernhardkaindl commented 7 months ago

While this reverts the removal of PBIS support in the master branch (that master would not need now), it would make sense to apply revert the removal of it in master as well to be able to use the master branch as basis for future hotfixes again like we did for UPD-948 (StatusQuo): One example where it could be decided to do this gain would be syncing of RRDs to disk before capturing them which is planned for 8.2 CU1. It would involve backporting the larger patch of https://github.com/xenserver/status-report/pull/51 which resulted in an issue that needs https://github.com/xenserver/status-report/pull/88. Using master like UPD-948 did would be an option for this future release. But for this to be have no regressions, we should also apply this to master:

XS8 migrated from PowerBroker Identity Service (PBIS) to Samba Winbind, but Yangtze keeps using PBIS: https://docs.xenserver.com/en-us/xenserver/8/whats-new.html (CP-36092)

In 2021 the collection of PBIS was replaced by collecting Samba Winbind data and for the previous hotfix, with the release of v1.2.10 for 8.2 CU1, that removal of PBIS collection was re-added, but only in the branch for Yangtze. But the commit was labelled as Backport (Backport means: The change is ported from the trunk/head/master to an older branch), which it wasn't a backport at all (it was a revert, like this PR):

It reverted removing PBIS that was removed in master. This together with the recommendation to update bugtool in Yangtze using the master branch led to this revert not being included in the new, updated release branch for Yangtze: https://github.com/xenserver/status-report/tree/release/yangtze

Fix this by forward-porting this revert of the PBIS removal (final commit of v1.2.10) to the new Yangtze branch for the next Yangtze hotfix: https://github.com/xenserver/status-report/compare/master...historic/yangtze/lcm

The aim of PR is to forward-port the "backport" to the master branch until the support for Yangtze ends to prevent this from happening in the future again, and then backport it to https://github.com/xenserver/status-report/tree/release/yangtze as it should have been done.

coveralls commented 7 months ago

Pull Request Test Coverage Report for Build 8437336319

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 95.084%

Totals
Change from base Build 8433749145:	0.0%
Covered Lines:	677
Relevant Lines:	712

💛 - Coveralls

codecov[bot] commented 7 months ago

Codecov Report

Merging #99 (995ed1d) into master (d0e569d) will increase coverage by 10%. Report is 19 commits behind head on master. The diff coverage is n/a.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #99 +/- ## ====================================== + Coverage 88% 98% +10% ====================================== Files 18 18 Lines 2196 665 -1531 ====================================== - Hits 1939 654 -1285 + Misses 257 11 -246 ``` [see 1 file with indirect coverage changes](https://app.codecov.io/gh/xenserver/status-report/pull/99/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | [Flag](https://app.codecov.io/gh/xenserver/status-report/pull/99/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | Coverage Δ | | |---|---|---| | [python2.7](https://app.codecov.io/gh/xenserver/status-report/pull/99/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | `96% <ø> (+9%)` | :arrow_up: | | [python3.10.13](https://app.codecov.io/gh/xenserver/status-report/pull/99/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver) | `100% <ø> (+12%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=xenserver#carryforward-flags-in-the-pull-request-comment) to find out more.

liulinC commented 7 months ago

Do we also use master for Yangtze? PBIS only lives in Yangtze now.
From the description we had a Yangtze release branch, why this PR just goes to Yangtze directly, to avoid confusing maser?

bernhardkaindl commented 7 months ago

Do we also use master for Yangtze? PBIS only lives in Yangtze now. From the description we had a Yangtze release branch, why this PR just goes to Yangtze directly, to avoid confusing maser?

We already used master for the new Yangtze branch for Hotfix UPD-948 where the recommendation of multiple developers was to use the code from rt-next under the hope or under the condition that it was still compatible with Yangtze. Trying to check this on the master branch, only saw commits that removed old features, which as far as I have seen have been removed from CH mostly long before Yangtze. I even also looked at the commit messages of the previous Yangtze LCM branch (you can check it yourself), the only commit that I saw on that previous LCM branch that was not on the master branch was a commit that said is a "Backport" of collecting PBIS data and logs to Yangtze.

It seems that the term "Backport" was used very liberally (in my humble opinion is a slightly confusing way) because this is what the term "backport" actually means: https://en.wikipedia.org/wiki/Backporting

Backporting is the action of taking parts from a newer version of a software system or software component and porting them to an older version of the same software.

Instead, this commit reverted the removal of PBIS data and log collection because the previous commit removed it from master. But this is completely different from backporting as described above:

Backporting is the action of taking parts from a newer version of a software system or software component and porting them to an older version of the same software.

If that commit that was described as a backport back was really a revert of a removal (a re-add of functionality) had been really been a backport, the master branch would have had PBIS data and log collection support.

But because it had not, but the commit message said so by using the term backport, the review on whether this change needs to be re-applied for the next Yangtze hotfixes (starting with UPD-948), was fooled:

UPD-948 therefore does not have PBIS data and log collection
PBIS data and log collection needs to be reapplied to
- The latest Yangtze branch release/yangtze in case the next hotfix is based on it (#100)
- The master branch in case the next hotfix is based on the master branch again like UPD-948 was.
- This is not just a theoretical possibility because the changes that are requested for the next Yangtze hotfix already would be a backport syncing all RRD data to disk before collecting the RRD data. This was done in master with generic change, so commands run from plugins can sync data to disk like the command used by the Xapi plugin does and only then start to traverse the directory trees for collecting the saved RRDs. This is a change which had an unintended side effect and this fix for the side effect also would have to be backported. In principle, it could be considered again a good idea to use the master branch for the next hotfix because of this because XS8 already uses this code with Python 2.7, so this code is already tested and would need no new version of the code base there things could (in theory) go wrong.

The idea here is: In order to be able to cut a new release for Yangtze from master (XS8), master (XS8) must not be made incompatible with Yangtze. All needed code needs to be in the master branch. In this sense it is not confusing master, it is about fixing the usability if it for Yangtze hofixes like we did with UPD-948.

xenserver / status-report