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

Allow passing labels to volumes #140

Closed

Conversation

msisj
Copy link
Contributor

@msisj msisj commented Oct 21, 2021

Signed-off-by: Jankowiak Szymon-PRFJ46 [email protected]

Before changes :

>>> import podman
>>> client = podman.PodmanClient(base_url='unix:///run/podman/podman.sock', timeout=300)
>>> v = client.volumes.create('test', labels={'test_label': 'test_value'})
>>> v.attrs.get('Labels')
{}
>>>

[root@host~]# podman inspect test
[
    {
        "Name": "test",
        "Driver": "local",
        "Mountpoint": "/var/lib/containers/storage/volumes/test/_data",
        "CreatedAt": "2021-10-21T10:18:06.845889973Z",
        "Labels": {},
        "Scope": "local",
        "Options": {}
    }
]


Ran 277 tests in 34.861s

FAILED (SKIP=3, errors=5, failures=3)

After changes :

>>> import podman
>>> client = podman.PodmanClient(base_url='unix:///run/podman/podman.sock', timeout=300)
>>> v = client.volumes.create('test', labels={'test_label': 'test_value'})
>>> v.attrs.get('Labels')
{'test_label': 'test_value'}
>>>

[root@host~]# podman inspect test
[
    {
        "Name": "test",
        "Driver": "local",
        "Mountpoint": "/var/lib/containers/storage/volumes/test/_data",
        "CreatedAt": "2021-10-21T09:27:52.934957768Z",
        "Labels": {
            "test_label": "test_value"
        },
        "Scope": "local",
        "Options": {}
    }
]


# tox -e coverage

...

Ran 277 tests in 35.419s

FAILED (SKIP=3, errors=5, failures=3)

Tested on:

[root@host~]# uname -a
Linux host.example.com 4.18.0-305.17.1.el8_4.x86_64 #1 SMP Mon Aug 30 07:26:31 EDT 2021 x86_64 x86_64 x86_64 GNU/Linux
[root@host~]# podman --version
podman version 3.2.3

I had to modify the test case since it failed on assertion (Labels vs Label), but I'm wondering how this test even passed when it allegedly compared labels from create volume (https://github.com/containers/podman-py/blob/main/podman/tests/unit/test_volumesmanager.py#L77-L82).

Signed-off-by: Jankowiak Szymon-PRFJ46 <[email protected]>
@msisj
Copy link
Contributor Author

msisj commented Oct 21, 2021

/assign @mwhahaha

@msisj
Copy link
Contributor Author

msisj commented Oct 22, 2021

/assign @TomSweeneyRedHat

Copy link
Contributor

@mwhahaha mwhahaha left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 22, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msisj, mwhahaha

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

@rhatdan
Copy link
Member

rhatdan commented Oct 25, 2021

@msisj can you add a test for this?

@TomSweeneyRedHat
Copy link
Member

My brain isn't parsing why your change changed the behavior here. Definitely want @jwhonce to take a looksee here.

@mwhahaha
Copy link
Contributor

Input attribute for the volume create call is "Label" not "Labels" per the docs https://docs.podman.io/en/latest/_static/api.html#operation/VolumeCreateLibpod

@TomSweeneyRedHat
Copy link
Member

@msisj yeah, but I'm looking through the code, and I'm only seeing "Labels" on both podman-py and in podman itself. I'm not finding "Label", but I'm probably looking in the wrong spot and @jwhonce will straighten me out (again).

@mwhahaha
Copy link
Contributor

It looks to be a discrepancy between the compat and libpod volumes api

libpod has input.Label
https://github.com/containers/podman/blob/main/pkg/api/handlers/libpod/volumes.go#L50-L52

compat as input.Labels
https://github.com/containers/podman/blob/main/pkg/api/handlers/compat/volumes.go#L139-L141

@rhatdan
Copy link
Member

rhatdan commented Oct 26, 2021

We should fix this in Podman to support Labels and Label in libpod mode and only document Labels.

@jwhonce
Copy link
Member

jwhonce commented Oct 27, 2021

@rhatdan Sounds good.

/lgtm

@jwhonce
Copy link
Member

jwhonce commented Oct 29, 2021

As of Podman 4.0 and with containers/podman#12133 podman now takes either key. We can merge this now and then revert in future.

@mwhahaha @rhatdan @msisj Sounds good?

@mwhahaha
Copy link
Contributor

wfm

@TomSweeneyRedHat
Copy link
Member

Since containers/podman#12133 has merged, I think we can close this PR and thank @msisj for their work here.

@rhatdan
Copy link
Member

rhatdan commented Oct 29, 2021

Yes I would rather get another Podman release out.

@rhatdan rhatdan closed this Oct 29, 2021
@mwhahaha
Copy link
Contributor

I would highlight that unless you plan on backporting that fix in podman, this means podman-py can't be used with podman 3.x which is less than ideal.

@rhatdan
Copy link
Member

rhatdan commented Oct 30, 2021

Yes I am going to ask @mheon to back port and create a new podman 3.4.* release.
Podman 4 is not coming to end of year or early next year, so there will me multiple bug fix releases of podman 3.4.*

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

Successfully merging this pull request may close these issues.

5 participants