Merge lp:~blake-rouse/maas/fix-1627362 into lp:~maas-committers/maas/trunk

[Blake Rouse]

Fix discovery websocket and factory to use first_seen as the batch key. Convert first_key to a timestamp with microseconds to ensure always unique.

[Andres Rodriguez (andreserl)]

lgtm! But maybe mike should review prior to landing.

review: Approve

[Mike Pontillo (mpontillo)]

There is a subtle problem with using the first_seen value as a primary key. (If we were going to do that, it would have been just as easy to use the .id field, which is backed by the auto-incrementing primary key field in the maasserver_neighbour table.)

The discovery_id was intended to uniquely identify a (ip, mac) pair. Importantly, only one (IP, MAC) pair (the most recently seen, over the set of all rack interfaces) will ever appear in the maasserver_discovery view at a time.

Then, the subtle flaw in using the first_seen value (or the id, for that matter, which will also always be unique) is that over time, the discovery view will change, especially in an environment with multiple rack controllers observing the same subnet(s). So the discovery_id was intended as a composite key to identify the most recent information for the observed (ip, mac). The view only ever selects one unique (ip, mac) at a time, so the discovery_id will be unique, in that every time you select from the view, you'll never see a duplicate. (though it might have different data, such as which rack observed it, what time it was last observed, etc.)

So, as an example, here's the problem with using first_seen (or id) as the primary key:

00:00:00 - Web UI opens, user views dashboard
00:01:00 - Rack A discovers (192.168.0.1, 01:02:03:04:05:06)
00:02:00 - Rack B discovers (192.168.0.1, 01:02:03:04:05:06)

So at first when the user launches the Web UI, the discovered devices list will be empty.

One minute later, when Rack A comes online and makes its discovery, the user will see the discovered IP address in the dashboard.

After another minute, Rack B comes online and reports its own discovery of the same (ip, mac). The discovery view will now prefer to present Rack B's view of the world, since it was seen more recently than Rack A's.

The intended behavior was that the Web UI would see this as an "update" based on seeing the same discovery_id and update the last_seen time to 00:02:00. What will happen with this branch is, the user will see two discoveries in the list, for the same (ip, mac). But only if they leave UI sitting on the dashboard for a long period of time. (Discoveries are generally throttled to one every ten minutes, unless the discovery represents a new or changed device, which could appear as duplicates almost immediately, depending on timing.)

All that said, I'm +1 on this branch since the behavior is much better than crashing. ;-) And I trust your judgement when it comes to the websocket/js code. But I'm also -1 because I would have preferred a simpler solution that doesn't involve changing the intended primary key for the view. So, IMHO, we still have work to do.

review: Abstain

[Blake Rouse (blake-rouse)]

The user will not actually see two discoveries, what you are describing is not an issue. The discovery_id should be distinct in the database view. With that then you will not see duplicates as the view removes the duplicates, hope that is the case as the API would be affect by this as well. Also since this is polling based the array is only replaced once all the items have been reloaded again. So in this case there will only ever be one discovery_id in the array at a time, even if the primary key is first_seen.

[Mike Pontillo (mpontillo)]

Ah, I didn't realize we don't do the "smart replace" with the polling mode. That being the case, I'm more comfortable with this branch. But I think it could still be an issue, if someone is editing a discovery that is subsequently replaced by another rack's observation.

=== modified file 'src/maasserver/static/js/angular/controllers/tests/test_dashboard.js'
--- src/maasserver/static/js/angular/controllers/tests/test_dashboard.js	2016-09-27 14:24:19 +0000
+++ src/maasserver/static/js/angular/controllers/tests/test_dashboard.js	2016-10-05 15:19:04 +0000
@@ -195,7 +195,7 @@
             };
             DomainsManager._items = [defaultDomain];
             var discovered = {
-                discovery_id: id,
+                first_seen: id,
                 hostname: makeName("hostname"),
                 subnet: makeInteger(0, 100)
             };
