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

ospkgs: fix checks on osimage.pkgdir #7380

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

samveen
Copy link
Member

@samveen samveen commented May 1, 2023

The PR is to fix issue #7370

The code for OSPKGDIR handling in the ospkgs postscript has a bug in the rhel section where the the tracking index is used before incrementing, causing the url path for the 2nd repo to be overwritten by the path of the 3rd one, and the 3rd one is left with a blank repo url.

a classic case of use before increment bug

The pkgdir handling code has slowly mutated into multiple overlapping sections such that bugs have been introduced into the processing. This PR changes the pkgdir handling code to be clearer and simpler in organization to:

  • Allow easier review
  • Avoid the introduction of subtle bugs.

@samveen
Copy link
Member Author

samveen commented May 1, 2023

@gurevichmark @besawn for your kind attention.

@besawn besawn added this to the 2.16.6 milestone May 1, 2023
@besawn
Copy link
Member

besawn commented May 1, 2023

@samveen Thank you for the contribution. @gurevichmark and I would like to try to figure out why our internal tests haven't been catching this issue before we merge this PR. We will follow up soon, thanks.

@samveen
Copy link
Member Author

samveen commented May 2, 2023

@samveen Thank you for the contribution. @gurevichmark and I would like to try to figure out why our internal tests haven't been catching this issue before we merge this PR. We will follow up soon, thanks.

@besawn Would you point me to the OSImage definitions being used for the internal tests, particularly the pkgdir attribute, which should be able to help me give you the point of failure in the tests.

As for the github workflow checks, the environment used in the tests is Ubuntu , which will never trigger the bug:

Run perl github_action_xcat_test.pl
Current OS information:
$VAR1 = [
          'NAME="Ubuntu"',
          'VERSION="20.04.6 LTS (Focal Fossa)"',
          'ID=ubuntu',
          'ID_LIKE=debian',
          'PRETTY_NAME="Ubuntu 20.04.6 LTS"',
          'VERSION_ID="20.04"',
          'HOME_URL="https://www.ubuntu.com/"',
          'SUPPORT_URL="https://help.ubuntu.com/"',
          'BUG_REPORT_URL="https://bugs.launchpad.net/ubuntu/"',
          'PRIVACY_POLICY_URL="https://www.ubuntu.com/legal/terms-and-policies/privacy-policy"',
          'VERSION_CODENAME=focal',
          'UBUNTU_CODENAME=focal'
        ];

@gurevichmark
Copy link
Contributor

@samveen Thanks for your contribution. I was able to recreate this problem in our environment. However, I think the solution under #7371 is better.

With an osimage which has
pkgdir=/install/rhels8.6.0/ppc64le,/install/post/otherpkgs/rhels8.4.0/ppc64le/xcat/xcat-core,
if I try the solution you propose in this PR, I get 4 .repo files generated:

[root@f1n03k06 yum.repos.d]# grep baseurl /etc/yum.repos.d/*
/etc/yum.repos.d/xCAT-rhels8.6.0-path0.repo:baseurl=http://f1n03k05:80/install/rhels8.6.0/ppc64le
/etc/yum.repos.d/xCAT-rhels8.6.0-path1.repo:baseurl=http://f1n03k05:80/install/rhels8.6.0/ppc64le/BaseOS
/etc/yum.repos.d/xCAT-rhels8.6.0-path2.repo:baseurl=http://f1n03k05:80/install/rhels8.6.0/ppc64le/AppStream
/etc/yum.repos.d/xCAT-rhels8.6.0-path3.repo:baseurl=http://f1n03k05:80/install/post/otherpkgs/rhels8.4.0/ppc64le/xcat/xcat-core
[root@f1n03k06 yum.repos.d]#

and the first one is a problem:

[root@f1n03k06 yum.repos.d]# yum makecache
Updating Subscription Management repositories.
xCAT-rhels8.6.0-path0                           3.2 kB/s | 196  B     00:00
Errors during downloading metadata for repository 'xCAT-rhels8.6.0-path0':
  - Status code: 404 for http://f1n03k05:80/install/rhels8.6.0/ppc64le/repodata/repomd.xml (IP: 10.10.113.5)
Error: Failed to download metadata for repo 'xCAT-rhels8.6.0-path0': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried
xCAT-rhels8.6.0-path1                           219 kB/s | 2.8 kB     00:00
xCAT-rhels8.6.0-path2                           250 kB/s | 3.2 kB     00:00
xCAT-rhels8.6.0-path3                           110 kB/s | 3.0 kB     00:00
Ignoring repositories: xCAT-rhels8.6.0-path0
Metadata cache created.
[root@f1n03k06 yum.repos.d]#

If I use the solution proposed in #7371 I get 3 .repo files generated:

[root@f1n03k06 yum.repos.d]# grep baseurl /etc/yum.repos.d/*
/etc/yum.repos.d/xCAT-rhels8.6.0-path0.repo:baseurl=http://f1n03k05:80/install/rhels8.6.0/ppc64le/BaseOS
/etc/yum.repos.d/xCAT-rhels8.6.0-path1.repo:baseurl=http://f1n03k05:80/install/rhels8.6.0/ppc64le/AppStream
/etc/yum.repos.d/xCAT-rhels8.6.0-path2.repo:baseurl=http://f1n03k05:80/install/post/otherpkgs/rhels8.4.0/ppc64le/xcat/xcat-core
[root@f1n03k06 yum.repos.d]#

and

[root@f1n03k06 yum.repos.d]# yum makecache
Updating Subscription Management repositories.
xCAT-rhels8.6.0-path0                           156 kB/s | 2.8 kB     00:00
xCAT-rhels8.6.0-path1                           202 kB/s | 3.2 kB     00:00
xCAT-rhels8.6.0-path2                           238 kB/s | 3.0 kB     00:00
Metadata cache created.
[root@f1n03k06 yum.repos.d]#

@samveen
Copy link
Member Author

samveen commented May 3, 2023

@gurevichmark

  • Is the first element of the osimage.pkgdir NOT a YUM repos? The path is meant to be a repo (have the repodata/repomd.xml and associated filelists). Please check on the management server and confirm.

  • If this is not the case (i.e. first element is not a yum repo), then the code logic of ospkgs has retained code logic from RHEL6/7 repository layouts and will need a cleanup. See line 284 and it's preceding check

  • I've rewritten the logic to be clearer. Please review.

@samveen samveen changed the title ospkgs: fix index use before increment bug ospkgs: fix checks on osimage.pkgdir May 5, 2023
@gurevichmark
Copy link
Contributor

@samveen Is there a case where the solution under #7371 does not work ?

I have tried that fix with multiple pkgdir entries on RH8 where BaseOS and AppStream need to be appended, and with multiple pkgdir entries on RH7.6 where it is just a path specified in pkgdir. Both produce correct set of .repo files.

@conxuro
Copy link

conxuro commented May 15, 2023

I have tested the new ospkgs and it works for me. Thanks for the fix!

Take into account the two commits (6397efd a95e293) since the first one (6397efd) standalone didn't work. I tried it before the solution proposed in #7371 and does the same issue but with the first repo file with void URL instead the third one. However, with the commit a95e293 now the postscript does the indexing properly without wrong URLs.

In addition, not directly related to this bug/fix, the ospkgs postscript creates one repo file per repository but the files created at install time have merged the two first repos into one file (that causes a discrepancy with the number of repo files before and after).

cat /etc/yum.repos.d/local-repository-0.repo
[local-rocky8.6-x86_64--install-rocky8.6-x86_64-BaseOS]
name=xCAT configured yum repository for /install/rocky8.6/x86_64/BaseOS
baseurl=http://10.3.0.1:80//install/rocky8.6/x86_64/BaseOS
enabled=1
gpgcheck=0

[local-rocky8.6-x86_64--install-rocky8.6-x86_64-AppStream]
name=xCAT configured yum repository for /install/rocky8.6/x86_64/AppStream
baseurl=http://10.3.0.1:80//install/rocky8.6/x86_64/AppStream
enabled=1
gpgcheck=0

I think this behavior and the change of the repo files names (local*.repo vs xCAT*.repo) can cause confusions and can be more prone to errors.

@samveen
Copy link
Member Author

samveen commented May 18, 2023

@conxuro

  • thank you for testing and the feedback.
  • 6397efd is based on incorrect assumptions and is thus by itself a toxic commit.
  • a95e293 reverts the previous commit and re-does the code cleanly and hopefully correctly.
  • The local repository names created during the kickstart process is done by adding appropriate cat commands to the pre-install script section of the kickstart files for the nodes (as generated by xCAT-server/lib/perl/xCAT/Template.pm), and are expected to be transient for the duration of the install. (git grep INSTALL_SOURCES_IN_PRE for more info)

@CLAassistant
Copy link

CLAassistant commented Jul 31, 2024

CLA assistant check
All committers have signed the CLA.

@Obihoernchen Obihoernchen modified the milestones: 2.17, 2.17.1 Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants