diff --git a/etc/nova/rootwrap.d/compute.filters b/etc/nova/rootwrap.d/compute.filters index 2d2b2df34955..ae65a0101a0f 100644 --- a/etc/nova/rootwrap.d/compute.filters +++ b/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 diff --git a/nova/privsep/libvirt.py b/nova/privsep/libvirt.py index 77eaaa3fdbec..f8c2ae3965f1 100644 --- a/nova/privsep/libvirt.py +++ b/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 '' diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 8e63fd5d8791..508f70fb4b50 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/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) diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index 82dc2b99f6ad..30baca8d1f96 100644 --- a/nova/virt/libvirt/driver.py +++ b/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 diff --git a/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml b/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml index 1f83e698fc69..d0dc32537d0a 100644 --- a/releasenotes/notes/privsep-queens-rootwrap-adds-907aa1bc8e3eb2ca.yaml +++ b/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. \ No newline at end of file