Skip to content

btrfs: use native go snapshotter implementation#5658

Closed
AkihiroSuda wants to merge 1 commit intocontainerd:mainfrom
AkihiroSuda:btrfs-no-cgo
Closed

btrfs: use native go snapshotter implementation#5658
AkihiroSuda wants to merge 1 commit intocontainerd:mainfrom
AkihiroSuda:btrfs-no-cgo

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Jun 25, 2021

Replace github.com/containerd/btrfs with github.com/dennwc/btrfs to eliminate cgo dependency.

How to test: go test -c ./snapshots/btrfs && sudo ./btrfs.test -test.root -test.v

Fix #5654
Fix containerd/btrfs#7

Based on Denys Smirnov (@dennwc)'s attempt in 2019: dennwc@3728f96

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jun 25, 2021

cc @dennwc @stevvooe

@AkihiroSuda AkihiroSuda mentioned this pull request Jun 25, 2021
Closed
@AkihiroSuda

This comment has been minimized.

Sign in to view
@AkihiroSuda AkihiroSuda added this to the 1.6 milestone Jun 25, 2021
@dennwc
Copy link
Copy Markdown

dennwc commented Jun 25, 2021

Merged the PR, thanks a lot for pushing this forward :)

@AkihiroSuda
btrfs: use native go snapshotter implementation
59be6ce 
Replace containerd/btrfs with dennwc/btrfs to eliminate cgo dependency.

How to test: `go test -c ./snapshots/btrfs && sudo ./btrfs.test -test.root -test.v`

Fix issue 5654
Fix containerd/btrfs issue 7

Based on Denys Smirnov's attempt in 2019: dennwc@3728f96

Co-authored-by: Denys Smirnov
Signed-off-by: Akihiro Suda <[email protected]>
@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jun 25, 2021

Thanks @dennwc !

samuelkarp
samuelkarp reviewed Jun 29, 2021
View reviewed changes
Comment thread .github/workflows/nightly.yml
Comment on lines -52 to -55
libseccomp-dev:amd64 \
libseccomp-dev:arm64 \
libseccomp-dev:s390x \
libseccomp-dev:ppc64el \
Copy link
Copy Markdown
Member

@samuelkarp samuelkarp Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need the libseccomp-dev packages anymore either?

Copy link
Copy Markdown
Member Author

@AkihiroSuda AkihiroSuda Jun 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need libseccomp-dev since 7332e2a

samuelkarp
samuelkarp approved these changes Jun 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

estesp
estesp approved these changes Jun 29, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on the code to switch; any thoughts as to supported nature/any concerns around a single-person Go project (other than your PR to fix 32-bit ARM)? :) I realize we "blindly" depended on libbtrfs from distros prior to this, but curious if any reason to think about that?

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jun 29, 2021

@dennwc

If you like, can we migrate the code to https://github.com/containerd/btrfs (as github.com/containerd/btrfs/v2 go module) and add you as a non-core maintainer of that repo? (Core maintainers also maintain the repo, too)

@dennwc
Copy link
Copy Markdown

dennwc commented Jun 29, 2021

Sure, sounds good! Do you need anything from my side in regards to the old repo?

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jun 29, 2021

Thanks, could you relicense the code to Apache License 2.0 (required by CNCF)?

Then I (or whoever) can open a PR in https://github.com/containerd/btrfs to add "/v2" module with your code.

@dennwc
Copy link
Copy Markdown

dennwc commented Jun 29, 2021

Done!

@AkihiroSuda AkihiroSuda mentioned this pull request Jul 1, 2021
Closed
@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Jul 1, 2021

Thanks, opened containerd/btrfs#34

@AkihiroSuda AkihiroSuda marked this pull request as draft July 15, 2021 05:38
stevvooe
stevvooe requested changes Sep 27, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@stevvooe stevvooe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not LGTM

Existing bindings can be made to not use cgo without breaking the interface. Rejection is in containerd/btrfs#34 (comment) with pointers on how to avoid cgo for struct unpacking.

@dmcgowan dmcgowan removed this from the 1.6 milestone Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevvooe stevvooe stevvooe requested changes

@samuelkarp samuelkarp samuelkarp approved these changes

@estesp estesp estesp approved these changes

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

No one assigned

Labels

impact/changelog status/has-dependency

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Remove necessity to link against C package Replace containerd/btrfs with dennwc/btrfs to eliminate cgo dependency

6 participants

@AkihiroSuda @dennwc @stevvooe @samuelkarp @estesp @dmcgowan