Merge lp:~allenap/maas/remove-squashfs into lp:~maas-committers/maas/trunk

[Gavin Panella]

Remove all install-via-squashfs support.

[Dave Walker (davewalker)]

This doesn't comment a bug, or rational for removing this support.

review: Needs Fixing

[Gavin Panella (allenap)]

> This doesn't comment a bug, or rational for removing this support.

Good point. I've linked to 3 related bugs.

Rationale: this feature is not ready.

It has no automated tests, is attracting a disproportionate number of bugs, and doesn't work by most/all accounts. It should not have been landed in the first place. For an optimisation it's causing too many issues, so we (Julian, Raphael, Martin, myself) agreed this morning to remove it. We can reintroduce it, or its successor, once it's rock-solid outside of MAAS.

[Julian Edwards (julian-edwards)]

Dave, as Gavin says this feature is utterly broken right now, has no tests and I personally have never seen it work properly. I would be happy to re-instate it when the above are fixed, but I am under the impression (correct me if wrong) that you guys are working on something else anyway? In addition, I remember being told at UDS that it doesn't really speed up installation much over the d-i method.

I'll approve the change here on condition that Dave is ok with it too, based on my comments.

review: Approve

[Gavin Panella (allenap)]

Daviey seems to be okay with this now (#maas, freenode) so I think we can move ahead.

[Andres Rodriguez (andreserl)]

So let me get this straight... the reason of the removal is because its causing too many issues (bugs).

What are those?

1. One bug report I see linked is server side and is *not* An ssue with this feature.
2. Another report is an issue of not having updated a file with the release we want to import which is not even related to the script maas-import-squashfs but also affects maas-import-pxe-files and maas-import-ephemerals. and this bug would have still being there even of the feature wouldnt have existed.
3. Legitimate maas-import-squashfs.

So from above.. how in the world this feature is attracting a disproportionate number of bugs if only 1 of them is related.... with a fix that should only take a couple lines of code???

So i think t is not fair to argue that this causes more issues than anything else.

While it does lack tests.. so that's other import script that we have also seen broken in worse ways. This scripts is rock solid outside Maas and not because there's one minor bug it isn't. Otherwise other things wouldn't be rock solid for same or even worse bugs found in released versions of Maas.

[Gavin Panella (allenap)]

> So let me get this straight... the reason of the removal is because its
> causing too many issues (bugs).
>
> What are those?
>
> 1. One bug report I see linked is server side and is *not* An ssue with this
> feature.
> 2. Another report is an issue of not having updated a file with the release we
> want to import which is not even related to the script maas-import-squashfs
> but also affects maas-import-pxe-files and maas-import-ephemerals. and this
> bug would have still being there even of the feature wouldnt have existed.
> 3. Legitimate maas-import-squashfs.

Removing squashfs support means we can close two of these bugs, and
the third becomes a trivial fix.

>
> So from above..  how in the world this feature is attracting a
> disproportionate number of bugs if only 1 of them is related.... with a fix
> that should only take a couple lines of code???

For the size and benefit of the feature - i.e. a ~1 minute speedier
install - it's attracting a disproportionately high number of
bugs. That's my impression - I haven't tried to quantify it - but I'm
not alone in thinking this.

> So i think t is not fair to argue that this causes more issues than
> anything else.

I'm not saying that. I'm saying that it's more trouble than it's worth
right now. We don't *need* it, so it's better to move it out so that
we can work on - and SRU - a less-featureful but more stable product.

What is broken is the lack of tests. "Untested code is broken code" is
a principle that the Launchpad teams have all tried to follow for
years. It would be good to reintroduce this feature later on, or FPI,
but not in its current form.

>
> While it does lack tests.. so that's other import script that we have also
> seen broken in worse ways. This scripts is rock solid outside Maas and not
> because there's one minor bug it isn't. Otherwise other things wouldn't be
> rock solid for same or even worse bugs found in released versions of Maas.

Bugs filed is not a measure of quality.

[Andres Rodriguez (andreserl)]

> > 1. One bug report I see linked is server side and is *not* An ssue with this
> > feature.
> > 2. Another report is an issue of not having updated a file with the release
> we
> > want to import which is not even related to the script maas-import-squashfs
> > but also affects maas-import-pxe-files and maas-import-ephemerals. and this
> > bug would have still being there even of the feature wouldnt have existed.
> > 3. Legitimate maas-import-squashfs.
>
> Removing squashfs support means we can close two of these bugs, and
> the third becomes a trivial fix.

Closing 1 doesn't change the fact that it is still an issue om the server side. If for some reason the PXE images would be unavailable from the server, then the same bug would apply to maas-import-pxe-files, or to maas-import-ephemerals for such case. That's even if we didn't have the support for squashfs. Either way, the bug reported by Diogo was only seen in the lab that maybe had bad mirrors, because it was proven that at other locations the bug wasn't present.

2 is a bug which is unrelated with squashfs support because this bug affects maas-import-pxe-files and maas-import-ephemerals. Removing the support doesn't fix it, because the bug will still be present even if there was no squashfs support. In fact, there's was a bug in MAAS that affected installations using squashfs, not because of the support but because of a bug in MAAS.

3. Simple fix.

>
> For the size and benefit of the feature - i.e. a ~1 minute speedier
> install - it's attracting a disproportionately high number of
> bugs. That's my impression - I haven't tried to quantify it - but I'm
> not alone in thinking this.

2 minute to be exact.

> What is broken is the lack of tests. "Untested code is broken code" is
> a principle that the Launchpad teams have all tried to follow for
> years. It would be good to reintroduce this feature later on, or FPI,
> but not in its current form.

Right, but I was just pointing out that it isn't the only thing untested that maas ships, is it? However, its been placed as a strong reason for dropping the support.

>
> Bugs filed is not a measure of quality.

Exactly, number of bugs is not a measure of quality; yet, the statement for removal of the feature is based on "disproportionate number of bugs" and "doesn't work by most/all accounts", which are both based and backed by 3 bug reports linked to this MP. However, both statements IMHO are incorrect.

Cheers

[Gavin Panella (allenap)]

On 13 November 2012 17:11, Andres Rodriguez <...> wrote:
...
>> For the size and benefit of the feature - i.e. a ~1 minute speedier
>> install - it's attracting a disproportionately high number of
>> bugs. That's my impression - I haven't tried to quantify it - but I'm
>> not alone in thinking this.
>
> 2 minute to be exact.

It's still an optimisation; it's neither needed for correct operation,
nor is it such a huge improvement that we could call it a killer
feature. FPI may qualify as the latter.

>
>
>> What is broken is the lack of tests. "Untested code is broken code" is
>> a principle that the Launchpad teams have all tried to follow for
>> years. It would be good to reintroduce this feature later on, or FPI,
>> but not in its current form.
>
> Right, but I was just pointing out that it isn't the only thing
> untested that maas ships, is it? However, its been placed as a
> strong reason for dropping the support.

Most parts that ship are tested; if something isn't tested it's a bug.

Notably the maas-import-* scripts are almost untested (there are basic
tests in provisioningserver.tests.test_maas_import_pxe_files, though
these just check the "happy path"; they don't demonstrate what happens
when there's an error).

I'd like to either massively increase test coverage for these scripts,
or remove them.

I'm not sure we have the time to improve test coverage, given that
writing tests often involves refactoring to make code testable, and
that we have no baseline to refactor against except for manual QA...
because there are almost no existing tests.

We need maas-import-pxe-files, -ephemerals and -isos, so we can't
remove them. However, we don't *need* squashfs support, so we don't
need maas-import-squashfs. Given that the feature does not work, and
that it needs non-trivial investment to get it working and covered by
tests, there's really no good argument for keeping it in trunk right
now.

[Julian Edwards (julian-edwards)]

On 14/11/12 03:55, Gavin Panella wrote:
> We need maas-import-pxe-files, -ephemerals and -isos, so we can't
> remove them. However, we don't *need* squashfs support, so we don't
> need maas-import-squashfs. Given that the feature does not work, and
> that it needs non-trivial investment to get it working and covered by
> tests, there's really no good argument for keeping it in trunk right
> now.

This is the key point for me. squashfs brings little relative
improvement for the amount of untested code that we're carrying to make
it happen. The risk of failure versus the return is quite bad.

We'll be very happy to work with the server team to make FPI well
engineered and tested.

=== modified file 'contrib/maas-http.conf'
--- contrib/maas-http.conf	2012-10-10 09:30:11 +0000
+++ contrib/maas-http.conf	2012-11-12 16:27:31 +0000
@@ -51,12 +51,6 @@
     SetHandler None
 </Directory>
 
-# Serve squashfs images
-Alias /MAAS/static/images/ /var/lib/maas/tftp/
-<Directory /var/lib/maas/tftp/>
-    SetHandler None
-</Directory>
-
 # Serve files from staticfiles.
 Alias /MAAS/static/ /usr/share/maas/web/static/
 <Directory /usr/share/maas/web/static/>

=== modified file 'contrib/preseeds_v2/generic'
--- contrib/preseeds_v2/generic	2012-11-08 11:23:19 +0000
+++ contrib/preseeds_v2/generic	2012-11-12 16:27:31 +0000
@@ -1,14 +1,5 @@
 {{inherit "preseed_master"}}
 
-{{def squashfs_image}}
-{{if node.distro_series in {'quantal'} and node.architecture in {'i386/generic', 'amd64/generic'} and use_squashfs is True }}
-d-i    preseed/early_command string anna-install live-installer
-d-i    live-installer/enable boolean true
-d-i    live-installer/mode select normal
-d-i    live-installer/net-image string http://{{server_host}}/MAAS/static/images/{{node.architecture}}/{{node.distro_series}}/filesystem/filesystem.squashfs
-{{endif}}
-{{enddef}}
-
 {{def proxy}}
 d-i     mirror/country string manual
 {{if node.architecture in {'i386/generic', 'amd64/generic'} }}

=== modified file 'contrib/preseeds_v2/preseed_master'
--- contrib/preseeds_v2/preseed_master	2012-10-02 19:08:05 +0000
+++ contrib/preseeds_v2/preseed_master	2012-11-12 16:27:31 +0000
@@ -3,9 +3,6 @@
 # * Cloud-init for bare-metal
 # * Cloud-init preseed data
 
-# Squashfs Installation
-{{self.squashfs_image}}
-
 # Locale
 d-i     debian-installer/locale string en_US.UTF-8
 

=== modified file 'etc/maas/import_pxe_files'
--- etc/maas/import_pxe_files	2012-10-25 13:25:46 +0000
+++ etc/maas/import_pxe_files	2012-11-12 16:27:31 +0000
@@ -17,4 +17,3 @@
 #ARCHES="amd64/generic i386/generic armhf/highbank"
 #LOCALE="en_US"
 #IMPORT_EPHEMERALS=1
-#IMPORT_SQUASHFS=1

=== removed file 'etc/maas/import_squashfs'
--- etc/maas/import_squashfs	2012-09-29 03:12:23 +0000
+++ etc/maas/import_squashfs	1970-01-01 00:00:00 +0000
@@ -1,6 +0,0 @@
-## get default settings from import_pxe_files
-[ ! -f /etc/maas/import_pxe_files ] || . /etc/maas/import_pxe_files
-
-#SQUASHFS_ARCHIVE="https://cdimage.ubuntu.com"
-#RELEASES="quantal"
-#ARCHES="amd64 i386"

=== modified file 'scripts/maas-import-pxe-files'
--- scripts/maas-import-pxe-files	2012-11-08 14:37:46 +0000
+++ scripts/maas-import-pxe-files	2012-11-12 16:27:31 +0000
@@ -42,14 +42,6 @@
 # directory.  A wget command line will work here, but curl will do as well.
 DOWNLOAD=${DOWNLOAD:-wget --no-verbose}
 
-# Whether to download squashfs images as well: "1" for yes, "0" for no.
-# Default is yes.
-IMPORT_SQUASHFS=${IMPORT_SQUASHFS:-1}
-
-# Whether to download squashfs images as well: "1" for yes, "0" for no.
-# Default is yes.
-IMPORT_SQUASHFS=${IMPORT_SQUASHFS:-1}
-
 # Whether to download ephemeral images as well: "1" for yes, "0" for no.
 # Default is yes.
 IMPORT_EPHEMERALS=${IMPORT_EPHEMERALS:-1}
@@ -198,24 +190,6 @@
 }
 
 
-# Download and install the squashfs root images.
-import_squashfs_images() {
-    if test "$IMPORT_SQUASHFS" != "0"
-    then
-        maas-import-squashfs
-    fi
-}
-
-
-# Download and install the squashfs root images.
-import_squashfs_images() {
-    if test "$IMPORT_SQUASHFS" != "0"
-    then
-        maas-import-squashfs
-    fi
-}
-
-
 # Download and install the ephemeral images.
 import_ephemeral_images() {
     if test "$IMPORT_EPHEMERALS" != "0"
@@ -232,7 +206,6 @@
 
     update_pre_boot_loader
     import_install_images
-    import_squashfs_images
     import_ephemeral_images
 }
 

=== removed file 'scripts/maas-import-squashfs'
--- scripts/maas-import-squashfs	2012-10-02 19:08:05 +0000
+++ scripts/maas-import-squashfs	1970-01-01 00:00:00 +0000
@@ -1,172 +0,0 @@
-#!/bin/bash
-#
-# maas-import-squashfs - sync and import squashfs images
-#
-# Copyright (C) 2012 Canonical
-#
-# Authors:
-#    Andres Rodriguez <[e-mail]>
-#
-# This program is free software: you can redistribute it and/or modify
-# it under the terms of the GNU Affero General Public License as
-# published by the Free Software Foundation, version 3 of the License.
-#
-# This program is distributed in the hope that it will be useful,
-# but WITHOUT ANY WARRANTY; without even the implied warranty of
-# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
-# GNU Affero General Public License for more details.
-#
-# You should have received a copy of the GNU Affero General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-# Exit immediately if a command exits with a non-zero status.
-set -o errexit
-# Treat unset variables as an error when substituting.
-set -o nounset
-
-# Load settings if available.
-settings="/etc/maas/import_squashfs"
-[ -r $settings ] && . $settings
-local_settings="$(pwd)/$settings"
-[ -r $local_settings ] && . $local_settings
-
-# Download locations for Ubuntu releases.
-#http://cdimage.ubuntu.com/ubuntu-server/daily/current/
-SQUASHFS_ARCHIVE=${ARCHIVE:-http://cdimage.ubuntu.com/}
-
-# Ubuntu releases that are to be downloaded.
-SUPPORTED_RELEASES=""
-for supported in $(distro-info --supported)
-do
-    if [ $(expr "$supported" \>= "quantal") -eq 1 ]; then
-        SUPPORTED_RELEASES="${SUPPORTED_RELEASES:-}${supported} "
-    fi
-done
-
-EFFECTIVE_RELEASES=""
-if [ -z "$RELEASES" ]; then
-    RELEASES=${RELEASES:-$SUPPORTED_RELEASES}
-else
-    for release in $RELEASES
-    do
-        [[ "$SUPPORTED_RELEASES" =~ "${release}" ]] && EFFECTIVE_RELEASES="${EFFECTIVE_RELEASES:-}${release} "
-    done
-    RELEASES="$EFFECTIVE_RELEASES"
-fi
-
-# The current Ubuntu release.
-STABLE_RELEASE=$(distro-info --stable)
-
-# Supported architectures.
-ARCHES=${ARCHES:-amd64/generic i386/generic}
-
-# Command line to download a resource at a given URL into the current
-# directory.  A wget command line will work here, but curl will do as well.
-DOWNLOAD=${DOWNLOAD:-wget --no-verbose}
-
-# Whether to download squashfs images as well: "1" for yes, "0" for no.
-# Default is yes.
-IMPORT_SQUASHFS=${IMPORT_SQUASHFS:-1}
-
-# Put together a full URL for where the installer files for architecture $1
-# and release $2 can be downloaded.
-compose_installer_download_url() {
-    local arch=$1 release=$2
-
-    case $arch in
-        amd64/*|i386/*)
-            local installer_url="$SQUASHFS_ARCHIVE/ubuntu-server/daily/current"
-            echo $installer_url
-            ;;
-        *)
-            echo "Unknown architecture: $arch" >&2
-            exit 1
-            ;;
-    esac
-}
-
-# Return a list of files for architecture $1 and release $2 that need to be
-# downloaded
-compose_installer_download_files() {
-    local arch=$1 release=$2
-
-    case $arch in
-        amd64/*|i386/*)
-            echo "$release-server-${arch%%/*}.squashfs"
-            ;;
-        *)
-            echo "Unknown architecture: $arch" >&2
-            exit 1
-            ;;
-    esac
-}
-
-# Rename downloaded files for architecture $1 and release $2 into the form that
-# MAAS expects them
-rename_installer_download_files() {
-    local arch=$1 release=$2
-
-    case $arch in
-        amd64/*|i386/*)
-            mv $release-server-${arch%%/*}.squashfs filesystem.squashfs
-            ;;
-        *)
-            echo "Unknown architecture: $arch" >&2
-            exit 1
-            ;;
-    esac
-}
-
-# Download kernel/initrd for installing Ubuntu release $2 for
-# architecture $1 and install it into the TFTP directory hierarchy.
-update_install_files() {
-    local arch=$1 release=$2
-    local files=$(compose_installer_download_files $arch $release)
-    local url=$(compose_installer_download_url $arch $release)
-    local file
-
-    mkdir "install"
-    pushd "install" >/dev/null
-    for file in $files
-    do
-        $DOWNLOAD $url/$file
-    done
-    rename_installer_download_files $arch $release
-    popd >/dev/null
-
-    maas-provision install-pxe-image \
-        --arch="${arch%%/*}" --subarch="${arch#*/}" \
-        --release=$release --purpose="filesystem" \
-        --image="install"
-}
-
-
-# Download and install the "install" images.
-import_install_images() {
-    local arch release DOWNLOAD_DIR
-
-    DOWNLOAD_DIR=$(mktemp -d)
-    echo "Downloading to temporary location $DOWNLOAD_DIR."
-    pushd -- $DOWNLOAD_DIR
-
-    for arch in $ARCHES
-    do
-        for release in $RELEASES
-        do
-            update_install_files $arch $release
-        done
-    done
-
-    popd
-    rm -rf -- $DOWNLOAD_DIR
-}
-
-
-main() {
-    # All files we create here are public.  The TFTP user will need to be
-    # able to read them.
-    umask a+r
-    import_install_images
-}
-
-main

=== modified file 'setup.py'
--- setup.py	2012-11-08 06:34:48 +0000
+++ setup.py	2012-11-12 16:27:31 +0000
@@ -58,7 +58,6 @@
              'etc/maas_cluster.conf',
              'etc/txlongpoll.yaml',
              'contrib/maas_local_celeryconfig.py',
-             'etc/maas/import_squashfs',
              'contrib/maas_local_celeryconfig_cluster.py',
              'etc/maas/import_ephemerals',
              'etc/maas/import_pxe_files',
@@ -77,8 +76,7 @@
              'contrib/preseeds_v2/enlist_userdata',
              'contrib/preseeds_v2/preseed_master']),
         ('/usr/sbin',
-            ['scripts/maas-import-squashfs',
-             'scripts/maas-import-ephemerals',
+            ['scripts/maas-import-ephemerals',
              'scripts/maas-import-pxe-files']),
     ],
 

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-11-09 09:12:01 +0000
+++ src/maasserver/preseed.py	2012-11-12 16:27:31 +0000
@@ -29,10 +29,7 @@
     NODE_STATUS,
     PRESEED_TYPE,
     )
-from maasserver.models import (
-    BootImage,
-    Config,
-    )
+from maasserver.models import Config
 from maasserver.server_address import get_maas_facing_server_host
 from maasserver.utils import absolute_reverse
 import tempita
@@ -246,20 +243,12 @@
             'preseed_data': compose_preseed(node),
             'node_disable_pxe_url': node_disable_pxe_url,
             'node_disable_pxe_data': node_disable_pxe_data,
-            'use_squashfs': is_squashfs_image_present(node),
         }
         context.update(node_context)
 
     return context
 
 
-def is_squashfs_image_present(node):
-    """Whether or not the SquashFS image can be used during installation."""
-    arch, subarch = node.architecture.split("/")
-    return BootImage.objects.have_image(
-        node.nodegroup, arch, subarch, node.get_distro_series(), "filesystem")
-
-
 def render_preseed(node, prefix, release=''):
     """Find and load a `PreseedTemplate` for the given node.
 

=== modified file 'src/maasserver/tests/test_preseed.py'
--- src/maasserver/tests/test_preseed.py	2012-11-09 09:45:28 +0000
+++ src/maasserver/tests/test_preseed.py	2012-11-12 16:27:31 +0000
@@ -37,7 +37,6 @@
     get_preseed_context,
     get_preseed_filenames,
     get_preseed_template,
-    is_squashfs_image_present,
     load_preseed_template,
     PreseedTemplate,
     render_preseed,
@@ -328,9 +327,8 @@
             ['node', 'release', 'metadata_enlist_url',
              'server_host', 'server_url', 'preseed_data',
              'node_disable_pxe_url', 'node_disable_pxe_data',
-             'use_squashfs', 'main_archive_hostname',
-             'main_archive_directory', 'ports_archive_hostname',
-             'ports_archive_directory'],
+             'main_archive_hostname', 'main_archive_directory',
+             'ports_archive_hostname', 'ports_archive_directory'],
             context)
 
     def test_get_preseed_context_if_node_None(self):
@@ -371,31 +369,6 @@
             ))
 
 
-class TestSquashFSAvailable(TestCase):
-    """Tests for `is_squashfs_image_present`."""
-
-    # Scenario defaults.
-    arch = "i386"
-    subarch = "generic"
-    series = "quantal"
-    purpose = "filesystem"
-
-    scenarios = (
-        ("mismatch-arch", dict(arch="amd64", present=False)),
-        ("mismatch-subarch", dict(subarch="special", present=False)),
-        ("mismatch-series", dict(series="precise", present=False)),
-        ("mismatch-purpose", dict(purpose="moonraking", present=False)),
-        ("match", dict(present=True)),
-        )
-
-    def test_squashfs_available(self):
-        node = factory.make_node(
-            architecture="i386/generic", distro_series="quantal")
-        BootImage.objects.register_image(
-            node.nodegroup, self.arch, self.subarch, self.series, self.purpose)
-        self.assertEqual(self.present, is_squashfs_image_present(node))
-
-
 class TestPreseedTemplate(TestCase):
     """Tests for class:`PreseedTemplate`."""
 

=== modified file 'src/provisioningserver/tests/test_maas_import_pxe_files.py'
--- src/provisioningserver/tests/test_maas_import_pxe_files.py	2012-11-08 06:34:48 +0000
+++ src/provisioningserver/tests/test_maas_import_pxe_files.py	2012-11-12 16:27:31 +0000
@@ -161,9 +161,6 @@
             # Suppress running of maas-import-ephemerals.  It gets too
             # intimate with the system to test here.
             'IMPORT_EPHEMERALS': '0',
-            # Suppress running of maas-import-squashfs.  It gets too
-            # intimate with the system to test here.
-            'IMPORT_SQUASHFS': '0',
         }
         env.update(self.config_fixture.environ)
         if arch is not None: