From bc45d8cc22c2e38172dba4c5633600bd78fc436f Mon Sep 17 00:00:00 2001 From: Gerrit User 14567 <14567@4a232e18-c5a9-48ee-94c0-e04e7cca6543> Date: Tue, 23 Apr 2024 11:24:50 +0000 Subject: [PATCH] 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 | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/2ef454c7bd556f3a425b40792899d7dc1f76eed5 b/2ef454c7bd556f3a425b40792899d7dc1f76eed5 index 0db78a25..c47044dc 100644 --- a/2ef454c7bd556f3a425b40792899d7dc1f76eed5 +++ b/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" } ] } \ No newline at end of file