Merge ~newell-jensen/maas:lp1713556 into maas:master

[Newell Jensen]

LP: #1713556 -- Add ability to set console_logging boolean for a pod in the API for whether or not console logging should be enabled.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f97362ced1eb2952bd10f3eff7a9fa3a8d6a5412

review: Approve

[Andres Rodriguez (andreserl)]

questions inline.

review: Needs Information

[Newell Jensen (newell-jensen)]

responses inline.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 9a9e717e2e2e5b8020de2821c31879af0dcaa0e2

review: Approve

[Andres Rodriguez (andreserl)]

review: Needs Information

[Newell Jensen (newell-jensen)]

replies inline.

[Andres Rodriguez (andreserl)]

comment inline.

[Mike Pontillo (mpontillo)]

I have some comments/questions inline below.

Also, I feel that (assuming the same strategy is used) unit testing for this change needs to be expanded. Ideally, it should explicitly cover the following scenarios:

(1) Updating the pod while specifying console_logging=True - ensure that the tag gets set
(2) Updating the pod while specifying console_log=False - ensure that the tag is unset
(3) Updating the pod while not specifying console_logging at all - ensure that the tag remains set if already set
(4) Updating the pod while not specifying console_logging at all - ensure that the tag remains unset unset if already unset

So my main concern with this branch is the fact that I don't see comprehensive testing of the above scenarios. That's the main thing that makes this a "Needs Fixing" (but see my inline comments below, too).

If you implement this as a separate operation rather than changing the exiting update mechanism, you wouldn't have to worry about (3) and (4).

On the other hand, I do like how updating this value is "seamless" and doesn't require looking around for a separate operation. I think that's a better user experience that way. But what would make it even better is if this logic was instead implemented in the form. If you do it that way, it will be trivial to update the UI to allow setting this parameter as well.

Lastly, there's a better way to get a boolean value from the request when using the API; I would do it more like this:

        console_logging = get_optional_param(
            request.POST, 'console_logging', default=False, validator=StringBool)

By doing it this way, you would handle more variations than just the case-insensitive "true" and "false" strings.

review: Needs Fixing

[Newell Jensen (newell-jensen)]

My original branch had it in the form with a data migration. After talking with you guys on IRC about the best route forward, will move the pod console logging tag creation to the form so that it can potentially be used in the UI.

[Mike Pontillo (mpontillo)]

Not sure if this is actually ready for another review yet, but I caught a couple things below.

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2cba8df0c7c08436f6d6b04d2e0b04ab9eb8cc15

review: Approve

[Andres Rodriguez (andreserl)]

lgtm! Please seek Mike's approval before landing.

review: Approve

[Mike Pontillo (mpontillo)]

Can you add a test that ensures that the following sequence of steps works as expected:

(1) Update a pod with console_logging=True
(2) Verify that the tag exists
(3) Update something unrelated on the pod, such as the name
(4) Verify that the tag still exists

[Mike Pontillo (mpontillo)]

Found a few more issues; see inline below.

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 2aae15b79c2b7c41d48a979aed47c6da30e218af

review: Approve

[Mike Pontillo (mpontillo)]

See inline.

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 8d6df467b2905130f0aad8af6317f3a7adc9689e

review: Approve

[Mike Pontillo (mpontillo)]

+1, thanks for the fixes!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1713556 lp:~newell-jensen/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/4173/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1713556 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

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

review: Needs Fixing

[Mike Pontillo (mpontillo)]

FYI, I tested this branch last week when I was gathering information for another bug report. It works great!

b218c14... by Newell Jensen on 2018-10-16

Fix lint.

diff --git a/src/maasserver/api/pods.py b/src/maasserver/api/pods.py
index 0c6b28c..42036a6 100644
--- a/src/maasserver/api/pods.py
+++ b/src/maasserver/api/pods.py
@@ -153,6 +153,11 @@ class PodHandler(OperationsHandler):
         :type default_macvlan_mode: unicode
         :param tags: A tag or tags (separated by comma) for the pod.
         :type tags: unicode
+        :param console_log: If True, created VMs for this pod will have
+            their console output logged.  To do this, a tag with the name
+            'pod-console-logging' is created.  If False, it checks to see if
+            this tag already exists and deletes it if it does.
+        :type console_log: boolean
 
         Note: 'type' cannot be updated on a Pod. The Pod must be deleted and
         re-added to change the type.
diff --git a/src/maasserver/api/tests/test_pods.py b/src/maasserver/api/tests/test_pods.py
index 5cc1da7..4600e3f 100644
--- a/src/maasserver/api/tests/test_pods.py
+++ b/src/maasserver/api/tests/test_pods.py
@@ -13,6 +13,7 @@ from maasserver.enum import NODE_CREATION_TYPE
 from maasserver.forms import pods
 from maasserver.models.bmc import Pod
 from maasserver.models.node import Machine