@@ -226,7 +226,7 @@
             };
             DomainsManager._items = [defaultDomain];
             var discovered = {
-                discovery_id: id,
+                first_seen: id,
                 hostname: makeName("hostname"),
                 subnet: null
             };
@@ -259,7 +259,7 @@
             };
             DomainsManager._items = [defaultDomain];
             var discovered = {
-                discovery_id: id,
+                first_seen: id,
                 hostname: makeName("hostname"),
                 subnet: makeInteger(0, 100),
                 mac_address: makeName("mac"),
@@ -301,7 +301,7 @@
             };
             DomainsManager._items = [defaultDomain];
             var discovered = {
-                discovery_id: id,
+                first_seen: id,
                 hostname: makeName("hostname"),
                 subnet: makeInteger(0, 100),
                 mac_address: makeName("mac"),

=== modified file 'src/maasserver/static/js/angular/factories/discoveries.js'
--- src/maasserver/static/js/angular/factories/discoveries.js	2016-09-22 15:05:38 +0000
+++ src/maasserver/static/js/angular/factories/discoveries.js	2016-10-05 15:19:04 +0000
@@ -21,7 +21,8 @@
         function DiscoveriesManager() {
             PollingManager.call(this);
 
-            this._pk = "discovery_id";
+            this._pk = "first_seen";
+            this._batchKey = "first_seen";
             this._handler = "discovery";
 
             // Poll every 10 seconds when its empty as its okay for

=== modified file 'src/maasserver/static/js/angular/factories/tests/test_discoveries.js'
--- src/maasserver/static/js/angular/factories/tests/test_discoveries.js	2016-09-22 15:05:38 +0000
+++ src/maasserver/static/js/angular/factories/tests/test_discoveries.js	2016-10-05 15:19:04 +0000
@@ -17,7 +17,8 @@
     }));
 
     it("set requires attributes", function() {
-        expect(DiscoveriesManager._pk).toBe("discovery_id");
+        expect(DiscoveriesManager._pk).toBe("first_seen");
+        expect(DiscoveriesManager._batchKey).toBe("first_seen");
         expect(DiscoveriesManager._handler).toBe("discovery");
         expect(DiscoveriesManager._pollEmptyTimeout).toBe(5000);
     });

=== modified file 'src/maasserver/static/partials/dashboard.html'
--- src/maasserver/static/partials/dashboard.html	2016-09-30 19:07:37 +0000
+++ src/maasserver/static/partials/dashboard.html	2016-10-05 15:19:04 +0000
@@ -68,10 +68,10 @@
                         </div>
                     </div>
                     <div class="table__row"
-                        data-ng-repeat="discovery in discoveredDevices track by discovery.discovery_id"
-                        data-ng-class="{'is-active' : discovery.discovery_id === selectedDevice}">
-                        <div data-ng-if="discovery.discovery_id !== selectedDevice"
-                            data-ng-dblclick="toggleSelected(discovery.discovery_id)">
+                        data-ng-repeat="discovery in discoveredDevices | orderBy:'-last_seen' track by discovery.first_seen"
+                        data-ng-class="{'is-active' : discovery.first_seen === selectedDevice}">
+                        <div data-ng-if="discovery.first_seen !== selectedDevice"
+                            data-ng-dblclick="toggleSelected(discovery.first_seen)">
                             <div class="table__data table-col--20">
                                 {$ getDiscoveryName(discovery) $}
                                 <i data-ng-show="discovery.is_external_dhcp === true" class="icon icon--info tooltip u-margin--left-tiny ng-hide" aria-label="This device is providing DHCP."></i>
@@ -93,10 +93,10 @@
                             </div>
                             <div class="table__data table-col--3">
                                 <i class="icon icon--open tooltip" aria-label="Open"
-                                    data-ng-click="toggleSelected(discovery.discovery_id)">Open</i>
+                                    data-ng-click="toggleSelected(discovery.first_seen)">Open</i>
                             </div>
                         </div>
