Merge ~ltrager/curtin:centos8 into curtin:master

[Lee Trager]

centos: Add centos/rhel 8 support, enable UEFI Secure Boot

This branch adds support for deploying CentOS and RHEL 8 images. As a
side effect this also enables secure UEFI boot for CentOS 7 when the
grub2-efi-x64 package is installed.

LP: #1788088

A few things to note

* I added support for secure boot on both CentOS/RHEL 7 and 8. Curtin will use the old method of generating an unsigned grub bootloader if only the grub2-x64-modules package is installed. This allows old images to continue to be deployable without having to access the Internet.
* Upstream has removed support for btrfs[1]. btrfs use will be prevented from being configured in MAAS.
* cloud-init on RHEL/CentOS 8 is configured to create the default user as cloud-user.

Test image:
https://private-fileshare.canonical.com/~ltrager/centos8.tar.gz

[1] https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html/considerations_in_adopting_rhel_8/file-systems-and-storage_considerations-in-adopting-rhel-8

[Ryan Harper (raharper)]

Thanks for putting this together. A couple of comments on the notes:

On btrfs, like bcache, MAAS doesn't allow such storage configs; I'd expect
MAAS to avoid creating btrfs filesystems for centos8 targets.

Per network-scripts, I've replied in the bug you filed; this isn't an issue
as long as the centos8 image includes the ifcfg-rh network-manager plugin.

W.r.t the cloud-init-el-release, we should continue to install the repo in
the case that we need to provide an update/hotfix prior to what would show
up in downstream centos8.

I've replied in-line below with some comments/questions on the changes.

review: Needs Fixing

6cdc28f... by Lee Trager on 2019-10-18

rharper fixes

[Lee Trager (ltrager)]

Thanks for the review. I've updated as suggested and responded inline.

I was able to get cloud-init to work with NetworkManager as you suggested once I made sure the plugin was installed.

I agree that we should have a cloud-init repository enabled however I think we need a new repository for CentOS 8. The current version is Python 2 while CentOS 8 has moved to Python 3.6. The version included with CentOS 8 also works with MAAS without requiring any packages from EPEL.

c23710b... by Lee Trager on 2019-10-19

Update redhat_upgrade_cloud_init for CentOS 8

[Ryan Harper (raharper)]

You're correct w.r.t Centos8 (we need to build/upload a py3 version). Cloud-init does have a branch to enable py3 builds from master, here:

https://code.launchpad.net/~raharper/cloud-init/+git/cloud-init/+merge/368845

We do have Centos8 builds in the COPR repo; We could push in one of the srpms from that branch into el-testing. Ideally I'd like to verify that the daily cloud-init on centos8 (py3) works.

I think since it's a different release (el8) I think the same repo can hold all 3 version (el6/7 and 8).

W.r.t EPEL, yes I think in Centos8, the cloud-init deps are present and we won't need to enable EPEL.

c9348f0... by Lee Trager on 2019-10-25

Merge branch 'master' into centos8

0cda0e9... by Lee Trager on 2019-10-25

Use helper to get RHEL version

[Ryan Harper (raharper)]

Sorry for the delay, a few more questions.

review: Needs Information

2dcbdbd... by Lee Trager on 2019-10-31

Only install bridge-utils on RHEL8

3cded85... by Lee Trager on 2019-10-31

Merge branch 'master' into centos8

[Lee Trager (ltrager)]

Thanks for the review. I fixed installing bridge-utils on CentOS8 and responded below.

[Ryan Harper (raharper)]

review: Needs Fixing

[Ryan Harper (raharper)]

One more comment around the efibootmgr change.

d8e6046... by Lee Trager on 2019-11-02

raharper fixes

[Lee Trager (ltrager)]

Thanks for the review. I've updated as suggested. Curtin will now try to determine the system partition from the storage configuration given. If no storage configuration was sent Curtin will fall back on auto detection.

[Ryan Harper (raharper)]

Thanks for the refactor, some more in-line comments. Specifically I want not try to grok the edi_dev and partition number if we know it curthooks.

