The behaviour of KeepLatestNVersionImagesByProperty is unclear and possibly incorrect

[kenny-monster]

I want to useKeepLatestNVersionImagesByProperty as a safeguard to ensure that a few versions are always retained. I've been debugging the method and I'm still not sure what the intended behaviour is.

Assume the input is (with importance to the ordering):

result_artifact = [
    {"properties":{"docker.manifest":"0.1.100"},"path":"foobar/0.1.100"}, # refer to below as A
    {"properties":{"docker.manifest":"0.1.200"},"path":"foobar/0.1.200"}, # refer to below as B
    {"properties":{"docker.manifest":"0.1.99"},"path":"foobar/0.1.99"}    # refer to below as C
]

and that the rule has been called like so:

KeepLatestNVersionImagesByProperty(count=2, custom_regexp='(^ \d*\.\d*\.\d*.\d+$) ', number_of_digits_in_version=1)

The final result ends up being that nothing is deleted. I would have expected that A and B would have been filtered out and that C remains for potential deletion.

If I then alter this line (https://github.com/devopshq/artifactory-cleanup/blob/master/artifactory_cleanup/rules/docker.py#L183) from:

key = artifact["path"] + "/" + version_splitted[0]

to

key = artifact["path"].split("/")[0] + "/" + version_splitted[0]

then B and C are filtered out and A remains for potential deletion. This is because the three image versions are able to be grouped together. Previously, the version would be part of the grouping which seems incorrect to me. This still doesn't meet what I expect the result to be.

From here, it seems like the sorting isn't right (assuming what I said above is correct). https://github.com/devopshq/artifactory-cleanup/blob/master/artifactory_cleanup/rules/docker.py#L189

key=lambda x: [int(x) for x in x[0].split(".")]

[allburov]

@kenny-monster

it seems like the sorting isn't right

Yep, it looks so.

We appreciate providing the detail explanation and examples and digging into the problem, it's helpful for debbuging!

[afolgado]

Apart from that, I've found that the property which the version is looked in ("docker.manifest") is set only on the manifest.json files, not in the layer files, resulting in a key error:

  File "artifactory-cleanup/artifactory_cleanup/rules/docker.py", line 179, in _filter_result
    property = artifact["properties"][self.property]
KeyError: 'docker.manifest'

So the search should be restricted to the "manifest.json" file names. Then, after the artifacts to be deleted have been determined, their parent (the folder that contains the manifest.json file) should be deleted, not only the manifest.

[allburov]

@afolgado I think the rule KeepLatestNVersionImagesByProperty is supposed to run with DeleteDockerImagesOlderThan or DeleteDockerImagesOlderThanNDaysWithoutDownloads rules.

But yeah, we can filter out manifest.json in the rule too, like https://github.com/devopshq/artifactory-cleanup/blob/master/artifactory_cleanup/rules/docker.py#L103-L105
Feel free to open PR for it!

their parent (the folder that contains the manifest.json file) should be deleted, not only the manifest

Yep, DeleteDockerImagesOlderThan does it under the hood too
https://github.com/devopshq/artifactory-cleanup/blob/master/artifactory_cleanup/rules/docker.py#L114-L119

[afolgado]

Thanks for the clarification, @allburov . Yes, maybe KeepLatestNVersionImagesByProperty should add a filter to match manifest.json files and adjust the paths of the artifacts to their parents like the other rules do. And maybe the right approach is to do both things in the common RuleForDocker class.

By the way, I can't see why KeepLatestNVersionImagesByProperty doesn't inherit from RuleForDocker like the others. Is there a reason for this or is it a bug?

[allburov]

is there a reason for this or is it a bug?

I don't remember :(

[allburov]

@afolgado added the expected behavior for KeepLatestNVersionImagesByProperty to the new version (interpreted it from RuleForDocker)
#63

[allburov]

@kenny-monster in the new version (which is coming soon) the rule KeepLatestNVersionImagesByProperty is fixed!

For you case (save 2 builds in major releases) the config is:

#artifactory-cleanup.yaml

artifactory-cleanup:
  server: https://repo.example.com/artifactory
  user: $ARTIFACTORY_USERNAME
  password: $ARTIFACTORY_PASSWORD

  policies:
    - name: Remove docker images older than 7 days, but keep 2 major anyway
      rules:
        - rule: Repo
          name: "repo-name-here"
        - rule: DeleteDockerImagesOlderThan
          days: 7
        - rule: KeepLatestNVersionImagesByProperty
          count: 2
          number_of_digits_in_version: 1

[kenny-monster]

Hey @allburov, I've finally had a chance to try the KeepLatestNVersionImagesByProperty again. It seems that its working when only one image is involved. When I include multiple images, it only seems to apply the logic to one of the images:

My policy looks like this:

- name: my-policy
      rules:
        - rule: Repo
          name: "my-repo"
        - rule: IncludeDockerImages
          masks:
            - "my-image-1:*"
            - "my-image-2:*"
        - rule: DeleteDockerImagesNotUsed
          days: 60
        - rule: KeepLatestNVersionImagesByProperty
          count: 2
          number_of_digits_in_version: 1

Both images have not been used for more than 60 days. Running with either image alone results in the two latest image versions being filtered out as expected.