Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix handling of /dev/shm mounting inside of containers #1545

Merged
merged 1 commit into from May 4, 2018

Conversation

rhatdan
Copy link
Contributor

@rhatdan rhatdan commented May 2, 2018

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 2, 2018
@rhatdan
Copy link
Contributor Author

rhatdan commented May 2, 2018

@mrunalp @umohnani8 PTAL, I think if this test passes, it proves that we are sharing /dev/shm between containers. For some reason tests are not currently running locally on my host.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 2, 2018
@rhatdan rhatdan force-pushed the shm branch 2 times, most recently from 0a9ad8d to f83d074 Compare May 2, 2018 22:51
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 2, 2018
@rhatdan rhatdan changed the title Add test to make sure /dev/shm is shared between containers in CRI-O Fix handling of /dev/shm mounting inside of containers May 2, 2018
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2018
test/shm.bats Outdated
echo "$output"
[ "$status" -eq 0 ]
pod_id="$output"
run crictl create "$pod_id" "$TESTDIR"/container_sleep.json "$TESTDATA"/sandbox_config.json
Copy link
Member

Choose a reason for hiding this comment

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

Should be $TESTDATA/container_sleep.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for hitting me with a clue bat.

test/shm.bats Outdated
echo "$output"
[ "$status" -eq 0 ]

run crictl create "$pod_id" "$TESTDIR"/container_redis.json "$TESTDATA"/sandbox_config.json
Copy link
Member

Choose a reason for hiding this comment

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

Should be $TESTDATA/container_redis.json

@umohnani8
Copy link
Member

Tests are blowing up because it is looking in the wrong place for the json files.
Code changes LGTM

{
"key": "PATH",
"value": "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
},
Copy link
Member

Choose a reason for hiding this comment

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

@rhatdan the comma is causing the tests to blow up

Add test to make sure /dev/shm is shared between containers in CRI-O

Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@rhatdan
Copy link
Contributor Author

rhatdan commented May 3, 2018

/test all

@rhatdan
Copy link
Contributor Author

rhatdan commented May 3, 2018

@mrunalp @umohnani8 @wking @runcom PTAL

@umohnani8
Copy link
Member

LGTM

@rhatdan
Copy link
Contributor Author

rhatdan commented May 3, 2018

/test kata-containers

Copy link
Member

@mrunalp mrunalp left a comment

Choose a reason for hiding this comment

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

LGTM

@rhatdan
Copy link
Contributor Author

rhatdan commented May 4, 2018

All Green Merging.

@rhatdan rhatdan merged commit 0489381 into cri-o:master May 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants