More docker compat API fixes

[mlegenovic]

Fixes wrong VirtualSize, ParentId, Architecture, Author, Os and OsVersion value

Signed-off-by: Milivoje Legenovic [email protected]

[mheon]

The doubled call to i.Size() should be avoided - it's a very expensive operation.

[mlegenovic]

@mheon What would you propose:

• to make a new field which is lazy initialized, and if called a second time no calculation is needed (I suppose image size cannot change)
• to remove VirtualSize function, and to always use Size because both attributes always have the same values. At the moment Size == VirtualSize is more or less everywhere I saw in code, but I do not know if this is just temporary solution, or if this will change. I did VirtualSize function just to have an abstraction for calculation logic, but if both values are always the same, it is not needed.

Or do you have any other ideas?

[mheon]

For now I would say remove VirtualSize and reuse size, with a comment about how this is not strictly correct and should be investigated later.

…

On Fri, Nov 27, 2020, 03:15 Milivoje Legenovic ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In libpod/image/image.go <#8494 (comment)>: > @@ -923,6 +923,11 @@ func (i *Image) Size(ctx context.Context) (*uint64, error) { return nil, errors.Wrap(err, "unable to determine size") } +//VirtualSize returns the virtual size of the image +func (i *Image) VirtualSize(ctx context.Context) (*uint64, error) { + return i.Size(ctx) @mheon <https://github.com/mheon> What would you propose: - to make a new field which is lazy initialized, and if called a second time no calculation is needed (I suppose image size cannot change) - to remove VirtualSize function, and to always use Size because both attributes always have the same values. At the moment Size == VirtualSize is more or less everywhere I saw in code, but I do not know if this is just temporary solution, or if this will change. I did VirtualSize function just to have an abstraction for calculation, but if both values are always the same, it is not needed. Or do you have any other ideas? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8494 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB3AOCFCIRS6Z6ZKBQLK5STSR5NZ3ANCNFSM4UEFH54A> .

[mheon]

Test failures are consistent and look legitimate, but the error doesn't seem to make much sense - missing top layers for images we built ourselves make no sense?

@vrothberg Mind taking a look here?

[mlegenovic]

@mheon I assume the problem is in GetParent function. Reason for the assumption is this comment:

// FIXME: GetParent() panics
// parent, err := l.GetParent(context.TODO())

which I have deleted because of small refactoring. GetParent is now called in my change-request every time when ParentId is determined. That is one of the fixed fields.

GetParent is also called in Remove function, but maybe this function is not called in a test, and therefore problem never occurs.

I will try to find out something with the debugger (just had no time to do it before).

[mlegenovic]

It is very strange. If I run podman command that is executed in test-code by hand, everything works. Then I saw test-code calls podman with vfs as storage backend, and I thought maybe that is the problem... but it is not. If I run test with go test <all_args> it fails.

What I noticed is, Runtime.layerTree() function has this tree.nodes map. If you run podman command by hand, content of this map is different (it has items) then if it is called by test-code (no items in map), but both are inspecting the same image.

[mheon]

This sounds like it could be because of the added store for image caching. We use an extra store for cached images (as described in https://www.redhat.com/sysadmin/image-stores-podman) in CI to speed things up (with an option to manually disable said caching for tests it breaks). We may need to disable the cache for these tests.

[mlegenovic]

Can I disable this cache somehow locally? I run test with
$ go test -v test/e2e/libpod_suite_test.go test/e2e/common_test.go test/e2e/config.go test/e2e/config_amd64.go test/e2e/build_test.go
Is there an option to disable it?

[mheon]

@baude is the expert on this, so he might be able to help

[mlegenovic]

@mheon I have found the reason for the failed test. "podman build and check identity" test in test/e2e/build_test.go calls podman once with podmanTest.Podman() and a second time with podmanTest.PodmanNoCache(). This combination does not work. It works if you run both commands with podmanTest.Podman() or with podmanTest.PodmanNoCache(), otherwise layers array is empty, and test fails as consequence.

[rhatdan]

@mlegenovic Could you rebase please.

[mheon]

The one failure looks like a flake
/approve

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

• OWNERS [mheon]

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

[jwhonce]

LGTM

[mheon]

/lgtm