)]}'
{"id":"openstack%2Fpuppet-ovn~779713","triplet_id":"openstack%2Fpuppet-ovn~master~Ib826152d215f13310b226138df2585a67c6cb1fd","project":"openstack/puppet-ovn","branch":"master","topic":"bug/1918418","hashtags":[],"change_id":"Ib826152d215f13310b226138df2585a67c6cb1fd","subject":"Add vlan_limit option to the ovn controller class","status":"ABANDONED","created":"2021-03-10 11:00:38.000000000","updated":"2021-03-10 14:13:35.000000000","total_comment_count":1,"unresolved_comment_count":1,"has_review_started":true,"meta_rev_id":"e34eb0d8a87c6d9f3bd62482bbd5959a0dc7907a","_number":779713,"virtual_id_number":779713,"owner":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"actions":{},"labels":{"Verified":{"disliked":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"all":[{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},{"tag":"autogenerated:zuul:check","value":-1,"date":"2021-03-10 11:33:27.000000000","permitted_voting_range":{"min":-2,"max":2},"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]}],"values":{"-2":"Fails","-1":"Doesn\u0027t seem to work"," 0":"No score","+1":"Works for me","+2":"Verified"},"description":"","value":-1,"default_value":0,"optional":true},"Code-Review":{"disliked":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"all":[{"value":-1,"date":"2021-03-10 11:22:10.000000000","permitted_voting_range":{"min":-2,"max":2},"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},{"value":-1,"date":"2021-03-10 11:10:41.000000000","permitted_voting_range":{"min":-1,"max":1},"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},{"value":0,"permitted_voting_range":{"min":-1,"max":1},"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]}],"values":{"-2":"Do not merge","-1":"This patch needs further work before it can be merged"," 0":"No score","+1":"Looks good to me, but someone else must approve","+2":"Looks good to me (core reviewer)"},"description":"","value":-1,"default_value":0,"optional":true},"Workflow":{"all":[{"value":0,"permitted_voting_range":{"min":-1,"max":1},"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]}],"values":{"-1":"Work in progress"," 0":"Ready for reviews","+1":"Approved"},"description":"","default_value":0,"optional":true}},"removable_reviewers":[],"reviewers":{"REVIEWER":[{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"}]},"pending_reviewers":{},"reviewer_updates":[{"updated":"2021-03-10 11:10:41.000000000","updated_by":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"reviewer":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"state":"REVIEWER"},{"updated":"2021-03-10 11:22:10.000000000","updated_by":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"reviewer":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"state":"REVIEWER"},{"updated":"2021-03-10 11:33:27.000000000","updated_by":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"reviewer":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"state":"REVIEWER"}],"messages":[{"id":"869658b45f2efb7eacb8caf144bcd7fbb2c2b222","tag":"autogenerated:gerrit:newPatchSet","author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"date":"2021-03-10 11:00:38.000000000","message":"Uploaded patch set 1.","accounts_in_message":[],"_revision_number":1},{"id":"2de2d9df0888e3bff7883cc24a1c422adba35944","author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"date":"2021-03-10 11:10:41.000000000","message":"Patch Set 1: Code-Review-1\n\nSince it\u0027s not an OVN specific option, I think this belongs to puppet-vswitch like for example:\n\nhttps://github.com/openstack/puppet-vswitch/blob/master/manifests/ovs.pp#L82","accounts_in_message":[],"_revision_number":1},{"id":"8fba9b3d54074ef21a9f39ecb5cb21d883497ece","author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"date":"2021-03-10 11:22:10.000000000","message":"Patch Set 1: Code-Review-1\n\n(1 comment)\n\n-1 to ask some thoughts about another version I have submitted.\n\n\u003e Patch Set 1: Code-Review-1\n\u003e \n\u003e Since it\u0027s not an OVN specific option, I think this belongs to puppet-vswitch like for example:\n\u003e \n\u003e https://github.com/openstack/puppet-vswitch/blob/master/manifests/ovs.pp#L82\n\nIIUC when we use ovn we don\u0027t rely on puppet-vswitch but puppet-ovn.\nSo we need to implement this to both modules.\n\nIn addition, I\u0027m not aware of any good use case where we want to change vlan limit in ovs, because we don\u0027t support vlan transparency with ovs.\nSo I think it would be enough if we implement this in puppet-ovn.\nPlease let me know if I misunderstand or miss something.","accounts_in_message":[],"_revision_number":1},{"id":"be8507b24e1962d426ba2049814c35d67df57cac","tag":"autogenerated:zuul:check","author":{"_account_id":22348,"name":"Zuul","username":"zuul","tags":["SERVICE_USER"]},"date":"2021-03-10 11:33:27.000000000","message":"Patch Set 1: Verified-1\n\nBuild failed (check pipeline).  For information on how to proceed, see\nhttps://docs.opendev.org/opendev/infra-manual/latest/developers.html#automated-testing\n\n\n- puppet-openstack-lint-ubuntu-bionic https://zuul.opendev.org/t/openstack/build/2998637a08794b24b4810d0dfdb86be8 : FAILURE in 4m 50s\n- puppet-openstack-syntax-6-ubuntu-bionic https://zuul.opendev.org/t/openstack/build/9728c78648d34ab198539701e50fdbf2 : SUCCESS in 4m 06s\n- puppet-openstack-unit-6.14-centos-8 https://zuul.opendev.org/t/openstack/build/4150025025d0495e8f5a4eef788aa59b : SUCCESS in 8m 41s\n- puppet-openstack-unit-6.14-centos-8-stream https://zuul.opendev.org/t/openstack/build/4fa7ac9ca06d4fccbb32918e878c3fdb : SUCCESS in 7m 59s (non-voting)\n- puppet-openstack-unit-6.14-ubuntu-bionic https://zuul.opendev.org/t/openstack/build/246023957fae4206b9dbbcc449c1fc5a : SUCCESS in 5m 19s\n- puppet-openstack-unit-latest-ubuntu-bionic https://zuul.opendev.org/t/openstack/build/3343f51078d541858540916330ff1e5d : SUCCESS in 6m 07s (non-voting)\n- puppet-openstack-litmus-centos-8 https://zuul.opendev.org/t/openstack/build/d48d753e67af4b81ae940b3bdd611263 : SUCCESS in 13m 28s\n- puppet-openstack-litmus-centos-8-stream https://zuul.opendev.org/t/openstack/build/6c9a39a9cf1940c38243b890f8454240 : SUCCESS in 13m 37s (non-voting)\n- puppet-openstack-litmus-ubuntu-bionic https://zuul.opendev.org/t/openstack/build/183ab05724ae4375aaa628d98cdf0716 : SUCCESS in 11m 27s (non-voting)","accounts_in_message":[],"_revision_number":1},{"id":"40e3e26440eabb52761baf3fec335726f50bfb4b","author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"date":"2021-03-10 11:45:42.000000000","message":"Patch Set 1:\n\nThanks Takashi,\n\n\u003e IIUC when we use ovn we don\u0027t rely on puppet-vswitch but puppet-ovn.\n\u003e So we need to implement this to both modules.\n\nThe other_config option is an OpenvSwitch option and affects all OVS bridges, not just the one managed by OVN. This is why I believe that the option should go into puppet-vswitch solely.\n\n\u003e \n\u003e In addition, I\u0027m not aware of any good use case where we want to change vlan limit in ovs, because we don\u0027t support vlan transparency with ovs.\n\nWhen you say \"we don\u0027t support vlan transparency with ovs\" I gather that you refer to ML2/OVS, ie. the Neutron mechanism driver. Which is accurate but it doesn\u0027t mean that it belongs to puppet-ovn. IMO, since it\u0027s a pure OVS option, the ability to drive it should be added into puppet-vswitch.\n\nHowever, as you correctly point out, only ML2/OVN deployments (as of today) will leverage such option. \nIn the future, if other drivers need to use it, it\u0027ll be there in the OVS common layer.\n\nPlease let me know if the above makes sense.","accounts_in_message":[],"_revision_number":1},{"id":"3f71304edb39e8f0813da30bdd8600151e5247ac","author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"date":"2021-03-10 12:25:24.000000000","message":"Patch Set 1:\n\nThanks Daniel for your clarification.\n\nI think I had some confusion about relations between ovn::controller and vswitch::ovs.\n\nI found that both class managed other_config:hw-offload thus I thought we should implement the parameter independently in both modules, but because we \"require\" vsiwitch::ovs from ovn::controller that was a wrong assumption.\n\nConsidering the parameter is common for ovs and ovn it would make sense to implement the parameter in vswitch::ovs, but the problem would be that vswitch::ovs is not included if dpdk is enabled then the parameter doesn\u0027t work with ovn+dpdk deployment.\n\nI\u0027m afraid we need to come up with a better handling about such common parameter used in ovs/ovn/ovn+dpdk and implement vlan-limit accordingly (and update current implementation of hw_offload as well... )","accounts_in_message":[],"_revision_number":1},{"id":"62a427133998b030988b070217b61797476c3dd2","author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"date":"2021-03-10 13:40:25.000000000","message":"Patch Set 1:\n\n\u003e Patch Set 1:\n\u003e \n\u003e Thanks Daniel for your clarification.\n\u003e \n\u003e I think I had some confusion about relations between ovn::controller and vswitch::ovs.\n\u003e \n\u003e I found that both class managed other_config:hw-offload thus I thought we should implement the parameter independently in both modules, but because we \"require\" vsiwitch::ovs from ovn::controller that was a wrong assumption.\n\u003e \n\u003e Considering the parameter is common for ovs and ovn it would make sense to implement the parameter in vswitch::ovs, but the problem would be that vswitch::ovs is not included if dpdk is enabled then the parameter doesn\u0027t work with ovn+dpdk deployment.\n\u003e \n\u003e I\u0027m afraid we need to come up with a better handling about such common parameter used in ovs/ovn/ovn+dpdk and implement vlan-limit accordingly (and update current implementation of hw_offload as well... )\n\nThx for all Your feedback but now I\u0027m a bit lost - do You think I should move it to puppet-vswitch module or keep it here for now? I\u0027m fine with both solutions.","accounts_in_message":[],"_revision_number":1},{"id":"1a33f4dc54bacedd5fb1d4cb4e6d16857801d403","author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"date":"2021-03-10 13:49:04.000000000","message":"Patch Set 1:\n\n\u003e Patch Set 1:\n\u003e \n\u003e \u003e Patch Set 1:\n\u003e \u003e \n\u003e \u003e Thanks Daniel for your clarification.\n\u003e \u003e \n\u003e \u003e I think I had some confusion about relations between ovn::controller and vswitch::ovs.\n\u003e \u003e \n\u003e \u003e I found that both class managed other_config:hw-offload thus I thought we should implement the parameter independently in both modules, but because we \"require\" vsiwitch::ovs from ovn::controller that was a wrong assumption.\n\u003e \u003e \n\u003e \u003e Considering the parameter is common for ovs and ovn it would make sense to implement the parameter in vswitch::ovs, but the problem would be that vswitch::ovs is not included if dpdk is enabled then the parameter doesn\u0027t work with ovn+dpdk deployment.\n\u003e \u003e \n\u003e \u003e I\u0027m afraid we need to come up with a better handling about such common parameter used in ovs/ovn/ovn+dpdk and implement vlan-limit accordingly (and update current implementation of hw_offload as well... )\n\u003e \n\u003e Thx for all Your feedback but now I\u0027m a bit lost - do You think I should move it to puppet-vswitch module or keep it here for now? I\u0027m fine with both solutions.\n\nBTW. I will abandon that patch and lets go with https://review.opendev.org/c/openstack/puppet-ovn/+/779421 :)","accounts_in_message":[],"_revision_number":1},{"id":"851bb5b8dde739395cc2cf0916aa4945164a5b7c","tag":"autogenerated:gerrit:abandon","author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"date":"2021-03-10 13:49:19.000000000","message":"Abandoned\n\nhttps://review.opendev.org/c/openstack/puppet-ovn/+/779421 is better approach, let\u0027s go with that one","accounts_in_message":[],"_revision_number":1},{"id":"8e9c196f127f147a70228ef1f8d9cfffc65a61e6","author":{"_account_id":23804,"name":"Daniel Alvarez","email":"dalvarez@redhat.com","username":"dalvarez"},"date":"2021-03-10 13:53:46.000000000","message":"Patch Set 1:\n\nThanks Takashi for the discussion.\n\n\u003e Considering the parameter is common for ovs and ovn it would make sense to implement the parameter in vswitch::ovs, but the problem would be that vswitch::ovs is not included if dpdk is enabled then the parameter doesn\u0027t work with ovn+dpdk deployment.\n\n\nThe dpdk config bits in puppet still belong to puppet-vswitch [0] so whoever drives vswitch::dpdk it makes sense to drive vswitch::ovs::vlan_limit accordingly as it\u0027s not dependent on the datapath used.\n\nSimilarly, the hardware offloading feature is now expected to be enabled through vswitch::ovs::enable_hw_offload for both kernel and dpdk datapaths [1].\n\nI see it no differently for the vlan_limit parameter that we\u0027re trying to introduce.\n\n\n[0] https://opendev.org/openstack/puppet-vswitch/src/branch/master/manifests/dpdk.pp\n[1] https://opendev.org/openstack/tripleo-heat-templates/src/branch/master/deployment/neutron/neutron-ovs-agent-container-puppet.yaml#L258-L262","accounts_in_message":[],"_revision_number":1},{"id":"e34eb0d8a87c6d9f3bd62482bbd5959a0dc7907a","author":{"_account_id":9816,"name":"Takashi Kajinami","email":"kajinamit@oss.nttdata.com","username":"kajinamit"},"date":"2021-03-10 14:13:35.000000000","message":"Patch Set 1:\n\nAfter investigating current usage in TripleO I now understand the situation.\n\nSo current TripleO doesn\u0027t set ovn::controller::enable_dpdk but only datapath_type. I\u0027ve not yet identified how it works without vswitch::dpdk class but because of that definitions we always include vswitch::ovs. In such case we don\u0027t rally need to implement the parameter in ovn::controller. We just need to implement the parameter to ovs::vswitch and can use its parameters.\n\nHowever the problem still exists in any non-TripleO case with ovn::controller::enable_dpdk: true is set. In this case, vswitch::ovs is not included buy vswitch::dpdk is included. This means that any parameter in vswitch::ovs doesn\u0027t work.\n\nSo to solve the problem only in TripleO deployment I think addint the parameter to vswitch::ovs and use that parameter should be enough. We can replace existing usage of ovn::controller::enable_hw_offlowad by vswitch::ovs::enable_hw_offload, unless we have any plan to use ovn::controller::enable_dpdk.\n\nWe do definitely need to come up with the better handling when enable_dpdk is used but we can leave it now maybe.","accounts_in_message":[],"_revision_number":1}],"current_revision_number":1,"current_revision":"0f896d62f21e9f0b4df7d16a090c40c9c5c9ce4d","revisions":{"0f896d62f21e9f0b4df7d16a090c40c9c5c9ce4d":{"kind":"REWORK","_number":1,"created":"2021-03-10 11:00:38.000000000","uploader":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"ref":"refs/changes/13/779713/1","fetch":{"anonymous http":{"url":"https://review.opendev.org/openstack/puppet-ovn","ref":"refs/changes/13/779713/1","commands":{"Checkout":"git fetch https://review.opendev.org/openstack/puppet-ovn refs/changes/13/779713/1 \u0026\u0026 git checkout FETCH_HEAD","Cherry Pick":"git fetch https://review.opendev.org/openstack/puppet-ovn refs/changes/13/779713/1 \u0026\u0026 git cherry-pick FETCH_HEAD","Format Patch":"git fetch https://review.opendev.org/openstack/puppet-ovn refs/changes/13/779713/1 \u0026\u0026 git format-patch -1 --stdout FETCH_HEAD","Pull":"git pull https://review.opendev.org/openstack/puppet-ovn refs/changes/13/779713/1"}}},"commit":{"parents":[{"commit":"3aa6ee0de90a2e5e7f57224c8ea5a641967fba86","subject":"Prepare Wallaby M2","web_links":[{"name":"gitea","tooltip":"Open in GitWeb","url":"https://opendev.org/openstack/puppet-ovn/commit/3aa6ee0de90a2e5e7f57224c8ea5a641967fba86"}]}],"author":{"name":"Slawek Kaplonski","email":"skaplons@redhat.com","date":"2021-03-10 10:57:46.000000000","tz":0},"committer":{"name":"Slawek Kaplonski","email":"skaplons@redhat.com","date":"2021-03-10 11:00:33.000000000","tz":0},"subject":"Add vlan_limit option to the ovn controller class","message":"Add vlan_limit option to the ovn controller class\n\nIt configures other_config:vlan-limit option in the ovsdb on nodes.\nBy default it sets value 1 which is also default in ovsdb.\n\nIn case when vlan_transparent is enabled in Neutron this value should be\nset to 0 or eventually 2 to make double tagged networks working with\nsecurity groups properly.\n\nRelated-Bug: #1918418\nChange-Id: Ib826152d215f13310b226138df2585a67c6cb1fd\n","web_links":[{"name":"gitea","tooltip":"Open in GitWeb","url":"https://opendev.org/openstack/puppet-ovn/commit/0f896d62f21e9f0b4df7d16a090c40c9c5c9ce4d"}],"resolve_conflicts_web_links":[{"name":"gitea","tooltip":"Open in GitWeb","url":"https://opendev.org/openstack/puppet-ovn/commit/0f896d62f21e9f0b4df7d16a090c40c9c5c9ce4d"}]},"branch":"refs/heads/master"}},"requirements":[],"submit_records":[],"submit_requirements":[]}