)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":12860,"name":"Dongcan Ye","email":"yedongcan@yeah.net","username":"yedongcan"},"change_message_id":"b005db960f20d76be2cc2f09c67aa11f3916fc4b","unresolved":false,"context_lines":[{"line_number":10,"context_line":"assigning IPs to newly created ports. If the port has a defined"},{"line_number":11,"context_line":"device_owner, this is used to filter available subnets to choose"},{"line_number":12,"context_line":"from only those with a matching service_type."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: If3dd94a46bdee24c13d1f17c4f2e69af0cb8af63"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"bacf61ea_e3acf4ee","line":13,"updated":"2016-08-05 14:01:24.000000000","message":"@John, I think this patch is part of blueprint service-subnets, so why not add Partially-implements tag here?","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"fce444e457c151b28ee9e5d65214d9f105b5c601","unresolved":false,"context_lines":[{"line_number":10,"context_line":"assigning IPs to newly created ports. If the port has a defined"},{"line_number":11,"context_line":"device_owner, this is used to filter available subnets to choose"},{"line_number":12,"context_line":"from only those with a matching service_type."},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"Change-Id: If3dd94a46bdee24c13d1f17c4f2e69af0cb8af63"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"bacf61ea_4654eec8","line":13,"in_reply_to":"bacf61ea_e3acf4ee","updated":"2016-08-05 14:25:40.000000000","message":"It\u0027s coming in the next patch.","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"}],"neutron/db/ipam_backend_mixin.py":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"177fb8599b4969c4227a783c1e38fcf5c104a39d","unresolved":false,"context_lines":[{"line_number":569,"context_line":"            return"},{"line_number":570,"context_line":"        Service_type \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d self._get_collection_query(context, Service_type)"},{"line_number":572,"context_line":"        query \u003d query.filter(Service_type.service_type \u003d\u003d service_type)"},{"line_number":573,"context_line":"        return [s_type.subnet_id for s_type in query]"},{"line_number":574,"context_line":""},{"line_number":575,"context_line":"    def _filter_service_subnets(self, context, subnets, service_type):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bacf61ea_73015506","line":572,"updated":"2016-08-03 22:17:11.000000000","message":"As the number of service subnets grows in a deployment, it might be worth narrowing this query further. Consider adding the following line:\n\nquery \u003d query.filter(Service_type.subnet_id.in_(subnet_ids))\n\nThis assumes that you pass to this method the argument subnet_ids, which can be created right before line 576 as follows:\n\nsubnet_ids \u003d [s[\u0027id\u0027] for s in subnets]\n\nI tested this changes with the unit tests and it works fine","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ecd10fdcda3aecdab9aff64e0d6045390eda84f3","unresolved":false,"context_lines":[{"line_number":569,"context_line":"            return"},{"line_number":570,"context_line":"        Service_type \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d self._get_collection_query(context, Service_type)"},{"line_number":572,"context_line":"        query \u003d query.filter(Service_type.service_type \u003d\u003d service_type)"},{"line_number":573,"context_line":"        return [s_type.subnet_id for s_type in query]"},{"line_number":574,"context_line":""},{"line_number":575,"context_line":"    def _filter_service_subnets(self, context, subnets, service_type):"}],"source_content_type":"text/x-python","patch_set":1,"id":"bacf61ea_1ee26b2f","line":572,"in_reply_to":"bacf61ea_73015506","updated":"2016-08-04 18:32:15.000000000","message":"What we really need to do is include the service subnets in the same query that gets the non-service subnets so that we don\u0027t add another query. In other words, that query should return subnets matching the service subnet *and* the ones that don\u0027t have any service type associated. Then, we need to sort the results so that the ones with the matching service type come first and are preferred for IPAM.\n\nThe cost of setting up an extra DB query is a lot higher than the cost of adding a little more complexity to an existing query.","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":576,"context_line":"                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":577,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        return query.from_self(Subnet)"},{"line_number":582,"context_line":""},{"line_number":583,"context_line":"    def _ipam_get_subnets(self, context, network_id, host, service_type\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_f8c1d92f","line":580,"range":{"start_line":579,"start_character":8,"end_line":580,"end_character":75},"updated":"2016-08-08 22:16:15.000000000","message":"It is tempting to think this filter is not necessary given L576. But, it is necessary to eliminate subnets with service types that don\u0027t match the given service type because, with the join, they will still appear in the result with a service type of null, I think.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":576,"context_line":"                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":577,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        return query.from_self(Subnet)"},{"line_number":582,"context_line":""},{"line_number":583,"context_line":"    def _ipam_get_subnets(self, context, network_id, host, service_type\u003dNone):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_553d34ba","line":580,"range":{"start_line":579,"start_character":8,"end_line":580,"end_character":75},"in_reply_to":"9ad45d7e_f8c1d92f","updated":"2016-08-10 16:22:39.000000000","message":"Yes, this is true. The left hand join means we have to remove the subnets with non-matching types. If we used a right hand join we\u0027d lose the subnets with no type at all.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        return query.from_self(Subnet)"},{"line_number":582,"context_line":""},{"line_number":583,"context_line":"    def _ipam_get_subnets(self, context, network_id, host, service_type\u003dNone):"},{"line_number":584,"context_line":"        Subnet \u003d models_v2.Subnet"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_d88975cc","line":581,"range":{"start_line":581,"start_character":21,"end_line":581,"end_character":30},"updated":"2016-08-08 22:16:15.000000000","message":"I\u0027m curious what this is needed for. I\u0027m not very familiar with it.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        return query.from_self(Subnet)"},{"line_number":582,"context_line":""},{"line_number":583,"context_line":"    def _ipam_get_subnets(self, context, network_id, host, service_type\u003dNone):"},{"line_number":584,"context_line":"        Subnet \u003d models_v2.Subnet"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_d502c408","line":581,"range":{"start_line":581,"start_character":21,"end_line":581,"end_character":30},"in_reply_to":"9ad45d7e_d88975cc","updated":"2016-08-10 16:22:39.000000000","message":"This removes the ServiceType column from the query, so we don\u0027t have to deal with iterating through it later, as we do on L639 with the host mapping.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"fc87beea6bb5eaaf63054610aca4823f438515fd","unresolved":false,"context_lines":[{"line_number":576,"context_line":"                                     ServiceType.service_type \u003d\u003d service_type))"},{"line_number":577,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        if not query.all():"},{"line_number":582,"context_line":"            raise ipam_exceptions.IpAddressGenerationFailureNoMatchingSubnet()"}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_d83c0f95","line":579,"range":{"start_line":579,"start_character":33,"end_line":579,"end_character":61},"updated":"2016-08-18 16:45:35.000000000","message":"Could we replace this with \"ServiceType.service_type.is_(None)\"","commit_id":"cf9a99271c80fb5d694f8c3315f32a16e93dd599"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"c426883d60ee63a03dfbd34d6c836e1b6e137d31","unresolved":false,"context_lines":[{"line_number":576,"context_line":"                                     ServiceType.service_type \u003d\u003d service_type))"},{"line_number":577,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        if not query.all():"},{"line_number":582,"context_line":"            raise ipam_exceptions.IpAddressGenerationFailureNoMatchingSubnet()"}],"source_content_type":"text/x-python","patch_set":4,"id":"1ac06dbe_0dfee236","line":579,"range":{"start_line":579,"start_character":33,"end_line":579,"end_character":61},"in_reply_to":"3ac371cc_d83c0f95","updated":"2016-08-22 16:36:37.000000000","message":"I now understand why we cannot do this. It is essentially the same thing that I noticed in PS2. Subnets with service types that don\u0027t match the one given will still appear in this outer join but with a null in the service type column.\n\nI actually want to think about possibly reorganizing this a bit to make it easier to reason about. The fact that I had to think through this twice to understand why it needs to be the way it is makes me think that finding a clearer way to express this would enhance readability. Let me think about it a little.","commit_id":"cf9a99271c80fb5d694f8c3315f32a16e93dd599"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"fc87beea6bb5eaaf63054610aca4823f438515fd","unresolved":false,"context_lines":[{"line_number":578,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":579,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"},{"line_number":580,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":581,"context_line":"        if not query.all():"},{"line_number":582,"context_line":"            raise ipam_exceptions.IpAddressGenerationFailureNoMatchingSubnet()"},{"line_number":583,"context_line":"        return query.from_self(Subnet)"},{"line_number":584,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"3ac371cc_f51474a0","line":581,"range":{"start_line":581,"start_character":21,"end_line":581,"end_character":24},"updated":"2016-08-18 16:45:35.000000000","message":"This should be 0 \u003d\u003d count() (or .exists() as you found)","commit_id":"cf9a99271c80fb5d694f8c3315f32a16e93dd599"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"a97426269f50e5c5453683ea417250ab1e9481e2","unresolved":false,"context_lines":[{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                and_(Subnet.id \u003d\u003d ServiceType.subnet_id,"},{"line_number":574,"context_line":"                                     ServiceType.service_type \u003d\u003d service_type))"},{"line_number":575,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":576,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":577,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"}],"source_content_type":"text/x-python","patch_set":6,"id":"1ac06dbe_83f463f2","line":574,"range":{"start_line":574,"start_character":37,"end_line":574,"end_character":77},"updated":"2016-08-22 17:16:44.000000000","message":"The other review where we were talking about what belongs in a join condition actually got me thinking about this one.\n\nI think having this condition here is hurting us. By filtering those rows that don\u0027t match the service type here in the join, we force the strange condition on L577 that I\u0027ve never liked and continually questioned at the mid-cycle. I knew there was something that I really didn\u0027t like about it but didn\u0027t figure it out until now.\n\nIf we take out this condition here then I think that we can go to the simpler condition that I mistakenly suggested in PS4 and it will work. I think it\u0027ll be a lot easier to reason about too.","commit_id":"5dbfb2bfe4d0db452bf5a80c4178a1ee3f531458"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"c04889e756da6a4fe6d4ea24c0d1a13f0ade0957","unresolved":false,"context_lines":[{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                and_(Subnet.id \u003d\u003d ServiceType.subnet_id,"},{"line_number":574,"context_line":"                                     ServiceType.service_type \u003d\u003d service_type))"},{"line_number":575,"context_line":"        # We can\u0027t use is_(None) for Subnet.service_types as it isn\u0027t an"},{"line_number":576,"context_line":"        # attribute in the Subnet model, but \u003d\u003d None fails pep8, so noqa."},{"line_number":577,"context_line":"        query \u003d query.filter(or_(Subnet.service_types \u003d\u003d None,  # noqa"}],"source_content_type":"text/x-python","patch_set":6,"id":"1ac06dbe_1c67294e","line":574,"range":{"start_line":574,"start_character":37,"end_line":574,"end_character":77},"in_reply_to":"1ac06dbe_83f463f2","updated":"2016-08-22 18:48:48.000000000","message":"For cross-reference: https://review.openstack.org/#/c/317358/22/neutron/plugins/ml2/db.py","commit_id":"5dbfb2bfe4d0db452bf5a80c4178a1ee3f531458"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"f302d2b69a78dd79759824a59769d60dadc4488c","unresolved":false,"context_lines":[{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                Subnet.id \u003d\u003d ServiceType.subnet_id)"},{"line_number":574,"context_line":"        query \u003d query.filter(or_(ServiceType.service_type.is_(None),"},{"line_number":575,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":576,"context_line":"        return query.from_self(Subnet)"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_743c7be2","line":573,"range":{"start_line":573,"start_character":32,"end_line":573,"end_character":66},"updated":"2016-08-22 17:24:26.000000000","message":"Actually, I just noticed that the join condition is redundant given that sqlalchemy will use the FK constraint. But, I\u0027m okay with it staying here. But, I checked and the unit tests pass after removing it.","commit_id":"ce2ee882d299e9c0f744b6d024d5782f0474e54f"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"4a947105deb268751743236e22627f8902dae723","unresolved":false,"context_lines":[{"line_number":569,"context_line":"        Subnet \u003d models_v2.Subnet"},{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                Subnet.id \u003d\u003d ServiceType.subnet_id)"},{"line_number":574,"context_line":"        query \u003d query.filter(or_(ServiceType.service_type.is_(None),"},{"line_number":575,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":576,"context_line":"        return query.from_self(Subnet)"},{"line_number":577,"context_line":""},{"line_number":578,"context_line":"    def _check_service_subnets(self, query, service_type):"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_d4f8cf4b","line":575,"range":{"start_line":572,"start_character":0,"end_line":575,"end_character":75},"updated":"2016-08-22 17:22:10.000000000","message":"Ahhhh. This is much more straight-forward. No hidden weirdness. I hope you don\u0027t mind that I updated this.","commit_id":"ce2ee882d299e9c0f744b6d024d5782f0474e54f"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"e6c863f77ab5d291b6c6579afefb5890b81d66a3","unresolved":false,"context_lines":[{"line_number":569,"context_line":"        Subnet \u003d models_v2.Subnet"},{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                Subnet.id \u003d\u003d ServiceType.subnet_id)"},{"line_number":574,"context_line":"        query \u003d query.filter(or_(ServiceType.service_type.is_(None),"},{"line_number":575,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":576,"context_line":"        return query.from_self(Subnet)"},{"line_number":577,"context_line":""},{"line_number":578,"context_line":"    def _check_service_subnets(self, query, service_type):"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_a8447349","line":575,"range":{"start_line":572,"start_character":0,"end_line":575,"end_character":75},"in_reply_to":"1ac06dbe_4f79bcc7","updated":"2016-08-24 00:39:11.000000000","message":"I was almost there. It was nagging at my mind (as you could probably tell) but I had to sit down and think it through without distractions to finally get it.","commit_id":"ce2ee882d299e9c0f744b6d024d5782f0474e54f"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"a8c506ad41059e15ed8bc951717a0fe91e0e3e18","unresolved":false,"context_lines":[{"line_number":569,"context_line":"        Subnet \u003d models_v2.Subnet"},{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType,"},{"line_number":573,"context_line":"                                Subnet.id \u003d\u003d ServiceType.subnet_id)"},{"line_number":574,"context_line":"        query \u003d query.filter(or_(ServiceType.service_type.is_(None),"},{"line_number":575,"context_line":"                                 ServiceType.service_type \u003d\u003d service_type))"},{"line_number":576,"context_line":"        return query.from_self(Subnet)"},{"line_number":577,"context_line":""},{"line_number":578,"context_line":"    def _check_service_subnets(self, query, service_type):"}],"source_content_type":"text/x-python","patch_set":7,"id":"1ac06dbe_4f79bcc7","line":575,"range":{"start_line":572,"start_character":0,"end_line":575,"end_character":75},"in_reply_to":"1ac06dbe_d4f8cf4b","updated":"2016-08-23 14:40:47.000000000","message":"Yes, this is much nicer! I don\u0027t know how we didn\u0027t figure this out last week.","commit_id":"ce2ee882d299e9c0f744b6d024d5782f0474e54f"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9c51148d5647d7c03d5bd3c152b3a4d5d917f0d","unresolved":false,"context_lines":[{"line_number":566,"context_line":"        return fixed_ip_list"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def _query_filter_service_subnets(self, query, service_type):"},{"line_number":569,"context_line":"        Subnet \u003d models_v2.Subnet"},{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_797aaf78","line":569,"range":{"start_line":569,"start_character":8,"end_line":569,"end_character":33},"updated":"2016-08-22 18:45:48.000000000","message":"nit: This local alias for Subnet is hardly needed anymore. Only fix if there is another patch set.","commit_id":"ee2283c9132a86ae602fd424e44fff0fc773f844"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"feb0596558950af4adc6678606d5ae79113c3b9d","unresolved":false,"context_lines":[{"line_number":566,"context_line":"        return fixed_ip_list"},{"line_number":567,"context_line":""},{"line_number":568,"context_line":"    def _query_filter_service_subnets(self, query, service_type):"},{"line_number":569,"context_line":"        Subnet \u003d models_v2.Subnet"},{"line_number":570,"context_line":"        ServiceType \u003d service_type_db.SubnetServiceType"},{"line_number":571,"context_line":"        query \u003d query.add_entity(ServiceType)"},{"line_number":572,"context_line":"        query \u003d query.outerjoin(ServiceType)"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_af503033","line":569,"range":{"start_line":569,"start_character":8,"end_line":569,"end_character":33},"in_reply_to":"1ac06dbe_797aaf78","updated":"2016-08-23 14:51:32.000000000","message":"Done","commit_id":"ee2283c9132a86ae602fd424e44fff0fc773f844"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9c51148d5647d7c03d5bd3c152b3a4d5d917f0d","unresolved":false,"context_lines":[{"line_number":636,"context_line":"            query \u003d query.filter(Subnet.network_id \u003d\u003d network_id)"},{"line_number":637,"context_line":"            query \u003d query.filter(Subnet.segment_id.isnot(None))"},{"line_number":638,"context_line":"            if query.count() \u003d\u003d 0:"},{"line_number":639,"context_line":"                self._check_service_subnets(unfiltered_query, service_type)"},{"line_number":640,"context_line":"                return []"},{"line_number":641,"context_line":"            # It is a routed network but no subnets found for host"},{"line_number":642,"context_line":"            raise segment_exc.HostNotConnectedToAnySegment("}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_79974f3a","line":639,"range":{"start_line":639,"start_character":16,"end_line":639,"end_character":75},"updated":"2016-08-22 18:45:48.000000000","message":"Note: I think if we worked a little harder, we could optimize this a little bit more. But, I don\u0027t think I want it in this patch set. But, I\u0027ll add this note about what I\u0027m thinking.\n\nIn the case where we really want to return [] here it takes us three queries to figure it out. First, the one on L630, second, the one on L638 and finally, the first one in _check_service_subnets. I think with a little bit of refactoring, we could figure it out with just two queries and save the other two queries for trying to figure out which error to raise when we know we have to raise some error.\n\nThe idea would be to just run the unfiltered query before L634 and check if it comes back empty (limit(1).count() \u003d\u003d 0).\n\nI\u0027m okay with this now because the use case for a port on a network with no subnets at all is not very common.","commit_id":"ee2283c9132a86ae602fd424e44fff0fc773f844"}],"neutron/db/ipam_non_pluggable_backend.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ecd10fdcda3aecdab9aff64e0d6045390eda84f3","unresolved":false,"context_lines":[{"line_number":282,"context_line":"        p \u003d port[\u0027port\u0027]"},{"line_number":283,"context_line":"        subnets \u003d self._ipam_get_subnets(context,"},{"line_number":284,"context_line":"                                         network_id\u003dp[\u0027network_id\u0027],"},{"line_number":285,"context_line":"                                         host\u003dp.get(portbindings.HOST_ID))"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"        v4, v6_stateful, v6_stateless \u003d self._classify_subnets("},{"line_number":288,"context_line":"            context, subnets)"}],"source_content_type":"text/x-python","patch_set":1,"id":"bacf61ea_9edf7b39","line":285,"range":{"start_line":285,"start_character":41,"end_line":285,"end_character":74},"updated":"2016-08-04 18:32:15.000000000","message":"You might want to look at the patch [1] where I added this parameter to make IPAM host aware. It might give you some ideas. Take a look, in particular at the changes to _ipam_get_subnets.\n\nOff the top of my head, I\u0027m thinking that if you add another filter, somewhat similar to the one here [2], and figure out how to sort the results, that might just work. The filter should essentially select subnets that either have *no* service type associated or have the matching service type. This will probably require a join similar to [3].\n\nWe also need to make sure that IPAM considers the subnets in the order they\u0027re returned with preference on the ones that have a service subnet.\n\n [1] https://github.com/openstack/neutron/commit/b32426c05e16a0d4aab3dbe5d2d7287dfec13ce1\n [2] https://github.com/openstack/neutron/blob/master/neutron/db/ipam_backend_mixin.py#L563-L566\n [3] https://github.com/openstack/neutron/blob/master/neutron/db/ipam_backend_mixin.py#L558-L561","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"651820203e408537447e1b4cdb0c6f540130ebff","unresolved":false,"context_lines":[{"line_number":282,"context_line":"        p \u003d port[\u0027port\u0027]"},{"line_number":283,"context_line":"        subnets \u003d self._ipam_get_subnets(context,"},{"line_number":284,"context_line":"                                         network_id\u003dp[\u0027network_id\u0027],"},{"line_number":285,"context_line":"                                         host\u003dp.get(portbindings.HOST_ID))"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"        v4, v6_stateful, v6_stateless \u003d self._classify_subnets("},{"line_number":288,"context_line":"            context, subnets)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9ad45d7e_bbe2c89d","line":285,"range":{"start_line":285,"start_character":41,"end_line":285,"end_character":74},"in_reply_to":"bacf61ea_9edf7b39","updated":"2016-08-08 16:51:42.000000000","message":"Hey Carl, I\u0027ve been working on using this approach today and so far it\u0027s working great. I\u0027ve got one more bug to work out of it, then I\u0027ll push it up for review. Thanks","commit_id":"8d61b612faab4416eaf2ad3c3d98933de7d6b349"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"d85377e6699b24cdf0167077624ee6b5931fc455","unresolved":false,"context_lines":[{"line_number":47,"context_line":"        the network."},{"line_number":48,"context_line":"        \"\"\""},{"line_number":49,"context_line":"        filtered_ips \u003d filtered_ips or []"},{"line_number":50,"context_line":"        # We want to allocate IPs first from subnets with a service type."},{"line_number":51,"context_line":"        # Sorting the subnets list in this way before generating the"},{"line_number":52,"context_line":"        # subnet_id_list allows us to do this."},{"line_number":53,"context_line":"        sorted_subnets \u003d reversed(sorted(subnets,"},{"line_number":54,"context_line":"                             key\u003dlambda subnet: subnet.get(\u0027service_types\u0027)))"},{"line_number":55,"context_line":"        subnet_id_list \u003d [subnet[\u0027id\u0027] for subnet in sorted_subnets]"},{"line_number":56,"context_line":"        pool_qry \u003d context.session.query(models_v2.IPAllocationPool)"},{"line_number":57,"context_line":"        pool_qry \u003d pool_qry.filter("}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_f86f9df9","line":54,"range":{"start_line":50,"start_character":8,"end_line":54,"end_character":77},"updated":"2016-08-17 15:50:13.000000000","message":"I\u0027m pushing the sorting into _ipam_get_subnets so we don\u0027t need to duplicate this in ipam_pluggable_backend.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":73,"context_line":"                                                ip_pool.last_ip))"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"        # Filter out any subnet IDs that may not have made their way"},{"line_number":76,"context_line":"        # into ip_pools"},{"line_number":77,"context_line":"        subnet_id_list \u003d [subnet_id for subnet_id in subnet_id_list if"},{"line_number":78,"context_line":"                          subnet_id in ip_pools.keys()]"},{"line_number":79,"context_line":"        for subnet_id in subnet_id_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_4c814a4a","line":76,"updated":"2016-08-08 22:16:15.000000000","message":"This can easily be done with just this in the for loop:\n\n  # Skip ineligible subnets\n  if subnet_id not in ip_pools:\n      continue","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":73,"context_line":"                                                ip_pool.last_ip))"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"        # Filter out any subnet IDs that may not have made their way"},{"line_number":76,"context_line":"        # into ip_pools"},{"line_number":77,"context_line":"        subnet_id_list \u003d [subnet_id for subnet_id in subnet_id_list if"},{"line_number":78,"context_line":"                          subnet_id in ip_pools.keys()]"},{"line_number":79,"context_line":"        for subnet_id in subnet_id_list:"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_15a9fcdf","line":76,"in_reply_to":"9ad45d7e_4c814a4a","updated":"2016-08-10 16:22:39.000000000","message":"Yes that\u0027s much cleaner, thanks.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        # Filter out any subnet IDs that may not have made their way"},{"line_number":76,"context_line":"        # into ip_pools"},{"line_number":77,"context_line":"        subnet_id_list \u003d [subnet_id for subnet_id in subnet_id_list if"},{"line_number":78,"context_line":"                          subnet_id in ip_pools.keys()]"},{"line_number":79,"context_line":"        for subnet_id in subnet_id_list:"},{"line_number":80,"context_line":"            subnet_ip_pools \u003d ip_pools[subnet_id]"},{"line_number":81,"context_line":"            subnet_ip_allocs \u003d ip_allocations[subnet_id]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_2cffb602","line":78,"range":{"start_line":78,"start_character":47,"end_line":78,"end_character":54},"updated":"2016-08-08 22:16:15.000000000","message":"nit: This is redundant. \"subnet_id in ip_pools\" works the same.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":75,"context_line":"        # Filter out any subnet IDs that may not have made their way"},{"line_number":76,"context_line":"        # into ip_pools"},{"line_number":77,"context_line":"        subnet_id_list \u003d [subnet_id for subnet_id in subnet_id_list if"},{"line_number":78,"context_line":"                          subnet_id in ip_pools.keys()]"},{"line_number":79,"context_line":"        for subnet_id in subnet_id_list:"},{"line_number":80,"context_line":"            subnet_ip_pools \u003d ip_pools[subnet_id]"},{"line_number":81,"context_line":"            subnet_ip_allocs \u003d ip_allocations[subnet_id]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_d5d3e45d","line":78,"range":{"start_line":78,"start_character":47,"end_line":78,"end_character":54},"in_reply_to":"9ad45d7e_2cffb602","updated":"2016-08-10 16:22:39.000000000","message":"Done","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"d9c51148d5647d7c03d5bd3c152b3a4d5d917f0d","unresolved":false,"context_lines":[{"line_number":68,"context_line":"                                                ip_pool.last_ip))"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        for subnet_id in subnet_id_list:"},{"line_number":71,"context_line":"            if subnet_id not in ip_pools:"},{"line_number":72,"context_line":"                continue"},{"line_number":73,"context_line":"            subnet_ip_pools \u003d ip_pools[subnet_id]"},{"line_number":74,"context_line":"            subnet_ip_allocs \u003d ip_allocations[subnet_id]"},{"line_number":75,"context_line":"            filter_set \u003d netaddr.IPSet()"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_9cdcb974","line":72,"range":{"start_line":71,"start_character":12,"end_line":72,"end_character":24},"updated":"2016-08-22 18:45:48.000000000","message":"This probably doesn\u0027t hurt but is it strictly necessary? Did you hit a problem with it?\n\nAs far as I can tell, if this weren\u0027t added and \"subnet_id not in ip_pools\" then we\u0027d get an empty set on L73 because of the defaultdict. We\u0027d build filter_set unnecessarily on L75-L77 (but it won\u0027t be very big so it isn\u0027t a big deal), then we\u0027d do a couple of trivial set differences, and finally we\u0027d continue anyway on L84. So, if I\u0027m right, this change boils down to an optimization who\u0027s merit is questionable because I don\u0027t really see any real algorithm complexity improvement.\n\nIf it is all the same, I think I\u0027d rather take this change out of here and post it as a separate patch set to discuss it.\n\nNot to mention, this is dead code anyway since the change to use pluggable IPAM everywhere. :)","commit_id":"ee2283c9132a86ae602fd424e44fff0fc773f844"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"feb0596558950af4adc6678606d5ae79113c3b9d","unresolved":false,"context_lines":[{"line_number":68,"context_line":"                                                ip_pool.last_ip))"},{"line_number":69,"context_line":""},{"line_number":70,"context_line":"        for subnet_id in subnet_id_list:"},{"line_number":71,"context_line":"            if subnet_id not in ip_pools:"},{"line_number":72,"context_line":"                continue"},{"line_number":73,"context_line":"            subnet_ip_pools \u003d ip_pools[subnet_id]"},{"line_number":74,"context_line":"            subnet_ip_allocs \u003d ip_allocations[subnet_id]"},{"line_number":75,"context_line":"            filter_set \u003d netaddr.IPSet()"}],"source_content_type":"text/x-python","patch_set":8,"id":"1ac06dbe_d2ab8773","line":72,"range":{"start_line":71,"start_character":12,"end_line":72,"end_character":24},"in_reply_to":"1ac06dbe_9cdcb974","updated":"2016-08-23 14:51:32.000000000","message":"You\u0027re right, this is an optimization. I added it to avoid adding unnecessary iterations due to the change on L70 - which is necessary to preserve the order of preference. As you say, with the switch to pluggable ipam this no longer actually improves anything important, so I\u0027m happy to remove it.","commit_id":"ee2283c9132a86ae602fd424e44fff0fc773f844"}],"neutron/db/ipam_pluggable_backend.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        subnets \u003d self._ipam_get_subnets(context,"},{"line_number":193,"context_line":"                                         network_id\u003dp[\u0027network_id\u0027],"},{"line_number":194,"context_line":"                                         host\u003dp.get(portbindings.HOST_ID),"},{"line_number":195,"context_line":"                                         service_type\u003dp.get(\u0027device_owner\u0027))"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"        v4, v6_stateful, v6_stateless \u003d self._classify_subnets("},{"line_number":198,"context_line":"            context, subnets)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_6c588efb","line":195,"updated":"2016-08-08 22:16:15.000000000","message":"Don\u0027t we need to sort in this implementation too?","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        subnets \u003d self._ipam_get_subnets(context,"},{"line_number":193,"context_line":"                                         network_id\u003dp[\u0027network_id\u0027],"},{"line_number":194,"context_line":"                                         host\u003dp.get(portbindings.HOST_ID),"},{"line_number":195,"context_line":"                                         service_type\u003dp.get(\u0027device_owner\u0027))"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"        v4, v6_stateful, v6_stateless \u003d self._classify_subnets("},{"line_number":198,"context_line":"            context, subnets)"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_dedc4f75","line":195,"in_reply_to":"9ad45d7e_6c588efb","updated":"2016-08-10 16:22:39.000000000","message":"Yes, we do. I\u0027m looking at simply sorting the list before it\u0027s returned by _ipam_get_subnets. If that works then we can get rid of the sorting in ipam_non_pluggable_backand and just keep it all in the backend mixin.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"d85377e6699b24cdf0167077624ee6b5931fc455","unresolved":false,"context_lines":[{"line_number":192,"context_line":"        subnets \u003d self._ipam_get_subnets(context,"},{"line_number":193,"context_line":"                                         network_id\u003dp[\u0027network_id\u0027],"},{"line_number":194,"context_line":"                                         host\u003dp.get(portbindings.HOST_ID),"},{"line_number":195,"context_line":"                                         service_type\u003dp.get(\u0027device_owner\u0027))"},{"line_number":196,"context_line":""},{"line_number":197,"context_line":"        v4, v6_stateful, v6_stateless \u003d self._classify_subnets("},{"line_number":198,"context_line":"            context, subnets)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_787c4db5","line":195,"in_reply_to":"9ad45d7e_dedc4f75","updated":"2016-08-17 15:50:13.000000000","message":"Done","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"}],"neutron/tests/unit/db/test_ipam_pluggable_backend.py":[{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"0bf00dc775d15fa23fe6a03c81be2f4869fa68ce","unresolved":false,"context_lines":[{"line_number":664,"context_line":"                    \u0027ip_version\u0027: 4,"},{"line_number":665,"context_line":"                    \u0027ipv6_address_mode\u0027: None,"},{"line_number":666,"context_line":"                    \u0027ipv6_ra_mode\u0027: None}]"},{"line_number":667,"context_line":"        get_service_subnets_mock \u003d mock.Mock(return_value\u003dNone)"},{"line_number":668,"context_line":"        get_subnets_mock \u003d mock.Mock(return_value\u003dsubnets)"},{"line_number":669,"context_line":"        get_subnet_mock \u003d mock.Mock(return_value\u003dsubnets[0])"},{"line_number":670,"context_line":"        mocks[\u0027ipam\u0027] \u003d ipam_pluggable_backend.IpamPluggableBackend()"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_3dbebfd4","line":667,"updated":"2016-08-19 09:08:25.000000000","message":"We actually don\u0027t need these changes anymore as _ipam_get_service_subnets was removed in patchset 2.","commit_id":"e1bda1d037b1cafebd00db4bfa1f4be80523360b"}],"neutron/tests/unit/extensions/test_subnet_service_types.py":[{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    def _create_subnet_with_service_types(self, service_types, cidr\u003dNone,"},{"line_number":58,"context_line":"                                          network\u003dNone):"},{"line_number":59,"context_line":"        if not network:"},{"line_number":60,"context_line":"            with self.network() as network:"},{"line_number":61,"context_line":"                pass"},{"line_number":62,"context_line":"        network \u003d network[\u0027network\u0027]"},{"line_number":63,"context_line":"        if not cidr:"},{"line_number":64,"context_line":"            cidr \u003d self.CIDRS[0]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_4c3aea37","line":61,"range":{"start_line":59,"start_character":8,"end_line":61,"end_character":20},"updated":"2016-08-08 22:16:15.000000000","message":"If you get a chance to crack it open, fold some of this in to the parent patch so that these changes are minimized in this patch. But, only if you get the chance for some other reason.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":56,"context_line":""},{"line_number":57,"context_line":"    def _create_subnet_with_service_types(self, service_types, cidr\u003dNone,"},{"line_number":58,"context_line":"                                          network\u003dNone):"},{"line_number":59,"context_line":"        if not network:"},{"line_number":60,"context_line":"            with self.network() as network:"},{"line_number":61,"context_line":"                pass"},{"line_number":62,"context_line":"        network \u003d network[\u0027network\u0027]"},{"line_number":63,"context_line":"        if not cidr:"},{"line_number":64,"context_line":"            cidr \u003d self.CIDRS[0]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_240df037","line":61,"range":{"start_line":59,"start_character":8,"end_line":61,"end_character":20},"in_reply_to":"9ad45d7e_4c3aea37","updated":"2016-08-10 16:22:39.000000000","message":"Done","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e7b0880917aecb5d2eaf8de43003d047d8dc6bde","unresolved":false,"context_lines":[{"line_number":61,"context_line":"                pass"},{"line_number":62,"context_line":"        network \u003d network[\u0027network\u0027]"},{"line_number":63,"context_line":"        if not cidr:"},{"line_number":64,"context_line":"            cidr \u003d self.CIDRS[0]"},{"line_number":65,"context_line":"        return self._create_subnet(self.fmt,"},{"line_number":66,"context_line":"                                   net_id\u003dnetwork[\u0027id\u0027],"},{"line_number":67,"context_line":"                                   tenant_id\u003dnetwork[\u0027tenant_id\u0027],"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_f1ad6f89","line":64,"updated":"2016-08-09 01:44:54.000000000","message":"Can\u0027t the arg on L57 just default to cidr\u003dself.CIDRS[0] ?","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":61,"context_line":"                pass"},{"line_number":62,"context_line":"        network \u003d network[\u0027network\u0027]"},{"line_number":63,"context_line":"        if not cidr:"},{"line_number":64,"context_line":"            cidr \u003d self.CIDRS[0]"},{"line_number":65,"context_line":"        return self._create_subnet(self.fmt,"},{"line_number":66,"context_line":"                                   net_id\u003dnetwork[\u0027id\u0027],"},{"line_number":67,"context_line":"                                   tenant_id\u003dnetwork[\u0027tenant_id\u0027],"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_752678cf","line":64,"in_reply_to":"9ad45d7e_f1ad6f89","updated":"2016-08-10 16:22:39.000000000","message":"It\u0027s not playing ball with having self.SOMETHING as a function parameter, so for now I think this part is necessary.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"041e70a0cb4c7c57f472552571ee07bb5bc23035","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_af68b8bb","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"updated":"2016-08-08 22:16:15.000000000","message":"Shouldn\u0027t we get an IpAddressGenerationFailure here instead of a successful port create?","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"ff7e1f1c32fb4c215f99dcfa9f4f7700a75fbfb1","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_889b0272","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"in_reply_to":"3ac371cc_85cb1972","updated":"2016-08-16 10:53:28.000000000","message":"For #1, I left something out. Let me try to be clearer.\n\nIf the network has no subnets, no error is returned. The port is created without any IPs. So, you\u0027re right.\n\nIf the network has subnets, *and the subnets have no IPs available*, then you\u0027ll get an error. I left out that important part, sorry for the omission.\n\nI guess I was thinking of this case like a network with subnets that don\u0027t have any IPs available for this port.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"d85377e6699b24cdf0167077624ee6b5931fc455","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_7bec47f3","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"in_reply_to":"3ac371cc_889b0272","updated":"2016-08-17 15:50:13.000000000","message":"So following our conversation at the midcycle, I\u0027m introduction a new ipam exception IpAddressGenerationFailureNoMatchingSubnet which subclasses IpAddressGenerationFailure. This will be thrown in two scenarios:\n1. A port is created with a device owner and neither a matching service subnet or a subnet without a service type exists.\n2. A port is created without a device owner and there are no subnets without service_types.\nIf neither scenario is true, and all eligible subnets are exhausted, then an IpAddressGenerationFailure is thrown as usual. If the network has no subnets at all, then the port is created without fixed_ips as usual.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_b2970feb","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"in_reply_to":"9ad45d7e_af68b8bb","updated":"2016-08-10 16:22:39.000000000","message":"That\u0027s a good question actually. With the way that _allocatate_ips_for_port and _ipam_get_subnets are curently written, what happens right now in this case is that _generate_ip is never called because _ipam_get_subnets returns an empty list. So we just end up with an empty list of ips and no exception. I\u0027m not sure what the implications of throwing an exception if _ipam_get_subnets returns an empty list would be. Presumably there are valid situations in which that should happen.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"332b7c2057d2192b093ff4dbbaa6f4db629e3905","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_d2ad519c","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"in_reply_to":"9ad45d7e_b2970feb","updated":"2016-08-11 18:16:04.000000000","message":"Sounds like we need to tread carefully here and not disturb existing use cases. Here are some of my thoughts. The following, I believe, are existing behaviors which should be preserved (hopefully they are tested.) I\u0027m currently building a devstack but it might not finish before I have to go today.\n\n1. If the user creates a port and doesn\u0027t pass fixed_ips an error is produced if and only if the network has subnets.\n2. If the user explicitly passes fixed_ips\u003d[] then Neutron doesn\u0027t allocate IPs and doesn\u0027t error.\n3. If the user passes fixed_ips and an IP can\u0027t be allocated for one of them, an error is produced.\n\nEnter service subnets: now that you\u0027ve got me thinking about this I\u0027m not sure what should happen. Should we think of it like the network has no subnets (#1 above) because there are no eligible ones? Or, should we think of it like a network with subnets but no IPs available to the port and produce an error? I was thinking it should be the latter but now I\u0027m not so sure.\n\nWhat do you think? I can think of arguments both ways. I was thinking that the presence of subnets on the network marked it an an L3 capable network and ports should get IP addresses.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"3fc32b4d5151b48c844a795896f809e466d44410","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                 device_owner\u003dmatching_type)"},{"line_number":246,"context_line":"        port \u003d self.deserialize(\u0027json\u0027, port)[\u0027port\u0027]"},{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_85cb1972","line":248,"range":{"start_line":248,"start_character":8,"end_line":248,"end_character":51},"in_reply_to":"9ad45d7e_d2ad519c","updated":"2016-08-15 19:05:15.000000000","message":"1. Did you mean the network has NO subnets? If a port is created on a network with no subnets, currently there is no error. Neutron will return a port without any fixed ips. If there are subnets, then fixed IPs are assigned if available.\n2. Yes, I believe that\u0027s the case.\n3. Ditto\n\nPersonally I see it as the first option - the network has no subnets. To me, an IPAddressGenerationFailure says that an attempt to assign an IP on an available subnet was made, but failed. In this case there are no subnets available for that attempt to be made.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":7448,"name":"Carl Baldwin","email":"carl@ecbaldwin.net","username":"carl-baldwin"},"change_message_id":"332b7c2057d2192b093ff4dbbaa6f4db629e3905","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_920bd9d5","line":250,"updated":"2016-08-11 18:16:04.000000000","message":"I can think of a couple more test cases that we should maybe add:\n\n3 subnets: non-matching service type, matching service type, no service type.\n\nThen we should have a couple of variations where we update allocation_pools to simulate some/all of eligible subnets being exhausted to test the fallback behavior. I\u0027m thinking specifically fallback to the non-service-type subnet and one where all subnets are exhausted and it produces an ip allocation error.\n\n [1] https://github.com/openstack/neutron/blob/ba3b4638d2a84e734b4e77759d5238c5edac9c46/neutron/tests/unit/extensions/test_segment.py#L1087","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"3fc32b4d5151b48c844a795896f809e466d44410","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_3d635fb3","line":250,"in_reply_to":"7ac969e8_affe1903","updated":"2016-08-15 19:05:15.000000000","message":"I don\u0027t anticipate any further changes to this function being needed for routed networks. What do you have in mind?","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"3fc32b4d5151b48c844a795896f809e466d44410","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"3ac371cc_84ff8d00","line":250,"in_reply_to":"9ad45d7e_920bd9d5","updated":"2016-08-15 19:05:15.000000000","message":"The 3 subnets case you describe is already covered by L176. Testing with some exhausted subnets is a good idea, thanks. I\u0027ll add that to my list for the next patchset.","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"7d2381fdea3eac2b71cba0e15319292c35a99e15","unresolved":false,"context_lines":[{"line_number":247,"context_line":"        # Assert that it is not given any fixed ips"},{"line_number":248,"context_line":"        self.assertEqual(0, len(port[\u0027fixed_ips\u0027]))"},{"line_number":249,"context_line":""},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"}],"source_content_type":"text/x-python","patch_set":2,"id":"7ac969e8_affe1903","line":250,"in_reply_to":"9ad45d7e_920bd9d5","updated":"2016-08-12 22:21:16.000000000","message":"The query created here https://review.openstack.org/#/c/350613/2/neutron/db/ipam_backend_mixin.py@589 will be built on for the case of routed networks. Would it be useful also to create a test case for that path?","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":1131,"name":"Brian Haley","email":"haleyb.dev@gmail.com","username":"brian-haley"},"change_message_id":"e7b0880917aecb5d2eaf8de43003d047d8dc6bde","unresolved":false,"context_lines":[{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"},{"line_number":254,"context_line":"    IP_VERSION \u003d 6"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_31d3873a","line":253,"updated":"2016-08-09 01:44:54.000000000","message":"nit: 2001:db8:1::/64, 2001:db8:2::/64...","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"52b44a0d3c569211ae109c9dbec6795b88225b3d","unresolved":false,"context_lines":[{"line_number":250,"context_line":""},{"line_number":251,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":252,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":253,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"},{"line_number":254,"context_line":"    IP_VERSION \u003d 6"}],"source_content_type":"text/x-python","patch_set":2,"id":"9ad45d7e_35136068","line":253,"in_reply_to":"9ad45d7e_31d3873a","updated":"2016-08-10 16:22:39.000000000","message":"Done","commit_id":"d8d12d392cfd9c4978c367370d2f9efcc3603a43"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"1ce72113a4e385d15498cc74c66fdcd8e4b88f71","unresolved":false,"context_lines":[{"line_number":298,"context_line":""},{"line_number":299,"context_line":"class SubnetServiceTypesExtensionTestCasev6("},{"line_number":300,"context_line":"        SubnetServiceTypesExtensionTestCase):"},{"line_number":301,"context_line":"    CIDRS \u003d [\u00272001:db8::/64\u0027, \u00272002:db8::/64\u0027, \u00272003:db8::/64\u0027]"},{"line_number":302,"context_line":"    IP_VERSION \u003d 6"}],"source_content_type":"text/x-python","patch_set":3,"id":"3ac371cc_c413cfb6","line":301,"range":{"start_line":301,"start_character":4,"end_line":301,"end_character":63},"updated":"2016-08-17 16:16:35.000000000","message":"A change to this line based on Brian\u0027s comment in PS2 was lost somewhere. Will include it after the next round of reviews.","commit_id":"cf88ef758ebcff76be81f58cf6d8874cfcbc9a52"}],"releasenotes/notes/add-subnet-service-types-bc81f6df9834f96e.yaml":[{"author":{"_account_id":4694,"name":"Miguel Lavalle","email":"miguel@mlavalle.com","username":"minsel"},"change_message_id":"0e2c77c1eb0ce6dac2e6148dcd2ee8018e9f4f66","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Subnets now have a new property \u0027service-types\u0027."},{"line_number":4,"context_line":"    This is a list of port device owners, such that"},{"line_number":5,"context_line":"    only ports with a matching device owner will be"},{"line_number":6,"context_line":"    given an IP from this subnet. If no matching"}],"source_content_type":"text/x-yaml","patch_set":10,"id":"1ac06dbe_322437ae","line":3,"range":{"start_line":3,"start_character":37,"end_line":3,"end_character":50},"updated":"2016-08-24 18:34:11.000000000","message":"Nit: It shows up as \u0027service_types\u0027: http://paste.openstack.org/show/563112/","commit_id":"79a128fcc6f31b299adf4f85d14941c4deb0be8e"},{"author":{"_account_id":6635,"name":"John Davidge","email":"john.davidge@rackspace.com","username":"john-davidge"},"change_message_id":"e0925300140c79ae6322280073d5fdec40f8e453","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Subnets now have a new property \u0027service-types\u0027."},{"line_number":4,"context_line":"    This is a list of port device owners, such that"},{"line_number":5,"context_line":"    only ports with a matching device owner will be"},{"line_number":6,"context_line":"    given an IP from this subnet. If no matching"}],"source_content_type":"text/x-yaml","patch_set":10,"id":"1ac06dbe_edf1226c","line":3,"range":{"start_line":3,"start_character":37,"end_line":3,"end_character":50},"in_reply_to":"1ac06dbe_322437ae","updated":"2016-08-24 19:03:33.000000000","message":"Done","commit_id":"79a128fcc6f31b299adf4f85d14941c4deb0be8e"}]}