Do not merge: additional support routines for CRI use cases #3
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Having a library vendor another library isn't a good practice in Go. It makes it hard for the end application to re-vendor everything. re: 9413337 |
Member
Author
|
Hmm, splitting out or dropping the CLI wrapper would probably help with that. |
nalind
force-pushed
the
cri
branch
2 times, most recently
from
dd2f7de to
fd3314a
Compare
Contributor
|
@nalind got it, understood. Probably not a huge deal, didn't notice the CLI wrapper. |
nalind
force-pushed
the
cri
branch
4 times, most recently
from
447c2c4 to
d1d9a95
Compare
nalind
force-pushed
the
cri
branch
8 times, most recently
from
e724bc0 to
cb7bbdc
Compare
Member
Author
|
Going to see if moving the logic changes into |
nalind
force-pushed
the
cri
branch
7 times, most recently
from
1a67c5f to
73575d3
Compare
Change the way we do vendoring to work the way GO15VENDOREXPERIMENT=1 does things. This is the way the various other 'containers' projects do things, and I suspect this repository being different was making things harder for other contributors. Signed-off-by: Nalin Dahyabhai <[email protected]>
Add an API layer that wraps the creation and removal of sandbox containers and regular containers into functions that more closely match where CRI-O expects to hand things off to containers/storage, and which also moves the formatting and storage of related metadata to a location which can be shared with others that want to integrate with it. This API is being revised as the implementation is being written. Signed-off-by: Nalin Dahyabhai <[email protected]>
Add my image add-storage-transport branch to the list of dependencies that vendor.sh fetches, and its dependencies. This ends up adding test-related files for some of the modules that we were already vendoring. Try not to merge this until containers/image has support for containers/storage, and we can switch to consuming that. Signed-off-by: Nalin Dahyabhai <[email protected]>
Signed-off-by: Nalin Dahyabhai <[email protected]>
Spotted in review of cri-o/cri-o#210 by Crazykev. Signed-off-by: Nalin Dahyabhai <[email protected]>
Use the image store's Get() method to find an image that matches the passed-in ID rather than iterating through them all ourselves, since Get() will soon be able to handle truncated IDs correctly. Signed-off-by: Nalin Dahyabhai <[email protected]>
Based on review comments from cri-o/cri-o#210: * use a deferred anonymous function to do cleanup if creating a container fails, instead of explicitly calling a local function * add missing godoc string for ImageServer.GetStore * remove context.Context arguments for ImageServer and RuntimeServer methods, which we ended up never using anyway Signed-off-by: Nalin Dahyabhai <[email protected]>
Revise based on review comments in cri-o/cri-o#210 Signed-off-by: Nalin Dahyabhai <[email protected]>
Though we can use a defaultTransport value in PullImage, handle the case where the value we were passed at startup isn't empty. Signed-off-by: Nalin Dahyabhai <[email protected]>
Don't require callers of CreateContainer() or CreatePodSandbox() to parse the source image's configuration to get at that information, when we're in a better position to do it for them. Signed-off-by: Nalin Dahyabhai <[email protected]>
Go ahead and force a pod's infrastructure container's ID to match its pod sandbox's ID, since we're depending on that correlation elsewhere. Signed-off-by: Nalin Dahyabhai <[email protected]>
Signed-off-by: Nalin Dahyabhai <[email protected]>
Member
|
@nalind isn't this surpassed by cri-o/cri-o#210? |
Member
Author
|
@runcom I think so. It certainly avoids some of the interdependency problems with |
Member
Author
|
Since cri-o/cri-o#210 was merged, these routines don't belong here any more. Closing without merging. |
kolyshkin
added a commit
to kolyshkin/storage
that referenced
this pull request
May 16, 2020
This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there"; - for `MkdirAll()`, `IsExist` error should NEVER be ignored. This commit removes ignoring the IsExist error, as it should not be ignored. For more details, a quote from my opencontainers/runc#162 (July 2015): -quote- TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: MkdirAll creates a directory named path, along with any necessary parents, and returns nil, or else returns an error. If path is already a directory, MkdirAll does nothing and returns nil. This means two things: If a directory to be created already exists, no error is returned. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in containers#2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of containers#1, IsExist check after MkdirAll is not needed. Because of containers#2 and containers#3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go -end-quote- Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
added a commit
to kolyshkin/storage
that referenced
this pull request
May 18, 2020
This subtle bug keeps lurking in because error checking for `Mkdir()` and `MkdirAll()` is slightly different wrt `EEXIST`/`IsExist`: - for `Mkdir()`, `IsExist` error should (usually) be ignored (unless you want to make sure directory was not there before) as it means "the destination directory was already there"; - for `MkdirAll()`, `IsExist` error should NEVER be ignored. This commit removes ignoring the IsExist error, as it should not be ignored. For more details, a quote from my opencontainers/runc#162 (July 2015): -quote- TL;DR: check for IsExist(err) after a failed MkdirAll() is both redundant and wrong -- so two reasons to remove it. Quoting MkdirAll documentation: MkdirAll creates a directory named path, along with any necessary parents, and returns nil, or else returns an error. If path is already a directory, MkdirAll does nothing and returns nil. This means two things: If a directory to be created already exists, no error is returned. If the error returned is IsExist (EEXIST), it means there exists a non-directory with the same name as MkdirAll need to use for directory. Example: we want to MkdirAll("a/b"), but file "a" (or "a/b") already exists, so MkdirAll fails. The above is a theory, based on quoted documentation and my UNIX knowledge. In practice, though, current MkdirAll implementation [1] returns ENOTDIR in most of cases described in containers#2, with the exception when there is a race between MkdirAll and someone else creating the last component of MkdirAll argument as a file. In this very case MkdirAll() will indeed return EEXIST. Because of containers#1, IsExist check after MkdirAll is not needed. Because of containers#2 and containers#3, ignoring IsExist error is just plain wrong, as directory we require is not created. It's cleaner to report the error now. Note this error is all over the tree, I guess due to copy-paste, or trying to follow the same usage pattern as for Mkdir(), or some not quite correct examples on the Internet. [1] https://github.com/golang/go/blob/f9ed2f75/src/os/path.go -end-quote- Signed-off-by: Kir Kolyshkin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These change introduces a new package which aims to make it easier for consumers of the library that want to support CRI use cases to interoperate with each other, by sharing a common metadata format that we can encode and store in the storage library's more basic locations.
The support for holding and tracking container/pod status in runtime.go might not be used, and if they go, fields which only they touch can be dropped, too.
We also take the vendoring script and move things around to better match the newer Go style of doing vendoring, which should probably be moved to a different PR for merging, but currently lives here because we make a minor change on top of it.