check: suppress certain errors when pull_options might cause them #1596
Conversation
nalind
force-pushed
the
check-ostree
branch
2 times, most recently
from
7369c29
to
18e1e87
Compare
check.go
Outdated
| for _, o := range s.graphOptions { | ||
| if strings.Contains(o, "ignore_chown_errors") { | ||
| ignoreChownErrors = true | ||
| } | ||
| } | ||
| for o := range s.pullOptions { | ||
| if strings.Contains(o, "ostree_repos") || strings.Contains(o, "use_hard_links") { |
There was a problem hiding this comment.
The other caller consumes use_hard_links via parseBooleanPullOption (apparently allowing "false"), not just a presence check.
(It seems tempting to suggest that the individual store.pullOptions values should somehow only be accessed via dedicated accessors, to make the primary consumers and check.go clearly connected in the call graph… except that that would require a c/storage/internal/$something and probably isn’t worth it for the three options that currently exist.)
|
@giuseppe PTAL |
nalind
force-pushed
the
check-ostree
branch
3 times, most recently
from
6de1a4e
to
18ca637
Compare
|
@mtrmac PTAL |
nalind
force-pushed
the
check-ostree
branch
2 times, most recently
from
cd870f8
to
784fc34
Compare
check.go
Outdated
| ignoreChownErrors = true | ||
| ignore.ownership = true | ||
| } | ||
| if strings.Contains(o, "force_mask") { |
There was a problem hiding this comment.
(Overlay also sets force_mask if isNetworkFileSystem. At some point it may be easiest to let the driver determine the right checkIgnore value.
Automatically-setting force_mask if isNetworkFileSystem only happens with a warning, so not handling that here seems fair enough.)
nalind
force-pushed
the
check-ostree
branch
2 times, most recently
from
cde19d7
to
dce15a7
Compare
|
I’m afraid I somehow lost this comment yesterday:
*shrug* this PR is a good improvement already. |
Files hard linked in from an OSTree repository won't tend to have the right timestamps on them, so we have to accept that they'll not match what we have on record when we're using one to speed up pulling images. If we're ignoring chown errors when populating layers, then there's no point in expecting the ownerships of the contents of on-disk layers to match expectations. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
tar.FileInfoHeader() doesn't produce TypeLink entries (it's not going to walk the filesystem to find other instances of the same inode), and TypeRegA has been deprecated for some time, so it's a waste of time to check for them. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Correctly handle path names that start with "." or "./", which are used for changes to the root directory's permissions. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
When we use diff headers to build a structure that represents what we expect to find when we look at a layer, make sure we process hard links last, so that we can refer to metadata about copies of the linked-to file from the layer whose diffs we're processing, instead of potentially metadata about versions of those files which came from earlier layers. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Handle old-fashioned ID mappings when looking at layers. Nowadays, we'll use an idmapped mount if we can, but we shouldn't blow up if we had to chown a layer because we couldn't use an idmapped mount. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
When we're comparing a layer's regenerated diff against the one that was used to initialize it, don't stop at just complaining about the digest if the length is also coming up wrong. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
Tweaked that just a bit. |
|
LGTM and happy green test buttons. Would like a head nod from @giuseppe, @vrothberg or @rhatdan too |
Files hard linked in from an OSTree repository won't tend to have the right ownership, permissions, or timestamps on them, so we have to accept that they'll frequently not match what we have on record when we're using one to speed up pulling images.