+from maasserver.models.tag import Tag
 from maasserver.testing.api import APITestCase
 from maasserver.testing.factory import factory
 from maasserver.utils.converters import json_load_bytes
@@ -276,7 +277,11 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
         self.become_admin()
         pod = factory.make_Pod(pod_type='virsh')
         new_name = factory.make_name('pod')
-        new_tags = [factory.make_name('tag'), factory.make_name('tag')]
+        new_tags = [
+            factory.make_name('tag'),
+            factory.make_name('tag'),
+            'pod-console-logging',
+        ]
         new_pool = factory.make_ResourcePool()
         new_zone = factory.make_Zone()
         new_power_parameters = {
@@ -291,10 +296,12 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
             'power_pass': new_power_parameters['power_pass'],
             'zone': new_zone.name,
             'pool': new_pool.name,
+            'console_logging': 'True',
         })
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         pod.refresh_from_db()
+        self.assertIsNotNone(Tag.objects.get(name="pod-console-logging"))
         self.assertEqual(new_name, pod.name)
         self.assertEqual(new_pool, pod.pool)
         self.assertItemsEqual(new_tags, pod.tags)
@@ -313,10 +320,13 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
             'power_address': pod_info['power_address'],
             'power_pass': pod_info['power_pass'],
             'zone': pod_info['zone'],
+            'console_logging': 'False',
         })
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         parsed_output = json_load_bytes(response.content)
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name="pod-console-logging")
         self.assertEqual(new_name, parsed_output['name'])
         self.assertEqual(discovered_pod.cores, parsed_output['total']['cores'])
 
@@ -337,6 +347,8 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
         self.assertEqual(
             http.client.OK, response.status_code, response.content)
         pod.refresh_from_db()
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name="pod-console-logging")
         self.assertEqual(new_name, pod.name)
         self.assertEqual(power_parameters, pod.power_parameters)
 
@@ -354,6 +366,100 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
         pod.refresh_from_db()
         self.assertEqual(pod.default_macvlan_mode, default_macvlan_mode)
 
