Proxmox and code reviews

We have previously had a look at lapses of Proxmox testing procedures, but nothing quite exhibits a core culture problem than a bug that should have never made it past an internal code review, let alone testing - and that still ships in a mature product - as of May 2025.

Proxmox cluster configuration database

The files presented under /etc/pve which hold all the vital cluster configurations are actually provided by the mounted virtual filesystem of pmxcfs, which in turn stores its data locally in an SQLite   database. While the database is only read from during a node start - this is possible because parallel data structure is kept in RAM at all times - it is being constantly written to.

Whether SQLite is the right backend of choice was already previously scrutinised here in relation to pmxcfs and its toll on regular SSDs. Proxmox are aware of its deficiencies and it is arguably why they chose to use very little of its built-in constraints features. Instead, attempts to detect any “corruption” within happens during node startup, programmatically.  

It is these bespoke checks you might have previously encountered boot-up errors, such as (excerpts only):

[database] crit: found entry with duplicate name ...
[database] crit: DB load failed
[main] crit: memdb_open failed - unable to open database '/var/lib/pve-cluster/config.db'
[main] notice: exit proxmox configuration filesystem (-1)

How to corrupt a database

Proxmox staff, including senior developers consider these “weird corruption”,   but are generally happy to help including with hands-on fixing up of what ended up stored in that database.   This has been going on ever since the pve-cluster service shipped - responsible for launching instance of pmxcfs which is necessary even for non-clustered nodes.

There’s one major consideration to make when it comes to ending up with a corrupt database like this: the circumstances under which it could happen. Proxmox chose to opt for so-called write-ahead-log (WAL)   mode instead of traditional journal with rollbacks - again - likely for performance reasons, but undisputedly also to minimise risk of data corruption.

Instead of the main database file being constantly written to and journal keeping the now-overwritten data for rollbacks, transactions cause constant barrage of appends to a separate WAL file only, which is then rolled over into the base at fixed points (or whenever first possible passing such points) - this event is also called a checkpoint. As a result, virtually the only situation when SQLite in WAL mode could experience data corruption, save for a hardware issue, is during this event as is well documented:  

SQLite in WAL mode is far more forgiving of out-of-order writes than in the default rollback journal modes. In WAL mode, the only time that a failed sync operation can cause database corruption is during a checkpoint operation. A sync failure during a COMMIT might result in loss of durability but not in a corrupt database file. Hence, one line of defense against database corruption due to failed sync operations is to use SQLite in WAL mode and to checkpoint as infrequently as possible.

Loss of durability

Loss of durability in terms of ACID principles basically means missing some of the previously committed transactions - this would be typically some most recent transactions that had yet to be checkpointed, and not some random transactions. But this is NOT an issue for Proxmox stack as it is exactly what happens when e.g. a node in a cluster goes down for some time. The transactions are not recorded by an offline node until next boot, when - first of all things - it syncs the missed out records from the rest of the cluster - it’s the whole point of having Corosync providing the extended virtual synchrony in Proxmox stack: to start up from it left off and get in sync in correct order with all the write operations.

Arguably, it is not an issue even with single node installs as restarting into a bit different state - with some most recent configuration changes missing - might be a surprise, but won’t ruin e.g. HA allocation of services in relation to any other node.

Power loss

So far, it would appear that it must be power loss events happening exactly during WAL checkpoint operations that bring up this “weird corruption”, but there was a recipe for minimising this risk above as well: checkpoint as infrequently as possible. While Proxmox stack produces a lot of writes, they are tiny and the default threshold of around 4MB sized WAL is the point when it gets first checkpointed - and it will take several minutes depending on the cluster size and activity.

And as much as one might be tempted to ascribe this corruption to e.g. sudden power-loss-like events of the often misunderstood auto-reboot feature associated with high availability and watchdogs, this simply CANNOT be the case for most cases for the simple reason that quorum would have been typically lost prior to such reboot events, which in turn makes /etc/pve a readonly filesystem - and therefore the backend database inactive. And checkpoints do NOT automatically happen when idle in this implementation.

It is simply very unlikely that multiple instances of user reports would be confirming they all were hitting a genuine power loss event exactly during a WAL checkpoint moment and even then in such an unfortunate way that the records got somehow mangled without the database itself overtly losing its consistency.

Not a database corruption case

And indeed, the corruption experienced above is not innate to the database file, strictly speaking. This is because Proxmox basically only use the most rudimentary of SQL constraints - see the schema in the pmxcfs mountpoint analysis - basically just NOT NULL and a single-column primary key is enforced.

Finding a duplicate filename (string field of a database record), within single virtually conceived directory (through its parent pointers), when that name is associated with two different IDs (inode being the primary key of the database table) is not something that SQLite could be made responsible for.

And so a curious developer would be self-invited onto a journey of analysing their own codebase and where they forgot to delete the old file prior to when they recreated a new one with the same name.

