Do not delete target wallet, do not fail migration on item-error #1006

Patrik-Stas · 2023-10-03T06:45:06Z

This is set of changes to make the migration.

More graceful, one migration error should not halt migration of the rest. It's up to migrating user to evaluate whether the migration result is satisfactory.
Idempotent - if migration fails for example due an IO error midway, it should be possible to finish it on 2nd try.

Changes:

If migration fails, do not delete the target wallet.
Disable adding records to vdrtool cache when running migration.
Do not fail migration if the record has unexpected format. Just skip it and log the record.
If migration of the record itself fail, skip it and log.
If adding item to target wallet fails due duplication error, skip it. For idempotency it would be ideal to overwrite it, but that would need digging deeper. For now, skipping these records still gives us idempotency under assumption item migration process was not changed between 2 migrations attempts.
If adding item fails for other reason, fail the migration (likely IO error).

codecov-commenter · 2023-10-03T07:02:51Z

Codecov Report

Merging #1006 (b572afc) into main (725797b) will decrease coverage by 0.02%.
The diff coverage is 13.63%.

@@            Coverage Diff             @@
##             main    #1006      +/-   ##
==========================================
- Coverage   30.39%   30.38%   -0.02%     
==========================================
  Files         422      422              
  Lines       26227    26242      +15     
  Branches     5084     5085       +1     
==========================================
+ Hits         7972     7973       +1     
- Misses      16038    16051      +13     
- Partials     2217     2218       +1

Flag	Coverage Δ
unittests-aries-vcx	`30.38% <13.63%> (-0.02%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
libvdrtools/indy-wallet/src/export_import.rs	`0.00% <ø> (ø)`
libvdrtools/src/controllers/wallet.rs	`32.30% <ø> (ø)`
libvdrtools/indy-wallet/src/wallet.rs	`48.08% <66.66%> (+0.01%)`	⬆️
...ibvdrtools/indy-api-types/src/domain/wallet/mod.rs	`7.14% <0.00%> (-0.55%)`	⬇️
libvdrtools/indy-wallet/src/lib.rs	`48.12% <8.33%> (-0.88%)`	⬇️

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

bobozaur

While I understand that this helped in troubleshooting some issues (practically unrelated to the migration code) I think most changes (maybe apart from logging) should be reverted.

The fact that the migrator cleans up after itself on failure might not be a big deal for us, but it probably is for consumers that will use it.

Likewise, I don't think the migration should just continue on record errors. This is important data and I personally believe that anything that does not go well should be manually investigated and dealt with and the migration re-attempted afterwards, essentially succeeding only if everything is in order.

bobozaur · 2023-10-03T11:27:12Z

libvcx_core/src/api_vcx/api_global/wallet.rs

-    if let Err(e) = migration_res {
-        close_wallet(dest_wallet_handle).await.ok();
-        delete_wallet(wallet_config).await.ok();
-        Err(LibvcxError::from_msg(
-            LibvcxErrorKind::WalletMigrationFailed,
-            e,
-        ))
-    } else {
-        setup_wallet(dest_wallet_handle)?;
-        close_wallet(src_wallet_handle).await?;
-        Ok(())
-    }


Why not delete the destination wallet on failure?

You should be able to rerun migration against the same target repeatedly, with predictable result. If I get IO error midway, I want to be able to retry.

bobozaur · 2023-10-03T11:29:41Z

libvdrtools/indy-wallet/src/wallet.rs

+        if cache_record {
+            self.cache.add(type_, &etype, &ename, &evalue, &etags);
+        }


What is this change supposed to do? I see the flag is set to true in both instances where this is used.

In migration, the flag is set to false.

bobozaur · 2023-10-03T11:30:13Z

libvcx_core/src/api_vcx/api_global/wallet.rs

+    info!(
+        "Creating or opening target wallet {}",
+        &wallet_config.wallet_name
+    );


I think it will always attempt to create it.

Patrik-Stas · 2023-10-03T11:59:08Z

While I understand that this helped in troubleshooting some issues (practically unrelated to the migration code) I think most changes (maybe apart from logging) should be reverted.

The fact that the migrator cleans up after itself on failure might not be a big deal for us, but it probably is for consumers that will use it.

If migration fails, you might want to inspect the wallet

Likewise, I don't think the migration should just continue on record errors. This is important data and I personally believe that anything that does not go well should be manually investigated and dealt with and the migration re-attempted afterwards, essentially succeeding only if everything is in order.

If you fail on first error, you don't even get to know all the problem you have, only the first one. The changed approach in this PR provide flexibility to migrators. You can but don't have to be strict about failures, it's your call based on MigrationResult. In case of libvcx, as the only users, I chose the approach of being relaxed about it and decide on upper level of abstractions.

Also, I don't think there's at the moment other users of this migration other than us. George is already using credx. As the user of the migration code, these were practically useful changes I'd suggest to merge, although most of this will be anyways deleted in subsequent commits, as we remove vdrtools anoncreds.

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

bobozaur

I got mixed up in the files being modified and forgot the design of the migration. Given that this affects libvcx stuff, I think the changes are then warranted. Thanks for clarifying!

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

Patrik-Stas force-pushed the credx/migration-tweaks branch from 73813f4 to 880734e Compare October 3, 2023 07:05

Do not delete target wallet, do not fail migration on item-error

0659b2f

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

Patrik-Stas force-pushed the credx/migration-tweaks branch from 880734e to 0659b2f Compare October 3, 2023 07:06

Tweak migration logging

cd5b8c3

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

bobozaur suggested changes Oct 3, 2023

View reviewed changes

Patrik-Stas mentioned this pull request Oct 3, 2023

Use credx anoncreds in napi wrapper #927

Merged

Tweak logs

0c8a2a9

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

Patrik-Stas force-pushed the credx/migration-tweaks branch from 9865772 to 0c8a2a9 Compare October 3, 2023 12:43

bobozaur previously approved these changes Oct 3, 2023

View reviewed changes

Fix formatting

b572afc

Signed-off-by: Patrik Stas <patrik.stas@absa.africa>

Patrik-Stas dismissed bobozaur’s stale review via b572afc October 3, 2023 13:19

Patrik-Stas merged commit 34c35de into main Oct 3, 2023
33 of 38 checks passed

Patrik-Stas deleted the credx/migration-tweaks branch October 3, 2023 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not delete target wallet, do not fail migration on item-error #1006

Do not delete target wallet, do not fail migration on item-error #1006

Patrik-Stas commented Oct 3, 2023 •

edited

Loading

codecov-commenter commented Oct 3, 2023 •

edited

Loading

bobozaur left a comment

bobozaur Oct 3, 2023

Patrik-Stas Oct 3, 2023 •

edited

Loading

bobozaur Oct 3, 2023

Patrik-Stas Oct 3, 2023

bobozaur Oct 3, 2023

Patrik-Stas commented Oct 3, 2023 •

edited

Loading

bobozaur left a comment

Do not delete target wallet, do not fail migration on item-error #1006

Do not delete target wallet, do not fail migration on item-error #1006

Conversation

Patrik-Stas commented Oct 3, 2023 • edited Loading

codecov-commenter commented Oct 3, 2023 • edited Loading

Codecov Report

bobozaur left a comment

Choose a reason for hiding this comment

bobozaur Oct 3, 2023

Choose a reason for hiding this comment

Patrik-Stas Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

bobozaur Oct 3, 2023

Choose a reason for hiding this comment

Patrik-Stas Oct 3, 2023

Choose a reason for hiding this comment

bobozaur Oct 3, 2023

Choose a reason for hiding this comment

Patrik-Stas commented Oct 3, 2023 • edited Loading

bobozaur left a comment

Choose a reason for hiding this comment

Patrik-Stas commented Oct 3, 2023 •

edited

Loading

codecov-commenter commented Oct 3, 2023 •

edited

Loading

Patrik-Stas Oct 3, 2023 •

edited

Loading

Patrik-Stas commented Oct 3, 2023 •

edited

Loading