+    def test_PUT_update_deletes_pod_console_logging_tag_if_not_in_use(self):
+        self.become_admin()
+        factory.make_Tag(name='pod-console-logging')
+        pod1_info = self.make_pod_info()
+        factory.make_Pod(pod_type=pod1_info['type'])
+        pod2_info = self.make_pod_info()
+        pod2 = factory.make_Pod(
+            pod_type=pod2_info['type'],
+            tags=['pod-console-logging'])
+        self.fake_pod_discovery()
+        response = self.client.put(get_pod_uri(pod2), {
+            'console_logging': 'False',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name='pod-console-logging')
+
+    def test_PUT_update_wont_delete_pod_console_logging_tag_if_in_use(self):
+        self.become_admin()
+        factory.make_Tag(name='pod-console-logging')
+        pod1_info = self.make_pod_info()
+        factory.make_Pod(
+            pod_type=pod1_info['type'],
+            tags=['pod-console-logging'])
+        pod2_info = self.make_pod_info()
+        pod2 = factory.make_Pod(
+            pod_type=pod2_info['type'],
+            tags=['pod-console-logging'])
+        self.fake_pod_discovery()
+        response = self.client.put(get_pod_uri(pod2), {
+            'console_logging': 'False',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        self.assertIsNotNone(Tag.objects.get(name='pod-console-logging'))
+
+    def test_PUT_update_creates_pod_console_logging_tag(self):
+        self.become_admin()
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name='pod-console-logging')
+        pod_info = self.make_pod_info()
+        pod = factory.make_Pod(pod_type=pod_info['type'])
+        self.fake_pod_discovery()
+        response = self.client.put(get_pod_uri(pod), {
+            'console_logging': 'True',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        self.assertIsNotNone(Tag.objects.get(name='pod-console-logging'))
+        response = self.client.put(get_pod_uri(pod), {
+            'console_logging': 'True',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        self.assertIsNotNone(Tag.objects.get(name='pod-console-logging'))
+        response = self.client.put(get_pod_uri(pod), {
+            'name': factory.make_name('name'),
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        pod.refresh_from_db()
+        self.assertIn('pod-console-logging', pod.tags)
+        self.assertIsNotNone(Tag.objects.get(name='pod-console-logging'))
+
+    def test_PUT_update_deletes_pod_console_logging_tag(self):
+        self.become_admin()
+        factory.make_Tag(name='pod-console-logging')
+        pod_info = self.make_pod_info()
+        pod = factory.make_Pod(
+            pod_type=pod_info['type'], tags=['pod-console-logging'])
+        self.fake_pod_discovery()
+        response = self.client.put(get_pod_uri(pod), {
+            'console_logging': 'False',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name='pod-console-logging')
+        response = self.client.put(get_pod_uri(pod), {
+            'console_logging': 'False',
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        response = self.client.put(get_pod_uri(pod), {
+            'name': factory.make_name('name'),
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        pod.refresh_from_db()
+        self.assertNotIn('pod-console-logging', pod.tags)
+        self.assertRaises(
+            Tag.DoesNotExist, Tag.objects.get, name='pod-console-logging')
+
     def test_refresh_requires_admin(self):
         pod = factory.make_Pod()
         response = self.client.post(get_pod_uri(pod), {
diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
index 77f9f3e..4a43f01 100644
--- a/src/maasserver/forms/pods.py
+++ b/src/maasserver/forms/pods.py
@@ -46,6 +46,7 @@ from maasserver.models import (
     PodStoragePool,
     RackController,
     ResourcePool,
+    Tag,
     Zone,
 )
 from maasserver.node_constraint_filter_forms import (
@@ -148,6 +149,23 @@ class PodForm(MAASModelForm):
         if data is None:
             data = {}
         type_value = data.get('type', self.initial.get('type'))
+        console_log = data.get('console_logging', '').lower()
+        if console_log in ['true', 'false']:
+            console_log = True if console_log == 'true' else False
+            if self.is_new:
+                self.update_console_log = True
+            elif 'pod-console-logging' in instance.tags and console_log:
+                self.update_console_log = False
+            elif 'pod-console-logging' in instance.tags and not console_log:
+                self.update_console_log = True
+            else:
+                self.update_console_log = True
+        else:
+            self.update_console_log = False
+
+        if self.update_console_log:
+            self.console_log = console_log
+
         self.drivers_orig = driver_parameters.get_all_power_types()
         self.drivers = {
             driver['name']: driver
@@ -243,6 +261,31 @@ class PodForm(MAASModelForm):
             self.instance = super(PodForm, self).save(commit=False)
             self.instance.power_type = power_type
             self.instance.power_parameters = power_parameters
+            # If console_log is set, create a tag for the kernel parameters
+            # if it does not already exist.  Delete otherwise.
+            if self.update_console_log:
+                if self.console_log:
+                    tag, _ = Tag.objects.get_or_create(
+                        name="pod-console-logging",
+                        kernel_opts="console=tty1 console=ttyS0")
+                    # Add this tag to the pod.
+                    self.instance.add_tag(tag.name)
+                else:
+                    try:
+                        tag = Tag.objects.get(name="pod-console-logging")
+                        # Remove this tag from the pod.
+                        self.instance.remove_tag(tag.name)
+                        # Delete the tag if there are no longer any other
+                        # pods using it.
+                        pods = Pod.objects.filter(
+                            tags__contains=['pod-console-logging']).exclude(
+                                id=self.instance.id)
+                        if not pods:
+                            tag.delete()
+                    except Tag.DoesNotExist:
+                        # There was no tag so just continue.
+                        pass
+
             return self.instance
 
         power_type = self.cleaned_data['type']
@@ -258,7 +301,7 @@ class PodForm(MAASModelForm):
             d = deferToDatabase(
                 transactional(check_for_duplicate),
                 power_type, power_parameters)
-            d.addCallback(update_obj)
+            d.addCallback(partial(deferToDatabase, transactional(update_obj)))
             d.addCallback(lambda _: self.discover_and_sync_pod())
             return d
         else:
diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
index 2a84bbb..69629d7 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -118,6 +118,7 @@ DOM_TEMPLATE_AMD64 = dedent("""\
             bus='0x00' slot='0x05' function='0x0'/>
         </controller>
         <serial type='pty'>
+          <log file="/var/log/libvirt/qemu/{name}-serial0.log" append="off" />
           <target port='0'/>
         </serial>
         <console type='pty'>
@@ -162,6 +163,7 @@ DOM_TEMPLATE_ARM64 = dedent("""\
         <emulator>{emulator}</emulator>
         <controller type='pci' index='0' model='pcie-root'/>
         <serial type='pty'>
+          <log file="/var/log/libvirt/qemu/{name}-serial0.log" append="off" />
           <target port='0'/>
         </serial>
         <console type='pty'>
@@ -192,6 +194,7 @@ DOM_TEMPLATE_PPC64 = dedent("""\
         <emulator>{emulator}</emulator>
         <controller type='pci' index='0' model='pci-root'/>
         <serial type='pty'>
+          <log file="/var/log/libvirt/qemu/{name}-serial0.log" append="off" />
           <target port='0'/>
         </serial>
         <console type='pty'>
@@ -218,8 +221,11 @@ DOM_TEMPLATE_S390X = dedent("""
         <pae/>
       </features>
       <devices>
-        <console type='pty' tty='/dev/pts/3'>
-          <source path='/dev/pts/3'/>
+        <serial type='pty'>
+          <log file="/var/log/libvirt/qemu/{name}-serial0.log" append="off" />
+          <target port='0'/>
+        </serial>
+        <console type='pty'>
           <target type='sclp' port='0'/>
           <alias name='console0'/>
         </console>