review: Needs Fixing

3abb50c... by Lee Trager on 2019-11-05

Merge branch 'master' into centos8

327abcc... by Lee Trager on 2019-11-06

rharper fixes

[Lee Trager (ltrager)]

Fixed some things up and responded below. Let me know if that makes sense.

[Ryan Harper (raharper)]

review: Needs Fixing

e678aca... by Lee Trager on 2019-11-13

rharper fixes

[Lee Trager (ltrager)]

Updated as suggested and responded below.

00a8d01... by Lee Trager on 2019-11-23

Merge branch 'master' into centos8

39af019... by Lee Trager on 2019-11-23

Fix RHEL UEFI deployments

[Ryan Harper (raharper)]

Thanks for meeting. here's a quick refactor of looking up the efi partition via storage config.

I think we should extract the list of 'grub_device' ids, and then any /boot/efi ids, then we can apply the get_path_to_storage_volume() to each item in the list of ids.

https://paste.ubuntu.com/p/JW58dz8WKv/

ad2478e... by Lee Trager on 2019-12-04

Merge branch 'master' into centos8

7246e40... by Lee Trager on 2019-12-05

Fixes from meeting with rharper

[Lee Trager (ltrager)]

Updated to remove the while loop. MAAS sets all grub_device to True on all block devices when a RAID1 array is configured. It does this for BIOS systems to ensure GRUB is on the super block of all RAID 1 devices. Since UEFI GRUB only support one system partition MAAS assumes Curtin will select the partition /boot/efi is mounted at.

This could be fixed in MAAS but would cause problems if a newer version of Curtin is used with an older version of MAAS.

[1] http://paste.ubuntu.com/p/ZQSCHHDDX8/

[Ryan Harper (raharper)]

That's fine for now, though changes are coming here:

# UEFI-Hybrid mode
https://bugs.launchpad.net/curtin/+bug/1750920

And newer maas should define both a bios_grub partition and the efi partition to allow an image to boot either way.

And there's more work coming to enhance boot resilience

https://docs.google.com/document/d/1ew9-vrha9xrrbVqiX0vhlV3W7pAInaNAbJvftFUNE7w/edit?ts=5dd71f82

I'll get some vmtests on this.

[Ryan Harper (raharper)]

Kicked off vmtest run on this branch.

https://jenkins.ubuntu.com/server/job/curtin-vmtest-devel-debug/205/console

[Ryan Harper (raharper)]

vmtest results looked good. We have some known failures, IO timeouts that we expect. I'll verify those; there's one failure which is fixed if you rebase this branch to master and force push here. Let's do that so I only need to verify the timeouts. I'll rerun the 4 failures after you've rebased this branch on master.

084a368... by Lee Trager on 2019-12-16

Merge branch 'master' into centos8

[Lee Trager (ltrager)]

Master has been merged :)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:084a368ac74eb3d69a4c740191f96d5ed0bf63bb
https://jenkins.ubuntu.com/server/job/curtin-ci/3807/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/3807/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/3807/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/3807/
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/3807/

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/3807//rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Thanks!

review: Approve

diff --git a/curtin/commands/curthooks.py b/curtin/commands/curthooks.py
index 4b36da2..43ebe8d 100644
--- a/curtin/commands/curthooks.py
+++ b/curtin/commands/curthooks.py
@@ -469,12 +469,25 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
 
     if storage_cfg_odict:
         storage_grub_devices = []
-        for item_id, item in storage_cfg_odict.items():
-            if not item.get('grub_device'):
-                continue
-            LOG.debug("checking: %s", item)
-            storage_grub_devices.append(
-                get_path_to_storage_volume(item_id, storage_cfg_odict))
+        if util.is_uefi_bootable():
+            # Curtin only supports creating one EFI system partition. Thus the
+            # grub_device can only be the default system partition mounted at
+            # /boot/efi.
+            for item_id, item in storage_cfg_odict.items():
+                if item.get('path') == '/boot/efi':
+                    efi_dev_id = storage_cfg_odict[item['device']]['volume']
+                    LOG.debug("checking: %s", item)
+                    storage_grub_devices.append(get_path_to_storage_volume(
+                        efi_dev_id, storage_cfg_odict))
+                    break
+        else:
+            for item_id, item in storage_cfg_odict.items():
+                if not item.get('grub_device'):
+                    continue
+                LOG.debug("checking: %s", item)
+                storage_grub_devices.append(
+                    get_path_to_storage_volume(item_id, storage_cfg_odict))
+
         if len(storage_grub_devices) > 0:
             grubcfg['install_devices'] = storage_grub_devices
 
@@ -575,7 +588,7 @@ def setup_grub(cfg, target, osfamily=DISTROS.debian):
         join_stdout_err = ['sh', '-c', 'exec "$0" "$@" 2>&1']
         out, _err = util.subp(
             join_stdout_err + args + instdevs, env=env, capture=True)
-        LOG.debug("%s\n%s\n", args, out)
+        LOG.debug("%s\n%s\n", args + instdevs, out)
 
     if util.is_uefi_bootable() and grubcfg.get('update_nvram', True):
         uefi_reorder_loaders(grubcfg, target)
@@ -978,10 +991,17 @@ def install_missing_packages(cfg, target, osfamily=DISTROS.debian):
     # UEFI requires grub-efi-{arch}. If a signed version of that package
     # exists then it will be installed.
     if util.is_uefi_bootable():
-        uefi_pkgs = []
+        uefi_pkgs = ['efibootmgr']
         if osfamily == DISTROS.redhat:
             # centos/redhat doesn't support 32-bit?
-            uefi_pkgs.extend(['grub2-efi-x64-modules'])
+            if 'grub2-efi-x64-modules' not in installed_packages:
+                # Previously Curtin only supported unsigned GRUB due to an
+                # upstream bug. By default lp:maas-image-builder and
+                # packer-maas have grub preinstalled. If grub2-efi-x64-modules
+                # is already in the image use unsigned grub so the install
+                # doesn't require Internet access. If grub is missing use the
+                # signed version.
+                uefi_pkgs.extend(['grub2-efi-x64', 'shim-x64'])
         elif osfamily == DISTROS.debian:
             arch = util.get_architecture()
             if arch == 'i386':
@@ -1261,40 +1281,43 @@ def redhat_upgrade_cloud_init(netcfg, target=None, osfamily=DISTROS.redhat):
             cloud_init_yum_repo = (
                 paths.target_path(target,
                                   'etc/yum.repos.d/curtin-cloud-init.repo'))
+            rhel_ver = distro.rpm_get_dist_id(target)
             # Inject cloud-init daily yum repo
             util.write_file(cloud_init_yum_repo,
-                            content=cloud_init_repo(
-                                distro.rpm_get_dist_id(target)))
+                            content=cloud_init_repo(rhel_ver))
 
-            # we separate the installation of repository packages (epel,
+            # ensure up-to-date ca-certificates to handle https mirror
+            # connections for epel and cloud-init-el.
+            packages = ['ca-certificates']
+
+            if int(rhel_ver) < 8:
+                # cloud-init in RHEL < 8 requires EPEL for dependencies.
+                packages += ['epel-release']
+                # RHEL8+ no longer ships bridge-utils. This does not effect
+                # bridge configuration. Only install on RHEL < 8 if not
+                # available, do not upgrade.
+                with util.ChrootableTarget(target) as in_chroot:
+                    try:
+                        in_chroot.subp(['rpm', '-q', 'bridge-utils'],
+                                       capture=False, rcs=[0])
+                    except util.ProcessExecutionError:
+                        LOG.debug(
+                            'Image missing bridge-utils package, installing')
+                        packages += ['bridge-utils']
+
+            packages += ['cloud-init-el-release', 'cloud-init']
+
+            # We separate the installation of repository packages (epel,
             # cloud-init-el-release) as we need a new invocation of yum
             # to read the newly installed repo files.