-                        <maas-obj-form data-ng-if="discovery.discovery_id === selectedDevice"
+                        <maas-obj-form data-ng-if="discovery.first_seen === selectedDevice"
                             obj="convertTo" manager="proxyManager" pre-process="preProcess"
                             after-save="afterSave" table-form="true" save-on-blur="false">
                             <div class="table__data table-col--20">
@@ -106,7 +106,7 @@
                             <div class="table__data table-col--77"></div>
                             <div class="table__data table-col--3">
                                 <i class="icon icon--close tooltip" aria-label="Close"
-                                    data-ng-click="toggleSelected(discovery.discovery_id)">Close</i>
+                                    data-ng-click="toggleSelected(discovery.first_seen)">Close</i>
                             </div>
                             <div class="table__dropdown">
                                 <div class="table__row is-active">

=== modified file 'src/maasserver/testing/sampledata.py'
--- src/maasserver/testing/sampledata.py	2016-09-23 18:48:54 +0000
+++ src/maasserver/testing/sampledata.py	2016-10-05 15:19:04 +0000
@@ -22,6 +22,7 @@
     Domain,
     Fabric,
     Node,
+    RackController,
     VersionedTextFile,
 )
 from maasserver.storage_layouts import STORAGE_LAYOUTS
@@ -131,7 +132,6 @@
                 interface=interface)
 
 
-@transactional
 def populate(seed="sampledata"):
     """Populate the database with example data.
 
@@ -158,7 +158,26 @@
 
     """
     random.seed(seed)
-
+    populate_main()
+    for _ in range(120):
+        make_discovery()
+
+
+@transactional
+def make_discovery():
+    """Make a discovery in its own transaction so each last_seen time
+    is different."""
+    random_rack = random.choice(RackController.objects.all())
+    random_interface = random.choice(random_rack.interface_set.all())
+    random_subnet = random.choice(random_interface.vlan.subnet_set.all())
+    factory.make_Discovery(
+        interface=random_interface,
+        ip=factory.pick_ip_in_Subnet(random_subnet))
+
+
+@transactional
+def populate_main():
+    """Populate the main data all in one transaction."""
     admin = factory.make_admin(
         username="admin", password="test", completed_intro=False)  # noqa
     user1, _ = factory.make_user_with_keys(
@@ -281,14 +300,6 @@
         factory.make_StaticIPAddress(
             alloc_type=IPADDRESS_TYPE.STICKY, ip="172.16.3.2",
             subnet=subnet_3, interface=bond0_10)
-        # Add some discovery devices for rack interfaces
-        for _ in range(3):
-            factory.make_Discovery(
-                interface=eth0, ip=factory.pick_ip_in_Subnet(subnet_1))
-            factory.make_Discovery(
-                interface=eth1, ip=factory.pick_ip_in_Subnet(subnet_1))
-            factory.make_Discovery(
-                interface=eth2, ip=factory.pick_ip_in_Subnet(subnet_2))
 
     # Rack controller (happy-rack)
     #   eth0     - fabric 0 - untagged
@@ -323,14 +334,6 @@
     factory.make_StaticIPAddress(
         alloc_type=IPADDRESS_TYPE.STICKY, ip="172.16.3.3",
         subnet=subnet_3, interface=bond0_10)
-    # Add some discovery devices for rack interfaces
-    for _ in range(3):
-        factory.make_Discovery(
-            interface=eth0, ip=factory.pick_ip_in_Subnet(subnet_1))
-        factory.make_Discovery(
-            interface=eth1, ip=factory.pick_ip_in_Subnet(subnet_1))
-        factory.make_Discovery(
-            interface=eth2, ip=factory.pick_ip_in_Subnet(subnet_2))
 
     # Region controller (happy-region)
     #   eth0     - fabric 0 - untagged

=== modified file 'src/maasserver/websockets/handlers/discovery.py'
--- src/maasserver/websockets/handlers/discovery.py	2016-09-06 20:31:47 +0000
+++ src/maasserver/websockets/handlers/discovery.py	2016-10-05 15:19:04 +0000
@@ -7,6 +7,9 @@
     "DiscoveryHandler",
     ]
 
