containers / docker-lvm-plugin

Docker volume plugin for LVM volumes
GNU Lesser General Public License v3.0
155 stars 64 forks source link

CreatedAt not being set correctly #58

Closed mortya closed 5 years ago

mortya commented 6 years ago

I have docker local volumes and lvm volumes. The local volumes set CreatedAt correctly. The lvm volumes don't.

This problem is referenced in #55 . But the focus there seems to be shutting up the warning about this rather than getting a valid value. If I'm misunderstanding and this is a duplicate, my apologies.

The reason I care is that I'm trying to set up pruning for lvm volumes. The default volume prune seems to only work for local volumes. One approach would be to delete all volumes, and rely on the fact that used volumes can't be deleted. But that opens up to a race condition, where a volume is created, the pruning code runs, and then the (no longer existing) volume is referenced. So the new approach is to list all volumes, check for CreatedAt being at least N minutes old, and then try deleting. But that only works if CreatedAt is valid. Which it does not appear to be.

# docker volume inspect local_volume
[
    {
        "CreatedAt": "2018-10-09T11:58:05-04:00",
        "Driver": "local",
        "Labels": {},
        "Mountpoint": "/srv/var/lib/docker/262144.262144/volumes/local_volume/_data",
        "Name": "local_volume",
        "Options": {},
        "Scope": "local"
    }
]
# docker volume inspect docker_lvm_test1
[
    {
        "CreatedAt": "0001-01-01T00:00:00Z",
        "Driver": "lvm",
        "Labels": {},
        "Mountpoint": "/var/lib/docker-lvm-plugin/docker_lvm_test1",
        "Name": "docker_lvm_test1",
        "Options": {
            "size": "0.3G"
        },
        "Scope": "local"
    }
]
#
ashald commented 5 years ago

@shishir-a412ed how would you prefer it implemented? Extract information from lvm dynamically or store it in json along with other metadata? I'd submit a PR as we need this implemented.

ashald commented 5 years ago

@shishir-a412ed given the codebase I assumed that extracting creation date from lvdisplay is preferred approach (as it's backward compatible to to no modifications to persisted metadata file) and submitted a pull request.

Please let me know what do you think of it. I did tested it locally:

$ docker volume create -d lvm --opt size=0.2G --name foobar
$ lvdisplay vg1/foobar
  --- Logical volume ---
  LV Path                /dev/vg1/foobar
  LV Name                foobar
  VG Name                vg1
  LV UUID                HjGYNd-hfyz-fgAi-sMG6-fefY-80Yp-Pma39Y
  LV Write Access        read/write
  LV Creation host, time localhost, 2018-11-18 14:39:40 +1300
  LV Status              available
  # open                 0
  LV Size                208.00 MiB
  Current LE             52
  Segments               1
  Allocation             inherit
  Read ahead sectors     auto
  - currently set to     8192
  Block device           253:7
$ docker volume inspect foobar
[
    {
        "CreatedAt": "2018-11-18T01:39:40Z",
        "Driver": "lvm",
        "Labels": null,
        "Mountpoint": "/var/lib/docker-lvm-plugin/foobar",
        "Name": "foobar",
        "Options": null,
        "Scope": "local"
    }
]
shishir-a412ed commented 5 years ago

@Ashald Thanks for opening up the PR ! @mortya is right, Fix for #55 (https://github.com/docker/go-plugins-helpers/pull/107) is only suppressing the warning, and not populating CreatedAt which he would need for his docker prune usecase.

I am a bit occupied right now, and will try to review it as soon as possible.

Extract information from lvm dynamically or store it in json along with other metadata?

Extracting the date/time from lvdisplay output should be the correct approach, which is what I believe you are doing.

shishir-a412ed commented 5 years ago

Fixed via #67 and #68 Closing this now.