openstack
/
neutron-specs
Code Issues Proposed changes
neutron-specs/84739ce8d7673bc2d14ee235294...

236 lines
13 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "590bda54_6812fc6c",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-01T16:32:40Z",
"side": 1,
"message": "It seems to me that neutron\u0027s ovn sync utility, should in general be designed in such a way that it only touches ports that it *knows* are neutron\u0027s as opposed to having to affirmatively declare things that it should ignore.",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "040b44a3_7f0e30db",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-01T17:12:23Z",
"side": 1,
"message": "To be more specific, I think most neutron-owned objects have very identifiable external_ids already. If any were missing that, it\u0027d be easy to add. It just seems like if there is a general issue of neutron-ovn-db-sync deleting things it doesn\u0027t own, special handling in two different ways (ignoring ovn-ic-specific external_ids, and then addding a more generic \"hey, ignore this!\" external-id) would be unnecessary and also leave it open for future problems down the road where some other thing creates objects in the ovn db, etc.",
"parentUuid": "590bda54_6812fc6c",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "24f78c8b_7629deda",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-01T17:13:54Z",
"side": 1,
"message": "Re: my previous comments, at least until they\u0027re discussed.",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "a9e38b9c_4124b982",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 35432
},
"writtenOn": "2023-11-01T18:05:49Z",
"side": 1,
"message": "Hi Terry, thanks for the review.\n\nyeah, my initial proposal was with this main idea, as you can see in the related LP BUG. However, after many discussions with Neutron\u0027 core members, this generic solution is not seen as viable since the source of truth would be the neutron DB, and it could get out of control or generate inconsistencies at some point (overlaps, collisions, etc). Other people have asked about the same question, I\u0027m sorry I don\u0027t have a better answer.\n\nIf it\u0027s consensus with the community members that the implementation could be generic, perfect! I can change the RFE without any problems.\n\nReviwers:\nWould it be possible to make it generic and have Neutron only manage its own resources in db_sync?",
"parentUuid": "040b44a3_7f0e30db",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "c39d285a_0e9ec3e3",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 35432
},
"writtenOn": "2023-11-01T18:37:23Z",
"side": 1,
"message": "BTW Although I started with this idea, I changed my mind along the way, as we have a number of issues on the OVN side that can cause resources duplication and strange behaviours [1,2].\n\nA generic implementation would make Neutron unpredictable and susceptible to BUGs on the OVN side.\n\nDoes it make sense to you?\n\n\n[1] https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/1961046\n[2] https://bugs.launchpad.net/neutron/+bug/1960006/comments/7",
"parentUuid": "a9e38b9c_4124b982",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "1891e985_ffd0b5c8",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-01T21:49:09Z",
"side": 1,
"message": "I\u0027m not sure I understand how refusing to delete non-neutron ports(i.e. ports that don\u0027t have a neutron-specific external_id) leads to stale ports or inconsistencies. \"Stale ports\" in the ovn db would have neutron-specific external_ids, right? I\u0027ll try to read more carefully and see if I can figure out what people are afraid of. Maybe if previous versions that were missing the particular external_id or whatever identifying characteristic? But especially with ports, those external_ids have been around for years and years if not for as long as ml2/ovn.\n\nFrom what I can tell, *most* things that are neutron-related in the OVN DB have some kind of neutron:... external id. provnet ports don\u0027t (but look like they\u0027re handled as a special case in ovn-db-sync), router gateway NAT entries don\u0027t. Most have a neutron:revision_number (ACLs/Port_Groups don\u0027t have this).",
"parentUuid": "c39d285a_0e9ec3e3",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "4ab8f795_3f21e01d",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 35432
},
"writtenOn": "2023-11-02T01:50:58Z",
"side": 1,
"message": "Thanks for bringing up this good discussion!\n\nI\u0027m sorry if I made you confused with my previouslly example. The point I tried to exemplify was not this specific bug but the possibility that the OVN code has a bug that could leave something inconsistent. And you\u0027re absolutely right, Neutron will always create these resources with a very specific \u0027signature\u0027 in the external_ids (neutron:...). \n\nAs you probably know, there are two classic resources repair cases used by DB sync: \u0027found in Neutron but not in OVN\u0027, and \u0027found in OVN but not in Neutron\u0027. I was trying to understand \u0027when and why\u0027 the \"found in OVN but not in Neutron\" comparison part was included but looking at the current code [1] (it\u0027s been as long as ml2/ovn), and in the networking_ovn code[2][3][4] it doesn\u0027t seem to have a specific reason. I mean, it\u0027s probably always been this way since the beginning of ml2/ovn.\n\nPerhaps most (if not all) sync_repair periodic executions by operators are related to resources created by Neutron that have become inconsistent and need to be restored (seems to be the stale ports case). So this is more about existing in the Neutron and not the OVN than the opposite.\n\nI don\u0027t have a strong final opinion on this, I would like to hear other points of view.\n\n[1] https://opendev.org/openstack/neutron/blame/commit/1daa0dd5bfa06f44cc49662aa97d3c7a7f4d36e4/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovn_db_sync.py#L714\n\n[2] https://opendev.org/openstack/networking-ovn/blame/commit/e2a23d69d70079f09555789b22dc1fd088242bcb/networking_ovn/ovn_db_sync.py#L406\n[3] https://opendev.org/openstack/networking-ovn/commit/e31919f3eef4f4aaf16f0992dc32ed4e5554023a\n[4] https://bugs.launchpad.net/networking-ovn/+bug/1546315",
"parentUuid": "1891e985_ffd0b5c8",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "1df0696f_fae75162",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 1131
},
"writtenOn": "2023-11-21T14:24:09Z",
"side": 1,
"message": "So after talking on IRC, I think we all agreed that the DB sync tool (and technically any other part of neutron, see the OVN LB recent change) shouldn\u0027t be removing resources it didn\u0027t add to the OVN DB.\n\nIn the current form of this spec, we\u0027d change it to detect things related to OVN-IC and leave them alone.\n\nThe problem is that for the next OVN-XYZ spec we\u0027ll add another thing, and so-on. I tend to agree that making it smarter by having it look for a neutron \"fingerprint\" is probably a better long-term solution.\n\nIt\u0027s really an implementation detail on how we change things in my opinion, again, assuming it can be be done the other way.\n\nDid I summarize that correctly? Do other reviewers agree?",
"parentUuid": "4ab8f795_3f21e01d",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": true,
"key": {
"uuid": "9cebd8ce_8d1e35f1",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 15554
},
"writtenOn": "2023-11-21T14:40:34Z",
"side": 1,
"message": "I did not comment earlier, but when I read this spec, the same was also my first question: Why don\u0027t we explicitly track who owns what in the ovn db? Instead of creating process specific ignore-flags we could track the ownership of records and make sure the db sync tool (starting from a particular version) only ever touches neutron-owned resources. If not all neutron created resources have a readily identifiable way to tell this, I believe the db sync tool itself could be modified to correct this situation one-time during an upgrade. While I\u0027m not familiar with the implementation details, I hope this would not be a complicated change.\n\nIn short: +1 for the generic way.",
"parentUuid": "1df0696f_fae75162",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "c4dd7f5a_2001e452",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 35432
},
"writtenOn": "2023-11-27T20:54:28Z",
"side": 1,
"message": "+1\n\nAFAIK all resources created by neutron via OVSDB IDL API have the \u0027neutron:\u0027 key in the external_ids register. So, it would be a simple change and would not require one-time adjustments during an upgrade.",
"parentUuid": "9cebd8ce_8d1e35f1",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "42e389ed_6053879b",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-28T15:11:57Z",
"side": 1,
"message": "Last I checked, provnet ports don\u0027t (but look like they\u0027re handled as a special case in ovn-db-sync), I see some related constants like OVN_PHYSNET_EXT_ID that aren\u0027t used. Router gateway NAT entries don\u0027t have a neutron: key. Most specifically have a neutron:revision_number (ACLs/Port_Groups don\u0027t have this).",
"parentUuid": "c4dd7f5a_2001e452",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "6e484f26_865e2ef7",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 35432
},
"writtenOn": "2023-11-28T15:34:36Z",
"side": 1,
"message": "Sure, the provnet ports are some special case in ovn-db-sync... but NAT/ACLs/Port_Groups have the external_ids register in the OVSDB schema [1][2][3], so it would be a case of including the neutron key if it does not exist. It can be any key that starts with \u0027neutron\u0027, including the neutron:revision_number.\n\n[1] https://github.com/ovn-org/ovn/blob/06c0a4a3ff23c3ffb25a502b1cf318e44e5e683a/ovn-nb.ovsschema#L283\n[2] https://github.com/ovn-org/ovn/blob/06c0a4a3ff23c3ffb25a502b1cf318e44e5e683a/ovn-nb.ovsschema#L187\n[3] https://github.com/ovn-org/ovn/blob/06c0a4a3ff23c3ffb25a502b1cf318e44e5e683a/ovn-nb.ovsschema#L519",
"parentUuid": "42e389ed_6053879b",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
},
{
"unresolved": false,
"key": {
"uuid": "6b2f9ed3_21f59677",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 8
},
"lineNbr": 0,
"author": {
"id": 5756
},
"writtenOn": "2023-11-29T15:14:09Z",
"side": 1,
"message": "Yes. Just trying to point out the specific cases I\u0027ve found where we will need to add those keys (NATs for router gateways and provnet ports). It should be easy to do, I\u0027m just trying to save a little work for the developer. ;) Also, I may have missed some things, so good to have additional eyes on it.",
"parentUuid": "6e484f26_865e2ef7",
"revId": "84739ce8d7673bc2d14ee23529460d47f95948fb",
"serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
}
]
}
View Git Blame Copy Permalink