Bug 330921 - Review Request: ltsp-config-dhcp - DHCP configuration files for LTSP5
Review Request: ltsp-config-dhcp - DHCP configuration files for LTSP5
Status: CLOSED DUPLICATE of bug 331651
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Warren Togami
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-13 16:00 EDT by Eric Harrison
Modified: 2007-11-30 17:12 EST (History)
3 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-10-18 04:40:05 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
wtogami: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Comment 1 Patrice Dumas 2007-10-13 16:52:04 EDT
I don't see the interest of of this package as a package.
Indeed the init file duplicates the dhcp init file, and the
conf file is an example so don't need to be shipped in 
the filesystem (and would better be in %doc than in /etc).

It seems to me that it would be better to add that file in 
the dhcp %doc instead than doing a specific package.
The only drawback I see would be that a release must either
wait for a dhcp release or force a dhcp release.
Comment 2 Warren Togami 2007-10-13 17:14:42 EDT
Historically K12LTSP shipped with a stock config file like this.  We will do so
only to allow a quick migration into Fedora.  Later we plan on using management
interfaces to configure it instead, so we don't rely upon inflexible stock
configs like this.

Comment 3 Patrice Dumas 2007-10-13 17:26:50 EDT
If it is only temporary, then adding the config file in %doc
in dhcp seems even more relevant.
Comment 4 Warren Togami 2007-10-13 17:49:07 EDT
http://togami.com/~warren/fedora/ltsp-config-dhcp-0-2.fc8.src.rpm
Some minor cleanups and thinko corrections, listed below.

diff -u ltsp-config-dhcp-0-1.fc8.orig/ltsp-config-dhcp.spec
ltsp-config-dhcp-0-1.fc8/ltsp-config-dhcp.spec
--- ltsp-config-dhcp-0-1.fc8.orig/ltsp-config-dhcp.spec 2007-10-13
14:33:25.000000000 -0400
+++ ltsp-config-dhcp-0-1.fc8/ltsp-config-dhcp.spec      2007-10-13
17:42:04.000000000 -0400
@@ -1,19 +1,22 @@
-Name: ltsp-config-dhcp         
-Version: 0       
-Release:        1%{?dist}
-Summary: Default configuration for LTSP5
-
-Group:  System       
-License: GPL
-Source0: ltsp-dhcpd.conf
-Source1: ltsp-dhcpd.init
-BuildArch: noarch
+Name:           ltsp-config-dhcp         
+Version:        0       
+Release:        2%{?dist}
+Summary:        Default configuration for LTSP5 dhcpd
+
+Group:          System Environment/Daemons     
+License:        GPL+ or ISC
+# LTSP is all GPL
+Source0:        ltsp-dhcpd.conf
+# Copied from dhcp-3.0.6 and slightly modified, ISC licensed
+Source1:        ltsp-dhcpd.init
+BuildArch:      noarch
 BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
 
 Requires:    dhcp >= 3.0 
 
 %description
-Default configurations for LTSP5
+Default configuration for LTSP5 dhcpd
+Highly recommended that you check/edit the network information before running!
 
 %prep
 
@@ -36,6 +39,8 @@
 
 
 %changelog
+* Sat Oct 13 2007 Warren Togami <wtogami@redhat.com> 0-2
+- minor cleanups
+
 * Sat Oct 13 2007 Eric Harrison <eharrison@mesd.k12.or.us>
 - first build
-
diff -u ltsp-config-dhcp-0-1.fc8.orig/ltsp-dhcpd.conf
ltsp-config-dhcp-0-1.fc8/ltsp-dhcpd.conf
--- ltsp-config-dhcp-0-1.fc8.orig/ltsp-dhcpd.conf       2006-03-30
21:55:17.000000000 -0500
+++ ltsp-config-dhcp-0-1.fc8/ltsp-dhcpd.conf    2007-10-13 17:42:08.000000000 -0400
@@ -1,7 +1,4 @@
-# Sample configuration file for ISCD dhcpd
-#
-# Don't forget to set run_dhcpd=1 in /etc/init.d/dhcpd
-# once you adjusted this file and copied it to /etc/dhcpd.conf.
+# Sample configuration file for K12LTSP dhcpd
 #
 
 default-lease-time            21600;
diff -u ltsp-config-dhcp-0-1.fc8.orig/ltsp-dhcpd.init
ltsp-config-dhcp-0-1.fc8/ltsp-dhcpd.init
--- ltsp-config-dhcp-0-1.fc8.orig/ltsp-dhcpd.init       2007-10-12
01:32:07.000000000 -0400
+++ ltsp-config-dhcp-0-1.fc8/ltsp-dhcpd.init    2007-10-13 17:42:08.000000000 -0400
@@ -1,7 +1,7 @@
 #!/bin/sh
 #
 ### BEGIN INIT INFO
-# Provides: dhcpd-ltsp
+# Provides: ltsp-dhcpd
 # Default-Start: 2 3 4 5
 # Default-Stop: 0 1 6
 # Should-Start:
@@ -75,7 +75,7 @@
     RETVAL=$?
     [ $RETVAL -eq 0 ] && return $RETVAL
 
-    echo -n $"Starting $prog: "
+    echo -n $"Starting ltsp-$prog: "
     daemon $dhcpd $DHCPDARGS 2>/dev/null
     RETVAL=$?
     echo
Comment 5 Warren Togami 2007-10-13 17:59:13 EDT
> If it is only temporary, then adding the config file in %doc
> in dhcp seems even more relevant.

We cannot lose the ability for this to work out-of-the-box as existing K12LTSP
users expect.  Sure this is problematic for other users, but then other users
shouldn't install this package.  The ltsp* prefix should be a big clue.

Package is APPROVED with the above changes.
Comment 6 Patrice Dumas 2007-10-13 18:10:32 EDT
That is not a way to handle reviews. There is a discussion
it should hold the approval. Remember, please, that there 
is community in Fedora.

What happens if the dhcp and the ltsp-dhcp services are
started simultaneously?

In my opinion, this is plain wrong to have 
ltsp-dhcpd.conf used without user edition. It is an example
file, it is not an out-of-the-box install. No K12LTSP user 
should expect to have a config file with specific defaults (as 
opposed to generic defaults) set up as it is the case here.
Comment 7 Warren Togami 2007-10-13 18:33:14 EDT
Patrice, I understand your concerns.  Let me try to explain our goal here.

A config file like this has been distributed for out-of-the-box usage for 6+
years in the K12LTSP distro.  Modern Edubuntu distributes exactly this way as
well.  Most users of K12LTSP today are using this configuration unmodified.

We are fully aware that this package is inflexible and possibly conflicting.

Despite these shortcomings, we cannot afford to make everything perfect before
inclusion in Fedora.  Our intent is to include basic functionality quickly, in
order to lower the barrier of entry of other people to contribute to its design
and improvement.

Despite not being perfect, it would be prudent to allow this in because:

- Prior to Fedora inclusion none of this is tracked in any source control,
making entry to development difficult and confusing.  With Ubuntu and every
several other distros already integrated with LTSP-5, this is a sorely needed
catalyst to keep Fedora relevant to LTSP users.
- This package does not cause problems for non-LTSP users, even if they
installed the package by accident because it is not enabled by default.


