Read from console ptys using privsep.

Instead of dd. The interesting bit here is that the read is
non-blocking, but apart from that its pretty cut and dried.

Change-Id: I14d2a15e4f5efbed605e581781873494f23393a4

etc/nova/rootwrap.d/compute.filters

@@ -85,9 +85,6 @@ mm-ctl: CommandFilter, mm-ctl, root
 # nova/network/linux_net.py: 'ovs-ofctl', ....
 ovs-ofctl: CommandFilter, ovs-ofctl, root
 
-# nova/virt/libvirt/driver.py: 'dd', if=%s % virsh_output, ...
-dd: CommandFilter, dd, root
-
 # nova/virt/xenapi/volume_utils.py: 'iscsiadm', '-m', ...
 iscsiadm: CommandFilter, iscsiadm, root
 

nova/privsep/libvirt.py

@@ -23,11 +23,16 @@ import os
 import stat
 
 from oslo_concurrency import processutils
+from oslo_log import log as logging
 from oslo_utils import units
 
+from nova.i18n import _
 import nova.privsep
 
 
+LOG = logging.getLogger(__name__)
+
+
 @nova.privsep.sys_admin_pctxt.entrypoint
 def last_bytes(path, num):
     # NOTE(mikal): this is implemented in this contrived manner because you
@@ -162,3 +167,46 @@ def disable_ipv6(interface):
     """Disable ipv6 for a bridge."""
     with open('/proc/sys/net/ipv6/conf/%s/disable_ipv' % interface, 'w') as f:
         f.write('1')
+
+
+@nova.privsep.sys_admin_pctxt.entrypoint
+def readpty(path):
+    # TODO(mikal): I'm not a huge fan that we don't enforce a valid pty path
+    # here, but I haven't come up with a great way of doing that.
+
+    # NOTE(mikal): I am deliberately not catching the ImportError
+    # exception here... Some platforms (I'm looking at you Windows)
+    # don't have a fcntl and we may as well let them know that
+    # with an ImportError, not that they should be calling this at all.
+    import fcntl
+
+    try:
+        with open(path, 'r') as f:
+            current_flags = fcntl.fcntl(f.fileno(), fcntl.F_GETFL)
+            fcntl.fcntl(f.fileno(), fcntl.F_SETFL,
+                        current_flags | os.O_NONBLOCK)
+
+            return f.read()
+
+    except Exception as e:
+        # NOTE(mikal): dear internet, I see you looking at me with your
+        # judging eyes. There's a story behind why we do this. You see, the
+        # previous implementation did this:
+        #
+        # out, err = utils.execute('dd',
+        #                          'if=%s' % pty,
+        #                          'iflag=nonblock',
+        #                          run_as_root=True,
+        #                          check_exit_code=False)
+        # return out
+        #
+        # So, it never checked stderr or the return code of the process it
+        # ran to read the pty. Doing something better than that has turned
+        # out to be unexpectedly hard because there are a surprisingly large
+        # variety of errors which appear to be thrown when doing this read.
+        #
+        # Therefore for now we log the errors, but keep on rolling. Volunteers
+        # to help clean this up are welcome and will receive free beverages.
+        LOG.info(_('Ignored error while reading from instance console '
+                   'pty: %s'), e)
+        return ''

nova/tests/unit/virt/libvirt/test_driver.py

@@ -11390,8 +11390,9 @@ class LibvirtConnTestCase(test.NoDBTestCase,
     @mock.patch('nova.privsep.libvirt.last_bytes',
                 return_value=(b'67890', 0))
     @mock.patch('nova.privsep.path.writefile')
-    def test_get_console_output_pty(self, mocked_writefile, mocked_last_bytes,
-                                    mocked_path_exists):
+    @mock.patch('nova.privsep.libvirt.readpty')
+    def test_get_console_output_pty(self, mocked_readfile, mocked_writefile,
+                                    mocked_last_bytes, mocked_path_exists):
         with utils.tempdir() as tmpdir:
             self.flags(instances_path=tmpdir)
 
@@ -11418,12 +11419,10 @@ class LibvirtConnTestCase(test.NoDBTestCase,
             def fake_lookup(id):
                 return FakeVirtDomain(fake_dom_xml)
 
-            def _fake_flush(self, fake_pty):
-                return 'foo'
+            mocked_readfile.return_value = 'foo'
 
             self.create_fake_libvirt_mock()
             libvirt_driver.LibvirtDriver._conn.lookupByUUIDString = fake_lookup
-            libvirt_driver.LibvirtDriver._flush_libvirt_console = _fake_flush
 
             drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)
 

nova/virt/libvirt/driver.py

@@ -2819,14 +2819,6 @@ class LibvirtDriver(driver.ComputeDriver):
         timer = loopingcall.FixedIntervalLoopingCall(_wait_for_boot)
         timer.start(interval=0.5).wait()
 
-    def _flush_libvirt_console(self, pty):
-        out, err = utils.execute('dd',
-                                 'if=%s' % pty,
-                                 'iflag=nonblock',
-                                 run_as_root=True,
-                                 check_exit_code=False)
-        return out
-
     def _get_console_output_file(self, instance, console_log):
         bytes_to_read = MAX_CONSOLE_BYTES
         log_data = b""  # The last N read bytes
@@ -2892,7 +2884,8 @@ class LibvirtDriver(driver.ComputeDriver):
             raise exception.ConsoleNotAvailable()
 
         console_log = self._get_console_log_path(instance)
-        data = self._flush_libvirt_console(pty)
+        data = nova.privsep.libvirt.readpty(pty)
+
         # NOTE(markus_z): The virt_types kvm and qemu are the only ones
         # which create a dedicated file device for the console logging.
         # Other virt_types like xen, lxc, uml, parallels depend on the

releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml

@@ -5,5 +5,5 @@ upgrade:
     rootwrap configuration.
   - |
     The following commands are no longer required to be listed in your rootwrap
-    configuration: cat; chown; cryptsetup; ploop; prl_disk_tool; readlink;
+    configuration: cat; chown; cryptsetup; dd; ploop; prl_disk_tool; readlink;
     tee; touch.