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

Ensure that the DF endpoint updated volume refcount #15753

Merged
merged 1 commit into from
Sep 13, 2022

Conversation

mheon
Copy link
Member

@mheon mheon commented Sep 12, 2022

The field was already exposed already in the system df output so this just required a bit of plumbing and testing.

Fixes #15720

Fixed a bug where the Compat Disk Usage endpoint did not properly report the reference count of volumes.

@openshift-ci openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 12, 2022
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
@benoitf FYI

@@ -25,6 +25,26 @@ t GET system/df 200 '.Volumes[0].Name=foo1'

t GET libpod/system/df 200 '.Volumes[0].VolumeName=foo1'

# Verify that no containers reference the volume
t GET system/df 200 '.Volumes[0].RefCount=0'
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this should be ....UsageData.Refcount

@@ -25,6 +25,26 @@ t GET system/df 200 '.Volumes[0].Name=foo1'

t GET libpod/system/df 200 '.Volumes[0].VolumeName=foo1'

# Verify that no containers reference the volume
t GET system/df 200 '.Volumes[0].RefCount=0'
t GET libpod/system/df 200 '.Volumes[0].RefCount=0'
Copy link
Member

Choose a reason for hiding this comment

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

Looks unimplemented: I see no field with anything looking like RefCount.

Handy tip: this is how I debug APIv2 tests:

t GET ... 200 sdf   <------ the "sdf" is the important thing

Since sdf does not match, the test just dumps the entire return value, in this case:

not ok 21 [45-system] GET libpod/system/df : output                                                                                                             
#  expected: dsf                                                                                                                                                
#    actual: {"Images":[],"Containers":[],"Volumes":[{"VolumeName":"foo1","Links":0,"Size":0,"ReclaimableSize":0}]}                                             
not ok 22 [45-system] GET libpod/system/df : .Volumes[0].RefCount                                                                                               
#  expected: 0                                                                                                                                                  
#    actual: null              

And OBTW the POST containers/create is failing too

@edsantiago
Copy link
Member

Container Create is still broken. From looking at other tests, I figured out that you need brackets. Then I figured out that you need a podman pull (which is stupid, the image should be preloaded, but I've kind of lost track of this test suite and bad things have happened).

diff --git a/test/apiv2/45-system.at b/test/apiv2/45-system.at
index 061fe5f98..096df5516 100644
--- a/test/apiv2/45-system.at
+++ b/test/apiv2/45-system.at
@@ -29,7 +29,8 @@ t GET libpod/system/df 200 '.Volumes[0].VolumeName=foo1'                                                                                     
 t GET system/df 200 '.Volumes[0].UsageData.RefCount=0'
 
 # Make a container using the volume
-t POST containers/create Image=$IMAGE Volumes='{"/test":{}}' HostConfig='{"Binds":"foo1:/test"}' 201 \
+podman pull $IMAGE &>/dev/null
+t POST containers/create Image=$IMAGE Volumes='{"/test":{}}' HostConfig='{"Binds":["foo1:/test"]}' 201 \
   .Id~[0-9a-f]\\{64\\}                                                            ^------------^
 cid=$(jq -r '.Id' <<<"$output")

And, something is still broken, because even after the container create the following test fails:

not ok 23 [45-system] GET system/df : .Volumes[0].UsageData.RefCount                                                                                            
#  expected: 1                                                                                                                                                  
#    actual: 0    

@mheon
Copy link
Member Author

mheon commented Sep 12, 2022

Volumes are apparently only considered in use if the container using them is running.

That doesn't seem right...

@mheon
Copy link
Member Author

mheon commented Sep 12, 2022

Apparently was done deliberately. Let me check and see if Docker does it.

@mheon
Copy link
Member Author

mheon commented Sep 12, 2022

Nope. Docker considers a volume active if it is used by any container, even one not running.

@mheon
Copy link
Member Author

mheon commented Sep 12, 2022

Repushed with test fix and a change to podman system df to better match Docker. We'll see if that breaks any tests.

@mheon mheon force-pushed the fix_15720 branch 2 times, most recently from 975d65e to 90a6e0c Compare September 12, 2022 17:54
@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2022

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@edsantiago
Copy link
Member

Should be an easy fix:

diff --git a/test/system/320-system-df.bats b/test/system/320-system-df.bats
index 217357b37..35e121c62 100644
--- a/test/system/320-system-df.bats
+++ b/test/system/320-system-df.bats
@@ -27,7 +27,7 @@ function teardown() {                                                                                                                        
     run_podman system df --format '{{ .Type }}:{{ .Total }}:{{ .Active }}'
     is "${lines[0]}" "Images:1:1"        "system df : Images line"
     is "${lines[1]}" "Containers:2:1"    "system df : Containers line"
-    is "${lines[2]}" "Local Volumes:2:1" "system df : Volumes line"
+    is "${lines[2]}" "Local Volumes:2:2" "system df : Volumes line"
 
     # Try -v. (Grrr. No way to specify individual formats)
     #

Looking back at the history of this test, I wrote it the way I did because that's the way the code worked. I made no attempt to understand whether or not it was correct.

The field was already exposed already in the `system df` output
so this just required a bit of plumbing and testing.

As part of this, fix `podman systemd df` volume in-use logic.
Previously, volumes were only considered to be in use if the
container using them was running. This does not match Docker's
behavior, where a volume is considered in use as long as a
container exists that uses the volume, even if said container is
not running.

Fixes containers#15720

Signed-off-by: Matthew Heon <[email protected]>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@edsantiago
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2022
@edsantiago
Copy link
Member

Flakes are all registry-related (on the registry end). Restarted once, all failed. Restarted twice, going to bed, SEP.

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
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mheon, vrothberg

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

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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

REST API /system/df UsageData.RefCount on volumes is always 1
5 participants