From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 1B5941FF15F for ; Mon, 21 Oct 2024 10:16:53 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 052411FAC8; Mon, 21 Oct 2024 10:17:30 +0200 (CEST) Message-ID: <0f6d244f-2f43-46e7-a5cf-92545f3538c7@proxmox.com> Date: Mon, 21 Oct 2024 10:16:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Gabriel Goller , Thomas Lamprecht References: <20241015131823.338766-1-g.goller@proxmox.com> <20241015131823.338766-2-g.goller@proxmox.com> <6376441e-f2ac-4cb6-ae63-39f21bb2c4c3@proxmox.com> <20241017132010.5q7i7llfkn6jcuqn@luna.proxmox.com> Content-Language: de-AT, en-US From: Lukas Wagner In-Reply-To: <20241017132010.5q7i7llfkn6jcuqn@luna.proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.009 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 1/3] fix #3786: api: add resync-corrupt option to sync jobs X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox Backup Server development discussion Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: pbs-devel-bounces@lists.proxmox.com Sender: "pbs-devel" On 2024-10-17 15:20, Gabriel Goller wrote: > On 17.10.2024 08:15, Thomas Lamprecht wrote: >> Am 15/10/2024 um 15:18 schrieb Gabriel Goller: >>> This option allows us to "fix" corrupt snapshots (and/or their chunks) >>> by pulling them from another remote. When traversing the remote >>> snapshots, we check if it exists locally, and if it is, we check if the >>> last verification of it failed. If the local snapshot is broken and the >>> `resync-corrupt` option is turned on, we pull in the remote snapshot, >>> overwriting the local one. >>> >>> This is very useful and has been requested a lot, as there is currently >>> no way to "fix" corrupt chunks/snapshots even if the user has a healthy >>> version of it on their offsite instance. >>> >>> Signed-off-by: Gabriel Goller >>> Originally-by: Shannon Sterz >> >> those two trailers probably should be switched to uphold causal ordering. > > Makes sense. > >> In general, it would be _really_ nice to get some regression testing for >> sync jobs covering the selection with different matching and options. >> The lack of that doesn't have to block your/shannon's patch, but IMO it >> should be added rather sooner than later. Sync is becoming more and more >> complex, and the selection of snapshots is a very important and fundamental >> part of it. > > Do you think we could mock ourselves out of this and unit-test it > somehow? For full regression tests we will need Lukas's CI work. > To chime in here, while it will be certainly possible to test such stuff once the end-to-end testing tooling is finished (it's getting there), in most cases it is preferable to test as much as possible in unit tests as they are - much faster (<<1s, compared to end-to-end tests which could take seconds or even minutes, depending on what they do) - easier to write and maintain (you are much closer to the code under test) - also serve as documentation for the code (e.g. what contracts a unit of code must uphold) - far shorter feedback loop / better DX (cargo test vs. having to set up/run test instance VMs or trigger some CI system) Especially for something like the snapshot selection in sync jobs, a good test suite might cover many different permutations and corner cases, which would be *a lot of* of work to write and also take *a long time* to execute in a full bells and whistles end-to-end test where you perform an actual sync job between two real PBS installations. That being said, it will be of course a challenge to factor out the sync logic to make it testable. Without me being familiar with the code, it could involve abstracting the interactions with the rest of the system with some trait, moving the sync logic into a separate structs / functions and use dependency injection to give the sync module/fn a concrete implementation to use (in tests you'd then use a fake implementation). For existing code these refactoring steps can be quite intimidating, which is the reason why I am a big fan of writing unit tests *while* writing the actual implementation (similar to, but not quite TDD; there you'd write the tests *first*), as this ensures that the code I'm writing is actually testable. -- - Lukas _______________________________________________ pbs-devel mailing list pbs-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel