14:00:49 <ralonsoh> #startmeeting neutron_drivers
14:00:49 <opendevmeet> Meeting started Fri Apr 21 14:00:49 2023 UTC and is due to finish in 60 minutes.  The chair is ralonsoh. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:00:49 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:00:49 <opendevmeet> The meeting name has been set to 'neutron_drivers'
14:00:50 <ralonsoh> hello
14:01:03 <lajoskatona> o/
14:01:16 <slaweq> o/
14:01:35 <rubasov> o/
14:02:17 <ralonsoh> ok, let's start
14:02:18 <obondarev> o/
14:02:24 <sahid_> o/
14:02:30 <ralonsoh> first topic is
14:02:34 <ralonsoh> #link https://bugs.launchpad.net/neutron/+bug/2013228
14:02:39 <ralonsoh> [rfe] Non-admin users unable to create SR-IOV switch dev ports
14:02:44 <ralonsoh> I'll present it
14:02:57 <ralonsoh> the idea, discussed during this PTG and the previous one
14:03:04 <ralonsoh> is to have another port extension
14:03:20 <ralonsoh> a flag, that will enable/disable the HW offload
14:03:36 <ralonsoh> instead of writing in the port binding, that is something Neutron shouldn't do
14:03:59 <ralonsoh> any backend supporting HWOL, will read it and create the corresponding interface
14:04:16 <ralonsoh> (that means communication with Nova, that needs to support this extension)
14:04:55 <ralonsoh> so that's the feature. That will require a spec because: we create a new API extension, DB changes, changes in 2 backends and Nova-Neutron sync
14:05:38 <lajoskatona> nova-spec also?
14:05:44 <ralonsoh> I don't think so
14:05:47 <lajoskatona> akc
14:05:49 <lajoskatona> ack
14:05:56 <ralonsoh> but this is something I can check with Nova folks
14:07:20 <sahid_> I may not understand all, but insn't that somehing whcih should be controled by nova, having a port created using HW offload?
14:07:36 <ralonsoh> no, the port is created by Neutron
14:07:41 <ralonsoh> the DB port
14:07:53 <ralonsoh> Nova, reading the Neutron port, creates the L1 port
14:08:17 <ralonsoh> ah, this flag will be by default admin only but configurable via policies
14:08:17 <slaweq> so currently this information is in binding profile and that's how nova and other backends expects it, right?
14:08:24 <ralonsoh> yes
14:09:00 <ralonsoh> well, the backend is controlled by Neutron, so that is something to be changed in our side
14:09:16 <ralonsoh> the only "external" issue is the info passed to Nova
14:09:30 <ralonsoh> to pass to os-vif the correct way to create the L1 port
14:09:40 <slaweq> why can't we add new extension to add new port's attribute for that so users can use it but then internally translate it into binding profile and send to e.g. nova?
14:09:42 <slaweq> that way impact of the change would be smaller, am I correct?
14:10:20 <ralonsoh> right, but the secondary goal of this feature was to avoid writing in the por tbinding dict
14:10:28 <ralonsoh> something that should be done only by nova
14:10:30 <ralonsoh> but
14:10:38 <ralonsoh> we can implement that in two phases
14:10:46 <ralonsoh> 1) add the port extension + port binding
14:11:07 <ralonsoh> 2) in the next release, if nova implements the port extension, remove the port binding stuff
14:11:24 <slaweq> yeah, IIUC what You actuallywant to do are 2 different things
14:11:33 <ralonsoh> yes
14:11:37 <slaweq> 1. new extension to allow users setting this HWOL flag and
14:11:45 <slaweq> 2. change/improve communication with nova
14:11:47 <ralonsoh> yes
14:12:00 <ralonsoh> and also allow non-admin users to create HWOL ports
14:12:13 <ralonsoh> **if** the admin allows that in the policies
14:12:24 <slaweq> yes, but that's kind of related to 1)
14:12:26 <mlavalle> and without touching the port binding
14:12:30 <ralonsoh> yes
14:13:56 <slaweq> yes, but that's kind of related to 1)
14:14:02 <mlavalle> honestly, the rfe is not as clear as what we've discussing here
14:14:15 <ralonsoh> no, it isn't
14:14:24 <lajoskatona> agree
14:14:36 <obondarev> same feeling
14:14:39 <ralonsoh> if approved, I'll add a comemnt of what should be implemented and what the spec will contain
14:14:52 <mlavalle> right, I was about to suggest that
14:15:01 <mlavalle> to make clear what we understood
14:15:06 <ralonsoh> I prefer no to change the description, that is not mine
14:15:15 <mlavalle> that's fine
14:15:22 <slaweq> ++
14:15:24 <lajoskatona> shall we expect johnthetubaguy to work on this?
14:15:29 <ralonsoh> no
14:15:30 <mlavalle> just in a coment leave in black and white our understanding
14:15:35 <ralonsoh> but I can ask him
14:15:49 <sahid_> why this was admin only from the beginning? something was just missing or it was legitimate?
14:16:00 <ralonsoh> port binding is admin only
14:16:03 <ralonsoh> and not configurable
14:16:44 <mlavalle> so who would write the spec?
14:16:58 <ralonsoh> I'll do
14:17:05 <ralonsoh> and I'll implement the feature
14:17:22 <obondarev> +1 then :)
14:17:24 <ralonsoh> but if you agree on the description given here
14:17:27 <slaweq> so I'm +1 for this RFE but for point 1) for now
14:17:38 <ralonsoh> ok, I'll add the description of the 2 phases
14:17:42 <slaweq> I think that neutron-nova thing is another story
14:17:52 <ralonsoh> that will be easy to implement
14:18:10 <slaweq> I'm also fine with that but this should probably be discussed separately in nova-neutron session probably
14:18:14 <ralonsoh> for sure
14:18:32 <ralonsoh> we can change the Neutron code isolating the neutron-nova communication
14:20:10 <ralonsoh> any other question?
14:20:30 <slaweq> nothing from me
14:20:41 <lajoskatona> not from me
14:20:46 <mlavalle> no, I think enabling users to create hwol ports, under controls, is a worthy cause
14:20:47 <lajoskatona> I agree with this plan
14:20:50 <ralonsoh> then please vote for this RFE
14:21:03 <mlavalle> +1
14:21:03 <lajoskatona> +1 with the  spec
14:21:18 <mlavalle> yeap, spec is needed
14:21:50 <ralonsoh> slaweq, obondarev ?
14:21:56 <obondarev> +1
14:22:02 <slaweq> +1
14:22:13 <ralonsoh> so approved with spec, including the description done in this conversation (and the 2 phases plan)
14:22:19 <ralonsoh> thank you all
14:22:24 <ralonsoh> next one
14:22:34 <ralonsoh> #link https://bugs.launchpad.net/neutron/+bug/2016197
14:22:39 <ralonsoh> neutron can create port from network which has no subnet
14:22:40 <ralonsoh> related to
14:22:45 <ralonsoh> #link https://bugs.launchpad.net/neutron/+bug/2016198
14:23:15 * sahid_ checked the spec to this why it has been implemented in that way considering admin only, but no mentions of that
14:24:06 <ralonsoh> OK, Liu is not here but I'll start the conversation
14:24:26 <ralonsoh> regardless of seeing the problem in lp#2016198
14:24:47 <ralonsoh> I don't think we should block the creation of a port in a network without subnets
14:25:10 <slaweq> if we already have it like that, I don't think we can remove it as it would be backward incompatible
14:25:11 <slaweq> and we can break someone's use case even if we don't know about it
14:25:18 <ralonsoh> although, being said, I don't see any useful thing
14:25:40 <ralonsoh> yeah, that's the point, this is something that the current API allows
14:25:48 <ralonsoh> to be honest I don't see any usecase
14:25:53 <ralonsoh> but is allowd
14:25:57 <slaweq> subnet is just our internal thing used for IPAM - network and port are things which IMO may be treated as representation of things from "physical" world
14:25:58 <sahid_> removing this will be an api breaking
14:26:08 <obondarev> seems like HA routers bug can be fixed with just checking HA network subnets
14:26:13 <slaweq> sahid_++
14:26:25 <ralonsoh> obondarev, right!
14:27:09 <ralonsoh> but this call should be done inside the same context creating the subnet
14:27:15 <ralonsoh> just to be transactional
14:27:16 <slaweq> maybe we should do something like allocating IP addresses to ports when subnet is created
14:27:33 <slaweq> something like:
14:27:54 <slaweq> for port in network.ports:
14:28:06 <slaweq> if not port.fixed_ips:
14:28:10 <slaweq> port.allocate_ip()
14:28:23 <mlavalle> ^^^inside subnet creation
14:28:25 <slaweq> which would be done after subnet creation
14:28:26 <obondarev> can it  be undesired behaviour for those using no-IP ports on purpose?
14:28:42 <ralonsoh> but we can have a subnet withtout ports
14:29:00 <slaweq> obondarev so we can add maybe config knob to disable this new behavior
14:29:26 <slaweq> but use it for HA networks mentioned in https://bugs.launchpad.net/neutron/+bug/2016198
14:29:43 <obondarev> yeah, but if the only reason to do this is HA bug - I'd still fix race condition in traditional way
14:30:07 <obondarev> by adding wait condition I mean
14:30:16 <ralonsoh> if this is a race condition, then the best way is to use somehow the DB engine
14:30:33 <mlavalle> I agree with ralonsoh
14:30:39 <opendevreview> Slawek Kaplonski proposed openstack/neutron master: [S-RBAC] Switch to new policies by default  https://review.opendev.org/c/openstack/neutron/+/879827
14:30:39 <obondarev> makes sense
14:31:09 <slaweq> ok, so for HA networks bug there are for sure better solutions and ways to fix it
14:31:11 <mlavalle> at the same time I agree with obondarev that we shouldn't add features to fix a bug
14:31:36 <slaweq> and for RFE I'm -1 to remove this functionality from our API
14:32:08 <ralonsoh> -1, agree, there should be another way to prevent this race condition
14:32:09 <obondarev> so I guess there are several ways to fix the bug, and disallow no-IP port creation seems an overkill
14:32:28 <ralonsoh> is there something that identifies an HA network?
14:32:31 <obondarev> -1 for RFE
14:32:35 <lajoskatona> agree, there should be users who depend on it
14:32:39 <ralonsoh> that makes it different from other HA networks?
14:32:44 <slaweq> ralonsoh nothing except name IIRC
14:33:02 <mlavalle> it's just a network
14:33:22 <ralonsoh> but is created for a router, right?
14:33:31 <slaweq> it's created for tenant
14:33:38 <ralonsoh> for a tenant, sorry
14:33:39 <ralonsoh> yes
14:33:58 <ralonsoh> so there we are: we can add a name check in the network creation
14:34:07 <ralonsoh> in the DB engine
14:34:36 <slaweq> but internally it can be identified easy
14:34:43 <slaweq> it has own OVO object https://github.com/openstack/neutron/blob/47d070c71e795e41e698cdb278d99dcfb3448bde/neutron/objects/l3_hamode.py#L58
14:34:58 <ralonsoh> so there it is!!
14:35:01 <slaweq> and there is method to find it https://github.com/openstack/neutron/blob/master/neutron/db/l3_hamode_db.py#L97
14:35:15 <ralonsoh> we can't have more that one L3HARouterNetwork per project
14:35:21 <ralonsoh> right?
14:35:25 <slaweq> yes
14:35:44 <ralonsoh> so perfect, this condition is perfect for the DB engine
14:35:56 <slaweq> https://github.com/openstack/neutron/blob/master/neutron/db/models/l3ha.py#L62
14:36:37 <mlavalle> the primary key is the project
14:36:47 <ralonsoh> but not unique
14:36:59 <ralonsoh> we need a new class for this
14:37:09 <ralonsoh> but easily doable
14:37:31 <ralonsoh> what do you think about proposing that in the LP bugs?
14:37:59 <mlavalle> yes, let's do it
14:38:05 <slaweq> ++
14:38:34 <obondarev> +
14:38:43 <lajoskatona> +1
14:38:45 <ralonsoh> perfect, thank you all, good stuff!
14:38:52 <ralonsoh> last topic
14:38:56 <ralonsoh> mlavalle: Change in implementation of metadata rate limiting vs spec in order to support IPv6
14:38:58 <ralonsoh> please
14:39:29 <mlavalle> I've continued the implementation of this feature after the original proposer indicated he couldn't continue doing it
14:39:58 <opendevreview> Slawek Kaplonski proposed openstack/neutron master: Fix functional tests modules which are using PluginFixture  https://review.opendev.org/c/openstack/neutron/+/881228
14:40:00 <mlavalle> the spec,  https://specs.openstack.org/openstack/neutron-specs/specs/2023.1/metadata-rate-limit.html, didn't address ipv6
14:40:36 <mlavalle> ipv6 came up in one comment from ralonsoh to the proposed code
14:40:55 <mlavalle> so addressing that comment, I've added ipv6 support
14:41:36 <mlavalle> there is an issue, though. the opensource haproxy implementation only allows the tracking of 3 sticky counters
14:42:36 <mlavalle> to implement the spec with rate limiting for ipv4 and ipv6 at he same time, we would need 4 sticky counters
14:43:19 <mlavalle> btw, in line 79 here you can see the haproxy limitation: https://paste.openstack.org/show/bAE6IhPh4fQOGqFoxmMu/
14:43:32 <mlavalle> so I'm proposing a slight change to the spec
14:44:11 <mlavalle> the deployer would need to decide whether the want to rate limit for ipv4 or ipv6, but not both at the same time
14:44:34 <mlavalle> that was, we only need two sticky counters
14:44:57 <slaweq> is that something what can be changed in haproxy in future?
14:45:09 <lajoskatona> hmmm, sad but reasonable limitation
14:45:26 <slaweq> TBH I don't think we need to have it for IPv6 for now
14:45:30 <slaweq> it's not that widely used
14:45:32 <mlavalle> their documentation explicitely indicates that the open source version only allows 3 sticky counters
14:45:37 <obondarev> better than just limiting to ipv4
14:45:54 <slaweq> IPv4 should be more important
14:46:04 <rubasov> I agree that currently we force the deployer to choose between ipv4 and ipv6, however I'd suggest to choose a config format that can allow both in the future, if/when we can support both at the same time
14:46:08 <mlavalle> with my proposal we support both, just not both at the same time
14:46:45 <lajoskatona> +1
14:47:05 <slaweq> ok
14:47:11 <lajoskatona> I mean +1 for cfg option which can be used for allowing both
14:47:22 <ralonsoh> +1, this is an external limitation and we provide to the user the ability to define which one is needed
14:48:11 <ralonsoh> so I think we agree on the proposal
14:48:17 <obondarev> +1
14:48:28 <ralonsoh> mlavalle, make a summary of this conversation in the LP bug
14:48:30 <lajoskatona> +1, thanks mlavalle
14:48:34 <ralonsoh> and thanks!
14:48:49 <mlavalle> thanks, will do
14:48:57 <ralonsoh> so that's all, anything else you want to discuss?
14:49:27 <ralonsoh> have a nice weekend!
14:49:31 <mlavalle> o/
14:49:31 <ralonsoh> #endmeeting