Please just let this go.  It doesn't harm anyone.
Comment 8 Eric Harrison 2007-10-13 18:46:58 EDT
Hi Patrice, the K12LTSP user very much expects to have a config file with
specific defaults set up. This is to support diskless workstations, DHCP is
required (with specific parameters) for the workstations to boot. (see 
http://wiki.ltsp.org/twiki/bin/view/Ltsp/IntegratingLtsp)

The dhcp & ltsp-dhcp services use the same lock file, same leases file, etc. You
cannot run both at the same time. 

Warren: this will not cause a conflict. Note that package will only be installed
if the (soon to be uploaded) ltsp5 packages are installed. It is disabled by
default, it will have no impact unless it is explicitly installed and enabled.
It will not run at the same time as dhcp started with a different conf file.

Comment 9 Patrice Dumas 2007-10-13 19:52:06 EDT
The config file will be much more generic if the group 
example is commented out.

Now in order to work out of the box there needs to be a 
dependency on a package setting up 
/lts/vmlinuz.ltsp
/lts/pxe/pxelinux.0
/opt/ltsp/*

Also it would be much better, in my opinion, to add
a README file explaining what is going on with this package.
Something along:



This package sets up a preconfigured dhcp server for 
ltsp. Beware that, contrary to the policy used in most
of the distribution, the dhcp server is started in the
default case. 

You should edit /etc/ltsp-dhcpd.conf if your network
doesn't match 192.168.0.0, or your router, dns, nfs 
hasn't the 192.168.0.255 ip address.

Please also note that the dhcp server using the 
regular /etc/dhcp.conf cannot be started at the same time
than the dhcp server in that package.
Comment 10 Patrice Dumas 2007-10-13 20:05:05 EDT
And it would also be nice to have that kind of text
in the %description, too.
Comment 11 Warren Togami 2007-10-13 20:07:06 EDT
Good idea about additional warnings and a README.

The dependencies are currently in churn, we will add dependencies when we have a
better idea how all the pieces fit together.
Comment 12 Patrice Dumas 2007-10-13 20:16:35 EDT
It would seem acceptable to me with some kind of
explanation as I propose above and the group stuff
commented out of the config file.

I also think that it shouldn't be added to another
fedora version than devel before appropriate requires 
are there. Even if it is broken in devel, it is not a 
real issue and indeed helps people to be able to test
other packages in progress.
Comment 13 Warren Togami 2007-10-13 20:26:07 EDT
nbd going into F-7 and devel was because it is a tiny and known good component
independent from K12LTSP.

The rest of LTSP, I'm generally in agreement.  We will definitely be testing
these pieces in devel before pushing them back into a stable release.  Only
known-working sets of packages will hit a stable release.
Comment 15 Patrice Dumas 2007-10-15 03:56:06 EDT
You have forgotten to add the README.
In my opinion it would be better to call it, in SOURCE, with
a prefix such as not to mix it with other README file that 
could be there, like ltsp-config-dhcp-README
Comment 16 Warren Togami 2007-10-15 15:47:06 EDT
In addition to the README,

-hasn't the 192.168.0.255 ip address.
+hasn't the 192.168.0.254 ip address.

You probably mean this in the %description?

Make these changes and this package is APPROVED.
Comment 17 Patrice Dumas 2007-10-15 15:55:09 EDT
(In reply to comment #16)

> -hasn't the 192.168.0.255 ip address.
> +hasn't the 192.168.0.254 ip address.
> 
> You probably mean this in the %description?

Indeed.
Comment 18 Patrice Dumas 2007-10-15 16:23:56 EDT
I think that we should wait a bit before adding the 
branch, it is even less clear that this package is
needed now that I had a look at ltsp-server-config.
Comment 19 Eric Harrison 2007-10-15 21:09:03 EDT
Yes, it does appear to fit well within ltsp-server-config.

I'll did a new build of ltsp-server-config that includes the contents of this
package. SPEC/SRPM links added to bz#331731
Comment 20 Warren Togami 2007-10-15 23:08:58 EDT
I am beginning to reconsider splitting ltsp-dhcpd into its own service script. 
Even though it is possible for us to make ltsp-dhcpd and dhcpd run concurrently
on different network interfaces, how often would that functionality be needed?

Also, how would it be an easier/better than putting the configs for both network
interfaces into a single dhcpd.conf?
Comment 21 Eric Harrison 2007-10-15 23:27:09 EDT
My old package added/removed "-cf /etc/ltsp-dhcpd.conf" in /etc/sysconfig/dhcpd
in %post/%postun. It worked, but strikes me as horribly wrong - how many would
think of looking in /etc/sysconfig/dhcpd to figure which conf file dhcpd is
using? Breaking it out as a separate init script at least makes it a bit more
obvious what is going on...

%post

CF_OPTION="-cf /etc/dhcpd-k12ltsp.conf"
CONF_FILE="/etc/sysconfig/dhcpd"

if [ -f $CONF_FILE -a -f /etc/dhcpd-k12ltsp.conf ]
then
   # change dhcpd.conf.k12ltsp to dhcpd-k12ltsp.conf
   perl -i -p -e "s,dhcpd.conf.k12ltsp,dhcpd-k12ltsp.conf," $CONF_FILE

   if [ "x`grep -- -cf $CONF_FILE`" = "x" ]
   then
      . $CONF_FILE
      NEWARGS="$DHCPDARGS $CF_OPTION"
      perl -i.ltsp -p -e "s,DHCPDARGS=.*,DHCPDARGS=\"$NEWARGS\"," $CONF_FILE
   fi
fi

%postun

CF_OPTION="-cf /etc/dhcpd-k12ltsp.conf"
CONF_FILE="/etc/sysconfig/dhcpd"

if [ -f $CONF_FILE ]
then
   if [ "`grep -- -cf $CONF_FILE`" ]
   then
      . $CONF_FILE
      NEWARGS="`echo $DHCPDARGS | sed -e \"s,$CF_OPTION,,g\"`"
      perl -i.ltsp -p -e "s,DHCPDARGS=.*,DHCPDARGS=\"$NEWARGS\"," $CONF_FILE
   fi
fi
Comment 22 Patrice Dumas 2007-10-16 04:06:27 EDT
(In reply to comment #20)
> I am beginning to reconsider splitting ltsp-dhcpd into its own service script. 
> Even though it is possible for us to make ltsp-dhcpd and dhcpd run concurrently
> on different network interfaces, how often would that functionality be needed?

Certainly never, but (unless I misunderstood things) ltsp-dhcpd and 
dhcpd won't run concurrently but dhcpd configuration is left to
the user while ltsp-dhcpd configuration is already setup.

> Also, how would it be an easier/better than putting the configs for both network
> interfaces into a single dhcpd.conf?

How would you achieve that off the box?
Comment 23 Patrice Dumas 2007-10-16 04:09:00 EDT
(In reply to comment #21)
> My old package added/removed "-cf /etc/ltsp-dhcpd.conf" in /etc/sysconfig/dhcpd
> in %post/%postun. It worked, but strikes me as horribly wrong - 

Not only, but also config files should never ever be modified
by %post and the like scripts. They are under the user responsibility
(through a GUI or vi).

Comment 24 Warren Togami 2007-10-16 14:35:21 EDT
> How would you achieve that off the box?

You don't.  It certainly requires manual configuration if you want such an
advanced setup.
Comment 25 Patrice Dumas 2007-10-18 04:40:05 EDT
Closing this bug as a duplicate since it is now merged in
ltsp-server.

*** This bug has been marked as a duplicate of 331651 ***
Comment 26 Patrice Dumas 2007-10-18 04:40:59 EDT
Warren, maybe you could unset the review flag? It will
be a bit confusing, otherwise.

Note You need to log in before you can comment on or make changes to this bug.