This repository was archived by the owner on Mar 9, 2022. It is now read-only.
Apply container image config #52
Merged
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
Member
Author
|
I've run all CRI validation test. Only the container log and |
Random-Liu
force-pushed
the
apply-container-image-config
branch
from
326b8b7 to
8de8304
Compare
Random-Liu
force-pushed
the
apply-container-image-config
branch
2 times, most recently
from
3c22097 to
0609417
Compare
Member
Author
yujuhong
reviewed
May 30, 2017
|
|
||
| // addImageEnvs adds environment variables from image config. It returns error if | ||
| // an invalid environment variable is encountered. | ||
| func addImageEnvs(g *generate.Generator, imageEnvs []string) error { |
Member
There was a problem hiding this comment.
I was wondering why you didn't do this in the sandbox PR ;)
| if err != nil { | ||
| glog.Exitf("Failed to create CRI containerd service %+v: %v", o, err) | ||
| } | ||
| if err := service.Start(); err != nil { |
Member
Author
There was a problem hiding this comment.
Found this when running cri validation container test.
I could send another PR to fix this.
Member
Author
There was a problem hiding this comment.
Since travis is overloaded, it takes a long time to test a PR.
I'll split this into another commit.
pkg/server/container_create.go
Outdated
| // Prepare container image snapshot. For container, the image should have | ||
| // been pulled before creating the container, so do not ensure the image. | ||
| image := config.GetImage().GetImage() | ||
| imageMeta, err := c.ensureImageExists(ctx, image, false /*ensure*/) |
Member
There was a problem hiding this comment.
Commented on #48. Would prefer breaking down the function.
pkg/server/container_start.go
Outdated
| func setOCIProcessArgs(g *generate.Generator, config *runtime.ContainerConfig, imageConfig *imagespec.ImageConfig) error { | ||
| command, args := config.GetCommand(), config.GetArgs() | ||
| // The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go | ||
| // TODO(random-liu): Figure out whether the following logic is |
Member
Author
There was a problem hiding this comment.
Incomplete comment, will complete it.
| command, args := config.GetCommand(), config.GetArgs() | ||
| // The following logic is migrated from https://github.com/moby/moby/blob/master/daemon/commit.go | ||
| // TODO(random-liu): Figure out whether the following logic is | ||
| if len(command) == 0 { |
Member
There was a problem hiding this comment.
I don't entirely understand why the code below is organized this way...
Member
Author
There was a problem hiding this comment.
This is the current behavior of docker, I don't fully understood it, either. However, there is no definition of how this should behave, so I just reuse the docker logic.
The TODO is to clearly define this and fix the logic here.
Random-Liu
force-pushed
the
apply-container-image-config
branch
from
0609417 to
5ba915b
Compare
Member
Author
|
@yujuhong Addressed comments. |
Member
|
LGTM |
Member
Author
|
Apply lgtm based on #52 (comment), will rebase and merge this PR after test passes and dependency is merged. |
Signed-off-by: Lantao Liu <[email protected]>
Signed-off-by: Lantao Liu <[email protected]>
Signed-off-by: Lantao Liu <[email protected]>
Random-Liu
force-pushed
the
apply-container-image-config
branch
from
5ba915b to
80384fc
Compare
lanchongyizu
pushed a commit
to lanchongyizu/cri-containerd
that referenced
this pull request
Sep 3, 2017
…e-config Apply container image config
jstarks
pushed a commit
to jstarks/cri
that referenced
this pull request
Jan 30, 2020
add support for kubernetes empty dir in LCOW
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 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.
Based on #41, #46 and #48.
This PR pulls container image and applies image config.
Only the last commit is new. Will rebase the PR and previous PRs soon.