From 3d2c085cf0065f3ff20f77f5ffbe41150b178708 Mon Sep 17 00:00:00 2001 From: "Brian C. Lane" Date: Wed, 23 Jan 2019 13:50:48 -0800 Subject: [PATCH] Clarify the ks repo only error message This also moves the run_creator kickstart checks into check_kickstart so that tests may be added. This will close #164 (cherry picked from commit 3676cb65bb6f1bcf8af3a35da774b46d1bdcc64c) --- src/pylorax/creator.py | 68 +++++++++++++-------- tests/pylorax/test_creator.py | 108 +++++++++++++++++++++++++++++++++- 2 files changed, 150 insertions(+), 26 deletions(-) diff --git a/src/pylorax/creator.py b/src/pylorax/creator.py index 72b1a09e..dc05b1d8 100644 --- a/src/pylorax/creator.py +++ b/src/pylorax/creator.py @@ -578,6 +578,47 @@ def make_live_images(opts, work_dir, disk_img): return work_dir +def check_kickstart(ks, opts): + """Check the parsed kickstart object for errors + + :param ks: Parsed Kickstart object + :type ks: pykickstart.parser.KickstartParser + :param opts: Commandline options to control the process + :type opts: Either a DataHolder or ArgumentParser + :returns: List of error strings or empty list + :rtype: list + """ + errors = [] + if opts.no_virt and ks.handler.method.method not in ("url", "nfs") \ + and not ks.handler.ostreesetup.seen: + errors.append("Only url, nfs and ostreesetup install methods are currently supported." + "Please fix your kickstart file." ) + + if ks.handler.repo.seen and ks.handler.method.method != "url": + errors.append("repo can only be used with the url install method. Add url to your " + "kickstart file.") + + if ks.handler.method.method in ("url", "nfs") and not ks.handler.network.seen: + errors.append("The kickstart must activate networking if " + "the url or nfs install method is used.") + + if ks.handler.displaymode.displayMode is not None: + errors.append("The kickstart must not set a display mode (text, cmdline, " + "graphical), this will interfere with livemedia-creator.") + + if opts.make_fsimage or (opts.make_pxe_live and opts.no_virt): + # Make sure the kickstart isn't using autopart and only has a / mountpoint + part_ok = not any(p for p in ks.handler.partition.partitions + if p.mountpoint not in ["/", "swap"]) + if not part_ok or ks.handler.autopart.seen: + errors.append("Filesystem images must use a single / part, not autopart or " + "multiple partitions. swap is allowed but not used.") + + if not opts.no_virt and ks.handler.reboot.action != KS_SHUTDOWN: + errors.append("The kickstart must include shutdown when using virt installation.") + + return errors + def run_creator(opts, cancel_func=None): """Run the image creator process @@ -612,31 +653,8 @@ def run_creator(opts, cancel_func=None): if not opts.ks: raise RuntimeError("Image creation requires a kickstart file") - errors = [] - if opts.no_virt and ks.handler.method.method not in ("url", "nfs") \ - and not ks.handler.ostreesetup.seen: - errors.append("Only url, nfs and ostreesetup install methods are currently supported." - "Please fix your kickstart file." ) - - if ks.handler.method.method in ("url", "nfs") and not ks.handler.network.seen: - errors.append("The kickstart must activate networking if " - "the url or nfs install method is used.") - - if ks.handler.displaymode.displayMode is not None: - errors.append("The kickstart must not set a display mode (text, cmdline, " - "graphical), this will interfere with livemedia-creator.") - - if opts.make_fsimage or (opts.make_pxe_live and opts.no_virt): - # Make sure the kickstart isn't using autopart and only has a / mountpoint - part_ok = not any(p for p in ks.handler.partition.partitions - if p.mountpoint not in ["/", "swap"]) - if not part_ok or ks.handler.autopart.seen: - errors.append("Filesystem images must use a single / part, not autopart or " - "multiple partitions. swap is allowed but not used.") - - if not opts.no_virt and ks.handler.reboot.action != KS_SHUTDOWN: - errors.append("The kickstart must include shutdown when using virt installation.") - + # Check the kickstart for problems + errors = check_kickstart(ks, opts) if errors: list(log.error(e) for e in errors) raise RuntimeError("\n".join(errors)) diff --git a/tests/pylorax/test_creator.py b/tests/pylorax/test_creator.py index c3245f63..14db20d2 100644 --- a/tests/pylorax/test_creator.py +++ b/tests/pylorax/test_creator.py @@ -19,11 +19,15 @@ import tempfile import unittest import xml.etree.ElementTree as ET +# For kickstart check tests +from pykickstart.parser import KickstartParser +from pykickstart.version import makeVersion + from ..lib import get_file_magic from pylorax import find_templates from pylorax.base import DataHolder from pylorax.creator import FakeDNF, create_pxe_config, make_appliance, make_squashfs, squashfs_args -from pylorax.creator import get_arch, find_ostree_root +from pylorax.creator import get_arch, find_ostree_root, check_kickstart from pylorax.executils import runcmd from pylorax.imgutils import mksparse from pylorax.sysutils import joinpaths @@ -138,6 +142,108 @@ class CreatorTest(unittest.TestCase): os.makedirs(joinpaths(work_dir, ostree_path)) self.assertEqual(find_ostree_root(work_dir), ostree_path) + def good_ks_novirt_test(self): + """Test a good kickstart with novirt""" + opts = DataHolder(no_virt=True, make_fsimage=False, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "shutdown\n") + self.assertEqual(check_kickstart(ks, opts), []) + + def good_ks_virt_test(self): + """Test a good kickstart with virt""" + opts = DataHolder(no_virt=False, make_fsimage=False, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "shutdown\n") + self.assertEqual(check_kickstart(ks, opts), []) + + def nomethod_novirt_test(self): + """Test a kickstart with repo and no url""" + opts = DataHolder(no_virt=True, make_fsimage=False, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "shutdown\n") + errors = check_kickstart(ks, opts) + self.assertTrue("Only url, nfs and ostreesetup" in errors[0]) + self.assertTrue("repo can only be used with the url" in errors[1]) + + def no_network_test(self): + """Test a kickstart with missing network command""" + opts = DataHolder(no_virt=True, make_fsimage=False, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "shutdown\n") + errors = check_kickstart(ks, opts) + self.assertTrue("The kickstart must activate networking" in errors[0]) + + def displaymode_test(self): + """Test a kickstart with displaymode set""" + opts = DataHolder(no_virt=True, make_fsimage=False, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "shutdown\n" + "graphical\n") + errors = check_kickstart(ks, opts) + self.assertTrue("must not set a display mode" in errors[0]) + + def autopart_test(self): + """Test a kickstart with autopart""" + opts = DataHolder(no_virt=True, make_fsimage=True, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "autopart\n" + "shutdown\n") + errors = check_kickstart(ks, opts) + self.assertTrue("Filesystem images must use a single" in errors[0]) + + def boot_part_test(self): + """Test a kickstart with a boot partition""" + opts = DataHolder(no_virt=True, make_fsimage=True, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "part /boot --size=1024\n" + "shutdown\n") + errors = check_kickstart(ks, opts) + self.assertTrue("Filesystem images must use a single" in errors[0]) + + def shutdown_virt_test(self): + """Test a kickstart with reboot instead of shutdown""" + opts = DataHolder(no_virt=False, make_fsimage=True, make_pxe_live=False) + ks_version = makeVersion() + ks = KickstartParser(ks_version, errorsAreFatal=False, missingIncludeIsFatal=False) + ks.readKickstartFromString("url --url=http://dl.fedoraproject.com\n" + "network --bootproto=dhcp --activate\n" + "repo --name=other --baseurl=http://dl.fedoraproject.com\n" + "part / --size=4096\n" + "reboot\n") + errors = check_kickstart(ks, opts) + self.assertTrue("must include shutdown when using virt" in errors[0]) + @unittest.skipUnless(os.geteuid() == 0 and not os.path.exists("/.in-container"), "requires root privileges, and no containers") def boot_over_root_test(self): """Test the mount_boot_part_over_root ostree function"""