Update installer.py #19

spyroot · 2023-02-21T05:03:10Z

additional_rpms is json list, not just a single entity. Hence it makes sense to iterate for each dir and invoke the rpm.
It also fixes the issue if the rpm_paths is a list; exists will raise an exception and crash since it can't process the list.

additional_rpms is json list, not just a single entity. Hence it makes sense to iterate for each dir and invoke the rpm

vmwclabot · 2023-02-21T05:03:17Z

@spyroot, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

VMware employees are not required to execute VMware CLAs.”

vmwclabot · 2023-02-21T05:06:05Z

@spyroot, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

vmwclabot · 2023-02-21T10:59:11Z

@spyroot, VMware has rejected your signed contributor license agreement. The merge can not proceed until the agreement has been resigned. Click here to resign the agreement. Reject reason:

Please provide full street address

vmwclabot · 2023-02-25T18:18:05Z

@spyroot, we have received your signed contributor license agreement. The review is usually completed within a week, but may take longer under certain circumstances. Another comment will be added to the pull request to notify you when the merge can proceed.

vmwclabot · 2023-02-27T10:44:34Z

@spyroot, VMware has approved your signed contributor license agreement.

sshedi

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

sshedi · 2023-04-18T04:56:54Z

photon_installer/installer.py

@@ -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 = []
+        if not rpms_path or len(rpm_dir) == 0:


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

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.

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

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", "/" ],

sshedi · 2023-04-18T04:58:26Z

photon_installer/installer.py

-        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)]


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

Throw exception if path doesn't exist

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.

sshedi · 2023-04-18T05:03:04Z

photon_installer/installer.py

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


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.

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

oliverkurth · 2023-04-18T04:57:51Z

photon_installer/installer.py

@@ -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 = []


Nit: please rename to rpms_dir.

oliverkurth · 2023-04-18T04:59:19Z

photon_installer/installer.py

+                       and os.path.exists(d)]
+        else:
+            if os.path.exists(rpms_path):
+                rpm_dir.append(rpm_dir)


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

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.

oliverkurth · 2023-04-18T05:02:27Z

photon_installer/installer.py

+            except FileNotFoundError as _:
+                pass
+
+        if cmd_err > 0:


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

oliverkurth · 2023-04-18T05:04:23Z

photon_installer/installer.py

-        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)]


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

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

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.

oliverkurth · 2023-04-18T05:05:04Z

photon_installer/installer.py

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


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

Update installer.py

a3820d8

additional_rpms is json list, not just a single entity. Hence it makes sense to iterate for each dir and invoke the rpm

vmwclabot added the cla-rejected label Feb 21, 2023

vmwclabot removed the cla-rejected label Feb 27, 2023

jaankit requested review from oliverkurth, gpiyush-dev, YustasSwamp, jaankit and sshedi April 18, 2023 04:29

sshedi requested changes Apr 18, 2023

View reviewed changes

oliverkurth reviewed Apr 18, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update installer.py #19

Update installer.py #19

spyroot commented Feb 21, 2023 •

edited

Loading

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 25, 2023

vmwclabot commented Feb 27, 2023

sshedi left a comment

sshedi Apr 18, 2023

spyroot Apr 18, 2023

sshedi Apr 18, 2023

spyroot Apr 18, 2023 •

edited

Loading

sshedi Apr 18, 2023

oliverkurth Apr 18, 2023

oliverkurth Apr 18, 2023

oliverkurth Apr 18, 2023

spyroot Apr 18, 2023

oliverkurth Apr 18, 2023

oliverkurth Apr 18, 2023

spyroot Apr 18, 2023

oliverkurth Apr 18, 2023

Update installer.py #19

Are you sure you want to change the base?

Update installer.py #19

Conversation

spyroot commented Feb 21, 2023 • edited Loading

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 21, 2023

vmwclabot commented Feb 25, 2023

vmwclabot commented Feb 27, 2023

sshedi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyroot Apr 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spyroot commented Feb 21, 2023 •

edited

Loading

spyroot Apr 18, 2023 •

edited

Loading