Skip to content

cri: append envs from image config to empty slice to avoid env lost #5024

Merged
estesp merged 1 commit intocontainerd:masterfrom
yadzhang:deepcopy-imageconfig
Feb 18, 2021
Merged

cri: append envs from image config to empty slice to avoid env lost #5024
estesp merged 1 commit intocontainerd:masterfrom
yadzhang:deepcopy-imageconfig

Conversation

@yadzhang
Copy link
Copy Markdown

@yadzhang yadzhang commented Feb 8, 2021

Fix #5023

When call CreateContainer for multiple pods with the same image config but different envs in container spec at nearly the same time, they get image config from imagestore but the Envs slice in the image config is pointing to the same object. The Envs is json.Unmarshal from the content db, so the cap and len of the Envs slice maybe not equal.

So the operation for appending container env into the image env may cause the first few envs of the first container is overlapped by the second container (maybe from another pods) with some probability if the two container has the same image and created at the very near same time.

To resolve this, we can deepcopy image config directly after get image to avoid env or other field conflict.

@k8s-ci-robot
Copy link
Copy Markdown

Hi @yadzhang. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 8, 2021

Build succeeded.

@mxpv
Copy link
Copy Markdown
Member

mxpv commented Feb 9, 2021

Could you please sign off your commits (try git commit -s) in order to pass CI checks?
https://github.com/containerd/project/blob/master/CONTRIBUTING.md#sign-your-work

@yadzhang yadzhang force-pushed the deepcopy-imageconfig branch from d93da42 to 82f27d1 Compare February 9, 2021 04:39
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 9, 2021

Build succeeded.

@yadzhang
Copy link
Copy Markdown
Author

yadzhang commented Feb 9, 2021

Could you please sign off your commits (try git commit -s) in order to pass CI checks?
https://github.com/containerd/project/blob/master/CONTRIBUTING.md#sign-your-work

Done, Thanks.

@ktock
Copy link
Copy Markdown
Member

ktock commented Feb 9, 2021

CI failure seems to related to #4924.

Exception 0xc0000420 0x0 0x0 0x7ff871002e0a
PC=0x7ff871002e0a
signal arrived during external code execution

@fuweid fuweid modified the milestones: 1.5, 1.4.4 Feb 10, 2021
Signed-off-by: Yadong Zhang <yadzhang@gmail.com>
@yadzhang yadzhang force-pushed the deepcopy-imageconfig branch from 82f27d1 to 08318b1 Compare February 18, 2021 03:40
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Feb 18, 2021

Build succeeded.

Copy link
Copy Markdown
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@yadzhang yadzhang changed the title cri: deepcopy imageconfig to avoid env append conflict cri: append envs from image config to empty slice to avoid env lost Feb 18, 2021
@fuweid
Copy link
Copy Markdown
Member

fuweid commented Feb 18, 2021

time="2021-02-18T08:45:54Z" level=info msg="running tests against containerd" revision=66f2b3647c7a3d0618085dc4276a39ab6f426859 runtime=io.containerd.runc.v2 version=66f2b36
time="2021-02-18T08:45:54Z" level=info msg="start to pull seed image" image="docker.io/library/alpine:latest"
--- FAIL: TestContainerPids (0.11s)
    container_test.go:459: expected 2 process but received 1
FAIL

Not sure it is flaky case. rerun the CI.

@fuweid
Copy link
Copy Markdown
Member

fuweid commented Feb 18, 2021

Hmm...

-- FAIL: TestSnapshotterSuite (0.77s)
    --- FAIL: TestSnapshotterSuite/DevMapperUsage (0.77s)
        snapshotter_test.go:141: assertion failed: expression is false: layer2Usage.Size < sizeBytes+256*dmsetup.SectorSize: 1179648 < 1179648
FAIL
FAIL	github.

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

@estesp estesp merged commit c32ccdf into containerd:master Feb 18, 2021
Copy link
Copy Markdown
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

env lost when call createcontainers for different pod with the same image for a high concurrency

7 participants