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.

Sign up for GitHub

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

Jump to bottom

Update installer.py #19

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update installer.py #19

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions photon_installer/installer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -926,10 +926,28 @@ def _setup_install_repo(self):
def _install_additional_rpms(self):
rpms_path = self.install_config.get('additional_rpms_path', None)

if not rpms_path or not os.path.exists(rpms_path):
rpm_dir = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: please rename to rpms_dir.

if not rpms_path or len(rpm_dir) == 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested this @spyroot ? AFAIK len(rpm_dir) is always 0 here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, what's the need for this requirement? We expect all extra rpms to be present in one place and keep it simple. RPM files at different locations during installation time look unusual.

Short answer : )
https://github.com/spyroot/photon_rt/blob/main/post.sh

Two cases. Consider during a boot, you really need to figure out where to look for RPMs. For example, you have the latest version for rpm without collision with the main RPM inside ISO in packages_rt_expanded.

  1. i.e., you want to indicate

/mnt/media
/mnt/cdrom
/mnt/dvd
etc

So if this name changes, you need to change all customization.
Assume you have a set of mandatory RPMs. For example, you need to build
the latest intel driver, you need to build the
latest kernel module, you need to build the latest DPKD, etc. So you really need
a toolchain with all dependencies. Now you want all this toolchain available on
the first boot. Install, reboot, post recompile the kernel module, and you
will have the latest driver.

https://github.com/spyroot/photon_rt/blob/main/post.sh

  1. If you want split no arch vs. SRPM vs. rpm, i.e., have a similar structure to match ISO.
    (i.e., some structure so you don't put rpm and mess all together) I hope it makes sense.

https://packages.vmware.com/photon/5.0/photon_updates_5.0_x86_64/
For example.

  • noarch
  • x86_64
  • arm

If you keep a flat structure, you can technically put RPM that will collide.
So on postscript customization, you can resolve where to look, fallback, etc.

  "additional_packages": [
    "docker",
    "vim",
    "gcc",
    "git",
    "wget",
    "linux-rt",
    "linux-rt-devel",
    "make",
    "build-essential"
  ],
  "additional_rpms_path": "/mnt/media/direct_rpms",
  "postinstallscripts": [
    "post.sh",
    "overwrite.env"
  ],
  "search_path": [
    "/mnt/media",
    "/tmp",
    "/mnt/media/direct_rpms",
    "/mnt/media/git_images",
    "/mnt/media/direct",
    "/"
  ],

return

if self.cmd.run(['rpm', '--root', self.photon_root, '-U', rpms_path + '/*.rpm']) != 0:
if isinstance(rpms_path, list):
rpm_dir = [d for d in rpm_dir if len(d) > 0
and os.path.exists(d)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't check for len(d) > 0, if user provides invalid rpm paths, it should be caught.

Throw exception if path doesn't exist

Copy link
Contributor Author

@spyroot spyroot Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I specifically did this because there is an issue with how cmd returns the exit code.

The exit code that we use is not a sufficient condition. I did a separate patch for that.
In essence, rpm can execute, but it returns an exit code that indicates something
different than an error. i.e., it is not a reliable indication. ( I need to cross-check my notes.
But consider the same RPM present in the main RPM dir and additional, i.e., something already installed, something requires another RPM. (From my memory, I'll check the rpm return for all these case exit codes != 0 )

Could you please also check I did a separate commit specifically for run

 process = subprocess.Popen(
                cmd, shell=use_shell,
                stdout=subprocess.PIPE,
                stderr=subprocess.PIPE
            )

So if an exception is raised, that retval is 0.
FileNotFoundError etc.

3431f6f

That way, the logical step considers this as the best-effort operation.
"Attempts" since it is additional RPMs technically optional, you can finish
the installation without crashing the installer. (Consider a person who wrote kick start
and put to additional rpm version of rpm that > then in ISO RPM, Person put the
same rpm by mistake that already in ISO RPM to additional rpm dir)

Because you can debug and fix any issue if we don't crash,
While if you raise an exception, it takes time to nail down the problem.
(When I say debug, I imply someone who wrote the kick-start file needs
the option to install and post-check why some rpm not install, etc.).

So the command run can interleave with the parent PID, then we have a concurrency issue.
Subprocess can raise exceptions ( run time exception, IO exception, etc)

i.e
process = subprocess.Popen(
cmd, shell=use_shell,
stdout=subprocess.PIPE,
stderr=subprocess.PIPE
)

This subprocess call might be executed before CDROM is mounted, and we don't read from stderr.
So we need a way to detect it and handle it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should throw an error if the path does not exist.

Better yet, check if the directory contains any rpm files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So in this patch, I indicate what the issue is closely related.
https://github.com/spyroot/photon-os-installer/tree/patch-2

The problem is that sub-process might not report the exit code.
So if you have an exception raised. We don't know about that; it not reported from cmd. run,
the same issue if you have a run time exception or if the child PID interleaved.

Since we don't read from stderr and parse. Another issue sub-process might
actually, execute before CDROM is even mounted.

If you check the exit code from the rpm tool, you will see it returns different exit codes
that is another issue.

Suppose RPM in additional rpm has conflict, i.e., ver. > Then, in the RPM in actual ISO,
if RPM is already installed, or exception raised, or a version of rpm i
n additional < than in ISO rpms.

For example, if generated in the script additional rpms, you always
download python3_something. ( latest from our repo).

Since we invoke rpm with a list if one rpm in that list is wrong based
on condition I indicated.

And because it is a sub-process, debugging is tough for someone who wrote Kickstart.

Essentially it boils down. Do you want a crash installer or not? If we want to crash,
we need to dump from the sub-process stderr, read it, and serialize it to a log.

That way, using the best-effort strategy here, IMHO better. so a person will be able to install OS
, figure out that rpms additional did not succeed, resolve conflict ( ver/path, etc.)
go back and fix the kickstart file.

else:
if os.path.exists(rpms_path):
rpm_dir.append(rpm_dir)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant to do rpm_dir.append(rpms_path), so we have list with just rpms_path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, So old behavior should work so it won't break old kick-start files., i.e if it is just a single value or if it lists, we check one by one as best effort. i.e., if you wrote kick start and have no clue where CDROM mounted /mnt/cdrom / mnt/media etc. so you can add a bunch of dir.

else:
return

cmd_err = 0
for rdir in rpm_dir:
try:
# best effort for each dir in list
cmd_err = self.cmd.run(['rpm', '--root', self.photon_root, '-U', rdir + '/*.rpm'])
except FileNotFoundError as _:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, don't ignore erroneous situations.

Small optimization here can be, passing all rpm dirs at once to rpm --root ... -U ...
Like:

rpm --root ... -U path1/*.rpm path2/*.rpm ...

An rpm in path1 might need an rpm from path2 as a requirement, so it will get satisfied.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we should use tdnf so it can resolve all dependencies.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We wouldn't need this if we had checked for rpm files above.

pass

if cmd_err > 0:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be within the loop above or we'd only see if the last iteration failed.

self.logger.info('Failed to install additional_rpms from ' + rpms_path)
self.exit_gracefully()

Expand Down