feature: support to choose storage driver #2525

wangforthinker · 2018-12-03T03:59:18Z

Signed-off-by: allen.wang [email protected]

Ⅰ. Describe what this PR did

This PR is supported to choose storage driver to start pouchd. Before this PR, pouchd only supports tp start with overlayfs.

for the containerd part, add a pr of #2525

Ⅱ. Does this pull request fix one issue?

fixes one part of #2486

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

add test cli_stroage_driver_test.
After containerd merge overlay1fs storage driver, I will add tests for overlay1fs.

Ⅳ. Describe how to verify it

add tests.

Ⅴ. Special notes for reviews

Where pouchd connects with containerd, I changed defaultSnapshotterName(overlayfs) to CurrentSnapshotterName().
Considered to that storage driver should be consistent once pouchd started, I add a check to verify the validity of given storage driver. And storage driver could be changed only after old snapshots are entirely removed.

codecov · 2018-12-03T03:59:22Z

Codecov Report

Merging #2525 into master will decrease coverage by 14.39%.
The diff coverage is 59.21%.

@@            Coverage Diff             @@
##           master    #2525      +/-   ##
==========================================
- Coverage   69.17%   54.77%   -14.4%     
==========================================
  Files         278      276       -2     
  Lines       18623    18655      +32     
==========================================
- Hits        12883    10219    -2664     
- Misses       4268     7329    +3061     
+ Partials     1472     1107     -365

Flag	Coverage Δ
#criv1alpha1test	`?`
#criv1alpha2test	`?`
#integrationtest	`40.71% <59.21%> (+0.04%)`	⬆️
#nodee2etest	`?`
#unittest	`26.66% <0%> (-0.1%)`	⬇️

Impacted Files	Coverage Δ
cri/v1alpha2/cri.go	`0% <ø> (-69.12%)`	⬇️
daemon/config/config.go	`66.21% <ø> (ø)`	⬆️
daemon/mgr/container_types.go	`70.71% <ø> (-2.15%)`	⬇️
cri/v1alpha1/cri.go	`0% <0%> (-60.93%)`	⬇️
daemon/mgr/snapshot.go	`33.33% <0%> (-56.53%)`	⬇️
daemon/mgr/system.go	`73.28% <100%> (ø)`	⬆️
ctrd/image.go	`75.21% <100%> (-3.73%)`	⬇️
ctrd/container.go	`52.96% <100%> (-5.85%)`	⬇️
daemon/daemon_utils.go	`50% <42.85%> (-12.5%)`	⬇️
daemon/daemon.go	`68.58% <50%> (-0.61%)`	⬇️
... and 58 more

allencloud · 2018-12-03T06:37:13Z

Thanks for your work, @wangforthinker . I am afraid that there are still a few things you need to do:

squash your commits into a single one.
please check the CI failures when code check.

daemon/daemon.go

daemon/daemon_utils.go

main.go

vendor/github.com/containerd/containerd/image.go

daemon/daemon_utils.go

allencloud · 2018-12-11T12:53:35Z

I have some confusions: @wangforthinker

Do we vendor container package from alibaba/container? if yes, is the update of vendor.json correct?

In addition, I would like to invite @Ace-Tang @fuweid to take a look at of the feature.
And also cc @HusterWan to check if this PR would break all current compatibility.

cri/v1alpha1/cri.go

daemon/daemon.go

rudyfly · 2018-12-12T15:20:41Z

daemon/daemon_utils.go

+		}
+
+		if found {
+			driverFound = found


I think it should break loop at here.

And no need to add more found, just use driverFound, like this:

for _, info := range infos { var err error driverFound, err = checkSnapshotterDir(snapshotter, containerdRootDir, info) if err != nil { return err } if driverFound { return nil } }

If a driver is found in containerd snapshotter, we also need to check other snapshotter dir whether it exists snapshots. If another snapshotter has snapshots, another snapshotter may be set last startup by pouchd. @rudyfly

Not followed what you say, why should check other snapshotter after found

For example, We start pouchd with overlayfs and then pull some images and create containers. Then we restart pouchd with brtfs. Overlayfs and btrfs are all supported by containerd, but the startup with btrfs should fail, otherwise previous images and containers may be in wrong use. It is important to make sure snapshotter to be consistent, except snapshots have been removed. @Ace-Tang

daemon/daemon_utils.go

wangforthinker · 2018-12-13T07:26:00Z

I have some confusions: @wangforthinker

Do we vendor container package from alibaba/container? if yes, is the update of vendor.json correct?

In addition, I would like to invite @Ace-Tang @fuweid to take a look at of the feature.
And also cc @HusterWan to check if this PR would break all current compatibility.

Yes, we vendor some update from aliababa/containerd. @allencloud

cri/v1alpha1/cri.go

fuweid · 2018-12-13T08:49:52Z

vendor/vendor.json

-			"revision": "773c489c9c1b21a6d78b5c538cd395416ec50f88",
-			"revisionTime": "2018-04-02T20:34:37Z",
+			"revision": "8d8f780274f3fcf2efb5d21c25569b6fc3bb08fb",
+			"revisionTime": "2018-12-05T03:53:22Z",


could you mind to provide the method about how to update vendor? Thanks

govendor update github.com/containerd/containerd

这个是在更新到本地的containerd版本。

daemon/daemon.go

daemon/daemon_utils.go

fuweid · 2018-12-18T07:42:01Z

ctrd/snapshot.go

-	service := wrapperCli.client.SnapshotService(defaultSnapshotterName)
+	// Allow user to walk snapshots of specific snapshotter
+	snapshotter := CurrentSnapshotterName()
+	if sn, ok := ctx.Value(SnapshotterKey).(string); ok {


I don't think that the change is reasonable. I think we should change the interface so that the user can pass the specific snapshoter type for walk.

WalkSnapshot(ctx context.Context, snapshot string, fn func(context.Context, snapshots.Info) error) error

WDYT?

fuweid · 2018-12-18T07:58:22Z

daemon/daemon_utils.go

 }
+
+// checkSnapshotter checks whether the given snapshotter is valid
+func checkSnapshotter(snapshotter string, ctrdClient ctrd.APIClient) error {


how about checkConflictSnapshotter? I think the function name should be more specific. WDYT?

fuweid · 2018-12-18T08:00:06Z

daemon/daemon_utils.go

+	return nil
+}
+
+func checkSnapshots(snapshotter string, ctrdClient ctrd.APIClient) (existSnapshot bool, err error) {


like the last comment. the developer will care about checking for what.

test/cli_snapshotter_test.go

Ace-Tang · 2018-12-18T09:31:04Z

We got different user cases, that means we may not always allow one graphdriver. Like kata want use one which not suit for runc, but we should always allow different runtimes. So maybe pouch will allow different snapshotters simultaneously in the feature.

allencloud · 2018-12-18T09:59:27Z

CI failure is unrelated:

FILE: ./README.md
[✖] https://app.fossa.io/api/projects/git%2Bhttps%3A%2F%2Fgithub.com%2Falibaba%2Fpouch.svg?type=shield

ERROR: dead links found!

wangforthinker · 2018-12-18T10:12:51Z

We got different user cases, that means we may not always allow one graphdriver. Like kata want use one which not suit for runc, but we should always allow different runtimes. So maybe pouch will allow different snapshotters simultaneously in the feature.

Yes, If different snapshotters is needed to be supportted, we should reconstruct containerd and pouch which need more relations between snapshots and image(or containers) . We may consider it in the feature.

Signed-off-by: allen.wang <[email protected]>

Ace-Tang · 2018-12-18T12:48:17Z

@wangforthinker , no, we do not need to change containerd but only pouch, containerd no need to know these informations

Ace-Tang · 2018-12-19T01:45:04Z

Overall the PR looks good to me.

pouchrobot added areas/storage size/XL labels Dec 3, 2018

wangforthinker force-pushed the feature/support-choose-storage-driver branch 2 times, most recently from 7f317c6 to b3a71b5 Compare December 3, 2018 06:05

wangforthinker requested review from Ace-Tang, fuweid and starnop December 3, 2018 06:06

wangforthinker force-pushed the feature/support-choose-storage-driver branch from b3a71b5 to bc8ce30 Compare December 3, 2018 06:08

allencloud changed the title ~~support to choose storage driver~~ feature: support to choose storage driver Dec 3, 2018

pouchrobot added the kind/feature label Dec 3, 2018

wangforthinker force-pushed the feature/support-choose-storage-driver branch from 6a53a15 to 37a4dda Compare December 3, 2018 07:00

Ace-Tang reviewed Dec 3, 2018

View reviewed changes

daemon/daemon.go Outdated Show resolved Hide resolved

wangforthinker force-pushed the feature/support-choose-storage-driver branch 4 times, most recently from 2761ba9 to efffb26 Compare December 4, 2018 06:34