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

Removed needless login shell from chroot calls where possible #1171

Merged

Conversation

jsmeix
Copy link
Member

@jsmeix jsmeix commented Jan 18, 2017

see #862

@jsmeix jsmeix added cleanup minor bug An alternative or workaround exists labels Jan 18, 2017
@jsmeix jsmeix added this to the Rear v2.1 milestone Jan 18, 2017
@jsmeix jsmeix self-assigned this Jan 18, 2017
@jsmeix
Copy link
Member Author

jsmeix commented Jan 18, 2017

The only "chroot ... bash --login" call that is left is in
finalize/Debian/i386/170_rebuild_initramfs.sh

  chroot $TARGET_FS_ROOT /bin/bash --login -c "/usr/share/mdadm/mkconf >/etc/mdadm/mdadm.conf"

because of the stdout redirection.

@jsmeix
Copy link
Member Author

jsmeix commented Jan 19, 2017

Do not merge it!
It does not yet work.
This one is RFC 1925 (8): "It is more complicated than you think."

@jsmeix jsmeix assigned jsmeix and unassigned jsmeix Jan 19, 2017
@jsmeix
Copy link
Member Author

jsmeix commented Jan 19, 2017

Now it works for me on SLES12:

RESCUE e205:~ # rear -d -D recover
...
Restoring finished.
...
Running mkinitrd...
Recreated initrd (/sbin/mkinitrd).
Installing GRUB2 boot loader
Finished recovering your system. You can explore it under '/mnt/local'.

FYI:
I need in local.conf

OS_MASTER_VENDOR="SUSE_LINUX/i386"

(or something else like that) because otherwise
finalize/SUSE_LINUX/i386/170_rebuild_initramfs.sh
is not sourced.
Whether or not a rebuild_initramfs script should be sourced
in any case is a different issue - I never had issues with the
initrd on my test systems so that a rebuild_initramfs script
seems to be not really mandatory.

@jsmeix jsmeix requested a review from gdha January 19, 2017 13:53
@jsmeix
Copy link
Member Author

jsmeix commented Jan 19, 2017

@gdha
could you please review whether or not
it still works for you with my changes in
finalize/Fedora/i386/170_rebuild_initramfs.sh

FYI:
By the way I found a chroot call in
finalize/Linux-i386/220_install_grub2.sh
where I think that cannot work:

chroot $TARGET_FS_ROOT $grub_name-mkconfig -o /boot/$grub_name/grub.cfg

because $grub_name evaluates to "grub2" or "grub"
but chroot requires usually a full path to be executed like

RESCUE e205:~ # chroot /mnt/local ifconfig
chroot: failed to run command 'ifconfig': No such file or directory

RESCUE e205:~ # chroot /mnt/local /sbin/ifconfig
eth0      Link encap:Ethernet  HWaddr 52:54:00:25:A4:49
...

see also my new code regarding "mkinitrd_binary" in
finalize/Fedora/i386/170_rebuild_initramfs.sh

Copy link
Member

@gdha gdha left a comment

Choose a reason for hiding this comment

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

The I was probably Schlomo?
The changes look sane and should improve statbility - thank you for that! I will run a validation test later today on the new code.

@gdha gdha merged commit 3baf9eb into rear:master Jan 20, 2017
@gdha
Copy link
Member

gdha commented Jan 20, 2017

@jsmeix Merged it - results will be posted in gdha/rear-automated-testing#13

@jsmeix jsmeix deleted the remove_needless_bash_from_chroot_calls_issue862 branch January 20, 2017 12:42
@jsmeix
Copy link
Member Author

jsmeix commented Jan 20, 2017

@gdha
many thanks for your careful review!

FYI regarding who the 'I' actually is:
To find out who made what in a file I use usually
the git command "git log -p --follow file.sh".
For the 170_rebuild_initramfs.sh files
that command shows as the oldest entry always

commit 844d50b75ac4b7722f4fee7a5ee3350b93f3adb7
Author: Schlomo Schapiro 
Date:   Sun Jun 6 08:30:21 2010 +0000

    - Integrated P2V patch from Heinlein Support. We start with 1.9 now to

It is the same commit
844d50b
for
finalize/Fedora/i386/170_rebuild_initramfs.sh
finalize/Debian/i386/170_rebuild_initramfs.sh
finalize/SUSE_LINUX/i386/170_rebuild_initramfs.sh
so that I think the 'I' could be Peer Heinlein
and Schlomo only comitted his changes?

@gdha
Copy link
Member

gdha commented Jan 20, 2017

@jsmeix I can confirm the modifications work well on CentOS7

# is backed by udev
# probably not required, but I prefer to rely on this information when it is backed by udev
# FIXME: who is 'I'?
# Perhaps Schlomo Schapiro or someone who made the "P2V patch from Heinlein Support"?
Copy link
Member

Choose a reason for hiding this comment

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

This is really long time ago. udev back then was indeed optional on some distros. I would say that this changed and udev is not optional any more.

@jsmeix
Copy link
Member Author

jsmeix commented Jan 23, 2017

@schlomo
many thanks for the information!

I will do a separated pull request
where I let ReaR show same kind of warning
as near the end of the code about
"Cannot create initramfs"
if the udev requirement is not fulfilled.

Regarding LogPrint messages like

WARNING:
Cannot create initramfs
...
Check the recreated system (mounted at /mnt/local)
and decide yourself, whether the system will boot or not.

@schlomo
because I know what you think about warning messages:
Schould I remove the word "WARNING:" from those
messages and make it a plain information?

@schlomo
Copy link
Member

schlomo commented Jan 23, 2017

Thanks @jsmeix for your consideration.

One one hand the warning is justified as it really means "I don't know".

On the other hand we should try to give the user even more information here. For example, which systems don't have udev nowadays? Maybe we can say that ReaR 2 actually requires udev and fails without? Then you could get rid of this warning.

Another thought: Maybe we should expand this to be a generic feature and collect such warnings in a list of RECOVERY_NOTICE[@] which we display at the end. In an automated recovery having a non-empty list of recovery notices would be an error, in a manual recovery it would be printed as user info.

@jsmeix
Copy link
Member Author

jsmeix commented Jan 23, 2017

I have to think a bit more about it.
I think the idea behind something like a RECOVERY_NOTICE array
is related to #1134
because also in an automated recovery proper exit codes
could be used to signal something to the user.
In other words:
If in an automated recovery a RECOVERY_NOTICE
would be an error, rear should exit with an appropriate
non-zero exit code in this case to singal the user:
Not a fatal error (e.g. after calling the Error function)
but also not a clean recovery.

@jsmeix
Copy link
Member Author

jsmeix commented May 8, 2017

It seems using no login shell for chroot calls
causes more issues that it solves, see
#1345

jsmeix added a commit that referenced this pull request May 9, 2017
mkinitrd need $PATH when running in chroot on sles11
because it calls other tools without full path, cf the
related issue #862
and #1171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup fixed / solved / done minor bug An alternative or workaround exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants