Refactor metastore transaction by iyear · Pull Request #7529 · containerd/containerd

iyear · 2022-10-14T12:14:07Z

For more information, please see: #7528

k8s-ci-robot · 2022-10-14T12:14:16Z

Hi @iyear. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2022-10-14T12:22:28Z

@iyear: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

samuelkarp · 2022-10-16T07:57:56Z

/ok-to-test

samuelkarp · 2022-10-16T08:00:17Z

You may want to rebase on main; TestSandboxRemoveWithoutIPLeakage failed in a few of the CI runs and that should be less flaky now on main.

iyear · 2022-10-17T07:23:38Z

Yesterday's CI only Vagrant BOX:rockylinux/8 reports errors, but this one had four failures. I didn't change any code, just sync fork :(

dcantah · 2022-12-24T01:03:24Z

As @kzys said on the issue, I did the same in #7781 🙃. I missed the issue and this PR so my apologies there. I think this is a much better model so it's a +1 from me at least for moving over the rest of the snapshotters. Luckily we chose the same API for how to expose this also with the exception of the callback being its own type, so a rebase should be super simple. I'll try and review this in the coming week!

iyear · 2022-12-24T02:19:38Z

@dcantah It doesn't matter. Thanks!

dcantah · 2022-12-24T07:28:09Z

snapshots/btrfs/btrfs.go

-	if err == nil && info.Kind == snapshots.KindActive && info.Parent != "" {
-		parentID, _, _, err = storage.GetInfo(ctx, info.Parent)
+func (b *snapshotter) usage(ctx context.Context, key string) (usage snapshots.Usage, err error) {
+	id, info, parentID := "", snapshots.Info{}, ""


Suggested change

id, info, parentID := "", snapshots.Info{}, ""

var (

id, parentID string

info snapshots.Info

)

dcantah · 2022-12-24T07:29:13Z

snapshots/btrfs/btrfs.go

-		}
-	}()
+func (b *snapshotter) makeSnapshot(ctx context.Context, kind snapshots.Kind, key, parent string, opts []snapshots.Opt) (_ []mount.Mount, err error) {
+	target, s := "", storage.Snapshot{}


Suggested change

target, s := "", storage.Snapshot{}

var (

target string

s storage.Snapshot

)

dcantah · 2022-12-24T07:33:56Z

snapshots/devmapper/snapshotter.go

 	)

-	err = s.withTransaction(ctx, false, func(ctx context.Context) error {
+	err = s.store.WithTransaction(ctx, false, func(ctx context.Context) error {


Luckily this file shouldn't need to change at all 🤣

dcantah · 2022-12-24T07:35:41Z

snapshots/btrfs/btrfs.go

+		return nil
+	})
+
+	defer func() {


I'd probably move this above the WithTransaction (and below the variable declarations) so you can see the cleanup case at the top of the method. Mostly a nit though

dcantah · 2022-12-24T07:36:07Z

snapshots/btrfs/btrfs.go

-	s, err := storage.GetSnapshot(ctx, key)
-	t.Rollback()
+func (b *snapshotter) Mounts(ctx context.Context, key string) (_ []mount.Mount, err error) {
+	s := storage.Snapshot{}


Suggested change

s := storage.Snapshot{}

var s storage.Snapshot

dcantah · 2022-12-24T07:40:36Z

snapshots/btrfs/btrfs.go

-	}
-	err = t.Commit()
-	t = nil
+		return btrfs.SubvolSnapshot(target, parentp, kind == snapshots.KindView)


Kind of a nit, but the explicit readOnly assignment made this slightly more readable. I'd have to go look at SubVolSnapshot's declaration to see what's the significance of either true or false (or be versed in what KindView signifies in containerd parlance), but before I can take a quick glance and go "oh, it's likely for a read only snapshot".

dcantah · 2022-12-24T07:41:55Z

snapshots/btrfs/btrfs.go

-			if rerr := t.Rollback(); rerr != nil {
-				log.G(ctx).WithError(rerr).Warn("failed to rollback transaction")
-			}
+	source, target := "", ""


Suggested change

source, target := "", ""

var source, target string

dcantah

Some Go nits and small refactorings, but change LGTM after! @iyear if we get this in, and others also like this model, do you want to do the honors for the remaining snapshotters?

iyear · 2022-12-24T08:40:47Z

@dcantah Thanks for review! The new commit has been pushed. I am glad to finish the remaining work, it's my pleasure.

I will solve the conflict later

iyear · 2022-12-24T08:58:21Z

What does the Project Check error mean? I just rebase and force push the code.

dcantah · 2022-12-24T09:02:23Z

@iyear Not sure if we figured this out, but our guess was a rate limit. I don't think there's anything wrong with your change 🫠. Don't have permissions to re-kick off the CI so I'd wait a little and re-push possibly to get a fresh run.

dcantah

LGTM on green

snapshots/btrfs/btrfs.go

fuweid · 2022-12-24T12:42:34Z

I wonder that we can introduce ebpf or interface wrapper to inject the failpoint to test the snapshot, because the eyeball is hard mode.

It can be followup.

dmcgowan

Changes look good, can you please squash the commits before merging

iyear · 2022-12-28T05:38:18Z

@dmcgowan done:)

snapshots/btrfs/btrfs.go

Signed-off-by: Junyu Liu <[email protected]>

fuweid

LGTM on green

Thanks!

k8s-ci-robot added the needs-ok-to-test label Oct 14, 2022

iyear force-pushed the refactor-metastore-tx branch from a1cad91 to 0eef6aa Compare October 14, 2022 12:21

samuelkarp added the kind/refactor label Oct 16, 2022

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 16, 2022

iyear force-pushed the refactor-metastore-tx branch 2 times, most recently from b4f96e3 to 3009337 Compare October 17, 2022 05:38

iyear force-pushed the refactor-metastore-tx branch from 3009337 to 60330f1 Compare October 18, 2022 02:03

dmcgowan self-assigned this Nov 30, 2022

dcantah reviewed Dec 24, 2022

View reviewed changes

iyear force-pushed the refactor-metastore-tx branch from b342510 to 826f53a Compare December 24, 2022 08:47

dcantah approved these changes Dec 24, 2022

View reviewed changes

iyear force-pushed the refactor-metastore-tx branch from 826f53a to 0978b07 Compare December 24, 2022 09:44

fuweid requested changes Dec 24, 2022

View reviewed changes

snapshots/btrfs/btrfs.go Outdated Show resolved Hide resolved

iyear force-pushed the refactor-metastore-tx branch from 04fbed4 to ea8adf0 Compare December 26, 2022 08:15

dmcgowan requested changes Dec 27, 2022

View reviewed changes

iyear force-pushed the refactor-metastore-tx branch from ea8adf0 to ba5c88c Compare December 28, 2022 05:37

iyear force-pushed the refactor-metastore-tx branch 2 times, most recently from ed51dcf to 6baa924 Compare December 28, 2022 09:36

fuweid reviewed Dec 28, 2022

View reviewed changes

snapshots/btrfs/btrfs.go Show resolved Hide resolved

Refactor metastore transaction

1d0619b

Signed-off-by: Junyu Liu <[email protected]>

iyear force-pushed the refactor-metastore-tx branch from 6baa924 to 1d0619b Compare December 28, 2022 10:37

fuweid approved these changes Dec 28, 2022

View reviewed changes

dmcgowan approved these changes Dec 29, 2022

View reviewed changes

dmcgowan merged commit cfe7ac9 into containerd:main Dec 29, 2022

dcantah mentioned this pull request Jan 4, 2023

Snapshotters: Move all to metadata.WithTransaction #7915

Closed

-	id, info, parentID := "", snapshots.Info{}, ""
+	var (
+	     id, parentID string
+	     info snapshots.Info
+	)

-	target, s := "", storage.Snapshot{}
+	var (
+	    target string
+	    s storage.Snapshot
+	)

Conversation

iyear commented Oct 14, 2022

Uh oh!

k8s-ci-robot commented Oct 14, 2022

Uh oh!

k8s-ci-robot commented Oct 14, 2022

Uh oh!

samuelkarp commented Oct 16, 2022

Uh oh!

samuelkarp commented Oct 16, 2022

Uh oh!

iyear commented Oct 17, 2022

Uh oh!

dcantah commented Dec 24, 2022

Uh oh!

iyear commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dcantah Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

iyear Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah Dec 24, 2022

Choose a reason for hiding this comment

Uh oh!

dcantah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iyear commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iyear commented Dec 24, 2022

Uh oh!

dcantah commented Dec 24, 2022

Uh oh!

dcantah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fuweid commented Dec 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

iyear commented Dec 28, 2022

Uh oh!

Uh oh!

fuweid left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iyear commented Dec 24, 2022 •

edited

Loading

dcantah Dec 24, 2022 •

edited

Loading

dcantah left a comment •

edited

Loading

iyear commented Dec 24, 2022 •

edited

Loading

fuweid commented Dec 24, 2022 •

edited

Loading