Added dry-run support. Pre-refactoring cleanup. Moved config file to /etc.

8 files changed, 207 insertions, 93 deletions

diff --git a/api.py b/api.py
index e55cb4e..36a736d 100644
--- a/api.py
+++ b/api.py
@@ -35,10 +35,6 @@ class BootAPI:
        if not os.path.exists(self.config.config_file):
            self.config.serialize()
 
-
-    def show_error(self):
-       print self.last_error
-
     """
     Forget about current list of groups, distros, and systems
     """
@@ -125,8 +121,8 @@ class Collection:
     """
     Return datastructure representation (to feed to serializer)
     """
-    def to_ds(self):
-        return [x.to_ds() for x in self.listing.values()]
+    def to_datastruct(self):
+        return [x.to_datastruct() for x in self.listing.values()]
     
      
     """
@@ -174,11 +170,11 @@ class Distros(Collection):
     """
     def remove(self,name):
         # first see if any Groups use this distro
-        for k,v in self.api.config.groups.listing.items():
+        for k,v in self.api.get_groups().listing.items():
             if v.distro == name:
                self.api.last_error = m("orphan_group")
                return False
-        if name in self.listing:
+        if self.find(name):
             del self.listing[name]
             return True
         self.api.last_error = m("delete_nothing")
@@ -204,11 +200,11 @@ class Groups(Collection):
     Remove element named 'name' from the collection
     """
     def remove(self,name):
-        for k,v in self.api.config.systems.listing.items():
+        for k,v in self.api.get_groups().listing.items():
            if v.group == name:
                self.api.last_error = m("orphan_system")
                return False
-        if name in self.listing:
+        if self.find(name):
             del self.listing[name]
             return True
         self.api.last_error = m("delete_nothing")
@@ -233,7 +229,7 @@ class Systems(Collection):
     Remove element named 'name' from the collection
     """
     def remove(self,name):
-        if name in self.listing:
+        if self.find(name):
             del self.listing[name]
             return True
         self.api.last_error = m("delete_nothing")
@@ -247,11 +243,15 @@ An Item is a serializable thing that can appear in a Collection
 """
 class Item:
   
+    """
+    All objects have names, and with the exception of System
+    they aren't picky about it.
+    """
     def set_name(self,name):
         self.name = name
         return True
 
-    def to_ds(self):
+    def to_datastruct(self):
         raise "not implemented"
    
     def is_valid(self):
@@ -290,7 +290,7 @@ class Distro(Item):
             if x is None: return False
         return True
 
-    def to_ds(self):
+    def to_datastruct(self):
         return { 
            'name': self.name, 
            'kernel': self.kernel, 
@@ -334,7 +334,7 @@ class Group(Item):
                 return False
         return True
 
-    def to_ds(self):
+    def to_datastruct(self):
         return { 
             'name' : self.name,
             'distro' : self.distro,
@@ -383,7 +383,7 @@ class System(Item):
             return False
         return True
 
-    def to_ds(self):
+    def to_datastruct(self):
         return {
            'name'   : self.name,
            'group'  : self.group 
diff --git a/bootconf b/bootconf
index ef79c3a..b370cdd 100755
--- a/bootconf
+++ b/bootconf
@@ -63,7 +63,7 @@ class BootCLI:
     def run(self):
         rc = self.curry_args(self.args[1:], self.toplevel_commands)
         if not rc:
-            self.api.show_error()
+            print self.api.last_error
         return rc
 
 
@@ -103,33 +103,30 @@ class BootCLI:
     Delete a system:  'bootconf system remove --name=foo'
     """
     def system_remove(self,args):
-        sys = self.api.new_system()
         commands = {
-           '--name'       : lambda(a):  sys.set_name(a)
+           '--name'       : lambda(a):  self.api.get_systems().remove(a) 
         }
-        on_ok = lambda: self.api.get_systems().remove(sys)
+        on_ok = lambda: True
         return self.apply_args(args,commands,on_ok,True)
 
     """
     Delete a group:   'bootconf group remove --name=foo'
     """
     def group_remove(self,args):
-        group = self.api.new_group()
         commands = {
-           '--name'       : lambda(a):  group.set_name(a)
+           '--name'       : lambda(a):  self.api.get_groups().remove(a)
         }
-        on_ok = lambda: self.api.get_groups.remove(group)
+        on_ok = lambda: True
         return self.apply_args(args,commands,on_ok,True)
 
     """
     Delete a distro:  'bootconf distro remove --name='foo'
     """
     def distro_remove(self,args):
-        distro = self.api.new_distro()
         commands = {
-           '--name'     : lambda(a):  distro.set_name(a)
+           '--name'     : lambda(a):  self.api.get_distros().remove(a)
         }
-        on_ok = lambda: self.api.get_distros().remove(distro)
+        on_ok = lambda: True
         return self.apply_args(args,commands,on_ok,True)
 
     """
@@ -219,7 +216,7 @@ class BootCLI:
     """
     def sync(self, args):
         status = None 
-        if args is not None and len(args) > 0 and args[0] == "--dryrun":
+        if args is not None and "--dryrun" in args:
             status = self.api.sync(dry_run=True)
         else:
             status = self.api.sync(dry_run=False)
diff --git a/check.py b/check.py
index b3c8ae9..311c23f 100644
--- a/check.py
+++ b/check.py
@@ -32,22 +32,38 @@ class BootCheck:
        self.check_dhcpd_conf(status)
        return status
 
+   """
+   Check if dhcpd is installed
+   """
    def check_dhcpd_bin(self,status):
        if not os.path.exists(self.config.dhcpd_bin):
           status.append(m("no_dhcpd"))
 
+   """
+   Check if pxelinux (part of syslinux) is installed
+   """
    def check_pxelinux_bin(self,status):
        if not os.path.exists(self.config.pxelinux):
           status.append(m("no_pxelinux"))
 
+   """
+   Check if tftpd is installed
+   """
    def check_tftpd_bin(self,status):
        if not os.path.exists(self.config.tftpd_bin):
           status.append(m("no_tftpd")) 
 
+   """
+   Check if bootconf.conf's tftpboot directory exists
+   """
    def check_tftpd_dir(self,status):
        if not os.path.exists(self.config.tftpboot):
           status.append(m("no_dir") % self.config.tftpboot)
    
+   """
+   Check that bootconf tftpd boot directory matches with tftpd directory
+   Check that tftpd is enabled to autostart
+   """
    def check_tftpd_conf(self,status):
        if os.path.exists(self.config.tftpd_conf):
           f = open(self.config.tftpd_conf)
@@ -66,6 +82,10 @@ class BootCheck:
        else:
           status.append(m("no_exist") % self.tftpd_conf)
    
+   """
+   Check that dhcpd *appears* to be configured for pxe booting.
+   We can't assure file correctness
+   """
    def check_dhcpd_conf(self,status):
        if os.path.exists(self.config.dhcpd_conf):
            match_next = False
@@ -86,3 +106,5 @@ class BootCheck:
           status.append(m("no_dir2") % (self.config.kernel_root, 'kernel_root'))
        if not os.path.exists(self.config.kickstart_root):
           status.append(m("no_dir2") % (self.config.kickstart_root, 'kickstart_root'))
+
+
diff --git a/config.py b/config.py
index cde7c72..7a3adcc 100644
--- a/config.py
+++ b/config.py
@@ -5,6 +5,7 @@
 
 import api 
 import util
+from msg import *
 
 import os
 import yaml
@@ -15,14 +16,14 @@ class BootConfig:
     """
     Constructor.  This class maintains both the logical
     configuration for Boot and the file representation thereof.
+    Creating the config object only loads the default values,
+    users of this class need to call deserialize() to load config
+    file values.
     """
     def __init__(self,api):
         self.api = api
-        # non-configurable......
-        self.config_file    = "bootconf.conf" # LATER: "/etc/bootconf.conf"
-        # reasonable defaults...(config settings)
+        self.config_file    = "/etc/bootconf.conf"
         self.set_defaults()
-        # initially empty containers...
         self.clear()
 
     """
@@ -38,9 +39,9 @@ class BootConfig:
     """
     def set_defaults(self):
         self.servername     = "your_server_ip"
-        self.kickstart_root = "/var/www/Boot"
+        self.kickstart_root = "/var/www/bootconf"
         self.kickstart_url  = "http://%s/kickstart" % (self.servername)
-        self.kernel_root    = "/var/www/Boot"
+        self.kernel_root    = "/var/www/bootconf"
         self.tftpboot       = "/tftpboot"
         self.dhcpd_conf     = "/etc/dhcpd.conf"
         self.tftpd_conf     = "/etc/xinetd.d/tftp"
@@ -109,9 +110,9 @@ class BootConfig:
     def to_hash(self):
         world = {} 
         world['config']  = self.config_to_hash()
-        world['distros'] = self.get_distros().to_ds()
-        world['groups']  = self.get_groups().to_ds()
-        world['systems'] = self.get_systems().to_ds()
+        world['distros'] = self.get_distros().to_datastruct()
+        world['groups']  = self.get_groups().to_datastruct()
+        world['systems'] = self.get_systems().to_datastruct()
         return world  
 
 
@@ -136,10 +137,11 @@ class BootConfig:
         try:
             conf = open(self.config_file,"w+")
         except IOError:
-            print "Can't create %s, are you root?" % (self.config_file)
-            sys.exit(1)
+            self.api.last_error = m("cant_create: %s" % self.config_file)
+            return False
         data = self.to_hash()
         conf.write(yaml.dump(data))
+        return True
 
     """
     Load everything from the config file.
@@ -147,9 +149,14 @@ class BootConfig:
     could use YAML later if we wanted.
     """
     def deserialize(self):
