Update patch set 3

Patch Set 3:

(1 comment)

Patch-set: 3
Attention: {"person_ident":"Gerrit User 10058 \u003c10058@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"ADD","reason":"\u003cGERRIT_ACCOUNT_14567\u003e replied on the change"}
Attention: {"person_ident":"Gerrit User 14567 \u003c14567@4a232e18-c5a9-48ee-94c0-e04e7cca6543\u003e","operation":"REMOVE","reason":"\u003cGERRIT_ACCOUNT_14567\u003e replied on the change"}

2ef454c7bd556f3a425b40792899d7dc1f76eed5

@@ -33,6 +33,24 @@
       "message": "This approach does not look correct to me. The bottom line is that we are adding a fix to charm-ovn-xxx on charm-nova-compute. This will be hard to control once we have more of this kind of cross-charm fixes.\nThe second thing I see here is that we are loading a module to nf_conntrack, while there are many other possible values to `sysctl` and we don\u0027t have how to cover all modules or other dependencies from those.\n\nIMO, the responsibility to load and set the module and sysconfig values should be to the networking layer, which should load all sysconfigs and modules it needs. That should be done on install time, not during config-changed.\n\nBTW, charm-neutron-gateway has a fix[1] in those lines, though it also does the module loading during config-changed.\n\n[1] https://review.opendev.org/c/openstack/charm-neutron-gateway/+/738116/4/hooks/neutron_hooks.py#b140",
       "revId": "2ef454c7bd556f3a425b40792899d7dc1f76eed5",
       "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
+    },
+    {
+      "unresolved": false,
+      "key": {
+        "uuid": "c398571b_5db2d132",
+        "filename": "/PATCHSET_LEVEL",
+        "patchSetId": 3
+      },
+      "lineNbr": 0,
+      "author": {
+        "id": 14567
+      },
+      "writtenOn": "2024-04-23T11:24:50Z",
+      "side": 1,
+      "message": "Thanks for the review Erlon. Regarding config-changed vs install hook, I initially had the same concern as you, but it does not matter because the config-changed hook runs immediately after install. Looking at the code, the install hook is extremely simple, and everything the charm does to set up upon installation is in config-changed. It is also run immediately after the upgrade-charm hook. Therefore, I put the code there. There is also the same code in neutron-openvswitch charm that runs it in config-changed.\n\nRegarding the fix being in charm-ovn-xxx vs nova-compute was discussed in the last meeting and most people agreed that it should be in nova-compute because it is nova-compute that is trying to load sysctls related to nf_conntrack, and it is doing so as the default values of the sysctls. There is a clear dependency relation there because of that. On the other hand, the charm-ovn-chassis does not have sysctls dependant on nf_conntrack and also does not care about the exact moment nf_conntrack loads. Nf_conntrack always loads, but it only needs to load as soon as possible at boot time because of the sysctl rules only nova-compute is applying. Looking from this perspective, charm-ovn-xxx does not need this fix, but nova-compute does.\n\nThis fix was inspired by the patch you linked, written by Mauricio.\n\nRegarding your second point, we are not claiming to address every possible dependency from sysctls values that could be there. I am only addressing what we currently need. If there is need in the future to do the same for other modules due to other sysctl rules then the code will have to be further patched to cover them.",
+      "parentUuid": "38e57b97_84f8da0e",
+      "revId": "2ef454c7bd556f3a425b40792899d7dc1f76eed5",
+      "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543"
     }
   ]
 }