-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Tree implementation for podman images #1642
Tree implementation for podman images #1642
Conversation
This is much shorter version of initial implementation I discussed in #415 (comment). After revisiting discussion points from @mtrmac, Instead of parsing information from I guess this PR is not yet complete in the sense Printing of tree should be done in sample output
I would like to have a review and discussion about if we agree with
//cc @vrothberg @mtrmac |
@fatherlinux FYI |
libpod/image/image.go
Outdated
} | ||
} | ||
|
||
func humanSize(raw uint64) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use
"github.com/docker/go-units"
And
size = units.HumanSizeWithPrecision(0, 0)
Will need man pages and command completion. Should this be a primary command or a subcommand? podman tree |
very nice, as @rhatdan said, I think this should be a subcommand of "image" |
libpod/image/image.go
Outdated
return nil, err | ||
} | ||
|
||
func printSubTree(root *Image, rootSize uint64, prefix string, treeDepth int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should have an error return value, so that I can be properly propagated.
libpod/image/image.go
Outdated
@@ -654,6 +654,7 @@ type History struct { | |||
CreatedBy string `json:"createdBy"` | |||
Size int64 `json:"size"` | |||
Comment string `json:"comment"` | |||
Tags []string `json:"tags"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this new field needed or an artifact from an earlier version?
I agree, this should go into |
cmd/podman/tree.go
Outdated
} | ||
|
||
var ( | ||
treeDescription = "Displays the dependent layers of an image. The information is printed as tree format" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggest replacing "as tree format" with "in tree format"
libpod/image/image.go
Outdated
} | ||
|
||
//Print all subtree(peers) to the depth of input image | ||
printSubTree(rootImage, 0, "└─ ", treeDepth) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to double check, are these standard ascii characters in the 3rd argument. Will there be any trouble displaying this on a Mac or other non-linux device?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified this ASCII character on both Mac & Windows, it works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ASCII is 0–127, this is not a part of it. But Golang source code is specified to be UTF-8, so it is unambiguous, and Unicode support has been good enough for about a decade.
Looks good overall to me, thanks for working on this @kunalkushwaha ! |
060ee90
to
6bb59b5
Compare
@rhatdan need approval |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most importantly, please see the big comment below; unless I’m missing something this does not quite do what we want, considering how widespread unsquashed images are.
I guess this PR is not yet complete in the sense Printing of tree should be done in
cmd/tree.go
instead oflibpod/image/image.go
.
Yes; please separate the computation of the tree from its presentation. Some users might well want to consume the same data in an easy-to-parse JSON, for example.
libpod/image/image.go
Outdated
|
||
for parent != nil { | ||
rootImage = parent | ||
parent, err = parent.GetParent() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems problematic; the more complex code, or maybe (more likely?) an entirely different data source might still be needed.
Most importantly, AFAICS there may not be image objects available at all for the shared parents: consider a layer tree like this:
fedora
- unnamed layer
- unnamed layer
base-environment
app-0
- unnamed layer
- unnamed layer [A]
app-1
- unnamed layer [A]
- unnamed layer
- unnamed layer
app-2
- unnamed layer
- unnamed layer
and a local environment which has done for x in 0 1 2; do podman pull app-$x; done
, with no other images pulled or known, so that the fedora
and base-environment
images have never been seen locally, nor have any possible in-between images created by buildah
.
AFAICS (but I have not tested this, to be fair), in such case the tree of layers in the containers/storage.LayerStore
will mirror the structure above exactly, but only the app-$x
images will be known to containers/storage.ImageStore
.
So, app-1
.GetParent()
will return nil
because the layer [A]
has no corresponding image; similarly app-0
.GetParent()
will return nil
because the base-environment
image is not available locally.
In both cases, the tree will only show a single element app-$0
. It seems to me (but, strictly speaking, I don’t know, because the specification is not yet precise enough) that the desired input is the full layer tree exactly as structured, with fedora
and base-environment
reported as “unnamed layer” [but after podman pull base-environment
, it could report the right image name].
Second, as discussed in
#415 (comment) , the incomplete data may be ambiguous: there might be several images with the same top layer but different configs, and GetParent
chooses one in random order (AFAICS the first one pulled/created in the local storage, but that’s not an API promise).
In this case, consider an image base-modified
, derived from base-environment
with an ENV a=a
. Depending on the implementation, this might cause an empty extra layer to be created, but there’s no real need for it: it’s just a different image with a different config but exactly the same layers. So, the base-environment
layer actually can have both base-environment
and base-modified
images as possible parents, and without knowing the true parent links somehow (AFAICS buildah
/podman
does not record that at all right now), there’s no way to tell.
If both are available locally, we can’t tell which one to output; if only the wrong one is available locally, we will probably report that incorrect base. (And, as discussed above, neither may be available locally, and we can’t tell between an unavailable base image and a non-image intermediate layer.)
AFAICS this is fundamentally unsolvable in general, but it is a design concern — do we want to report a single, possibly incorrect, image, or all of the candidates, of which all but one are certainly incorrect, and the last one may or may not be correct?)
Eventually, we have been talking about adding true/reliable “parent image” information, which would make this unambiguous — only if the image is new enough to have been built with such hypothetical parent information; we would still have to guess for any image that exists today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, app-1.GetParent() will return nil because the layer [A] has no corresponding image; similarly app-0.GetParent() will return nil because the base-environment image is not available locally.
No, In this case a layerID (maybe without RepoTags) will be shown if image is not flatten/squashed by any tool.
- If image is flatten/squashed, it has become independent, so no there will be no parent layer.
While pulling children layer, always dependent parent layers are downloaded and that’s how the image rootfs should be build.
For build informationpodman history
should be used
with fedora and base-environment reported as “unnamed layer” [but after podman pull base-environment, it could report the right image name].
AFAIU tree
or any other script that finds dependencies locally, resolves to local registry. i.e. information available locally instead of fetching to remote registry/store.
If it involves fetching information from remote registry, than we need to decide which all remote registries need to be looked at also, as podman supports multiple registries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, In this case a layerID (maybe without RepoTags) will be shown if image is not flatten/squashed by any tool.
How does that work? https://github.com/containers/libpod/blob/604728d65dd0110b28c79acafc5b214886b3b484/libpod/image/image.go#L984-L999 clearly can return nil
if there is no image corresponding to a layer, and reading c/storage/layerStore.Put
, creating a layer does not require a corresponding image to exist.
If it involves fetching information from remote registry, than we need to decide which all remote registries need to be looked at also, as podman supports multiple registries.
That’s the least of our problems; there is no way to look up a remote image config by the layer set (and again, this can be return many different results with no way to decide), and no way to lookup a remote manifest by the config.
/ok-to-test |
@kunalkushwaha Are you still working on this? |
@rhatdan This PR is stuck for what information is expected in this tree command. If the information which all previous layers of given image is fine, I guess this PR is output wise working. Though the performance improvement is required as suggested by @mtrmac. Also, I am not clear on the comments, the parent layer can be retured as nil, if there is no coresponding image present. I think to prepare a rootfs from Image, we need all layers to be present in local registry and if layers are present in local registry, Parent layer should be reachable. |
Are you saying that my analysis in
(I don’t think this should block on performance; it’s notable but it’s clear that that can be fixed later if it turns out to be necessary.)
You’re right that a parent layer always exists locally (for derived images); but not all layers correspond to an image (= layer sequence + config). See the comment linked above. |
Updated this PR with following.
sample output
@mtrmac @vrothberg PTAL |
This looks really good! Thanks a lot, @kunalkushwaha. I spotted just minor formatting issues that could addressed with the following code change (the comment must be updated as the intend is always set to diff --git a/cmd/podman/tree.go b/cmd/podman/tree.go
index ca23d8756dda..d4e9e85b9197 100644
--- a/cmd/podman/tree.go
+++ b/cmd/podman/tree.go
@@ -155,7 +155,7 @@ func printImageChildren(layerMap map[string]*image.LayerInfo, layerID string, pr
// If its not last node, it has to be treated as middleItem i.e. ├──
// add continueItem i.e. '|' for next itteration prefix
prefix = prefix + continueItem
- } else if len(ll.ChildID) > 1 || len(ll.ChildID) == 0 {
+ } else if len(ll.ChildID) == 0 {
// The above condition ensure, alignment happens for node, which has more then 1 childern.
// If node is last in printing hierarchy, it should not be printed as middleItem i.e. ├──
intend = lastItem
@@ -164,9 +164,9 @@ func printImageChildren(layerMap map[string]*image.LayerInfo, layerID string, pr
var tags string
if len(ll.RepoTags) > 0 {
- tags = fmt.Sprintf("Top Layer of: %s", ll.RepoTags)
+ tags = fmt.Sprintf(" Top Layer of: %s", ll.RepoTags)
}
- fmt.Printf("%s ID: %s Size: %7v %s\n", intend, ll.ID[:12], units.HumanSizeWithPrecision(float64(ll.Size), 4), tags)
+ fmt.Printf("%sID: %s Size: %7v%s\n", intend, ll.ID[:12], units.HumanSizeWithPrecision(float64(ll.Size), 4), tags)
for count, childID := range ll.ChildID {
if err := printImageChildren(layerMap, childID, prefix, (count == len(ll.ChildID)-1)); err != nil {
return err The diff changes how a "fork/branch" looks like by left alinging the first child:
... and it removes some leading and trailing whitespace. Maybe I am overseeing a side-effect of the upper diff but I figured it's easier to illustrate what to change formatting wise. Other than that: LGTM |
diff --git a/cmd/podman/tree.go b/cmd/podman/tree.go
index ca23d8756dda..d4e9e85b9197 100644
--- a/cmd/podman/tree.go
+++ b/cmd/podman/tree.go
@@ -155,7 +155,7 @@ func printImageChildren(layerMap map[string]*image.LayerInfo, layerID string, pr
// If its not last node, it has to be treated as middleItem i.e. ├──
// add continueItem i.e. '|' for next itteration prefix
prefix = prefix + continueItem
- } else if len(ll.ChildID) > 1 || len(ll.ChildID) == 0 {
+ } else if len(ll.ChildID) == 0 { @vrothberg Are you really sure of this change? This will loose child parent relationship structure. There are three images as follows
now if
With suggested change initial output would be as follows
after removing the
@mtrmac Please you also check this. |
b02aca2
to
57851ae
Compare
/retest |
The CI is failing due to
|
/retest |
bot, retest this please |
@kunalkushwaha, it looks like a flake in the CI. If it doesn't get green, you may need to repush the change to kick off a fresh CI run. |
57851ae
to
0843bbb
Compare
seems there is issue with CI, failing randomly on tests. |
0843bbb
to
8c809b1
Compare
☔ The latest upstream changes (presumably #2527) made this pull request unmergeable. Please resolve the merge conflicts. |
4f28ac2
to
c290648
Compare
@vrothberg Please help for this CI error. 2019/03/13 02:32:21 Clone https://github.com/QiWang19/buildah to github.com/containers/buildah, revision 345ffc2b29b4255a83cfa763db88799d8ec9c569
2019/03/13 02:32:23 Finished clone github.com/containers/buildah
2019/03/13 02:32:24 Errors on clone:
github.com/containers/buildah: Err: exit status 128, out: fatal: reference is not a tree: 345ffc2b29b4255a83cfa763db88799d8ec9c569
make: *** [Makefile:343: vendor] Error 1
also @mtrmac PTAL at output as described in #1642 (comment) and suggestion by @vrothberg in #1642 (comment). If any changes are required, I can do it in parallel. |
@kunalkushwaha can you do a |
c290648
to
a4b3b9f
Compare
Signed-off-by: Kunal Kushwaha <[email protected]>
@mtrmac @rhatdan PTAL. Let's get this PR merged. @kunalkushwaha, sorry for the flakes. We're also suffering from the CI atm. |
/lgtm |
@kunalkushwaha Thank you very much for you hard work on this PR over the past several months. I think you've earned Podman's first Triple Gold Star for the effort. It will be a nice addition to the tool. |
Absolutely. Thanks a lot, @kunalkushwaha! |
Thank you @TomSweeneyRedHat :). |
Signed-off-by: Kunal Kushwaha [email protected]