openstack/charm-mysql-router
Code Issues Proposed changes

Update patch set 3

Browse Source 
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
This commit is contained in:
Gerrit User 20870 2023-06-01 09:11:56 +00:00 committed by Gerrit Code Review
parent 27ae62b719
commit 8fb7c84d3d
1 changed files with 17 additions and 0 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
fbc3965e0a130d4099b8be28b8dd62c6b8db329d
View File

@ -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"
}
]
}