Skip to content

Rename graph drivers to storage drivers#22228

Closed
samoht wants to merge 9 commits intomoby:masterfrom
samoht:storage-drivers
Closed

Rename graph drivers to storage drivers#22228
samoht wants to merge 9 commits intomoby:masterfrom
samoht:storage-drivers

Conversation

@samoht
Copy link
Member

@samoht samoht commented Apr 21, 2016 •
edited
Loading

- What I did

I've renamed the occurrences of GraphDriver by StorageDriver in the code base to be more consistent with the docs.

The only externally-visible change of that PR is a direct API translation of the experimental storage driver plugins API

- How I did it

I've renamed things via grep/replace but I tried to do that carefully, so variable names and comments are also kept consistent.

- How to verify it

I've run make and make test and it seems to work. I need to update docker/go-plugins-helpers#42 to use the new StorageDriver naming scheme, I'll do that once/if that PR is merged.

I've tried to run the integration tests for the CLI. The experimental storage driver plugins seem broken (see below).

- A picture of a cute animal (not mandatory but encouraged)

cute camel

@samoht samoht changed the title Storage drivers Rename graph drivers to storage drivers Apr 21, 2016
@samoht samoht force-pushed the storage-drivers branch 4 times, most recently from b50f38f to f49deef Compare April 23, 2016 13:44
@samoht
Copy link
Member Author

samoht commented Apr 23, 2016

Hum I seem to have broken the integration-cli tests for experimental storage drivers. I'll investigate that next week:

----------------------------------------------------------------------
14:16:58 FAIL: docker_cli_external_storage_driver_unix_test.go:340: DockerExternalStorageDriverSuite.TestExternalStorageDriver
14:16:58 
14:16:58 [d51499870] waiting for daemon to start
14:16:58 [d51499870] exiting daemon
14:16:58 docker_cli_external_storage_driver_unix_test.go:341:
14:16:58     s.testExternalStorageDriver("test-external-storage-driver", "spec", c)
14:16:58 docker_cli_external_storage_driver_unix_test.go:348:
14:16:58     c.Assert(err, check.IsNil, check.Commentf("\n%s", string(b)))
14:16:58 ... value *errors.errorString = &errors.errorString{s:"[d51499870] Daemon exited during startup"} ("[d51499870] Daemon exited during startup")
14:16:58 ... 
14:16:58 time="2016-04-23T14:16:58Z" level=warning msg="Running experimental build" 
14:16:58 time="2016-04-23T14:16:58.364985085Z" level=debug msg="docker group found. gid: 999" 
14:16:58 time="2016-04-23T14:16:58.365020773Z" level=debug msg="Listener created for HTTP on unix (/go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/d51499870/docker.sock)" 
14:16:58 time="2016-04-23T14:16:58.366134145Z" level=debug msg="Using default logging driver json-file" 
14:16:58 time="2016-04-23T14:16:58.366173315Z" level=debug msg="Golang's threads limit set to 114570" 
14:16:58 time="2016-04-23T14:16:58.367042471Z" level=debug msg="containerd connection state change: READY" 
14:16:58 time="2016-04-23T14:16:58.367453406Z" level=debug msg="received past containerd event: &types.Event{Type:\"live\", Id:\"\", Status:0x0, Pid:\"\", Timestamp:0x571b83da}" 
14:16:58 time="2016-04-23T14:16:58.419219301Z" level=debug msg="[storage] trying provided driver \"test-external-storage-driver\"" 
14:16:58 time="2016-04-23T14:16:58.419982126Z" level=error msg="Failed to GetDriver storage test-external-storage-driver /go/src/github.com/docker/docker/bundles/1.12.0-dev/test-integration-cli/d51499870/root" 
14:16:58 time="2016-04-23T14:16:58.420009911Z" level=debug msg="Cleaning up old mountid : start." 
14:16:58 time="2016-04-23T14:16:58.420061568Z" level=fatal msg="Error starting daemon: error initializing storage: driver not supported" 
14:16:58

@thaJeztah thaJeztah added status/failing-ci Indicates that the PR in its current state fails the test suite status/1-design-review and removed status/0-triage labels Apr 25, 2016
@thaJeztah
Copy link
Member

thaJeztah commented May 12, 2016

ping @samoht have you had time to look into those failing tests?

@samoht
Copy link
Member Author

samoht commented May 12, 2016

I plan to look at that again in the following days. Do you think that renaming is worth continuing?

@thaJeztah
Copy link
Member

thaJeztah commented May 12, 2016

Hm, not sure, I know @tiborvass proposed doing so in the past, but not sure if it's really important (well, it's more consistent probably)

@samoht
Copy link
Member Author

samoht commented May 12, 2016

ok, so I'll rebase and see what I can do with the failing tests.

@samoht samoht force-pushed the storage-drivers branch from f49deef to edcf328 Compare May 12, 2016 16:00
@thaJeztah
Copy link
Member

thaJeztah commented May 12, 2016

ok! We discussed this, and we like it, so moving to code review. Would be nice if we could give the graph driver plugin maintainers a notice :)

@samoht samoht force-pushed the storage-drivers branch 2 times, most recently from 77c3f88 to c768b24 Compare May 13, 2016 09:53
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 13, 2016
@samoht samoht force-pushed the storage-drivers branch from c768b24 to 0461640 Compare May 13, 2016 09:54
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 13, 2016
@samoht samoht force-pushed the storage-drivers branch from 0461640 to 7ac43c5 Compare May 13, 2016 10:08
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 13, 2016
@cpuguy83
Copy link
Member

cpuguy83 commented May 13, 2016

ping @kolyshkin

@samoht samoht force-pushed the storage-drivers branch 3 times, most recently from a03d204 to 1561a8f Compare May 13, 2016 17:13
@samoht samoht force-pushed the storage-drivers branch from afb395d to 6d8ab5e Compare May 13, 2016 17:31
@GordonTheTurtle GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 13, 2016
@samoht samoht force-pushed the storage-drivers branch from 6d8ab5e to 548ebe3 Compare May 13, 2016 17:32
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label May 13, 2016
@samoht
Copy link
Member Author

samoht commented May 13, 2016

I think I've fixed the docker_cli_external_storage_driver_unix_test issue.

Normally, all my changes are not visible from the user, apart the changes to the experimental storage driver plugins. The remaining bits where GraphDriver still appears are:

I don't plan to change theses. Next step for me is to update docker/go-plugins-helpers#42

@samoht samoht force-pushed the storage-drivers branch from 548ebe3 to dc2fc2f Compare May 13, 2016 17:37
@kolyshkin
Copy link
Contributor

kolyshkin commented May 13, 2016

LGTM

@samoht
Copy link
Member Author

samoht commented May 13, 2016

A interesting new error for wind2lin:

19:22:28 + git clone https://github.com/docker/notary.git /tmp/tmp.vCngyuJWWd/src/github.com/docker/notary
19:22:28 Cloning into '/tmp/tmp.vCngyuJWWd/src/github.com/docker/notary'...
19:32:30 error: RPC failed; result=56, HTTP code = 200
19:32:30 fatal: The remote end hung up unexpectedly
19:32:30 fatal: early EOF
19:32:30 fatal: index-pack failed

For TP5, I'm not sure where this comes from...

17:42:03 ---> Making bundle: .ensure-frozen-images-windows (in bundles/1.12.0-dev/test-integration-cli)
17:42:03 ERROR: Could not find windowsservercore images

@tiborvass
Copy link
Contributor

tiborvass commented May 16, 2016

LGTM

samoht added 9 commits May 16, 2016 16:12
@samoht
Rename Graphdriver into Storage Drivers in daemon/
95bbfdd 
This is already the case in most of the CLI. This PR updates the codebase
to be more consistent. This should not change anything for the user.

Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename GraphDrivers into Storage Drivers in layer/
5ce62b6 
This is already the case in most of the CLI. This PR updates the codebase
to be more consistent. This should not change anything for the user.

Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename GraphDriver into Storage Driver in pkg/archive comments
2c59872 
Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename GraphDriver in Storage Driver in container/ comments
f286a72 
Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename GraphDriver into StorageDriver in contrib/
cce36df 
Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename experimental GraphDriver plugin API to StorageDriver
61ac716 
This is to keep consistent with the name that we are using in the documentation,
where "storage drivers" is prefered over the now deprecated "graph drivers"
terminology.

Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Update integration-cli tests to GraphDriver => StorageDriver API changes
4a527f1 
Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Rename defaultGraph to defaultRuntimeRoot in the daemon.
2d1d799 
This match with what the documentation for `--graph` says.

Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht
Update internal docs to refer to storage driver instead of graphdriver
4fdd20a 
Signed-off-by: Thomas Gazagnaire <[email protected]>
@samoht samoht force-pushed the storage-drivers branch from dc2fc2f to 4fdd20a Compare May 16, 2016 23:12
@samoht
Copy link
Member Author

samoht commented May 16, 2016

Rebased on master, and got new funky errors (yay):

23:14:23 docker: Error response from daemon: devmapper: Error mounting '/dev/mapper/docker-202:1-75497728-1220c22c26ac33994ddca2ef67a0e6d2abb2f46ef1b6069e92e32469063ddd87-init' on '/var/lib/docker/devicemapper/mnt/1220c22c26ac33994ddca2ef67a0e6d2abb2f46ef1b6069e92e32469063ddd87-init': no space left on device.

@samoht
Copy link
Member Author

samoht commented May 19, 2016

Still an issue with TP5:

ERROR: Could not find windowsservercore images

Is that something expected?

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 19, 2016
@thaJeztah
Copy link
Member

thaJeztah commented May 19, 2016

Restarted the Windows build, doesn't look like its related

@thaJeztah
Copy link
Member

thaJeztah commented Jun 3, 2016

oh boy; this needs a rebase now

@vdemeester vdemeester mentioned this pull request Jun 3, 2016
Closed
@thaJeztah
Copy link
Member

thaJeztah commented Jun 10, 2016

let me close this for now, because it's carried in #23237, but we still need to figure out the Windows issue 😢

@thaJeztah thaJeztah closed this Jun 10, 2016
@thaJeztah thaJeztah mentioned this pull request Sep 14, 2016
Closed
@thaJeztah thaJeztah mentioned this pull request Nov 22, 2016
Merged
@thaJeztah thaJeztah mentioned this pull request Aug 4, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@samoht @thaJeztah @cpuguy83 @kolyshkin @tiborvass @GordonTheTurtle