Multi-threaded environment

Debugging multi-threaded system could be hard at times, it’s perhaps why they should be best avoided in the first place when there’s a better solution, but that’s not a choice the developer always has. Arguably, it is a bit difficult to be checking consistency of a database with duplicated in-memory structures when it is never read from - until next reboot - as this is the Proxmox setup. But then again, this would have to be done as part of proper debugging process.

Reading through the code, there is, for example a situation when a file is renamed eventually resulting in database DELETE operation preceding any subsequent INSERT  . It just makes no sense how a new file of the same name could then appear somewhere with this ordering of database operations unless failed operations were also failing to rollback and failing to log the same.

The other suspect is that, transactionally, e.g. DELETE and INSERT and not put together, but this would not be a problem given proper use of mutex constructs when it comes to accessing the same resource, in this case both the SQLite database and the in-memory structures, which appears to be the case here, extensively.  

While these blocks of code should have received extensive scrutiny, and likely have due to plentiful debug logging, one would likely arrive at the same conclusion that all in all, in the worst case, there should be instances of missing files, not duplicate files.

The above statement is not meant to be interpreted that Proxmox thread implementation is necessarily sound, but SQLite is thread-safe:  

API calls to affect or use any SQLite database connection or any object derived from such a database connection can be made safely from multiple threads. The effect on an individual object is the same as if the API calls had all been made in the same order from a single thread. The name “serialized” arises from the fact that SQLite uses mutexes to serialize access to each object.

Must be the database

Anyone seriously reviewing this codebase would have been at least tempted to raise a bugreport with SQLite team about these mysterious issues, if for no other reason then at least to externalise the culprit, however there does not seem to be a single instance of a bugreport filed by Proxmox with SQLite, unlike with e.g. the Corosync project.

The above is a disconcerting case - not least because anyone building up with SQLite in their C stack would have noticed the unthinkable.

Do not carry a connection over

When service unit of pve-cluster starts the pmxcfs process, there is an old-fashioned case of turning a process into a daemon - or service - going on, that is, unless the foreground switch (command-line argument) has been passed to it:  

	if (!foreground) {
		if (pipe(pipefd) == -1) {
			cfs_critical("pipe error: %s", strerror(errno));
			goto err;
		}

		pid_t cpid = fork();

It is this mechanism that lets another process continue running in the background even as it returned from its original invocation. While not necessary to be done in this way - as systemd took place of traditional init systems - it was fairly common once. But wait, this is already towards the end of the whole initialisation, including:

	gboolean create = !g_file_test(DBFILENAME, G_FILE_TEST_EXISTS);

	if (!(memdb = memdb_open (DBFILENAME))) {
		cfs_critical("memdb_open failed - unable to open database '%s'", DBFILENAME);
		goto err;

And opening the memdb means also opening the backend SQLite database file   within database.c code.   Did you see that? Look again.

The database is first opened from disk, then process forked in order to deamonise it. Should this have been ever given a closer look in any code review or got spotted by another development team member, they would have known, not to (excerpt only):  

Do not open an SQLite database connection, then fork(), then try to use that database connection in the child process. All kinds of locking problems will result and you can easily end up with a corrupt database. SQLite is not designed to support that kind of behavior. Any database connection that is used in a child process must be opened in the child process, not inherited from the parent.

At this point, it would take us to get quite intimate with SQLite codebase itself to fully understand consequences of this, especially in a multi-threaded implementation that is at play here, so we will leave off at that for the purposes of this post.

Baggage

As per the Git records, the implementation has been like this at least since August 2011 when it got imported from older versioning system of Proxmox. It is rather unfortunate that when it was getting a second look,   in April 2018, it was because (excerpt only):

since systemd depends that parent exits only when the service is actually started, we need to wait for the child to get to the point where it starts the fuse loop and signal the parent to now exit and write the pid file

This was a great opportunity to rewrite the piece for systemd specifically without any forks necessary, instead taking advantage of systemd-notify   mechanism.

And so while running the pve-cluster service with pmxcfs launched into foreground would certainly be possible (simple -f switch), it would then exhibit the problems that were discovered in 2018, i.a.:

we had an issue, where the ExecStartPost hook (which runs pvecm updatecerts) did not run reliably, but which is necessary to setup the nodes/ dir in /etc/pve and generating the ssl certificates this could also affect every service which has an After=pve-cluster

In other words, this has no workaround, but needs to be fixed by Proxmox.

When no one is looking

It is quite common to point out that projects which are open source are somehow more immune from bugs, but as this case demonstrates, there are cases when no one reads, or scrutinises the otherwise “open” code. For many years, even decades. This is exacerbated by the fact that Proxmox do everything at their disposal to dissuade external contributors to participate, if only by random code reviews. And last, but not least, it brings up yet another issue that comes with small core development team that does not welcome peers - that no one will be looking.