+from datetime import datetime
+import time
+
 from maasserver.models import Discovery
 from maasserver.websockets.base import dehydrate_datetime
 from maasserver.websockets.handlers.viewmodel import ViewModelHandler
@@ -22,10 +25,6 @@
         queryset = (
             Discovery.objects.by_unknown_ip_and_mac()
         )
-        # This batch key isn't guaranteed to be stable, since newly-discovered
-        # items can come in as the new first-items in the query. But that's why
-        # we're also going to poll. But using row_number() seems to be a good
-        # compromise for now.
         batch_key = 'first_seen'
         pk = 'discovery_id'
         allowed_methods = [
@@ -33,13 +32,31 @@
             'get',
         ]
 
+    def list(self, params):
+        """List objects.
+
+        :param start: A value of the `batch_key` column and NOT `pk`. They are
+            often the same but that is not a certainty. Make sure the client
+            also understands this distinction.
+        :param offset: Offset into the queryset to return.
+        :param limit: Maximum number of objects to return.
+        """
+        if "start" in params:
+            params["start"] = datetime.fromtimestamp(float(params['start']))
+        return super(DiscoveryHandler, self).list(params)
+
     def dehydrate(self, obj, data, for_list=False):
         """Add extra fields to `data`."""
         data["mac_organization"] = obj.mac_organization
         return data
 
-    def dehydrate_last_seen(self, datetime):
-        return dehydrate_datetime(datetime)
+    def dehydrate_last_seen(self, obj):
+        return dehydrate_datetime(obj)
 
-    def dehydrate_first_seen(self, datetime):
-        return dehydrate_datetime(datetime)
+    def dehydrate_first_seen(self, obj):
+        # This is rendered all they way to microseconds so its always
+        # unique. This is because each discovery item is always created in
+        # is own transaction. If this changes then the barch key needs to
+        # be changed to something that is ordered and unique.
+        return str(
+            time.mktime(obj.timetuple()) + obj.microsecond / 1e6)

=== modified file 'src/maasserver/websockets/handlers/tests/test_discovery.py'
--- src/maasserver/websockets/handlers/tests/test_discovery.py	2016-09-30 18:07:10 +0000
+++ src/maasserver/websockets/handlers/tests/test_discovery.py	2016-10-05 15:19:04 +0000
@@ -9,6 +9,7 @@
     datetime,
     timedelta,
 )
+import time
 
 from maasserver.dbviews import register_view
 from maasserver.models.discovery import Discovery
@@ -46,7 +47,9 @@
             "subnet_cidr": discovery.subnet_cidr,
             "vid": discovery.vid,
             "vlan": discovery.vlan_id,
-            "first_seen": dehydrate_datetime(discovery.first_seen),
+            "first_seen": str(
+                time.mktime(discovery.first_seen.timetuple()) +
+                discovery.first_seen.microsecond / 1e6),
             "last_seen": dehydrate_datetime(discovery.last_seen)
         }
         return data
@@ -90,3 +93,25 @@
         self.assertEquals(
             expected_discoveries,
             handler.list({}))
+
+    def test_list_starts_after_first_seen(self):
+        user = factory.make_User()
+        handler = DiscoveryHandler(user, {})
+        now = datetime.now()
+        factory.make_Discovery(created=now)
+        d4 = factory.make_Discovery(created=(now + timedelta(days=4)))
+        d3 = factory.make_Discovery(created=(now + timedelta(days=3)))
+        factory.make_Discovery(created=(now + timedelta(days=1)))
+        factory.make_Discovery(created=(now + timedelta(days=2)))
+        first_seen = now + timedelta(days=2)
+        first_seen = str(
+            time.mktime(first_seen.timetuple()) + first_seen.microsecond / 1e6)
+        # Test for the expected order independent of how the database
+        # decided to sort.
+        expected_discoveries = [
+            self.dehydrate_discovery(discovery, for_list=True)
+            for discovery in [d3, d4]
+            ]
+        self.assertEquals(
+            expected_discoveries,
+            handler.list({"start": first_seen}))