-        conf = yaml.loadFile(self.config_file)
-        raw_data = conf.next()
-        if raw_data is not None:
-            self.from_hash(raw_data)
+        try:
+            conf = yaml.loadFile(self.config_file)
+            raw_data = conf.next()
+            if raw_data is not None:
+                self.from_hash(raw_data)
+            return True
+        except:
+            self.api.last_error = m("parse_error")
+            return False
 
  
diff --git a/msg.py b/msg.py
index 1301a6b..b2a3006 100644
--- a/msg.py
+++ b/msg.py
@@ -5,6 +5,8 @@
 # Michael DeHaan <mdehaan@redhat.com>
 
 msg_table = {
+  "parse_error"     : "could not parse /etc/bootconf.conf",
+  "no_create"       : "cannot create: %s",
   "no_args"         : "this command requires arguments.",
   "missing_options" : "cannot add, all parameters have not been set",
   "unknown_cmd"     : "bootconf doesn't understand '%s'",
@@ -36,7 +38,11 @@ msg_table = {
   "no_kernel"       : "the kernel needs to be a directory containing a kernel, or a full path.  Kernels must be named just 'vmlinuz' or in the form 'vmlinuz-AA.BB.CC-something'",
   "no_initrd"       : "the initrd needs to be a directory containing an initrd, or a full path.  Initrds must be named just 'initrd.img' or in the form 'initrd-AA.BB.CC-something.img",
   "check_ok"        : """
-No setup problems found, though we can't tell if /etc/dhcpd.conf is totally correct, so that's left as an exercise for the reader.  Network boot infrastructure configuration via other 'bootconf' commands should be good to go, though you should correct any errors found as a result of running 'bootconf sync' as well.  Next look over /etc/bootconf.conf and edit any global settings that are 'wrong'.  Ensure that dhcpd and tftpd are started after you are done, but they do not need to be started now.  Note: making changes to /etc/dhcpd.conf always requires a restart of dhcpd. Good luck!
+No setup problems found.  
+
+Manual editing of /etc/dhcpd.conf and /etc/bootconf.conf is suggested to tailor them to your specific configuration.  Kickstarts will not work without editing the URL in /etc/bootconf.conf, for instance. Your dhcpd.conf has some PXE related information in it, but it's imposible to tell automatically that it's totally correct in a general sense.  We'll leave this up to you. 
+
+Good luck.
 """,
   "help"           : """
 bootconf is a simple network boot configuration tool.
@@ -102,8 +108,12 @@ That's it!
    """
 }
 
+"""
+Return the lookup of a string key.
+"""
 def m(key):
    if key in msg_table:
+       # localization could use different tables or just gettext.
        return msg_table[key]
    else:
        return "?%s?" % key
diff --git a/sync.py b/sync.py
index dfa2691..63be50c 100644
--- a/sync.py
+++ b/sync.py
@@ -18,16 +18,15 @@ class BootSync:
 
     def __init__(self,api):
         self.api = api
+        self.verbose = True
 
     """
     Syncs the current bootconf configuration.  
-    Automatically runs the 'check_install'
+    Automatically runs the 'check' function first to eliminate likely failures.
     FUTURE: make dryrun work.
     """
-    def sync(self,dry_run=False):
-        if dry_run:
-            print "WARNING: dryrun hasn't been implemented yet.  Try not using dryrun at your own risk."
-            sys.exit(1)
+    def sync(self,dry_run=False,verbose=True):
+        self.dry_run = dry_run
         results = self.api.check()
         if results != []:
             self.api.last_error = m("run_check")
@@ -43,22 +42,38 @@ class BootSync:
             return False
         return True
 
+    """
+    Copy syslinux to the configured tftpboot directory
+    """
     def copy_pxelinux(self):
-        shutil.copy(self.api.config.pxelinux, os.path.join(self.api.config.tftpboot, "pxelinux.0"))
+        self.copy(self.api.config.pxelinux, os.path.join(self.api.config.tftpboot, "pxelinux.0"))
 
+
+    """
+    Delete any previously built pxelinux.cfg tree for individual systems.
+    This is better than trying to just add additional entries
+    as both MAC and IP settings could have been added and the MACs will
+    take precedence.  So we can't really trust human edits won't
+    conflict.
+    """
     def clean_pxelinux_tree(self):
-        shutil.rmtree(os.path.join(self.api.config.tftpboot, "pxelinux.cfg"), True) 
+        self.rmtree(os.path.join(self.api.config.tftpboot, "pxelinux.cfg"), True) 
 
+    """
+    A distro is a kernel and an initrd.  Copy all of them and error
+    out if any files are missing.  The conf file was correct if built
+    via the CLI or API, though it's possible files have been moved
+    since or perhaps they reference NFS directories that are no longer
+    mounted.
+    """
     def copy_distros(self):
         # copy is a 4-letter word but tftpboot runs chroot, thus it's required.
         images = os.path.join(self.api.config.tftpboot, "images")
-        shutil.rmtree(os.path.join(self.api.config.tftpboot, "images"), True)
-        os.mkdir(images)
+        self.rmtree(os.path.join(self.api.config.tftpboot, "images"), True)
+        self.mkdir(images)
         for d in self.api.get_distros().contents():
             kernel = self.api.utils.find_kernel(d.kernel) # full path
             initrd = self.api.utils.find_initrd(d.initrd) # full path
-            print "DEBUG: kernel = %s" % kernel
-            print "DEBUG: initrd = %s" % initrd
             if kernel is None:
                self.api.last_error = "Kernel for distro (%s) cannot be found and needs to be fixed: %s" % (d.name, d.kernel)
                raise "error"
@@ -67,9 +82,17 @@ class BootSync:
                raise "error"
             b_kernel = os.path.basename(kernel)
             b_initrd = os.path.basename(initrd)
-            shutil.copyfile(kernel, os.path.join(images, b_kernel))
-            shutil.copyfile(initrd, os.path.join(images, b_initrd))
+            self.copyfile(kernel, os.path.join(images, b_kernel))
+            self.copyfile(initrd, os.path.join(images, b_initrd))
 
+    """
+    Similar to what we do for distros, ensure all the kickstarts
+    in conf file are valid.  Since kickstarts are referenced by URL
+    (http or ftp), we do not have to copy them.  They are already
+    expected to be in the right place.  We can't check to see that the
+    URLs are right (or we don't, we could...) but we do check to see
+    that the files are at least still there.
+    """
     def validate_kickstarts(self):
         # ensure all referenced kickstarts exist
         # these are served by either NFS, Apache, or some ftpd, so we don't need to copy them
@@ -80,13 +103,18 @@ class BootSync:
               self.api.last_error = "Kickstart for group (%s) cannot be found and needs to be fixed: %s" % (g.name, g.kickstart)
               raise "error"
             
+    """
+    Now that kernels and initrds are copied and kickstarts are all valid,
+    build the pxelinux.cfg tree, which contains a directory for each 
+    configured IP or MAC address.
+    """
     def build_pxelinux_tree(self):
         # create pxelinux.cfg under tftpboot
         # and file for each MAC or IP (hex encoded 01-XX-XX-XX-XX-XX-XX)
         systems = self.api.get_systems()
         groups  = self.api.get_groups()
         distros = self.api.get_distros()
-        os.mkdir(os.path.join(self.api.config.tftpboot,"pxelinux.cfg"))
+        self.mkdir(os.path.join(self.api.config.tftpboot,"pxelinux.cfg"))
         for system in self.api.get_systems().contents():
             group = groups.find(system.group)
             if group is None:
@@ -100,6 +128,13 @@ class BootSync:
             filename = os.path.join(self.api.config.tftpboot, "pxelinux.cfg", filename)
             self.write_pxelinux_file(filename,system,group,distro)
 
+    """
+    The configuration file for each system pxelinux uses is either
+    a form of the MAC address of the hex version of the IP.  Not sure
+    about ipv6 (or if that works).  The system name in the config file
+    is either a system name, an IP, or the MAC, so figure it out, resolve
+    the host if needed, and return the pxelinux directory name.
+    """
     def get_pxelinux_filename(self,name_input):
         name = self.api.utils.find_system_identifier(name_input)
         if self.api.utils.is_ip(name):
@@ -110,24 +145,79 @@ class BootSync:
             self.api.last_error = "system name (%s) couldn't resolve and is not an IP or a MAC address." % name
             raise "error"
       
+    """
+    Write a configuration file for the pxelinux boot loader.
+    More system-specific configuration may come in later, if so
+    that would appear inside the system object in api.py
+    """
     def write_pxelinux_file(self,filename,system,group,distro):
         kernel_path = os.path.join("/images",os.path.basename(distro.kernel))
         initrd_path = os.path.join("/images",os.path.basename(distro.initrd))
         kickstart_path = self.api.config.kickstart_url + "/" + os.path.basename(group.kickstart)
-        file = open(filename,"w+")
-        file.write("default linux\n")
-        file.write("prompt 0\n")
-        file.write("timeout 1\n")
-        file.write("label linux\n")
-        file.write("   kernel %s\n" % kernel_path)
+        self.sync_log("writing: %s" % filename)
+        self.sync_log("---------------------------------")
+        if self.dry_run:
+            file = None
+        else:
+            file = open(filename,"w+")
+        self.tee(file,"default linux\n")
+        self.tee(file,"prompt 0\n")
+        self.tee(file,"timeout 1\n")
+        self.tee(file,"label linux\n")
+        self.tee(file,"   kernel %s\n" % kernel_path)
         # FIXME: leave off kickstart if no kickstart...
         # FIXME: allow specifying of other (system specific?) 
         #        parameters in bootconf.conf ???
-        file.write("   append devfs=nomount ramdisk_size=16438 lang= vga=788 ksdevice=eth0 initrd=%s ks=%s console=ttyS0,38400n8\n" % (initrd_path, kickstart_path))
-        file.close()
+        self.tee(file,"   append devfs=nomount ramdisk_size=16438 lang= vga=788 ksdevice=eth0 initrd=%s ks=%s console=ttyS0,38400n8\n" % (initrd_path, kickstart_path))
+        if not self.dry_run: 
+            file.close()
+        self.sync_log("--------------------------------")
+
+    """
+    For dry_run support, and logging...
+    """
+    def tee(self,file,text):
+        self.sync_log(text)
+        if not self.dry_run:
+            file.write(text)
+  
+    def copyfile(self,src,dst):
+       self.sync_log("copy %s to %s" % (src,dst))
+       if self.dry_run:
+           return True
+       return shutil.copyfile(src,dst)
 
+    def copy(self,src,dst):
+        self.sync_log("copy %s to %s" % (src,dst))
+        if self.dry_run:
+            return True
+        return shutil.copy(src,dst)
+
+    def rmtree(self,path,ignore):
+       self.sync_log("removing dir %s" % (path))
+       if self.dry_run:
+           return True
+       return shutil.rmtree(path,ignore)
+
+    def mkdir(self,path,mode=0777):
+       self.sync_log("creating dir %s" % (path))
+       if self.dry_run:
+           return True
+       return os.mkdir(path,mode)
+
+    """
+    Used to differentiate dry_run output from the real thing
+    automagically
+    """
+    def sync_log(self,message):
+        if self.verbose:
+            if self.dry_run:
+                print "dry_run | %s" % message
+            else:
+                print message
+            
+ 
     # FUTURE: would be nice to check if dhcpd and tftpd are running...
     # and whether kickstart url works (nfs, http, ftp)
     # at least those that work with open-uri
-    # possibly file permissions...
 
diff --git a/test.py b/test.py
index ded4cdb..70f192c 100644
--- a/test.py
+++ b/test.py
@@ -22,7 +22,7 @@ class BootTest(unittest.TestCase):
     def setUp(self):
         try:
            # it will interfere with results...
-           os.file.remove("bootconf.conf")
+           os.file.remove("/etc/bootconf.conf")
         except:
            pass
         self.api = api.BootAPI()
@@ -38,6 +38,7 @@ class BootTest(unittest.TestCase):
         self.api = None
 
     def make_basic_config(self):
+        self.assertTrue(os.getuid() == 0)
         distro = self.api.new_distro()
         self.assertTrue(distro.set_name("testdistro0"))
         self.assertTrue(distro.set_kernel(FAKE_KERNEL))
diff --git a/util.py b/util.py
index c990461..f5450e4 100644
--- a/util.py
+++ b/util.py
@@ -9,26 +9,8 @@ import re
 import socket
 import glob
 
-# FIXME: use python logging?
-
-def debug(msg):
-   print "debug: %s" % msg
-
-def info(msg):
-   print "%s" % msg
-
-def error(msg):
-   print "error: %s" % msg
-
-def warning(msg):
-   print "warning: %s" % msg
-
 class BootUtil:
 
-
-   # TO DO:  functions for manipulation of all things important.
-   #         yes, that's a lot...
-
    def __init__(self,api,config):
        self.api = api
        self.config = config
@@ -114,7 +96,11 @@ class BootUtil:
            if os.path.exists(last_chance):
                return last_chance
            return None
-  
+ 
+   """
+   Given a directory or a filename, find if the path can be made
+   to resolve into a kernel, and return that full path if possible.
+   """ 
    def find_kernel(self,path):
        if os.path.isfile(path):
            filename = os.path.basename(path)
@@ -126,8 +112,12 @@ class BootUtil:
            return self.find_highest_files(path,"vmlinuz",self.re_kernel)
        return None
 
+   """
+   Given a directory or a filename, see if the path can be made 
+   to resolve into an intird, return that full path if possible.
+   """
    def find_initrd(self,path):
-       # FUTURE: add another function to see if kernel and initrd have matched numbers (warning?)
+       # FUTURE: add another function to see if kernel and initrd have matched numbers (and throw a warning?)
        if os.path.isfile(path):
            filename = os.path.basename(path)
            if self.re_initrd.match(filename):
@@ -138,6 +128,10 @@ class BootUtil:
            return self.find_highest_files(path,"initrd.img",self.re_initrd)  
        return None
  
+   """
+   Similar to find_kernel and find_initrd, see if a path or filename
+   references a kickstart...
+   """
    def find_kickstart(self,path):
        # Kickstarts must be explicit.
        # FUTURE:  Look in configured kickstart path and don't require full paths to kickstart
@@ -149,10 +143,3 @@ class BootUtil:
            return joined
        return None
 
-    
-
-   def sync(self,dryrun=False):
-       # FIXME: IMPLEMENT
-       return False
-
-