From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D33A494AB2 for ; Thu, 22 Feb 2024 23:02:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B2A1EFF0D for ; Thu, 22 Feb 2024 23:01:49 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 22 Feb 2024 23:01:48 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E9F5344B8F for ; Thu, 22 Feb 2024 23:01:47 +0100 (CET) Date: Thu, 22 Feb 2024 23:01:46 +0100 From: Stoiko Ivanov To: Friedrich Weber Cc: pmg-devel@lists.proxmox.com Message-ID: <20240222230146.09a75559@rosa.proxmox.com> In-Reply-To: <20240222163535.1112846-1-f.weber@proxmox.com> References: <20240222163535.1112846-1-f.weber@proxmox.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com, dbtools.pm] Subject: [pmg-devel] applied: [PATCH v2 pmg-api] fix #5189: cluster: avoid sync errors for statistics and quarantine X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Feb 2024 22:02:19 -0000 Thanks for the patch, thinking this through again, and the exceptional commit-message! looked it through, applied it in my reproducer and it fixes the issue - so applied it still 2 comments inline: On Thu, 22 Feb 2024 17:35:35 +0100 Friedrich Weber wrote: > After restoring a backup from a cluster on a fresh node with > statistics, and then creating a cluster, the following can happen > (node 1 being master and node 2 being a node): `ClusterInfo` on node > 1 has no record about the last-synchronized `CStatistic` row id of > node 2. Thus, pmgmirror on node 1 initializes the record with -1 and > tries to synchronize *all* `CStatistic` rows with cid 2 from node 2. > But (some of) these rows may already exist on cid 1, because they > were part of the backup, so pmgmirror on node 1 triggers a Postgres > unique constraint violation, statistics synchronization on node 1 > fails, and node 1 remains in the "synchronizing" state. > > Fix this as follows: When a new node is added to a cluster, the master > now initializes its `ClusterInfo` record of the last-synchronized > `CStatistic` row id for that node cid with the maximum row id that > exists in the local `CStatistic` for that node cid, or with -1 if the > local `CStatistic` has no row for that node cid. This is valid because Would it maybe make sense to scan the existing "lastid_tables" (Cstatistic and CMailStore) when the cluster is created - and use the maximal value of both as the maxcid value in the cluster-conf? (I guess we will then eventually see 5-digit cluster-ids, but it might help for the few cases of broken clusters due to constraint violations) Alternatively we could make `pmgcm delete X` update the cid-columns for all relevant tables, replacing X with a special value (probably 0 should work, but did not check that). Some kind of idempotent cleanup function yielding a state where joining a node with the same id just works(tm) but that's mostly a note to future us, after we see the next actual issue with cluster-joining that we can reproduce. > the newly-added node copies the master's `CStatistic` table during > cluster join. > > Do the same for the `CMailStore` table, where a similar sync error > could happen e.g. if the table has rows for both node cids, node 2 is > shut down and manually deleted from the cluster.conf, the maxcid is > manually reset to 1, and a fresh node is joined to the cluster and > gets assigned cid 2. > > Signed-off-by: Friedrich Weber > --- > > Notes: > v1 -> v2: > - initialize master `ClusterInfo` record for `CStatistic` once when > joining a new node instead of initializing on the first cluster > sync, as discussed with Stoiko off-list > - do the same for `CMailStore`, where a similar sync error can happen > > As a side effect of this change, `ClusterInfo` on node 2 will now also > have `lastid_CStatistic` and `lastid_CMailStore` rows for its own cid > 2 after join, because the database is copied over *after* the master > runs `update_master_clusterinfo`, and `update_client_clusterinfo` on > node 2 only deletes rows with the master cid 1. However, if I > understand the cluster sync code correctly (and that's a big if), > these additional rows should not have any effect, as node 2 will > only ever read and update `ClusterInfo` rows for other cids, never > for its own cid. That's my current thinking too - regarding a node not syncing it's own information (and updating ClusterInfo alongside it) - short of someone randomly incrementing/decrementing the id's in the cluster.conf... > > v1: https://lists.proxmox.com/pipermail/pmg-devel/2024-January/002657.html > > src/PMG/DBTools.pm | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm > index 6112566..8770d06 100644 > --- a/src/PMG/DBTools.pm > +++ b/src/PMG/DBTools.pm > @@ -1132,6 +1132,14 @@ sub update_master_clusterinfo { > $dbh->do ("INSERT INTO ClusterInfo (cid, name, ivalue) select $clientcid, 'lastmt_$table', " . > "EXTRACT(EPOCH FROM now())::INTEGER"); > } > + > + my @lastid_tables = ('CStatistic', 'CMailStore'); > + > + for my $table (@lastid_tables) { > + $dbh->do("INSERT INTO ClusterInfo (cid, name, ivalue) " . > + "SELECT $clientcid, 'lastid_$table', COALESCE (max (rid), -1) FROM $table " . > + "WHERE cid = $clientcid"); > + } > } > > sub update_client_clusterinfo {