-
-            # ensure up-to-date ca-certificates to handle https mirror
-            # connections
-            distro.install_packages(['ca-certificates'], target=target,
-                                    osfamily=osfamily)
-            distro.install_packages(['epel-release'], target=target,
-                                    osfamily=osfamily)
-            distro.install_packages(['cloud-init-el-release'], target=target,
-                                    osfamily=osfamily)
-            distro.install_packages(['cloud-init'], target=target,
-                                    osfamily=osfamily)
+            for package in packages:
+                distro.install_packages(
+                    [package], target=target, osfamily=osfamily)
 
             # remove cloud-init el-stable bootstrap repo config as the
             # cloud-init-el-release package points to the correct repo
             util.del_file(cloud_init_yum_repo)
 
-            # install bridge-utils if needed
-            with util.ChrootableTarget(target) as in_chroot:
-                try:
-                    in_chroot.subp(['rpm', '-q', 'bridge-utils'],
-                                   capture=False, rcs=[0])
-                except util.ProcessExecutionError:
-                    LOG.debug('Image missing bridge-utils package, installing')
-                    distro.install_packages(['bridge-utils'], target=target,
-                                            osfamily=osfamily)
-
     LOG.info('Passing network configuration through to target')
     net.render_netconfig_passthrough(target, netconfig={'network': netcfg})
 
diff --git a/curtin/distro.py b/curtin/distro.py
index 9714515..b233e26 100644
--- a/curtin/distro.py
+++ b/curtin/distro.py
@@ -284,7 +284,13 @@ def run_yum_command(mode, args=None, opts=None, env=None, target=None,
     if opts is None:
         opts = []
 
-    cmd = ['yum'] + defopts + opts + [mode] + args
+    # dnf is a drop in replacement for yum. On newer RH based systems yum
+    # is just a sym link to dnf.
+    if which('dnf', target=target):
+        cmd = ['dnf']
+    else:
+        cmd = ['yum']
+    cmd += defopts + opts + [mode] + args
     if not execute:
         return env, cmd
 
@@ -311,8 +317,14 @@ def yum_install(mode, packages=None, opts=None, env=None, target=None,
         raise ValueError(
             'Unsupported mode "%s" for yum package install/upgrade' % mode)
 
+    # dnf is a drop in replacement for yum. On newer RH based systems yum
+    # is just a sym link to dnf.
+    if which('dnf', target=target):
+        cmd = ['dnf']
+    else:
+        cmd = ['yum']
     # download first, then install/upgrade from cache
-    cmd = ['yum'] + defopts + opts + [mode]
+    cmd += defopts + opts + [mode]
     dl_opts = ['--downloadonly', '--setopt=keepcache=1']
     inst_opts = ['--cacheonly']
 
diff --git a/helpers/common b/helpers/common
index 5928150..6c8e4f0 100644
--- a/helpers/common
+++ b/helpers/common
@@ -679,7 +679,7 @@ install_grub() {
 
     # grub is not the bootloader you are looking for
     if [ "${target_arch}" = "s390x" ]; then
-	    return 0;
+        return 0;
     fi
 
     # set correct grub package
@@ -694,7 +694,7 @@ install_grub() {
         x86_64)
             case $rhel_ver in
                6) grub_name="grub";;
-               7) grub_name="grub2-pc";;
+               7|8) grub_name="grub2-pc";;
                *)
                    error "Unknown rhel_ver [$rhel_ver]";
                    return 1;
@@ -711,7 +711,10 @@ install_grub() {
         case "$target_arch" in
             x86_64)
                 # centos 7+, no centos6 support
-                grub_name="grub2-efi-x64-modules"
+                # grub2-efi-x64 installs a signed grub bootloader while
+                # curtin uses grub2-efi-x64-modules to generate grubx64.efi.
+                # Either works just check that one of them is installed.
+                grub_name="grub2-efi-x64 grub2-efi-x64-modules"
                 grub_target="x86_64-efi"
                 ;;
             amd64)
