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 parsing of Tmpfs field in compat create #9512

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

mheon
Copy link
Member

@mheon mheon commented Feb 25, 2021

Create is not formatted as key=value but rather key:value (technically path:option1,option2). As such we can't use the stringMapToArray function, and instead need to generate it manually.

Fixes #9511

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2021
@mheon
Copy link
Member Author

mheon commented Feb 25, 2021

CI will fail because I didn't add tests. Will do so tomorrow morning.

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

@edsantiago
Copy link
Member

@mheon this is a hard one to test; I'm working on it but need to head out for a few hours

@edsantiago
Copy link
Member

@mheon FWIW here's my test so far; I'm wondering if there's a bug in your patch? Basically, the filesystem seems to be getting rw appended to it instead of =rw (I don't think it should get either):

# -*- sh -*-

podman pull $IMAGE &>/dev/null

# Test various HostConfig options
tmpfs_name="/mytmpfs"
t POST containers/create?name=hostconfig_test '"Image":"'$IMAGE'","Cmd":["df"],"HostConfig":{"TmpFs":{"'$tmpfs_name'":"rw"}}' 201 \
  .Id~[0-9a-f]\\{64\\}
cid=$(jq -r '.Id' <<<"$output")

# Prior to #9512, the tmpfs would be called '/mytmpfs=rw', with the '=rw'
t GET containers/${cid}/json 200 dsf \
  .HostConfig.Tmpfs[\"${tmpfs_name}\"]~rw,

# Run the container, verify output
t POST containers/${cid}/start '' 204
t POST containers/${cid}/wait  '' 200
t GET  containers/${cid}/logs?stdout=true  200

like "$(<$WORKDIR/curl.result.out)" ".* ${tmpfs_name}x" \
     "'df' output includes tmpfs name"

Save that as test/apiv2/44-foo.at or whatever you like; then

$ test/apiv2/test-apiv2 foo                                                                                                                     pr/9512
ok 1 [44-foo] POST containers/create?name=hostconfig_test [-d {"Image":"quay.io/libpod/alpine_labels:latest","Cmd":["df"],"HostConfig":{"TmpFs":{"/mytmpfs":"rw"}}}] : status=201
ok 2 [44-foo] POST containers/create?name=hostconfig_test [-d {"Image":"quay.io/libpod/alpine_labels:latest","Cmd":["df"],"HostConfig":{"TmpFs":{"/mytmpfs":"rw"}}}] : .Id ('cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326') ~ [0-9a-f]\{64\}
ok 3 [44-foo] GET containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/json : status=200
not ok 4 [44-foo] GET containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/json : output
#  expected: dsf
#    actual: {"Id":"cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326","Created":"2021-02-25T10:12:39.903218994-07:00","Path":"df","Args":["df"],"State":{"Status":"created","Running":false,"Paused":false,"Restarting":false,"OOMKilled":false,"Dead":false,"Pid":0,"ExitCode":0,"Error":"","StartedAt":"0001-01-01T00:00:00Z","FinishedAt":"0001-01-01T00:00:00Z"},"Image":"quay.io/libpod/alpine_labels:latest","ResolvConfPath":"","HostnamePath":"","HostsPath":"","LogPath":"/dev/shm/test-apiv2.tmp.5oWFkz/overlay-containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/userdata/ctr.log","Name":"/hostconfig_test","RestartCount":0,"Driver":"overlay","Platform":"linux","MountLabel":"system_u:object_r:container_file_t:s0:c225,c288","ProcessLabel":"system_u:system_r:container_t:s0:c225,c288","AppArmorProfile":"","ExecIDs":[],"HostConfig":{"Binds":[],"ContainerIDFile":"","LogConfig":{"Type":"json-file","Config":null},"NetworkMode":"slirp4netns","PortBindings":{},"RestartPolicy":{"Name":"","MaximumRetryCount":0},"AutoRemove":false,"VolumeDriver":"","VolumesFrom":null,"CapAdd":[],"CapDrop":["MKNOD","AUDIT_WRITE","NET_RAW"],"CgroupnsMode":"","Dns":[],"DnsOptions":[],"DnsSearch":[],"ExtraHosts":[],"GroupAdd":[],"IpcMode":"private","Cgroup":"","Links":null,"OomScoreAdj":0,"PidMode":"private","Privileged":false,"PublishAllPorts":false,"ReadonlyRootfs":false,"SecurityOpt":[],"Tmpfs":{"/mytmpfsrw":"rw,rprivate,nosuid,nodev,tmpcopyup"},"UTSMode":"private","UsernsMode":"","ShmSize":65536000,"Runtime":"oci","ConsoleSize":[0,0],"Isolation":"","CpuShares":0,"Memory":0,"NanoCpus":0,"CgroupParent":"user.slice","BlkioWeight":0,"BlkioWeightDevice":null,"BlkioDeviceReadBps":null,"BlkioDeviceWriteBps":null,"BlkioDeviceReadIOps":null,"BlkioDeviceWriteIOps":null,"CpuPeriod":0,"CpuQuota":0,"CpuRealtimePeriod":0,"CpuRealtimeRuntime":0,"CpusetCpus":"","CpusetMems":"","Devices":[],"DeviceCgroupRules":null,"DeviceRequests":null,"KernelMemory":0,"KernelMemoryTCP":0,"MemoryReservation":0,"MemorySwap":0,"MemorySwappiness":0,"OomKillDisable":false,"PidsLimit":2048,"Ulimits":[],"CpuCount":0,"CpuPercent":0,"IOMaximumIOps":0,"IOMaximumBandwidth":0,"MaskedPaths":null,"ReadonlyPaths":null},"GraphDriver":{"Data":{"LowerDir":"/dev/shm/test-apiv2.tmp.5oWFkz/overlay/df64d3292fd6194b7865d7326af5255db6d81e9df29f48adde61a918fbd8c332/diff","UpperDir":"/dev/shm/test-apiv2.tmp.5oWFkz/overlay/898f964440a7d527d5d3e7b6fe06902251a08e133228c73a58d47a99b3e7f227/diff","WorkDir":"/dev/shm/test-apiv2.tmp.5oWFkz/overlay/898f964440a7d527d5d3e7b6fe06902251a08e133228c73a58d47a99b3e7f227/work"},"Name":"overlay"},"SizeRootFs":0,"Mounts":[],"Config":{"Hostname":"cb1b6971df4d","Domainname":"","User":"","AttachStdin":false,"AttachStdout":false,"AttachStderr":false,"Tty":false,"OpenStdin":false,"StdinOnce":false,"Env":["PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin","TERM=xterm","container=podman"],"Cmd":["df"],"Image":"quay.io/libpod/alpine_labels:latest","Volumes":null,"WorkingDir":"/","Entrypoint":[],"OnBuild":null,"Labels":{"PODMAN":"/usr/bin/podman run -it --name NAME -e NAME=NAME -e IMAGE=IMAGE IMAGE echo podman","install":"docker run -it --name NAME -e NAME=NAME -e IMAGE=IMAGE IMAGE echo install","options":"/usr/bin/podman run -it --name NAME -e NAME=NAME -e IMAGE=IMAGE IMAGE echo $OPT1 ","run":"docker run -it --name NAME -e NAME=NAME -e IMAGE=IMAGE IMAGE echo RUN","uninstall":"/usr/bin/docker run -it --name NAME -e NAME=NAME -e IMAGE=IMAGE IMAGE echo uninstall","useradds":"/usr/bin/docker run -it IMAGE echo"},"StopSignal":"15","StopTimeout":0},"NetworkSettings":{"Bridge":"","SandboxID":"","HairpinMode":false,"LinkLocalIPv6Address":"","LinkLocalIPv6PrefixLen":0,"Ports":{},"SandboxKey":"","SecondaryIPAddresses":null,"SecondaryIPv6Addresses":null,"EndpointID":"","Gateway":"","GlobalIPv6Address":"","GlobalIPv6PrefixLen":0,"IPAddress":"","IPPrefixLen":0,"IPv6Gateway":"","MacAddress":"","Networks":null}}
not ok 5 [44-foo] GET containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/json : .HostConfig.Tmpfs["/mytmpfs"]
#  expected: ~ rw,
#    actual: null
ok 6 [44-foo] POST containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/start [-d {}] : status=204
ok 7 [44-foo] POST containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/wait [-d {}] : status=200
ok 8 [44-foo] GET containers/cb1b6971df4da69ee329c830209f148b1c3938a3b5cb3bc994829db2f9c05326/logs?stdout=true : status=200
/home/esm/src/atomic/2018-02.podman/libpod/test/apiv2/44-foo.at: line 20: warning: command substitution: ignored null byte in input
not ok 9 [44-foo] 'df' output includes tmpfs name
#  expected: ~ .* /mytmpfsx
#    actual: CFilesystem           1K-blocks      Used Available Use% Mounted on
:fuse-overlayfs        16360504      5516  16354988   0% /
=tmpfs                    65536         0     65536   0% /dev
Ctmpfs                 16360504         0  16360504   0% /mytmpfsrw
Etmpfs                  3272100      2084   3270016   0% /run/secrets
Itmpfs                  3272100      2084   3270016   0% /etc/resolv.conf
Ctmpfs                  3272100      2084   3270016   0% /etc/hosts
Ashm                      64000         0     64000   0% /dev/shm
Ftmpfs                  3272100      2084   3270016   0% /etc/hostname
Ktmpfs                  3272100      2084   3270016   0% /run/.containerenv
Bdevtmpfs              16340068         0  16340068   0% /dev/null
Bdevtmpfs              16340068         0  16340068   0% /dev/zero
Bdevtmpfs              16340068         0  16340068   0% /dev/full
Adevtmpfs              16340068         0  16340068   0% /dev/tty
Ddevtmpfs              16340068         0  16340068   0% /dev/random
Edevtmpfs              16340068         0  16340068   0% /dev/urandom
Ddevtmpfs              16340068         0  16340068   0% /proc/kcore
Cdevtmpfs              16340068         0  16340068   0% /proc/keys
Ldevtmpfs              16340068         0  16340068   0% /proc/latency_stats
Idevtmpfs              16340068         0  16340068   0% /proc/timer_list
Jdevtmpfs              16340068         0  16340068   0% /proc/sched_debug
1..9

The dsf in the t is shorthand for "this will never pass, but I want you to dump the entire resulting JSON for me to debug".

@mheon
Copy link
Member Author

mheon commented Feb 25, 2021

@edsantiago Ah, I'm missing a : - what I get for doing patches at midnight.

@mheon
Copy link
Member Author

mheon commented Feb 25, 2021

Re-pushed

@mheon mheon force-pushed the fix_9511 branch 2 times, most recently from be08f39 to 0c62069 Compare February 25, 2021 17:25
@mheon
Copy link
Member Author

mheon commented Feb 25, 2021

And added your test, thanks

Create is not formatted as `key=value` but rather `key:value`
(technically `path:option1,option2`). As such we can't use the
stringMapToArray function, and instead need to generate it
manually.

Fixes containers#9511

Signed-off-by: Matthew Heon <[email protected]>

# Test various HostConfig options
tmpfs_name="/mytmpfs"
t POST containers/create?name=hostconfig_test '"Image":"'$IMAGE'","Cmd":["df"],"HostConfig":{"TmpFs":{"'$tmpfs_name'":"rw"}}' 201 \
Copy link
Member

Choose a reason for hiding this comment

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

If for some reason you need to resubmit, I think a better test would be ["df","-P","'$tmpfs_name'"] and include a status-code check in the wait. The reason is that line 20 bothers me a lot: it will pass on /mytmpfs but also on /mytmpfsrw or /mytmpfs=rw because I have no sane way to test for EOL. Explicitly dfing the desired filesystem is a safer test. But this is absolutely not worth re-pushing for.

t POST containers/${cid}/wait '' 200
t GET containers/${cid}/logs?stdout=true 200

like "$(<$WORKDIR/curl.result.out)" ".* ${tmpfs_name}" \
Copy link
Member

Choose a reason for hiding this comment

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

Likewise, if you have to re-push for other reasons, I think a comment here would be helpful:

# /logs returns application/octet-stream, which our test helper saves in an outfile
# rather than returning in $output. That's why we can't test this directly in the /logs
# test above; instead, we rely on knowing the path to the stored results.

Again, not worth a re-push for this alone.

@TomSweeneyRedHat
Copy link
Member

LGTM
I didn't see anything other than @edsantiago 's nits that needed attention.

@baude
Copy link
Member

baude commented Mar 2, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2021
@openshift-merge-robot openshift-merge-robot merged commit b9181cf into containers:master Mar 2, 2021
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong tmpfs created for container
7 participants