Merge ~andreserl/maas:lp1513775_fix_named_init_config into maas:master

[Andres Rodriguez]

LP: #1513775 - Ensure 'dnssec-validation' gets always parsed

On the original bind config, 'dnssec-validation' is set. In some cases, when parsing the config, 'dnssec-validation' is not removed automatically from the bind's config, but it is always added to MAAS' config. This, sometimes, causes bind to fail to start because of duplicate options.

[Andres Rodriguez (andreserl)]

@Mike,

I'm thinking that with this change we can drop migrate-conflicting-options, and always call these options without having to invoke migrate-conflicting-options. Thoutghs?

[Mike Pontillo (mpontillo)]

I think this change is good, as long as we can guarantee that the database is always up and running when MAAS is installed or upgraded. So +1, as long as the the maas-dns package is refactored to stop calling edit_named_options when the database isn't necessarily up and running. (That is, I'm not sure this should land separately from refactoring the packaging, but I'll leave it up to you.)

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 07ef3b9f5fc72a82ebb551f6fb188a78ee19db2d

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2127/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2131/console
COMMIT: cf13ead6cb288592106d236941a65201335dcc14

review: Needs Fixing

[MAAS Lander (maas-lander)]

LANDING
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2135/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2139/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0db71cc47f37686f2a8e5fe19c775fb25e056342

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1513775_fix_named_init_config lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2157/consoleText

diff --git a/src/maasserver/management/commands/edit_named_options.py b/src/maasserver/management/commands/edit_named_options.py
index cef2d49..960697f 100644
--- a/src/maasserver/management/commands/edit_named_options.py
+++ b/src/maasserver/management/commands/edit_named_options.py
@@ -63,10 +63,8 @@ class Command(BaseCommand):
         parser.add_argument(
             '--migrate-conflicting-options', default=False,
             dest='migrate_conflicting_options', action='store_true',
-            help="Causes any options that conflict with MAAS-managed options "
-                 "to be deleted from the BIND configuration and moved to the "
-                 "MAAS-managed configuration. Requires the MAAS database to "
-                 "be configured and running.")
+            help="**This option is now deprecated**. It no longer has any "
+                 "effect and it may be removed in a future release.")
 
     def read_file(self, config_path):
         """Open the named file and return its contents as a string."""
@@ -113,27 +111,36 @@ class Command(BaseCommand):
         if 'forwarders' in options_block:
             bind_forwarders = options_block['forwarders']
 
+            delete_forwarders = False
             if not dry_run:
-                config, created = Config.objects.get_or_create(
-                    name='upstream_dns',
-                    defaults={'value': ' '.join(bind_forwarders)})
-                if not created:
-                    # A configuration value already exists, so add the
-                    # additional values we found in the configuration file to
-                    # MAAS.
-                    if config.value is None:
-                        config.value = ''
-                    maas_forwarders = OrderedDict.fromkeys(
-                        config.value.split())
-                    maas_forwarders.update(bind_forwarders)
-                    config.value = ' '.join(maas_forwarders)
-                    config.save()
+                try:
+                    config, created = Config.objects.get_or_create(
+                        name='upstream_dns',
+                        defaults={'value': ' '.join(bind_forwarders)})
+                    if not created:
+                        # A configuration value already exists, so add the
+                        # additional values we found in the configuration
+                        # file to MAAS.
+                        if config.value is None:
+                            config.value = ''
+                        maas_forwarders = OrderedDict.fromkeys(
+                            config.value.split())
+                        maas_forwarders.update(bind_forwarders)
+                        config.value = ' '.join(maas_forwarders)
+                        config.save()
+                    delete_forwarders = True
+                except:
+                    pass
             else:
                 stdout.write(
                     "// Append to MAAS forwarders: %s\n"
                     % ' '.join(bind_forwarders))
 
-            del options_block['forwarders']
+            # Only delete forwarders from the config if MAAS was able to
+            # migrate the options. Otherwise leave them in the original
+            # config.
+            if delete_forwarders:
+                del options_block['forwarders']
 
     def migrate_dnssec_validation(self, options_block, dry_run, stdout):
         """Remove existing dnssec-validation from the options block.
@@ -148,19 +155,23 @@ class Command(BaseCommand):
             dnssec_validation = options_block['dnssec-validation']
 
             if not dry_run:
-                config, created = Config.objects.get_or_create(
-                    name='dnssec_validation',
-                    defaults={'value': dnssec_validation})
-                if not created:
-                    # Update the MAAS configuration to reflect the new setting
-                    # found in the configuration file.
-                    config.value = dnssec_validation
-                    config.save()
+                try:
+                    config, created = Config.objects.get_or_create(
+                        name='dnssec_validation',
+                        defaults={'value': dnssec_validation})
+                    if not created:
+                        # Update the MAAS configuration to reflect the new
+                        # setting found in the configuration file.
+                        config.value = dnssec_validation
+                        config.save()
+                except:
+                    pass
             else:
                 stdout.write(
                     "// Set MAAS dnssec_validation to: %s\n"
                     % dnssec_validation)
-
+            # Always attempt to delete this option as MAAS will always create
+            # a default for it.
             del options_block['dnssec-validation']
 
     def back_up_existing_file(self, config_path):
@@ -199,8 +210,6 @@ class Command(BaseCommand):
         stdout = options.get('stdout')
         if stdout is None:
             stdout = sys.stdout
-        migrate_conflicting_options = options.get(
-            'migrate_conflicting_options')
 
         options_file = self.read_file(config_path)
         config_dict = self.parse_file(config_path, options_file)
@@ -211,9 +220,10 @@ class Command(BaseCommand):
         # Modify the configuration (if necessary).
         self.set_up_include_statement(options_block, config_path)
 
-        if migrate_conflicting_options:
-            self.migrate_forwarders(options_block, dry_run, stdout)
-            self.migrate_dnssec_validation(options_block, dry_run, stdout)
+        # Attempt to migrate the conflicting options if there's
+        # database connection.
+        self.migrate_forwarders(options_block, dry_run, stdout)
+        self.migrate_dnssec_validation(options_block, dry_run, stdout)
 
         # Re-parse the new configuration, so we can detect any changes.
         new_content = make_isc_string(config_dict)
diff --git a/src/maasserver/tests/test_commands_edit_named_options.py b/src/maasserver/tests/test_commands_edit_named_options.py
index 776a861..c59c125 100644
--- a/src/maasserver/tests/test_commands_edit_named_options.py
+++ b/src/maasserver/tests/test_commands_edit_named_options.py
@@ -120,14 +120,14 @@ class TestEditNamedOptionsCommand(MAASServerTestCase):
         self.assertContentFailsWithMessage(
             OPTIONS_FILE, "Failed to make a backup")
 
-    def test_does_not_remove_existing_forwarders_config(self):
+    def test_remove_existing_forwarders_config(self):
         options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
         call_command(
             "edit_named_options", config_path=options_file,
             stdout=self.stdout)
 
         options = read_isc_file(options_file)
-        self.assertThat(make_isc_string(options), Contains('forwarders'))
+        self.assertThat(make_isc_string(options), Not(Contains('forwarders')))
 
     def test_removes_existing_forwarders_config_if_migrate_set(self):
         options_file = self.make_file(contents=OPTIONS_FILE_WITH_FORWARDERS)
@@ -152,7 +152,7 @@ class TestEditNamedOptionsCommand(MAASServerTestCase):
         # that's now in the included file).
         options = read_isc_file(options_file)
         self.assertThat(
-            make_isc_string(options), Contains('dnssec-validation'))
+            make_isc_string(options), Not(Contains('dnssec-validation')))
 
     def test_removes_existing_dnssec_validation_config_if_migration_set(self):
         options_file = self.make_file(contents=OPTIONS_FILE_WITH_DNSSEC)