@@ -739,11 +742,13 @@ install_grub() {
         error "failed to check if $grub_name installed";
         return 1;
     fi
-    case "$tmp" in
-        install\ ok\ installed) :;;
-        *) debug 1 "$grub_name not installed, not doing anything";
-            return 1;;
-    esac
+    # Check that any of the packages in $grub_name are installed. If
+    # grub_name contains multiple packages, as it does for CentOS 7+,
+    # only one package has to be installed for this to pass.
+    if ! echo $tmp | grep -q 'install ok installed'; then
+        debug 1 "$grub_name not installed, not doing anything"
+        return 1
+    fi
 
     local grub_d="etc/default/grub.d"
     # ubuntu writes to /etc/default/grub.d/50-curtin-settings.cfg
@@ -822,11 +827,31 @@ install_grub() {
         if [ "$update_nvram" -ge 1 ]; then
             nvram=""
         fi
+        if [ "${#grubdevs_new[@]}" -eq 1 ] && [ -f "${grubdevs_new[0]}" ]; then
+            # Currently UEFI can only be pointed to one system partition. If
+            # for some reason multiple install locations are given only use the
+            # first.
+            efi_dev="${grubdevs_new[0]}"
+        elif [ "${#grubdevs_new[@]}" -gt 1 ]; then
+            error "Only one grub device supported on UEFI!"
+            exit 1
+        else
+            # If no storage configuration was given try to determine the system
+            # partition.
+            efi_dev=$(awk -v "MP=${mp}/boot/efi" '$2 == MP { print $1 }' /proc/mounts)
+        fi
+        # The partition number of block device name need to be determined here
+        # so both getting the UEFI device from Curtin config and discovering it
+        # work.
+        efi_part_num=$(cat /sys/class/block/$(basename $efi_dev)/partition)
+        efi_disk="/dev/$(lsblk -no pkname $efi_dev)"
         debug 1 "curtin uefi: installing ${grub_name} to: /boot/efi"
         chroot "$mp" env DEBIAN_FRONTEND=noninteractive sh -exc '
             echo "before grub-install efiboot settings"
             efibootmgr -v || echo "WARN: efibootmgr exited $?"
             bootid="$4"
+            efi_disk="$5"
+            efi_part_num="$6"
             grubpost=""
             case $bootid in
                 debian|ubuntu)
@@ -836,7 +861,15 @@ install_grub() {
                     ;;
                 centos|redhat|rhel)
                     grubcmd="grub2-install"
-                    grubpost="grub2-mkconfig -o /boot/grub2/grub.cfg"
+                    # RHEL uses redhat instead of the os_variant rhel for the bootid.
+                    if [ "$bootid" = "rhel" ]; then
+                        bootid="redhat"
+                    fi
+                    if [ -f /boot/efi/EFI/$bootid/grubx64.efi ]; then
+                        grubpost="grub2-mkconfig -o /boot/efi/EFI/$bootid/grub.cfg"
+                    else
+                        grubpost="grub2-mkconfig -o /boot/grub2/grub.cfg"
+                    fi
                     ;;
                 *)
                     echo "Unsupported OS: $bootid" 1>&2
@@ -852,10 +885,45 @@ install_grub() {
             echo "$gi_out" | grep -q -- "$no_nvram" || no_nvram=""
             echo "$gi_out" | grep -q -- "--target" || target=""
             echo "$gi_out" | grep -q -- "--efi-directory" || efi_dir=""
-            $grubcmd $target $efi_dir \
-                --bootloader-id=$bootid --recheck $no_nvram
+
+            # Do not overwrite grubx64.efi if it already exists. grub-install
+            # generates grubx64.efi and overwrites any existing binary in
+            # /boot/efi/EFI/$bootid. This binary is not signed and will cause
+            # secure boot to fail.
+            #
+            # CentOS, RHEL, Fedora ship the signed boot loader in the package
+            # grub2-efi-x64 which installs the signed boot loader to
+            # /boot/efi/EFI/$bootid/grubx64.efi. All Curtin has to do is
+            # configure the firmware. This mirrors what Anaconda does.
+            #
+            # Debian and Ubuntu come with a patched version of grub which
+            # add the install flag --uefi-secure-boot which is enabled by
+            # default. When enabled if a signed version of grub exists on
+            # the filesystem it will be copied into /boot/efi/EFI/$bootid.
+            # Stock Ubuntu images do not ship with anything in /boot. Those
+            # files are generated by installing a kernel and grub.
+            if [ -f /boot/efi/EFI/$bootid/grubx64.efi ]; then
+                if [ -z "$no_nvram" ]; then
+                    # UEFI firmware should be pointed to the shim if available to
+                    # enable secure boot.
+                    for boot_uefi in \
+                            /boot/efi/EFI/$bootid/shimx64.efi \
+                            /boot/efi/EFI/BOOT/BOOTX64.EFI \
+                            /boot/efi/EFI/$bootid/grubx64.efi; do
+                        if [ -f $boot_uefi ]; then
+                            break
+                        fi
+                    done
+                    loader=$(echo ${boot_uefi##/boot/efi} | sed "s|/|\\\|g")
+                    efibootmgr --create --write-signature --label $bootid \
+                        --disk $efi_disk --part $efi_part_num --loader $loader
+                fi
+            else
+                $grubcmd $target $efi_dir \
+                    --bootloader-id=$bootid --recheck $no_nvram
+            fi
             [ -z "$grubpost" ] || $grubpost;' \
-            -- "${grub_name}" "${grub_target}" "$nvram" "$os_variant" </dev/null ||
+            -- "$grub_name" "$grub_target" "$nvram" "$os_variant" "$efi_disk" "$efi_part_num" </dev/null ||
             { error "failed to install grub!"; return 1; }
 
         chroot "$mp" sh -exc '
@@ -885,8 +953,11 @@ install_grub() {
                 centos|redhat|rhel)
                     case $bootver in
                         6) grubcmd="grub-install";;
-                        7) grubcmd="grub2-install"
+                        7|8) grubcmd="grub2-install"
                            grubpost="grub2-mkconfig -o /boot/grub2/grub.cfg";;
+                        *)
+                           echo "Unknown rhel_ver [$bootver]"
+                           exit 1
                     esac
                     ;;
                 *)
diff --git a/tests/unittests/test_curthooks.py b/tests/unittests/test_curthooks.py
index a1e13de..8533f5b 100644
--- a/tests/unittests/test_curthooks.py
+++ b/tests/unittests/test_curthooks.py
@@ -391,7 +391,8 @@ class TestInstallMissingPkgs(CiTestCase):
         arch = 'amd64'
         self.mock_arch.return_value = arch
         self.mock_machine.return_value = 'x86_64'
-        expected_pkgs = ['grub-efi-%s' % arch,
+        expected_pkgs = ['efibootmgr',
+                         'grub-efi-%s' % arch,
                          'grub-efi-%s-signed' % arch,
                          'shim-signed']
         self.mock_machine.return_value = 'x86_64'
@@ -408,7 +409,7 @@ class TestInstallMissingPkgs(CiTestCase):
         arch = 'i386'
         self.mock_arch.return_value = arch
         self.mock_machine.return_value = 'i386'
-        expected_pkgs = ['grub-efi-ia32']
+        expected_pkgs = ['efibootmgr', 'grub-efi-ia32']
         self.mock_machine.return_value = 'i686'
         self.mock_uefi.return_value = True
         target = "not-a-real-target"
@@ -422,7 +423,7 @@ class TestInstallMissingPkgs(CiTestCase):
         arch = 'arm64'
         self.mock_arch.return_value = arch
         self.mock_machine.return_value = 'aarch64'
-        expected_pkgs = ['grub-efi-%s' % arch]
+        expected_pkgs = ['efibootmgr', 'grub-efi-%s' % arch]
         self.mock_uefi.return_value = True
         target = "not-a-real-target"
         cfg = {}
@@ -430,6 +431,38 @@ class TestInstallMissingPkgs(CiTestCase):
         self.mock_install_packages.assert_called_with(
                 expected_pkgs, target=target, osfamily=self.distro_family)
 
+    @patch.object(events, 'ReportEventStack')
+    def test_install_packages_on_uefi_amd64_centos(self, mock_events):
+        arch = 'amd64'
+        self.mock_arch.return_value = arch
+        self.mock_machine.return_value = 'x86_64'
+        expected_pkgs = ['efibootmgr', 'grub2-efi-x64', 'shim-x64']
+        self.mock_uefi.return_value = True
+        self.mock_haspkg.return_value = True
+        target = "not-a-real-target"
+        cfg = {}
+        curthooks.install_missing_packages(
+            cfg, target=target, osfamily=distro.DISTROS.redhat)
+        self.mock_install_packages.assert_called_with(
+                expected_pkgs, target=target, osfamily=distro.DISTROS.redhat)
+
+    @patch.object(events, 'ReportEventStack')
+    def test_install_packages_on_uefi_amd64_centos_legacy(self, mock_events):
+        arch = 'amd64'
+        self.mock_arch.return_value = arch
+        self.mock_machine.return_value = 'x86_64'
+        self.mock_get_installed_packages.return_value = [
+            'grub2-efi-x64-modules']
+        expected_pkgs = ['efibootmgr']
+        self.mock_uefi.return_value = True
+        self.mock_haspkg.return_value = True
+        target = "not-a-real-target"
+        cfg = {}
+        curthooks.install_missing_packages(
+            cfg, target=target, osfamily=distro.DISTROS.redhat)
+        self.mock_install_packages.assert_called_with(
+                expected_pkgs, target=target, osfamily=distro.DISTROS.redhat)
+
 
 class TestSetupZipl(CiTestCase):
 
@@ -539,6 +572,58 @@ class TestSetupGrub(CiTestCase):
                 self.target, '/dev/vdb'],),
             self.mock_subp.call_args_list[0][0])
 
