)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":16,"context_line":"   another instance is already using it."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Unfortunately, while this works for serial mounts and unmounts, there"},{"line_number":19,"context_line":"are multiple failure cases when volumes from the same export and"},{"line_number":20,"context_line":"mounted and unmounted simultaneously."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"To fix this, the libvirt driver now holds a single instance of a"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"5a74a57a_6dbc0b06","line":19,"range":{"start_line":19,"start_character":61,"end_line":19,"end_character":64},"updated":"2016-11-23 21:10:42.000000000","message":"are","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":16,"context_line":"   another instance is already using it."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Unfortunately, while this works for serial mounts and unmounts, there"},{"line_number":19,"context_line":"are multiple failure cases when volumes from the same export and"},{"line_number":20,"context_line":"mounted and unmounted simultaneously."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"To fix this, the libvirt driver now holds a single instance of a"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"5a74a57a_f792854a","line":19,"range":{"start_line":19,"start_character":61,"end_line":19,"end_character":64},"in_reply_to":"5a74a57a_6dbc0b06","updated":"2016-11-24 16:19:20.000000000","message":"Done","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"}],"nova/tests/unit/virt/libvirt/volume/test_mount.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cce0239cb52902face48fc7a99cd2a25720164c8","unresolved":false,"context_lines":[{"line_number":227,"context_line":"        self._sentinel_mount(m, mock.sentinel.vol_b)"},{"line_number":228,"context_line":"        mock_execute.assert_has_calls(expected_calls)"},{"line_number":229,"context_line":""},{"line_number":230,"context_line":"        # Unmount vol_a. We shouldn\u0027t have nmounted"},{"line_number":231,"context_line":"        self._sentinel_umount(m, mock.sentinel.vol_a)"},{"line_number":232,"context_line":"        mock_execute.assert_has_calls(expected_calls)"},{"line_number":233,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a6eadb0_34b6e5dc","line":230,"range":{"start_line":230,"start_character":43,"end_line":230,"end_character":51},"updated":"2016-12-13 11:21:04.000000000","message":"unmounted","commit_id":"663dfe6a5ae0ae1b6080c92a78680f9969924c86"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5a4bfb408607b34c534111460e360a0436080702","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":2,"context_line":"#    not use this file except in compliance with the License. You may obtain"},{"line_number":3,"context_line":"#    a copy of the License at"},{"line_number":4,"context_line":"#"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_7d4e8a97","line":1,"updated":"2017-02-02 18:19:03.000000000","message":"You don\u0027t want your Red Hat copyright in here?\n\n# Copyright 2016,2017 Red Hat, Inc.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4b23313ed54e2ed9b7d09b62ae687b672104c39f","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":2,"context_line":"#    not use this file except in compliance with the License. You may obtain"},{"line_number":3,"context_line":"#    a copy of the License at"},{"line_number":4,"context_line":"#"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_b0be77fd","line":1,"in_reply_to":"1a430d35_7dda357a","updated":"2017-02-07 16:58:35.000000000","message":"Done","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":2,"context_line":"#    not use this file except in compliance with the License. You may obtain"},{"line_number":3,"context_line":"#    a copy of the License at"},{"line_number":4,"context_line":"#"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_7dda357a","line":1,"in_reply_to":"3a461143_7d4e8a97","updated":"2017-02-03 10:26:08.000000000","message":"I\u0027ve always done it for the joy of coding, but the lawyers probably want it there ;)","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a2b13212a1bcc7c8a04e4cbb37cf827d68591b0","unresolved":false,"context_lines":[{"line_number":480,"context_line":"        # Test that when umount fails a subsequent mount doesn\u0027t try to"},{"line_number":481,"context_line":"        # remount it."},{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        orig_execute \u003d mount.utils.execute.side_effect"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        def raise_on_umount(cmd, *args, **kwargs):"},{"line_number":486,"context_line":"            if cmd \u003d\u003d \u0027umount\u0027:"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_ba99287a","line":483,"range":{"start_line":483,"start_character":8,"end_line":483,"end_character":54},"updated":"2017-04-18 15:31:09.000000000","message":"How come it\u0027s necessary to do this rather than simply using the decorator/context manager? Something to do with the use of threads?","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"77b1f2bd99d013f2ec652ff51ff4829161957607","unresolved":false,"context_lines":[{"line_number":480,"context_line":"        # Test that when umount fails a subsequent mount doesn\u0027t try to"},{"line_number":481,"context_line":"        # remount it."},{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        orig_execute \u003d mount.utils.execute.side_effect"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        def raise_on_umount(cmd, *args, **kwargs):"},{"line_number":486,"context_line":"            if cmd \u003d\u003d \u0027umount\u0027:"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_faed6d8b","line":483,"range":{"start_line":483,"start_character":8,"end_line":483,"end_character":54},"in_reply_to":"7ffa3b31_ba99287a","updated":"2017-04-18 16:51:27.000000000","message":"mount.utils.execute is already monkey patched (see setUp), and we\u0027re using that here. We just want to alter the behaviour on unmount, so we still want to call the original method. We could use a decorator, but we\u0027d lose the existing mock.\n\nNothing to do with threading in this case.","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"5c8b1067e736792297b3e032f46e9bc0431d02f3","unresolved":false,"context_lines":[{"line_number":480,"context_line":"        # Test that when umount fails a subsequent mount doesn\u0027t try to"},{"line_number":481,"context_line":"        # remount it."},{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        orig_execute \u003d mount.utils.execute.side_effect"},{"line_number":484,"context_line":""},{"line_number":485,"context_line":"        def raise_on_umount(cmd, *args, **kwargs):"},{"line_number":486,"context_line":"            if cmd \u003d\u003d \u0027umount\u0027:"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_26401aa5","line":483,"range":{"start_line":483,"start_character":8,"end_line":483,"end_character":54},"in_reply_to":"7ffa3b31_faed6d8b","updated":"2017-04-20 13:15:26.000000000","message":"Could you add a note to this effect?","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a2b13212a1bcc7c8a04e4cbb37cf827d68591b0","unresolved":false,"context_lines":[{"line_number":587,"context_line":"        # Host goes down"},{"line_number":588,"context_line":"        self.m.host_down()"},{"line_number":589,"context_line":""},{"line_number":590,"context_line":"        # Call host_up in a separate thread because it will block, and give"},{"line_number":591,"context_line":"        # it plenty of time to race"},{"line_number":592,"context_line":"        host_up \u003d eventlet.greenthread.spawn(self.m.host_up,"},{"line_number":593,"context_line":"                                             mock.sentinel.host)"},{"line_number":594,"context_line":"        time.sleep(0.01)"},{"line_number":595,"context_line":""},{"line_number":596,"context_line":"        # Assert that we haven\u0027t instantiated a new state while there\u0027s an"},{"line_number":597,"context_line":"        # ongoing operation from the previous state"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_ba724848","line":594,"range":{"start_line":590,"start_character":0,"end_line":594,"end_character":24},"updated":"2017-04-18 15:31:09.000000000","message":"I get that this is necessary, but still - eew :(","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"77b1f2bd99d013f2ec652ff51ff4829161957607","unresolved":false,"context_lines":[{"line_number":587,"context_line":"        # Host goes down"},{"line_number":588,"context_line":"        self.m.host_down()"},{"line_number":589,"context_line":""},{"line_number":590,"context_line":"        # Call host_up in a separate thread because it will block, and give"},{"line_number":591,"context_line":"        # it plenty of time to race"},{"line_number":592,"context_line":"        host_up \u003d eventlet.greenthread.spawn(self.m.host_up,"},{"line_number":593,"context_line":"                                             mock.sentinel.host)"},{"line_number":594,"context_line":"        time.sleep(0.01)"},{"line_number":595,"context_line":""},{"line_number":596,"context_line":"        # Assert that we haven\u0027t instantiated a new state while there\u0027s an"},{"line_number":597,"context_line":"        # ongoing operation from the previous state"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_7a55dd1c","line":594,"range":{"start_line":590,"start_character":0,"end_line":594,"end_character":24},"in_reply_to":"7ffa3b31_ba724848","updated":"2017-04-18 16:51:27.000000000","message":"Yeah. Frankly testing this stuff is a complete nightmare, which is probably why we don\u0027t normally test for races at all. I\u0027ve done my best here, but the Nova standard would be to ignore this entirely.","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4645ed3cc50147a87f236db2c71f0c66ecbe1a16","unresolved":false,"context_lines":[{"line_number":480,"context_line":"        # Test that when umount fails a subsequent mount doesn\u0027t try to"},{"line_number":481,"context_line":"        # remount it."},{"line_number":482,"context_line":""},{"line_number":483,"context_line":"        # We\u0027ve already got a fake execute (see setUp) which is ensuring mount,"},{"line_number":484,"context_line":"        # umount, and ismount work as expected. We don\u0027t want to mess with"},{"line_number":485,"context_line":"        # that, except that we want umount to raise an exception. We store the"},{"line_number":486,"context_line":"        # original here so we can call it if we\u0027re not unmounting, and so we"},{"line_number":487,"context_line":"        # can restore it when we no longer want the exception."},{"line_number":488,"context_line":"        orig_execute \u003d mount.utils.execute.side_effect"},{"line_number":489,"context_line":""},{"line_number":490,"context_line":"        def raise_on_umount(cmd, *args, **kwargs):"}],"source_content_type":"text/x-python","patch_set":22,"id":"7ffa3b31_fb398bf1","line":487,"range":{"start_line":483,"start_character":0,"end_line":487,"end_character":62},"updated":"2017-04-21 12:39:37.000000000","message":"ta","commit_id":"4ffefca7da76fab1a25e2a90f0873744c7b35cf3"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":3345,"context_line":"                         \u0027due to an unexpected exception.\u0027), CONF.host,"},{"line_number":3346,"context_line":"                     exc_info\u003dTrue)"},{"line_number":3347,"context_line":""},{"line_number":3348,"context_line":"        if enabled:"},{"line_number":3349,"context_line":"            mount.MountManager.host_up(self._host)"},{"line_number":3350,"context_line":"        else:"},{"line_number":3351,"context_line":"            mount.MountManager.host_down()"},{"line_number":3352,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_0d7d571c","line":3349,"range":{"start_line":3348,"start_character":8,"end_line":3349,"end_character":50},"updated":"2016-11-23 21:10:42.000000000","message":"Is this the case when the libvirt driver is first initialized?\n\nTrying to trace this, it looks like init_host will call _do_quality_warnings which will call self._host.get_capabilities() which will call host.get_connection() which because host._wrapped_conn will be None the Host object will call _get_new_connection() which gets the wrapped_conn and calls:\n\n self._conn_event_handler(bool(wrapped_conn), disable_reason)\n\nWhere _conn_event_handler is this method, and bool(wrapped_conn) will be True, so we\u0027ll initialize the MountManager with the host up.\n\nIs that all correct? If so, we should probably have a comment in here explaining that when the libvirt driver is initialized and the nova-compute process starts up, it calls init_host() which eventually gets a new host connection which triggers the callback to this code to initialize the MountManager. Whew. It\u0027s all very roundabout. I might be totally missing something obvious though.\n\n--\n\nIt might also be good to just have a debug statement in here when we\u0027re calling host_up and host_down on the mount manager.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":3345,"context_line":"                         \u0027due to an unexpected exception.\u0027), CONF.host,"},{"line_number":3346,"context_line":"                     exc_info\u003dTrue)"},{"line_number":3347,"context_line":""},{"line_number":3348,"context_line":"        if enabled:"},{"line_number":3349,"context_line":"            mount.MountManager.host_up(self._host)"},{"line_number":3350,"context_line":"        else:"},{"line_number":3351,"context_line":"            mount.MountManager.host_down()"},{"line_number":3352,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_c3ba27e1","line":3349,"range":{"start_line":3348,"start_character":8,"end_line":3349,"end_character":50},"in_reply_to":"5a74a57a_0d7d571c","updated":"2016-11-24 16:19:20.000000000","message":"While it\u0027s desirable that it happens during init_host, it\u0027s not actually vital. There are 2 important things here:\n\n1. It\u0027s kicked off when we call get_connection()\n2. Any mount()/umount() will block while MountManager is initialising, or fail if it isn\u0027t up.\n\nIt also occurred to me it would be better if it were more explicit. We could add a dummy get_connection() in init_host with a comment as to why it\u0027s there. However, the addition of MountManager doesn\u0027t change this (it\u0027s the same for the service enabled/disabled code), so I wouldn\u0027t do it in this patch.\n\nWe should be logging host_up and host_down, though. I\u0027ll add that.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"5054ecba0a0619dbb4ccf5719a1b89a9cd972537","unresolved":false,"context_lines":[{"line_number":3442,"context_line":"                         \u0027due to an unexpected exception.\u0027), CONF.host,"},{"line_number":3443,"context_line":"                     exc_info\u003dTrue)"},{"line_number":3444,"context_line":""},{"line_number":3445,"context_line":"        if enabled:"},{"line_number":3446,"context_line":"            mount.get_manager().host_up(self._host)"},{"line_number":3447,"context_line":"        else:"},{"line_number":3448,"context_line":"            mount.get_manager().host_down()"},{"line_number":3449,"context_line":""},{"line_number":3450,"context_line":"    def _get_guest_cpu_model_config(self):"},{"line_number":3451,"context_line":"        mode \u003d CONF.libvirt.cpu_mode"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_024bc559","line":3448,"range":{"start_line":3445,"start_character":7,"end_line":3448,"end_character":43},"updated":"2017-02-08 19:48:26.000000000","message":"Why do we want to re-initialize the mounts on disable/enable (as opposed to just doing it once in __init__())?  Is this to allow a last-ditch method of recovering from errors?","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0e2707d2b01c4827ca42eeeca548d7a11998d8e4","unresolved":false,"context_lines":[{"line_number":3442,"context_line":"                         \u0027due to an unexpected exception.\u0027), CONF.host,"},{"line_number":3443,"context_line":"                     exc_info\u003dTrue)"},{"line_number":3444,"context_line":""},{"line_number":3445,"context_line":"        if enabled:"},{"line_number":3446,"context_line":"            mount.get_manager().host_up(self._host)"},{"line_number":3447,"context_line":"        else:"},{"line_number":3448,"context_line":"            mount.get_manager().host_down()"},{"line_number":3449,"context_line":""},{"line_number":3450,"context_line":"    def _get_guest_cpu_model_config(self):"},{"line_number":3451,"context_line":"        mode \u003d CONF.libvirt.cpu_mode"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_32847809","line":3448,"range":{"start_line":3445,"start_character":7,"end_line":3448,"end_character":43},"in_reply_to":"1a430d35_024bc559","updated":"2017-02-09 09:44:51.000000000","message":"In the first instance we do it on host_up() rather than __init__() because we need the libvirt connection to be up first so we can enumerate guests.\n\nRe-doing it if the connection drops is an additional robustness measure as you suggest. Weird things happened to stuff we depend on: let\u0027s go re-check reality.","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a2b13212a1bcc7c8a04e4cbb37cf827d68591b0","unresolved":false,"context_lines":[{"line_number":3435,"context_line":"                         \u0027due to an unexpected exception.\u0027), CONF.host,"},{"line_number":3436,"context_line":"                     exc_info\u003dTrue)"},{"line_number":3437,"context_line":""},{"line_number":3438,"context_line":"        if enabled:"},{"line_number":3439,"context_line":"            mount.get_manager().host_up(self._host)"},{"line_number":3440,"context_line":"        else:"},{"line_number":3441,"context_line":"            mount.get_manager().host_down()"}],"source_content_type":"text/x-python","patch_set":21,"id":"7ffa3b31_dfdaa615","line":3438,"range":{"start_line":3438,"start_character":8,"end_line":3438,"end_character":19},"updated":"2017-04-18 15:31:09.000000000","message":"femtonit: using \u0027disable_service\u0027 (well, \u0027not disable_service\u0027) would make this ever so slightly more readable","commit_id":"b434fc419e6302959f6b7f816b6c65fc7d65531b"}],"nova/virt/libvirt/volume/fs.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"91ebed625a32cf284a774999e57695e60f47a47b","unresolved":false,"context_lines":[{"line_number":112,"context_line":"        vol_name \u003d connection_info[\u0027data\u0027][\u0027name\u0027]"},{"line_number":113,"context_line":"        mountpoint \u003d self._get_mount_path(connection_info)"},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"        self.connection.mount_manager.mount("},{"line_number":116,"context_line":"            self.fstype, export, vol_name, mountpoint,"},{"line_number":117,"context_line":"            self._mount_options(connection_info))"},{"line_number":118,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"ba5da102_626beaf1","line":115,"updated":"2016-11-01 14:06:02.000000000","message":"I\u0027m not a fan of accessing more methods in self.connection - we\u0027ve got it down to just self.connection._host, which enables us to remove self.connection entirely, replacing it with a direct reference to the Host object.\n\nCan we pass in the mount manager explicitly, or alternatively make it a standalone singleton, so it can be accessed directly. eg the \u0027mount_manager\u0027 method in driver.py could be just a global \u0027get_manager\u0027 method in mount.py","commit_id":"24451e5bfa47bbac862e234abf89eb9fb9daeaf8"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":130,"context_line":"        mount_manager.umount(vol_name, mountpoint)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @abc.abstractmethod"},{"line_number":133,"context_line":"    def _mount_options(self, connection_info):"},{"line_number":134,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_902c204c","line":133,"updated":"2016-11-23 21:10:42.000000000","message":"Should provide a docstring for an ABC method.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":130,"context_line":"        mount_manager.umount(vol_name, mountpoint)"},{"line_number":131,"context_line":""},{"line_number":132,"context_line":"    @abc.abstractmethod"},{"line_number":133,"context_line":"    def _mount_options(self, connection_info):"},{"line_number":134,"context_line":"        pass"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_63cdd35c","line":133,"in_reply_to":"5a74a57a_902c204c","updated":"2016-11-24 16:19:20.000000000","message":"Done","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cce0239cb52902face48fc7a99cd2a25720164c8","unresolved":false,"context_lines":[{"line_number":118,"context_line":"            self.fstype, export, vol_name, mountpoint,"},{"line_number":119,"context_line":"            self._mount_options(connection_info))"},{"line_number":120,"context_line":""},{"line_number":121,"context_line":"        connection_info[\u0027data\u0027][\u0027device_path\u0027] \u003d \\"},{"line_number":122,"context_line":"            self._get_device_path(connection_info)"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    def disconnect_volume(self, connection_info, disk_dev):"},{"line_number":125,"context_line":"        \"\"\"Disconnect the volume.\"\"\""}],"source_content_type":"text/x-python","patch_set":12,"id":"1a6eadb0_d6585959","line":122,"range":{"start_line":121,"start_character":49,"end_line":122,"end_character":50},"updated":"2016-12-13 11:21:04.000000000","message":"supernit: \u0027self._get_device_path(\u0027 would fit here, I guess","commit_id":"663dfe6a5ae0ae1b6080c92a78680f9969924c86"}],"nova/virt/libvirt/volume/mount.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"d984c654adbc707acb7de682e2472df2b121b148","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                    yield mount"},{"line_number":79,"context_line":"                    break"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def mount(self, driver, connection_info):"},{"line_number":82,"context_line":"        export \u003d connection_info[\u0027data\u0027][\u0027export\u0027]"},{"line_number":83,"context_line":"        vol_name \u003d connection_info[\u0027data\u0027][\u0027name\u0027]"},{"line_number":84,"context_line":"        mountpoint \u003d self._get_mount_path(connection_info)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a95cdbc_52ef4b1d","line":81,"updated":"2016-10-10 09:29:17.000000000","message":"I don\u0027t like the way you\u0027ve tied this API to the current volume driver API, as well as to the connection_info data format.\n\nIMHO, we should have an standalone API here, with formal parameters for each important attribute eg\n\n   def mount(self, fstype, export, vol_name, target_dir, options):\n\nso that we\u0027ve got clean separate between the two. \n\nAlso, there should be API docs for the public entry points\n\nOh, and we should call the file mountmanager.py","commit_id":"f7d42d3efa4100084a8e497f7f4fd851d2c94c64"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fc63f19368142cb35a68907e6c2b3f946e5683f9","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                    yield mount"},{"line_number":79,"context_line":"                    break"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def mount(self, driver, connection_info):"},{"line_number":82,"context_line":"        export \u003d connection_info[\u0027data\u0027][\u0027export\u0027]"},{"line_number":83,"context_line":"        vol_name \u003d connection_info[\u0027data\u0027][\u0027name\u0027]"},{"line_number":84,"context_line":"        mountpoint \u003d self._get_mount_path(connection_info)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a95cdbc_92e9d3b9","line":81,"in_reply_to":"1a95cdbc_52ef4b1d","updated":"2016-10-10 09:36:31.000000000","message":"I take your point, and this is actually how I initially wrote it. However, I noticed when implementing it for 3 backends that I was pulling the same info out of connection_info in the same way for each, and decided it pull it in here. It means the caller has only 1 LOC instead of 4. I\u0027ll have a think.","commit_id":"f7d42d3efa4100084a8e497f7f4fd851d2c94c64"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"fc63f19368142cb35a68907e6c2b3f946e5683f9","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                    yield mount"},{"line_number":79,"context_line":"                    break"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def mount(self, driver, connection_info):"},{"line_number":82,"context_line":"        export \u003d connection_info[\u0027data\u0027][\u0027export\u0027]"},{"line_number":83,"context_line":"        vol_name \u003d connection_info[\u0027data\u0027][\u0027name\u0027]"},{"line_number":84,"context_line":"        mountpoint \u003d self._get_mount_path(connection_info)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a95cdbc_f2c3f735","line":81,"in_reply_to":"1a95cdbc_52ef4b1d","updated":"2016-10-10 09:36:31.000000000","message":"I take your point, and this is actually how I initially wrote it. However, I noticed when implementing it for 3 backends that I was pulling the same info out of connection_info in the same way for each, and decided it pull it in here. It means the caller has only 1 LOC instead of 4. I\u0027ll have a think.","commit_id":"f7d42d3efa4100084a8e497f7f4fd851d2c94c64"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"da775b0048188f08d593d2ff792e35d4fc659946","unresolved":false,"context_lines":[{"line_number":78,"context_line":"                    yield mount"},{"line_number":79,"context_line":"                    break"},{"line_number":80,"context_line":""},{"line_number":81,"context_line":"    def mount(self, driver, connection_info):"},{"line_number":82,"context_line":"        export \u003d connection_info[\u0027data\u0027][\u0027export\u0027]"},{"line_number":83,"context_line":"        vol_name \u003d connection_info[\u0027data\u0027][\u0027name\u0027]"},{"line_number":84,"context_line":"        mountpoint \u003d self._get_mount_path(connection_info)"}],"source_content_type":"text/x-python","patch_set":1,"id":"1a95cdbc_0d2f6688","line":81,"in_reply_to":"1a95cdbc_f2c3f735","updated":"2016-10-10 09:43:27.000000000","message":"You could still have some helpers in LibvirtBaseFileSystemVolumeDriver to reduce the duplication there.","commit_id":"f7d42d3efa4100084a8e497f7f4fd851d2c94c64"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"570a2424ae5b7471c3b1a5c38bea58e63681b517","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                #   volume which isn\u0027t mounted because the host just rebooted."},{"line_number":51,"context_line":"                # and this is good enough. We could probably do better here"},{"line_number":52,"context_line":"                # with more thought."},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"                mountpoint \u003d os.path.dirname(disk.source_path)"},{"line_number":55,"context_line":"                if not os.path.ismount(mountpoint):"},{"line_number":56,"context_line":"                    continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa6399be_93ce7d2c","line":53,"updated":"2016-10-13 17:56:08.000000000","message":"You\u0027re likely correct here but isn\u0027t the export two levels up from source_path? I don\u0027t have an env to hand but I thought there was an additional directory in between the export and actual disk file here.","commit_id":"fce2dc12f1039e3c56b0cff3be151a7342b8cfec"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"6d9bd46421ad548dafd2208b79dcb2ef863e3e92","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                #   volume which isn\u0027t mounted because the host just rebooted."},{"line_number":51,"context_line":"                # and this is good enough. We could probably do better here"},{"line_number":52,"context_line":"                # with more thought."},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"                mountpoint \u003d os.path.dirname(disk.source_path)"},{"line_number":55,"context_line":"                if not os.path.ismount(mountpoint):"},{"line_number":56,"context_line":"                    continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"da6895a0_6f309157","line":53,"in_reply_to":"da6895a0_8f6ec5c7","updated":"2016-10-21 16:00:36.000000000","message":"Yeah it doesn\u0027t, apologies for the noise, I think I was confused by _get_mount_path and _get_mount_point_base.","commit_id":"fce2dc12f1039e3c56b0cff3be151a7342b8cfec"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"abe22f11a560b9628490b86eb36b837a5332da86","unresolved":false,"context_lines":[{"line_number":50,"context_line":"                #   volume which isn\u0027t mounted because the host just rebooted."},{"line_number":51,"context_line":"                # and this is good enough. We could probably do better here"},{"line_number":52,"context_line":"                # with more thought."},{"line_number":53,"context_line":""},{"line_number":54,"context_line":"                mountpoint \u003d os.path.dirname(disk.source_path)"},{"line_number":55,"context_line":"                if not os.path.ismount(mountpoint):"},{"line_number":56,"context_line":"                    continue"}],"source_content_type":"text/x-python","patch_set":3,"id":"da6895a0_8f6ec5c7","line":53,"in_reply_to":"fa6399be_93ce7d2c","updated":"2016-10-21 15:44:31.000000000","message":"I don\u0027t think so. LibvirtBaseFileSystemVolumeDriver._get_device_path() does:\n\n  mount_path \u003d self._get_mount_path(connection_info)\n  return os.path.join(mount_path, connection_info[\u0027data\u0027][\u0027name\u0027])\n\nSo unless connection_info[\u0027data\u0027][\u0027name\u0027] contains a path separator (sanity check, please!) the device path should be an immediate child of the mount point. The scality driver overrides _get_device_path, but it follows the same pattern.","commit_id":"fce2dc12f1039e3c56b0cff3be151a7342b8cfec"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":14,"context_line":"import contextlib"},{"line_number":15,"context_line":"import logging"},{"line_number":16,"context_line":"import os.path"},{"line_number":17,"context_line":"import six"},{"line_number":18,"context_line":"import threading"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"from oslo_concurrency import processutils"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_0d739781","line":17,"updated":"2016-11-23 21:10:42.000000000","message":"nit: this is a third party lib so it should be grouped with oslo_concurrency.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":14,"context_line":"import contextlib"},{"line_number":15,"context_line":"import logging"},{"line_number":16,"context_line":"import os.path"},{"line_number":17,"context_line":"import six"},{"line_number":18,"context_line":"import threading"},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"from oslo_concurrency import processutils"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_0a835405","line":17,"in_reply_to":"5a74a57a_0d739781","updated":"2016-11-24 16:19:20.000000000","message":"Done","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    class _ManagedMount(object):"},{"line_number":36,"context_line":"        def __init__(self):"},{"line_number":37,"context_line":"            self.lock \u003d threading.Lock()"},{"line_number":38,"context_line":"            self.volumes \u003d []"},{"line_number":39,"context_line":"            self.mounted \u003d False"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def __init__(self, host):"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_cd127f62","line":38,"range":{"start_line":38,"start_character":27,"end_line":38,"end_character":29},"updated":"2016-11-23 21:10:42.000000000","message":"Seems this could be a set but not sure it\u0027s necessary, would only be useful for avoiding duplicates in the list, which shouldn\u0027t happen if the locking is all happening properly.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":35,"context_line":"    class _ManagedMount(object):"},{"line_number":36,"context_line":"        def __init__(self):"},{"line_number":37,"context_line":"            self.lock \u003d threading.Lock()"},{"line_number":38,"context_line":"            self.volumes \u003d []"},{"line_number":39,"context_line":"            self.mounted \u003d False"},{"line_number":40,"context_line":""},{"line_number":41,"context_line":"    def __init__(self, host):"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_f7bfe55d","line":38,"range":{"start_line":38,"start_character":27,"end_line":38,"end_character":29},"in_reply_to":"5a74a57a_cd127f62","updated":"2016-11-24 16:19:20.000000000","message":"As it happens, we explicitly want duplicates in the list (this came up in a review of an earlier attempt to fix this). It means that this code supports multi-attach, even if Nova doesn\u0027t yet. When we call mount.volumes.remove() in umount() it will only remove the first entry in the list, so if a volume is mounted multiple times it will only be removed when the last instance is unmounted.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":45,"context_line":"            for disk in guest.get_all_disks():"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"                # All remote filesystem volumes are files"},{"line_number":48,"context_line":"                if disk.type !\u003d \u0027file\u0027:"},{"line_number":49,"context_line":"                    continue"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"                # NOTE(mdbooth): We\u0027re assuming that the mountpoint is our"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_104c9041","line":48,"range":{"start_line":48,"start_character":24,"end_line":48,"end_character":28},"updated":"2016-11-23 21:10:42.000000000","message":"Shouldn\u0027t this be source_type? See:\n\nhttps://review.openstack.org/#/c/383859/11/nova/virt/libvirt/volume/nfs.py@38","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":45,"context_line":"            for disk in guest.get_all_disks():"},{"line_number":46,"context_line":""},{"line_number":47,"context_line":"                # All remote filesystem volumes are files"},{"line_number":48,"context_line":"                if disk.type !\u003d \u0027file\u0027:"},{"line_number":49,"context_line":"                    continue"},{"line_number":50,"context_line":""},{"line_number":51,"context_line":"                # NOTE(mdbooth): We\u0027re assuming that the mountpoint is our"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_4adc6ce4","line":48,"range":{"start_line":48,"start_character":24,"end_line":48,"end_character":28},"in_reply_to":"5a74a57a_104c9041","updated":"2016-11-24 16:19:20.000000000","message":"This object is actually a LibvirtConfigGuestDisk, not a LibvirtNFSVolumeDriver, but still yes. (I hate python: this just shouldn\u0027t be possible in a modern language.)\n\nIt\u0027s tested, but I\u0027ve made the same error in the tests so I didn\u0027t pick it up. I don\u0027t understand why it didn\u0027t fail with an AttributeError in Tempest, though. Given that it looks ok and it doesn\u0027t cause a failure, I\u0027m astounded you spotted it.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":126,"context_line":"                    # retries."},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"                    # Delete while holding lock"},{"line_number":129,"context_line":"                    del self.mountpoints[mountpoint]"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"                    raise"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"                mount.mounted \u003d True"},{"line_number":134,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_ad68e3f6","line":131,"range":{"start_line":129,"start_character":20,"end_line":131,"end_character":25},"updated":"2016-11-23 21:10:42.000000000","message":"Use \"with excutils.save_and_reraise_exception()\" here?","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":126,"context_line":"                    # retries."},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"                    # Delete while holding lock"},{"line_number":129,"context_line":"                    del self.mountpoints[mountpoint]"},{"line_number":130,"context_line":""},{"line_number":131,"context_line":"                    raise"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"                mount.mounted \u003d True"},{"line_number":134,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_e8d62026","line":131,"range":{"start_line":129,"start_character":20,"end_line":131,"end_character":25},"in_reply_to":"5a74a57a_ad68e3f6","updated":"2016-11-24 16:19:20.000000000","message":"The complexity of that method isn\u0027t required here because the del command can\u0027t fail: we created the entry above and we hold the lock so nothing else can delete it from under us. If this ever did happen it would be a straightforward programming error, so we wouldn\u0027t want to hide it.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":136,"context_line":""},{"line_number":137,"context_line":"    def umount(self, vol_name, mountpoint):"},{"line_number":138,"context_line":"        with self._get_locked(mountpoint) as mount:"},{"line_number":139,"context_line":"            # This will raise ValueError if share isn\u0027t in volumes, which is"},{"line_number":140,"context_line":"            # as good an error as any."},{"line_number":141,"context_line":"            mount.volumes.remove(vol_name)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"            if len(mount.volumes) \u003d\u003d 0:"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_b004bc15","line":140,"range":{"start_line":139,"start_character":12,"end_line":140,"end_character":38},"updated":"2016-11-23 21:10:42.000000000","message":"I\u0027m not sure that ValueError on umount is a great error. I\u0027m thinking more along the lines of something like a NovaException with an error message explaining that we can\u0027t umount vol_name because we\u0027re not tracking it as already mounted. I\u0027m not sure what kind of recovery / investigation should be performed by the admin if that\u0027s the case though...unless things got umounted out from under us.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":136,"context_line":""},{"line_number":137,"context_line":"    def umount(self, vol_name, mountpoint):"},{"line_number":138,"context_line":"        with self._get_locked(mountpoint) as mount:"},{"line_number":139,"context_line":"            # This will raise ValueError if share isn\u0027t in volumes, which is"},{"line_number":140,"context_line":"            # as good an error as any."},{"line_number":141,"context_line":"            mount.volumes.remove(vol_name)"},{"line_number":142,"context_line":""},{"line_number":143,"context_line":"            if len(mount.volumes) \u003d\u003d 0:"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_de02fe08","line":140,"range":{"start_line":139,"start_character":12,"end_line":140,"end_character":38},"in_reply_to":"5a74a57a_b004bc15","updated":"2016-11-24 16:19:20.000000000","message":"Thinking about this again, I think we should log it as an ERROR and then continue anyway. We\u0027ve been erroneously asked to unmount something which is not mounted. That\u0027s an error, but we\u0027re already at the desired end state.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"aa8259101fa938dbd36237877caa2f02c23614ec","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"                    # Delete while holding lock"},{"line_number":149,"context_line":"                    del self.mountpoints[mountpoint]"},{"line_number":150,"context_line":"                    self.mounted \u003d False"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"                    utils.execute(\u0027rmdir\u0027, mountpoint)"},{"line_number":153,"context_line":"                except processutils.ProcessExecutionError as ex:"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_f02554ab","line":150,"range":{"start_line":150,"start_character":20,"end_line":150,"end_character":32},"updated":"2016-11-23 21:10:42.000000000","message":"Shouldn\u0027t this be mount.mounted \u003d False? This is setting MountManager.mounted \u003d False.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4c8c644ed96af1bd22336c092a99a2558e8733c0","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"                    # Delete while holding lock"},{"line_number":149,"context_line":"                    del self.mountpoints[mountpoint]"},{"line_number":150,"context_line":"                    self.mounted \u003d False"},{"line_number":151,"context_line":""},{"line_number":152,"context_line":"                    utils.execute(\u0027rmdir\u0027, mountpoint)"},{"line_number":153,"context_line":"                except processutils.ProcessExecutionError as ex:"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a74a57a_c8bc046f","line":150,"range":{"start_line":150,"start_character":20,"end_line":150,"end_character":32},"in_reply_to":"5a74a57a_f02554ab","updated":"2016-11-24 16:19:20.000000000","message":"Eurgh, python :(\n\nThere\u0027s also a race here: we shouldn\u0027t delete the mountpoint until after the rmdir below, or the rmdir could race with creation of the same director from another thread. I\u0027ll fix both things.","commit_id":"d14d3df0c7429d09c29f161cd6c04e93734a0162"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cce0239cb52902face48fc7a99cd2a25720164c8","unresolved":false,"context_lines":[{"line_number":1,"context_line":"#    Licensed under the Apache License, Version 2.0 (the \"License\"); you may"},{"line_number":2,"context_line":"#    not use this file except in compliance with the License. You may obtain"},{"line_number":3,"context_line":"#    a copy of the License at"},{"line_number":4,"context_line":"#"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a6eadb0_1432c9f1","line":1,"updated":"2016-12-13 11:21:04.000000000","message":"Copyright?","commit_id":"663dfe6a5ae0ae1b6080c92a78680f9969924c86"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cce0239cb52902face48fc7a99cd2a25720164c8","unresolved":false,"context_lines":[{"line_number":28,"context_line":"LOG \u003d logging.getLogger(__name__)"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":""},{"line_number":31,"context_line":"class MountManager(object):"},{"line_number":32,"context_line":"    __instance__ \u003d None"},{"line_number":33,"context_line":"    __instance_lock__ \u003d threading.Lock()"},{"line_number":34,"context_line":"    __gen__ \u003d 0"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a6eadb0_b96f4e91","line":31,"updated":"2016-12-13 11:21:04.000000000","message":"I can infer its function from the commit message here, but a docstring explaining the function of this class would be really helpful for future maintainers","commit_id":"663dfe6a5ae0ae1b6080c92a78680f9969924c86"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":14,"context_line":""},{"line_number":15,"context_line":"import collections"},{"line_number":16,"context_line":"import contextlib"},{"line_number":17,"context_line":"import logging"},{"line_number":18,"context_line":"import os.path"},{"line_number":19,"context_line":"import threading"},{"line_number":20,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_df1f7607","line":17,"updated":"2017-02-02 18:02:06.000000000","message":"Why aren\u0027t you using oslo_log?","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":125,"context_line":"            while self.use_count !\u003d 0:"},{"line_number":126,"context_line":"                self.cond.wait()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"},{"line_number":130,"context_line":"            if self.state is None:"},{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_3fa79a50","line":129,"range":{"start_line":128,"start_character":12,"end_line":129,"end_character":23},"updated":"2017-02-02 18:02:06.000000000","message":"How is that possible if the only things that touch the state are reset_state and host_down? And host_down is in the same lock as this code in host_up.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":125,"context_line":"            while self.use_count !\u003d 0:"},{"line_number":126,"context_line":"                self.cond.wait()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"},{"line_number":130,"context_line":"            if self.state is None:"},{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_e0811e4f","line":129,"range":{"start_line":128,"start_character":12,"end_line":129,"end_character":23},"in_reply_to":"3a461143_3fa79a50","updated":"2017-02-03 10:26:08.000000000","message":"cond.wait() releases the lock while it waits and reacquires it when it finishes, which is why it\u0027s pretty much always code smell if it\u0027s not in a loop.\n\nThat said, it would be pretty hard to hit this in normal operation because it would require us to have stacked calls to host_up(). You could probably construct a way for this to occur if the libvirt connection was bouncing, but that\u0027s the purview of the driver module. The approach I\u0027ve taken here is that *this* module is predictable regardless of how it is called, so we handle this.\n\nSo lets say there\u0027s a thread busy doing a mount operation which is going to take a few seconds. While this mount is ongoing, use_count is at least 1. During this time we get a call to host_down. We now have state \u003d\u003d None, but use_count \u003e\u003d 1 while that mount using the old state is still ongoing. Before the mount completes, we have *2* calls to host_up. Both calls find that state \u003d\u003d None and proceed, but they wait in the loop above for use count to reach zero. When the mount completes, the thread notifies the condition, and the 2 waiting host_up calls race to wake up. One of them wins, re-takes the condition lock, finds that state is still None, and initialises it. When it completes it releases the lock again so the other thread can also continue. This thread finds that the state has already been initialised, so does nothing.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"},{"line_number":133,"context_line":"                self.state \u003d _HostMountState(host, self.generation)"},{"line_number":134,"context_line":"                self.generation +\u003d 1"},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"    def host_down(self):"},{"line_number":137,"context_line":"        \"\"\"Destroy the current _HostMountState when the libvirt connection"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_62d26936","line":134,"updated":"2017-02-02 18:02:06.000000000","message":"As far as I can tell from the logs in:\n\nhttp://logs.openstack.org/59/383859/13/experimental/gate-tempest-dsvm-full-devstack-plugin-nfs-nv/d22eff3/logs/screen-n-cpu.txt.gz\n\nWe never get anything but 0 here, presumably because we don\u0027t hit host_up more than once.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"},{"line_number":133,"context_line":"                self.state \u003d _HostMountState(host, self.generation)"},{"line_number":134,"context_line":"                self.generation +\u003d 1"},{"line_number":135,"context_line":""},{"line_number":136,"context_line":"    def host_down(self):"},{"line_number":137,"context_line":"        \"\"\"Destroy the current _HostMountState when the libvirt connection"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_6020ce4a","line":134,"in_reply_to":"3a461143_62d26936","updated":"2017-02-03 10:26:08.000000000","message":"Yeah, generation would only be useful if we were trying to debug weirdness when the libvirt connection was bouncing and we had multiple generations of _HostMountState.\n\nThis module intends to handle any amount of bouncing safely, and there are obviously no bugs in my code. Purely for the sake of completeness, though, I thought this might make hypothetical bugs slightly easier to find.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    def _host_down(self):"},{"line_number":149,"context_line":"        LOG.debug(\u0027Destroying MountManager generation %(gen)i\u0027,"},{"line_number":150,"context_line":"                  {\u0027gen\u0027: self.state.generation})"},{"line_number":151,"context_line":"        self.state \u003d None"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_bf5e2a74","line":150,"range":{"start_line":150,"start_character":26,"end_line":150,"end_character":36},"updated":"2017-02-02 18:02:06.000000000","message":"nit: might be extra safe to check if self.state is not None before logging this. It\u0027s not a problem right now since everything calling this checks self.state first, but for future guarding. Although, one could argue that we might not want to guard that and if it blows up, we want to see the programming error.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":147,"context_line":""},{"line_number":148,"context_line":"    def _host_down(self):"},{"line_number":149,"context_line":"        LOG.debug(\u0027Destroying MountManager generation %(gen)i\u0027,"},{"line_number":150,"context_line":"                  {\u0027gen\u0027: self.state.generation})"},{"line_number":151,"context_line":"        self.state \u003d None"},{"line_number":152,"context_line":""},{"line_number":153,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_a020f6f8","line":150,"range":{"start_line":150,"start_character":26,"end_line":150,"end_character":36},"in_reply_to":"3a461143_bf5e2a74","updated":"2017-02-03 10:26:08.000000000","message":"Thinking myself in knots about this.\n\nOn balance, I think because it\u0027s a very small local abstraction I wouldn\u0027t guard it.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            # if the containing _MountPoint is deleted."},{"line_number":181,"context_line":"            self.lock \u003d threading.Lock()"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"            # The list of volumes in use on this mountpoint."},{"line_number":184,"context_line":"            # NOTE(mdbooth): Although Nova does not yet support it, this class"},{"line_number":185,"context_line":"            # intentionally supports a single volume being in use multiple"},{"line_number":186,"context_line":"            # times. Consequently we use a list here rather than a set."}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_428d6d7a","line":183,"updated":"2017-02-02 18:02:06.000000000","message":"nit: are these objects, or volume ids, or other?","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":180,"context_line":"            # if the containing _MountPoint is deleted."},{"line_number":181,"context_line":"            self.lock \u003d threading.Lock()"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"            # The list of volumes in use on this mountpoint."},{"line_number":184,"context_line":"            # NOTE(mdbooth): Although Nova does not yet support it, this class"},{"line_number":185,"context_line":"            # intentionally supports a single volume being in use multiple"},{"line_number":186,"context_line":"            # times. Consequently we use a list here rather than a set."}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_60544e54","line":183,"in_reply_to":"3a461143_428d6d7a","updated":"2017-02-03 10:26:08.000000000","message":"Good point. I think I\u0027ll have to update anyway, so I\u0027ll improve this.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"fd8c1a7844b98aaf3b3ce032479c90dd85013f76","unresolved":false,"context_lines":[{"line_number":260,"context_line":"                        passed to the mount command immediate before export"},{"line_number":261,"context_line":"                        and mountpoint."},{"line_number":262,"context_line":"        \"\"\""},{"line_number":263,"context_line":"        LOG.debug(\u0027_HostMountState.mount(fstype\u003d%(fstype)s, \u0027"},{"line_number":264,"context_line":"                  \u0027export\u003d%(export)s, vol_name\u003d%(vol_name)s, %(mountpoint)s, \u0027"},{"line_number":265,"context_line":"                  \u0027options\u003d%(options)s) generation %(gen)s\u0027,"},{"line_number":266,"context_line":"                  {\u0027fstype\u0027: fstype, \u0027export\u0027: export, \u0027vol_name\u0027: vol_name,"},{"line_number":267,"context_line":"                   \u0027mountpoint\u0027: mountpoint, \u0027options\u0027: options,"},{"line_number":268,"context_line":"                   \u0027gen\u0027: self.generation})"},{"line_number":269,"context_line":"        with self._get_locked(mountpoint) as mount:"},{"line_number":270,"context_line":"            if not os.path.ismount(mountpoint):"},{"line_number":271,"context_line":"                LOG.debug(\u0027Mounting %(mountpoint)s generation %(gen)s\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_5d124058","line":268,"range":{"start_line":263,"start_character":8,"end_line":268,"end_character":43},"updated":"2017-01-17 15:22:09.000000000","message":"I know this style of logging is hated by some but I genuinely find it useful so thanks!","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"8045ec81b374477229bc6db18f6ea901a6049b46","unresolved":false,"context_lines":[{"line_number":260,"context_line":"                        passed to the mount command immediate before export"},{"line_number":261,"context_line":"                        and mountpoint."},{"line_number":262,"context_line":"        \"\"\""},{"line_number":263,"context_line":"        LOG.debug(\u0027_HostMountState.mount(fstype\u003d%(fstype)s, \u0027"},{"line_number":264,"context_line":"                  \u0027export\u003d%(export)s, vol_name\u003d%(vol_name)s, %(mountpoint)s, \u0027"},{"line_number":265,"context_line":"                  \u0027options\u003d%(options)s) generation %(gen)s\u0027,"},{"line_number":266,"context_line":"                  {\u0027fstype\u0027: fstype, \u0027export\u0027: export, \u0027vol_name\u0027: vol_name,"},{"line_number":267,"context_line":"                   \u0027mountpoint\u0027: mountpoint, \u0027options\u0027: options,"},{"line_number":268,"context_line":"                   \u0027gen\u0027: self.generation})"},{"line_number":269,"context_line":"        with self._get_locked(mountpoint) as mount:"},{"line_number":270,"context_line":"            if not os.path.ismount(mountpoint):"},{"line_number":271,"context_line":"                LOG.debug(\u0027Mounting %(mountpoint)s generation %(gen)s\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_737f2bab","line":268,"range":{"start_line":263,"start_character":8,"end_line":268,"end_character":43},"in_reply_to":"7a3c09a3_5d124058","updated":"2017-01-17 15:38:19.000000000","message":"Indeed. I added it here explicitly in response to troubles we had debugging a related issue.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"fd8c1a7844b98aaf3b3ce032479c90dd85013f76","unresolved":false,"context_lines":[{"line_number":267,"context_line":"                   \u0027mountpoint\u0027: mountpoint, \u0027options\u0027: options,"},{"line_number":268,"context_line":"                   \u0027gen\u0027: self.generation})"},{"line_number":269,"context_line":"        with self._get_locked(mountpoint) as mount:"},{"line_number":270,"context_line":"            if not os.path.ismount(mountpoint):"},{"line_number":271,"context_line":"                LOG.debug(\u0027Mounting %(mountpoint)s generation %(gen)s\u0027,"},{"line_number":272,"context_line":"                          {\u0027mountpoint\u0027: mountpoint, \u0027gen\u0027: self.generation})"},{"line_number":273,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_1d5d1879","line":270,"range":{"start_line":270,"start_character":12,"end_line":270,"end_character":47},"updated":"2017-01-17 15:22:09.000000000","message":"As below, eventually it might be nice to ensure that the expected export is mounted here.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"8d5604fa861caae9cd1f104f42b564bd613f6998","unresolved":false,"context_lines":[{"line_number":283,"context_line":"                except Exception:"},{"line_number":284,"context_line":"                    # Check to see if mountpoint is mounted despite the error"},{"line_number":285,"context_line":"                    # eg it was already mounted"},{"line_number":286,"context_line":"                    if os.path.ismount(mountpoint):"},{"line_number":287,"context_line":"                        # We\u0027re not going to raise the exception because we\u0027re"},{"line_number":288,"context_line":"                        # in the desired state anyway. However, this is still"},{"line_number":289,"context_line":"                        # unusual so we\u0027ll log it."}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_4cae14e7","line":286,"range":{"start_line":286,"start_character":0,"end_line":286,"end_character":51},"updated":"2017-01-17 14:16:54.000000000","message":"More of a nit but do you want to use my rewritten libvirt_utils.is_mounted check here to confirm this mountpoint is using the export [1]?\n\n[1] https://review.openstack.org/#/c/420146/","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"fd8c1a7844b98aaf3b3ce032479c90dd85013f76","unresolved":false,"context_lines":[{"line_number":283,"context_line":"                except Exception:"},{"line_number":284,"context_line":"                    # Check to see if mountpoint is mounted despite the error"},{"line_number":285,"context_line":"                    # eg it was already mounted"},{"line_number":286,"context_line":"                    if os.path.ismount(mountpoint):"},{"line_number":287,"context_line":"                        # We\u0027re not going to raise the exception because we\u0027re"},{"line_number":288,"context_line":"                        # in the desired state anyway. However, this is still"},{"line_number":289,"context_line":"                        # unusual so we\u0027ll log it."}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_c2956b33","line":286,"range":{"start_line":286,"start_character":0,"end_line":286,"end_character":51},"in_reply_to":"7a3c09a3_3f71ec76","updated":"2017-01-17 15:22:09.000000000","message":"Yup, that\u0027s a valid point that I\u0027ll raise and look into in the other review.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"b6abfc6bd1a011bd0c2bf58ce32b631d1e466c14","unresolved":false,"context_lines":[{"line_number":283,"context_line":"                except Exception:"},{"line_number":284,"context_line":"                    # Check to see if mountpoint is mounted despite the error"},{"line_number":285,"context_line":"                    # eg it was already mounted"},{"line_number":286,"context_line":"                    if os.path.ismount(mountpoint):"},{"line_number":287,"context_line":"                        # We\u0027re not going to raise the exception because we\u0027re"},{"line_number":288,"context_line":"                        # in the desired state anyway. However, this is still"},{"line_number":289,"context_line":"                        # unusual so we\u0027ll log it."}],"source_content_type":"text/x-python","patch_set":13,"id":"7a3c09a3_3f71ec76","line":286,"range":{"start_line":286,"start_character":0,"end_line":286,"end_character":51},"in_reply_to":"7a3c09a3_4cae14e7","updated":"2017-01-17 14:22:49.000000000","message":"Good idea, but I don\u0027t want to add the dependency right now. I\u0027ll update if yours lands first, or patch later if this lands first.\n\nIncidentally, what would you do if they didn\u0027t match? The behaviour would be different to what you\u0027d do if it wasn\u0027t mounted. Your is_mounted currently returns False in both cases, but thinking about it that\u0027s not correct if it *is* mounted, but incorrectly. Maybe it should raise an exception?","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":304,"context_line":""},{"line_number":305,"context_line":"                        raise"},{"line_number":306,"context_line":""},{"line_number":307,"context_line":"            mount.volumes.append(vol_name)"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        LOG.debug(\u0027_HostMountState.mount() for %(mountpoint)s \u0027"},{"line_number":310,"context_line":"                  \u0027generation %(gen)s completed successfully\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_a23a91e5","line":307,"updated":"2017-02-02 18:02:06.000000000","message":"So when looking at this:\n\nhttp://logs.openstack.org/59/383859/13/experimental/gate-tempest-dsvm-full-devstack-plugin-nfs-nv/d22eff3/logs/screen-n-cpu.txt.gz#_2017-01-20_14_14_48_232\n\nThe vol_name is volume-\u003cuuid\u003e:\n\nvol_name\u003dvolume-d74a57bd-5c73-4c3e-be8f-b635671c6eb5\n\nThat won\u0027t match what\u0027s put into mount.volumes on L227 will it? Because that one will be a path it looks like.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"4b23313ed54e2ed9b7d09b62ae687b672104c39f","unresolved":false,"context_lines":[{"line_number":304,"context_line":""},{"line_number":305,"context_line":"                        raise"},{"line_number":306,"context_line":""},{"line_number":307,"context_line":"            mount.volumes.append(vol_name)"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        LOG.debug(\u0027_HostMountState.mount() for %(mountpoint)s \u0027"},{"line_number":310,"context_line":"                  \u0027generation %(gen)s completed successfully\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_90a09b92","line":307,"in_reply_to":"1a430d35_60252ec1","updated":"2017-02-07 16:58:35.000000000","message":"Now that I come to fix this I notice that the previous code is doing os.path.relpath(mountpoint, volume). Given the constraints described in the comment block above, this is identical to os.path.basename(volume), so it\u0027s actually ok.\n\nHowever, given that we both misread it and I have to update anyway I\u0027m going to change that. I\u0027m also going to add some extra debug logging there.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":304,"context_line":""},{"line_number":305,"context_line":"                        raise"},{"line_number":306,"context_line":""},{"line_number":307,"context_line":"            mount.volumes.append(vol_name)"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        LOG.debug(\u0027_HostMountState.mount() for %(mountpoint)s \u0027"},{"line_number":310,"context_line":"                  \u0027generation %(gen)s completed successfully\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_60252ec1","line":307,"in_reply_to":"3a461143_a23a91e5","updated":"2017-02-03 10:26:08.000000000","message":"Nice catch! The initialisation should use volume name rather than full path. Although it would otherwise be safe, I think this would prevent a mountpoint from ever being unmounted if it was already mounted with nova compute came up. That\u0027s pretty bad.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f867355c4d6f93399b837c3e309dcf98a4c3818d","unresolved":false,"context_lines":[{"line_number":326,"context_line":"            try:"},{"line_number":327,"context_line":"                mount.volumes.remove(vol_name)"},{"line_number":328,"context_line":"            except ValueError:"},{"line_number":329,"context_line":"                LOG.error(_LE(\"Request to unmount %(vol_name)s from \""},{"line_number":330,"context_line":"                              \"%(mountpoint)s, but we don\u0027t think it\u0027s in \""},{"line_number":331,"context_line":"                              \"use.\"),"},{"line_number":332,"context_line":"                          {\u0027vol_name\u0027: vol_name, \u0027mountpoint\u0027: mountpoint})"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"            if len(mount.volumes) \u003d\u003d 0:"},{"line_number":335,"context_line":"                LOG.debug(\u0027Unmounting %(mountpoint)s generation %(gen)s\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"3a461143_bfeb8aaf","line":332,"range":{"start_line":329,"start_character":16,"end_line":332,"end_character":75},"updated":"2017-02-02 18:02:06.000000000","message":"I\u0027m seeing this in the NFS job run here:\n\nhttp://logs.openstack.org/59/383859/13/experimental/gate-tempest-dsvm-full-devstack-plugin-nfs-nv/d22eff3/logs/screen-n-cpu.txt.gz?level\u003dTRACE#_2017-01-20_14_18_19_144\n\nIf this doesn\u0027t cause a failure, should it be a warning instead of an error?\n\nAlternatively, is this possibly related to the uuid/path thing I\u0027m asking about above?","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"1f8cb024af9c18c9cf0af5b9d5a195c98acfa98f","unresolved":false,"context_lines":[{"line_number":326,"context_line":"            try:"},{"line_number":327,"context_line":"                mount.volumes.remove(vol_name)"},{"line_number":328,"context_line":"            except ValueError:"},{"line_number":329,"context_line":"                LOG.error(_LE(\"Request to unmount %(vol_name)s from \""},{"line_number":330,"context_line":"                              \"%(mountpoint)s, but we don\u0027t think it\u0027s in \""},{"line_number":331,"context_line":"                              \"use.\"),"},{"line_number":332,"context_line":"                          {\u0027vol_name\u0027: vol_name, \u0027mountpoint\u0027: mountpoint})"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"            if len(mount.volumes) \u003d\u003d 0:"},{"line_number":335,"context_line":"                LOG.debug(\u0027Unmounting %(mountpoint)s generation %(gen)s\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_15873d62","line":332,"range":{"start_line":329,"start_character":16,"end_line":332,"end_character":75},"in_reply_to":"1a430d35_c02c22ae","updated":"2017-02-03 15:35:44.000000000","message":"I\u0027ve got to the bottom of this. As I noted in a comment, and I\u0027ve even got a test for it, I explicitly wanted to support multiple uses of a single volume. In doing this I made a basic assumption: we call mount()/umount() exactly once each per attach()/detach(). However, this assumption is invalid because various operations which restart a stopped instance simply call mount() again (via driver._hard_reboot()), even though the expectation should be that it is already mounted.\n\nThis means that we fundamentally can\u0027t manage multi-attach at this level because we don\u0027t have enough information. This is mildly annoying, but given that Nova doesn\u0027t do it anyway this isn\u0027t a great loss.\n\nThere\u0027s a simple fix, which is just to use a set instead of a list for the volume list. I\u0027ll make this change and also remove the multi-attach test. I\u0027ll also add a comment for posterity about the potential for multiple mount() calls as it\u0027s slightly surprising.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"48d9dc825ed5667f679dc2fc40e10b327dcb2633","unresolved":false,"context_lines":[{"line_number":326,"context_line":"            try:"},{"line_number":327,"context_line":"                mount.volumes.remove(vol_name)"},{"line_number":328,"context_line":"            except ValueError:"},{"line_number":329,"context_line":"                LOG.error(_LE(\"Request to unmount %(vol_name)s from \""},{"line_number":330,"context_line":"                              \"%(mountpoint)s, but we don\u0027t think it\u0027s in \""},{"line_number":331,"context_line":"                              \"use.\"),"},{"line_number":332,"context_line":"                          {\u0027vol_name\u0027: vol_name, \u0027mountpoint\u0027: mountpoint})"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"            if len(mount.volumes) \u003d\u003d 0:"},{"line_number":335,"context_line":"                LOG.debug(\u0027Unmounting %(mountpoint)s generation %(gen)s\u0027,"}],"source_content_type":"text/x-python","patch_set":13,"id":"1a430d35_c02c22ae","line":332,"range":{"start_line":329,"start_character":16,"end_line":332,"end_character":75},"in_reply_to":"3a461143_bfeb8aaf","updated":"2017-02-03 10:26:08.000000000","message":"Going to look into this in detail.","commit_id":"64caff7ca9ab5fe2404c032ff4928271fc6f0f93"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"b1381bb0771833255319210a326b8645f8145b7a","unresolved":false,"context_lines":[{"line_number":295,"context_line":"                        # We\u0027re not going to raise the exception because we\u0027re"},{"line_number":296,"context_line":"                        # in the desired state anyway. However, this is still"},{"line_number":297,"context_line":"                        # unusual so we\u0027ll log it."},{"line_number":298,"context_line":"                        LOG.exception(_LE(\u0027Error mounting %(fstype)s export \u0027"},{"line_number":299,"context_line":"                                          \u0027%(export)s on %(mountpoint)s. \u0027"},{"line_number":300,"context_line":"                                          \u0027Continuing because mountpount is \u0027"},{"line_number":301,"context_line":"                                          \u0027mounted despite this.\u0027),"},{"line_number":302,"context_line":"                                      {\u0027fstype\u0027: fstype, \u0027export\u0027: export,"},{"line_number":303,"context_line":"                                       \u0027mountpoint\u0027: mountpoint})"},{"line_number":304,"context_line":""},{"line_number":305,"context_line":"                    else:"},{"line_number":306,"context_line":"                        # If the mount failed there\u0027s no reason for us to keep"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a430d35_b6140e4d","line":303,"range":{"start_line":298,"start_character":0,"end_line":303,"end_character":65},"updated":"2017-02-08 11:39:31.000000000","message":"As discussed, we should drop this down to a warning as races between detach and delete as well as rebuilds can hit this without causing wider issues.","commit_id":"6f448cc4caae4583c985d736b0dc0ac32581919d"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"5054ecba0a0619dbb4ccf5719a1b89a9cd972537","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                                \"already up\"))"},{"line_number":116,"context_line":"                self._host_down()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"            # Wait until all operations using a previous state generation are"},{"line_number":119,"context_line":"            # complete before initialising a new one. Note that self.state is"},{"line_number":120,"context_line":"            # already None, set either by initialisation or by host_down. This"},{"line_number":121,"context_line":"            # means the current state will not be returned to any new callers,"},{"line_number":122,"context_line":"            # and use_count will eventually reach zero."},{"line_number":123,"context_line":"            # We do this to avoid a race between _HostMountState initialisation"},{"line_number":124,"context_line":"            # and an on-going mount/unmount operation"},{"line_number":125,"context_line":"            while self.use_count !\u003d 0:"},{"line_number":126,"context_line":"                self.cond.wait()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_2e9a706e","line":126,"range":{"start_line":118,"start_character":36,"end_line":126,"end_character":32},"updated":"2017-02-08 19:48:26.000000000","message":"Is it possible this could block indefinitely on a hung mount/unmount operation?","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0e2707d2b01c4827ca42eeeca548d7a11998d8e4","unresolved":false,"context_lines":[{"line_number":115,"context_line":"                                \"already up\"))"},{"line_number":116,"context_line":"                self._host_down()"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"            # Wait until all operations using a previous state generation are"},{"line_number":119,"context_line":"            # complete before initialising a new one. Note that self.state is"},{"line_number":120,"context_line":"            # already None, set either by initialisation or by host_down. This"},{"line_number":121,"context_line":"            # means the current state will not be returned to any new callers,"},{"line_number":122,"context_line":"            # and use_count will eventually reach zero."},{"line_number":123,"context_line":"            # We do this to avoid a race between _HostMountState initialisation"},{"line_number":124,"context_line":"            # and an on-going mount/unmount operation"},{"line_number":125,"context_line":"            while self.use_count !\u003d 0:"},{"line_number":126,"context_line":"                self.cond.wait()"},{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_522904e5","line":126,"range":{"start_line":118,"start_character":36,"end_line":126,"end_character":32},"in_reply_to":"1a430d35_2e9a706e","updated":"2017-02-09 09:44:51.000000000","message":"Yes. However, I\u0027m not sure what other action would be correct here. If we wanted to gold-plate it we could stick a timeout here and add a log every minute or so?\n\nI like that idea, actually. I\u0027ll add a follow-on patch, or update here if I have to change it.","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":8768,"name":"Chris Friesen","email":"chris.friesen@windriver.com","username":"cbf123"},"change_message_id":"5054ecba0a0619dbb4ccf5719a1b89a9cd972537","unresolved":false,"context_lines":[{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"},{"line_number":130,"context_line":"            if self.state is None:"},{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"},{"line_number":133,"context_line":"                self.state \u003d _HostMountState(host, self.generation)"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_d1fc2dba","line":130,"range":{"start_line":130,"start_character":12,"end_line":130,"end_character":34},"updated":"2017-02-08 19:48:26.000000000","message":"How could this ever be false?  We\u0027re holding self.cond, and if self.state was not None we\u0027ve already called self._host_down() which sets it to None.","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":9555,"name":"Matthew Booth","email":"mbooth@redhat.com","username":"MatthewBooth"},"change_message_id":"0e2707d2b01c4827ca42eeeca548d7a11998d8e4","unresolved":false,"context_lines":[{"line_number":127,"context_line":""},{"line_number":128,"context_line":"            # Another thread might have initialised state while we were"},{"line_number":129,"context_line":"            # wait()ing"},{"line_number":130,"context_line":"            if self.state is None:"},{"line_number":131,"context_line":"                LOG.debug(\u0027Initialising _HostMountState generation %(gen)i\u0027,"},{"line_number":132,"context_line":"                          {\u0027gen\u0027: self.generation})"},{"line_number":133,"context_line":"                self.state \u003d _HostMountState(host, self.generation)"}],"source_content_type":"text/x-python","patch_set":17,"id":"1a430d35_f27e70e2","line":130,"range":{"start_line":130,"start_character":12,"end_line":130,"end_character":34},"in_reply_to":"1a430d35_d1fc2dba","updated":"2017-02-09 09:44:51.000000000","message":"We cond.wait() above, which drops the lock while it\u0027s sleeping. We can\u0027t strictly assume anything about our state immediately after a wait.\n\nImagine a bizarre situation in which weirdness in LibvirtDriver causes 2 host_up() calls to stack. I haven\u0027t actually worked out if this is even possible, but I suspect it might be if the connection were bouncing or something, and you got a really fast sequence of up/down/up/down/...\n\nAnyway, the critical feature is that rather than trying to convince myself that:\n\n a) This cannot occur\n b) Future code changes will never allow it to occur\n\nI just handled it here :)","commit_id":"b7a07397917cb1e8e24fefb3f70cb3ee9b065cd0"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"181b0f69d91dd46ae83ca68859ae7c0487212859","unresolved":false,"context_lines":[{"line_number":185,"context_line":"            # The set of attachments on this mountpoint."},{"line_number":186,"context_line":"            self.attachments \u003d set()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        def add_attachment(self, vol_name, instance_uuid):"},{"line_number":189,"context_line":"            self.attachments.add((vol_name, instance_uuid))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        def remove_attachment(self, vol_name, instance_uuid):"},{"line_number":192,"context_line":"            self.attachments.remove((vol_name, instance_uuid))"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"        def in_use(self):"},{"line_number":195,"context_line":"            return len(self.attachments) \u003e 0"}],"source_content_type":"text/x-python","patch_set":19,"id":"ba2be162_2e6a531b","line":192,"range":{"start_line":188,"start_character":8,"end_line":192,"end_character":62},"updated":"2017-02-28 14:17:11.000000000","message":"At what point are you using the instance to tell if you can disconnect the volume?","commit_id":"09150a4e62654c839bb487ee5c653712bd22155b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"e552b5102beb0e12f492e34258913601e57f880b","unresolved":false,"context_lines":[{"line_number":185,"context_line":"            # The set of attachments on this mountpoint."},{"line_number":186,"context_line":"            self.attachments \u003d set()"},{"line_number":187,"context_line":""},{"line_number":188,"context_line":"        def add_attachment(self, vol_name, instance_uuid):"},{"line_number":189,"context_line":"            self.attachments.add((vol_name, instance_uuid))"},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"        def remove_attachment(self, vol_name, instance_uuid):"},{"line_number":192,"context_line":"            self.attachments.remove((vol_name, instance_uuid))"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"        def in_use(self):"},{"line_number":195,"context_line":"            return len(self.attachments) \u003e 0"}],"source_content_type":"text/x-python","patch_set":19,"id":"ba2be162_5c1d7d65","line":192,"range":{"start_line":188,"start_character":8,"end_line":192,"end_character":62},"in_reply_to":"ba2be162_2e6a531b","updated":"2017-02-28 14:40:16.000000000","message":"Nevermind, missed the point of the set. So (volA, instA) and (volA, instB) could be attached, and we can remove (volA, instB) and the mount is still in use because the attachments set still has (volA, instA).","commit_id":"09150a4e62654c839bb487ee5c653712bd22155b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"b03eff7406e90eb30276686bcde02b766d8667fe","unresolved":false,"context_lines":[{"line_number":411,"context_line":"    return __manager__"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":""},{"line_number":414,"context_line":"def mount(fstype, export, vol_name, mountpoint, instance, options\u003dNone):"},{"line_number":415,"context_line":"    \"\"\"A convenience wrapper around _HostMountState.mount(), called via the"},{"line_number":416,"context_line":"    _HostMountStateManager singleton."},{"line_number":417,"context_line":"    \"\"\""},{"line_number":418,"context_line":"    with __manager__.get_state() as mount_state:"},{"line_number":419,"context_line":"        mount_state.mount(fstype, export, vol_name, mountpoint, instance,"},{"line_number":420,"context_line":"                          options)"},{"line_number":421,"context_line":""},{"line_number":422,"context_line":""},{"line_number":423,"context_line":"def umount(vol_name, mountpoint, instance):"}],"source_content_type":"text/x-python","patch_set":19,"id":"9a30ddce_6cfef36c","line":420,"range":{"start_line":414,"start_character":0,"end_line":420,"end_character":34},"updated":"2017-03-07 14:34:50.000000000","message":"nice addition","commit_id":"09150a4e62654c839bb487ee5c653712bd22155b"}],"nova/virt/libvirt/volume/nfs.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"cce0239cb52902face48fc7a99cd2a25720164c8","unresolved":false,"context_lines":[{"line_number":24,"context_line":"class LibvirtNFSVolumeDriver(fs.LibvirtMountedFileSystemVolumeDriver):"},{"line_number":25,"context_line":"    \"\"\"Class implements libvirt part of volume driver for NFS.\"\"\""},{"line_number":26,"context_line":""},{"line_number":27,"context_line":"    def __init__(self, connection):"},{"line_number":28,"context_line":"        super(LibvirtNFSVolumeDriver, self).__init__(connection, \u0027nfs\u0027)"},{"line_number":29,"context_line":""},{"line_number":30,"context_line":"    def _get_mount_point_base(self):"}],"source_content_type":"text/x-python","patch_set":12,"id":"1a6eadb0_2f821053","line":27,"range":{"start_line":27,"start_character":23,"end_line":27,"end_character":33},"updated":"2016-12-13 11:21:04.000000000","message":"nit: Could we call this host to keep the signature similar to the base class?","commit_id":"663dfe6a5ae0ae1b6080c92a78680f9969924c86"}]}