From 8fb7c84d3daf4c244df0605ac45818f99134e028 Mon Sep 17 00:00:00 2001 From: Gerrit User 20870 <20870@4a232e18-c5a9-48ee-94c0-e04e7cca6543> Date: Thu, 1 Jun 2023 09:11:56 +0000 Subject: [PATCH] Update patch set 3 Patch Set 3: Code-Review-1 (1 comment) Patch-set: 3 Reviewer: Gerrit User 20870 <20870@4a232e18-c5a9-48ee-94c0-e04e7cca6543> Label: Code-Review=-1, 911bc84c1ba0c1920db363271d2c4809d44e6f99 --- fbc3965e0a130d4099b8be28b8dd62c6b8db329d | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/fbc3965e0a130d4099b8be28b8dd62c6b8db329d b/fbc3965e0a130d4099b8be28b8dd62c6b8db329d index 9a351a4..2e6bd51 100644 --- a/fbc3965e0a130d4099b8be28b8dd62c6b8db329d +++ b/fbc3965e0a130d4099b8be28b8dd62c6b8db329d @@ -16,6 +16,23 @@ "message": "This is my understanding of the situation here:\n\n1) the mysqlrouter.conf file is generated by mysqlrouter bootstrap process, this is a program provided by the deb package, the file is produced once when the unit gets deployed.\n2) On day-2 operations (e.g. upgrade-charm), we massage this file to make it meet the new expectations (e.g. add/remove sections, add/remove key-values, etc) based on the new config options (or the ones deprecated) since mysql8 point releases introduce new features and deprecate functionality.\n\nNow if for any reason we manipulate the file in a way that we can no longer \u0027fix\u0027 it, it\u0027s game over, we have no template re-render the config from.\n\nIf all what I said is accurate, I believe this process of copying the file produced on (1) and keeping it around, it\u0027s not inherently more robust than what we have today. We should consider the possibility of using a proper Jinja template for the generation of mysqlrouter.conf[0]\n\n\nNow reading the bug, it says this:\n\n```\nAfter the unit\u0027s filesystem ran out of disk space, the config file \u003d\u003e /var/lib/mysql/\u003capp-name\u003e-mysql-router/mysqlrouter.conf had some lines with invalid characters: [...]\n```\n\nso we are fixing a scenario where basically way too many things will fail, we should make this piece of code atomic[1], basically do the dance of writing to a temporary file, rename `mysqlrouter.conf` to `.mysqlrouter.conf.$DATE`, then the temporary file to `mysqlrouter.conf`, it would give you the guarantee that the file file was fully written to disk.\n\n[0] this may be a not so small change to this method https://opendev.org/openstack/charm-mysql-router/src/branch/master/src/lib/charm/openstack/mysql_router.py#L505\n[1] https://opendev.org/openstack/charm-mysql-router/src/branch/master/src/lib/charm/openstack/mysql_router.py#L743", "revId": "fbc3965e0a130d4099b8be28b8dd62c6b8db329d", "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543" + }, + { + "unresolved": false, + "key": { + "uuid": "3676b164_be954572", + "filename": "/PATCHSET_LEVEL", + "patchSetId": 3 + }, + "lineNbr": 0, + "author": { + "id": 20870 + }, + "writtenOn": "2023-06-01T09:11:56Z", + "side": 1, + "message": "I actually think this review should be abandoned, on reflection. We\u0027re essentially trying to recover from a situation where a unit ran out of disk space and corrupted the unit. In this scenario, we probably ought to be discarding the unit and starting again with a new unit as it\u0027s not clear what else on the unit is broken.\n\nAlternatively, add an action to rerun the bootstrap process (using bootstrap_mysqlrouter() method in the src/lib/charm/openstack/mysql_router.py file), would guarantee getting the \u0027latest\u0027 best config file.\n\nAdding a backup file just sounds like additional \u0027caching\u0027 for not much extra benefit. Maintaining the backup file is more work for the charm and more complexity.\n\nIt\u0027s also possible to manually force the unit to re-write the bootstrap by clearing the \"charm.mysqlrouter.bootstrapped\" flag, which would then re-run the bootstrap process.", + "revId": "fbc3965e0a130d4099b8be28b8dd62c6b8db329d", + "serverId": "4a232e18-c5a9-48ee-94c0-e04e7cca6543" } ] } \ No newline at end of file