+    @patch('curtin.block.is_valid_device')
+    @patch('curtin.commands.curthooks.os.path.exists')
+    def test_uses_grub_install_on_storage_config_uefi(
+            self, m_exists, m_is_valid_device):
+        self.mock_is_uefi_bootable.return_value = True
+        cfg = {
+            'storage': {
+                'version': 1,
+                'config': [
+                    {
+                        'id': 'vdb',
+                        'type': 'disk',
+                        'name': 'vdb',
+                        'path': '/dev/vdb',
+                        'ptable': 'gpt',
+                    },
+                    {
+                        'id': 'vdb-part1',
+                        'type': 'partition',
+                        'device': 'vdb',
+                        'number': 1,
+                    },
+                    {
+                        'id': 'vdb-part1_format',
+                        'type': 'format',
+                        'volume': 'vdb-part1',
+                        'fstype': 'fat32',
+                    },
+                    {
+                        'id': 'vdb-part1_mount',
+                        'type': 'mount',
+                        'device': 'vdb-part1_format',
+                        'path': '/boot/efi',
+                    },
+                ]
+            },
+            'grub': {
+                'update_nvram': False,
+            },
+        }
+        self.subp_output.append(('', ''))
+        m_exists.return_value = True
+        m_is_valid_device.side_effect = (False, True)
+        curthooks.setup_grub(cfg, self.target, osfamily=distro.DISTROS.redhat)
+        self.assertEquals(
+            ([
+                'sh', '-c', 'exec "$0" "$@" 2>&1',
+                'install-grub', '--uefi',
+                '--os-family=%s' % distro.DISTROS.redhat, self.target,
+                '/dev/vdb1'],),
+            self.mock_subp.call_args_list[0][0])
+
     def test_grub_install_installs_to_none_if_install_devices_None(self):
         cfg = {
             'grub': {
diff --git a/tests/unittests/test_distro.py b/tests/unittests/test_distro.py
index b079162..dc1038c 100644
--- a/tests/unittests/test_distro.py
+++ b/tests/unittests/test_distro.py
@@ -254,6 +254,44 @@ class TestYumInstall(CiTestCase):
         distro.install_packages(pkglist, osfamily=osfamily, target=target)
         m_subp.assert_has_calls(expected_calls)
 
+    @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    @mock.patch('curtin.util.subp')
+    @mock.patch('curtin.distro.which')
+    def test_dnf_install(self, m_which, m_subp):
+        pkglist = ['foobar', 'wark']
+        target = 'mytarget'
+        mode = 'install'
+        m_which.return_value = '/usr/bin/dnf'
+        expected_calls = [
+            mock.call(['dnf', '--assumeyes', '--quiet', 'install',
+                       '--downloadonly', '--setopt=keepcache=1'] + pkglist,
+                      env=None, retries=[1] * 10,
+                      target=paths.target_path(target)),
+            mock.call(['dnf', '--assumeyes', '--quiet', 'install',
+                       '--cacheonly'] + pkglist, env=None,
+                      target=paths.target_path(target))
+        ]
+
+        # call yum_install directly
+        self.assertFalse(m_subp.called)
+        distro.yum_install(mode, pkglist, target=target)
+        m_subp.assert_has_calls(expected_calls)
+
+        # call yum_install through run_yum_command; expect the same calls
+        # so clear m_subp's call stack.
+        m_subp.reset_mock()
+        self.assertFalse(m_subp.called)
+        distro.run_yum_command('install', pkglist, target=target)
+        m_subp.assert_has_calls(expected_calls)
+
+        # call yum_install through install_packages; expect the same calls
+        # so clear m_subp's call stack.
+        m_subp.reset_mock()
+        self.assertFalse(m_subp.called)
+        osfamily = distro.DISTROS.redhat
+        distro.install_packages(pkglist, osfamily=osfamily, target=target)
+        m_subp.assert_has_calls(expected_calls)
+
 
 class TestSystemUpgrade(CiTestCase):
 
@@ -289,6 +327,39 @@ class TestSystemUpgrade(CiTestCase):
         m_subp.assert_has_calls(expected_calls)
 
     @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
+    @mock.patch('curtin.util.subp')
+    @mock.patch('curtin.distro.which')
+    def test_system_upgrade_redhat_dnf(self, m_which, m_subp):
+        """system_upgrade osfamily=redhat calls run_yum_command mode=upgrade"""
+        osfamily = distro.DISTROS.redhat
+        target = 'mytarget'
+        mode = 'upgrade'
+        m_which.return_value = '/usr/bin/dnf'
+        pkglist = []
+        expected_calls = [
+            mock.call(['dnf', '--assumeyes', '--quiet', mode,
+                       '--downloadonly', '--setopt=keepcache=1'] + pkglist,
+                      env=None, retries=[1] * 10,
+                      target=paths.target_path(target)),
+            mock.call(['dnf', '--assumeyes', '--quiet', mode,
+                       '--cacheonly'] + pkglist, env=None,
+                      target=paths.target_path(target))
+        ]
+        # call system_upgrade via osfamily; note that we expect the same calls
+        # call system_upgrade via osfamily; note that we expect the same calls
+
+        # call yum_install through run_yum_command
+        distro.run_yum_command(mode, pkglist, target=target)
+        m_subp.assert_has_calls(expected_calls)
+
+        # call system_upgrade via osfamily; note that we expect the same calls
+        # but to prevent a false positive we clear m_subp's call stack.
+        m_subp.reset_mock()
+        self.assertFalse(m_subp.called)
+        distro.system_upgrade(target=target, osfamily=osfamily)
+        m_subp.assert_has_calls(expected_calls)
+
+    @mock.patch.object(util.ChrootableTarget, "__enter__", new=lambda a: a)
     @mock.patch('curtin.distro.os.environ')
     @mock.patch('curtin.distro.apt_update')
     @mock.patch('curtin.distro.which')