fix: Refactor `LayersMap` to correct old strange code behavior #2066

gabyx · 2022-04-28T07:28:05Z

Added a test.

Fixes #1420
Fixes #1944

Description

Flatten function is still wrong.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes unit tests
Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

The code flow looks good.
Unit tests and or integration tests added.

gabyx · 2022-05-10T07:20:52Z

My observation is the following:

I have made the following (maybe trivial) observations: LayeredMap.layers never contains any .wh.* whiteout files. This is strange because I assumed that, and the function getFlattenedPaths was probably containing old code which did nothing.

Instead I hardly guess

LayeredMap.layers contains the added files
LayeredMap.whiteouts contains the deleted files.

However LayeredMap.getFlattenedPaths needs to take these two maps into account, otherwise
we (main) we are passing only the accumulated added files to the util.WalkFS function which is totaly wrong.

gabyx · 2022-05-10T13:22:14Z

The other thing is, that this improvements do not yet resolve the symlink bug I see over multiple stages:

FROM base as A
...
RUN echo "DEBUGGINGCHECK:" && ls -al /usr/lib/x86_64-linux-gnu/libbsd.so.0

FROM A as End
RUN echo "DEBUGGINGCHECK:" && ls -al /usr/lib/x86_64-linux-gnu/libbsd.so.0 /// FAILING HERE

Seems that extracting the stage 0 is somehow flawed and does not produce the symlink.

- Added a test. - Cache current image, track deletes in `whiteouts` as well as normal adds in `layers`. - Fix ugly delete behavior of `layerHashCache`. Delete it when crerating a new snapshot. - Slight cleanup in `snapshot.go`. - Format ugly `WalkFS` function.

gabyx · 2022-05-16T19:02:35Z

@tejal29 Could you have a look at this changes that would be great! Especially the firsts commit.

@imjasonh and me are investigating these changes which fixes some ugly bugs with missing files in images when multiple stages are involved.
The current changes vastly reduce some strange old code in favor of some more clean behavior in LayeredMap

gabyx · 2022-05-17T22:06:33Z

@imjasonh: Hurai, finally CI runs through, with minor modifications. IMO only improvements to status quo. Some local test fixes have been applied too to make local testing run through smoothly.

- Correct some linting.

- Merge only last layer onto `currentImage`.

imjasonh

There's kind of a lot of changes in this PR, but I think I'm following them all, and at this point it's probably easier just to power through than to split it up. In the future though, smaller single-purpose PRs would be great so we can iteratively make small improvements.

pkg/snapshot/layered_map.go

gabyx · 2022-05-18T06:10:02Z

@imjasonh Totally agree. I still try to find the bug dockerfile to add as regression test

- Test `Dockerfile_test_issue_2066` fails on main@28432d3c

* fix: Regression test for #2066 - Test `Dockerfile_test_issue_2066` fails on main@28432d3c * np: Comment

- This missing files problem should be fixed by PR GoogleContainerTools#2066.

- This missing files problem should be fixed by PR #2066.

Highlights - Installed binaries are missing from image [#2049](#2049) - proc: detect kubernetes runtime by mounts [#2054](#2054) - Fixes #2046: make target stage lookup case insensitive [#2047](#2047) - fix: Refactor LayersMap to correct old strange code behavior [#2066](#2066) - Fix missing setuid flags on COPY --from=build operation [#2089](#2089) - Fixes #2046: make target stage lookup case insensitive [#2047](#2047) - Add GitLab CI credentials helper [#2040]((#2040)) - and a number of dependency bumps

Highlights - Installed binaries are missing from image #2049 - proc: detect kubernetes runtime by mounts #2054 - Fixes #2046: make target stage lookup case insensitive #2047 - Fix: Refactor LayersMap to correct old strange code behavior #2066 - Fix missing setuid flags on COPY --from=build operation #2089 - Fixes #2046: make target stage lookup case insensitive #2047 - Add GitLab CI credentials helper #2040 - And a number of dependency bumps

gabyx mentioned this pull request Apr 28, 2022

Missing symlink in the images built using Kaniko #1944

Closed

gabyx force-pushed the bugfix/correct-flatten-paths branch 2 times, most recently from 111279b to 9da0603 Compare May 10, 2022 09:38

gabyx mentioned this pull request May 10, 2022

Parallel build of multi-stage Docker images #1548

Open

gabyx force-pushed the bugfix/correct-flatten-paths branch from 72754eb to 2549ee3 Compare May 10, 2022 20:14

This was referenced May 10, 2022

Bump go-containerregistry dependency #2076

Merged

Image built by kaniko might have whiteout files that *seems* unnecessary #2050

Closed

gabyx force-pushed the bugfix/correct-flatten-paths branch 3 times, most recently from 9676a69 to 78eee0c Compare May 12, 2022 21:19

gabyx force-pushed the bugfix/correct-flatten-paths branch from 78eee0c to e0b84c5 Compare May 12, 2022 21:21

gabyx added 2 commits May 12, 2022 23:35

fix: Add symbolic link changes to Hasher and CacheHasher

f58df66

fix: Better log messages

d2c02eb

gabyx force-pushed the bugfix/correct-flatten-paths branch from acc64db to b769a7e Compare May 12, 2022 21:35

gabyx force-pushed the bugfix/correct-flatten-paths branch 4 times, most recently from 1dbdede to e0d27aa Compare May 17, 2022 20:39

gabyx changed the title ~~fix: Correct getFlattenedPaths function in LayersMap~~ fix: Refactor LayersMap to correct old strange code behavior May 17, 2022

gabyx added 3 commits May 18, 2022 00:28

fix(ci): Integration tests

3cba8d0

fix(ci): Add --no-cache to docker builds

ff0fc51

fix(ci): Pass credentials for error integration test

4dcbda7

gabyx force-pushed the bugfix/correct-flatten-paths branch from 65d4b92 to 7083da8 Compare May 17, 2022 22:29

gabyx added 2 commits May 18, 2022 00:30

np: Missing .gitignore in hack

d0cbec8

np: Capitalize every log message

62747f6

- Correct some linting.

gabyx force-pushed the bugfix/correct-flatten-paths branch from 7083da8 to 62747f6 Compare May 17, 2022 22:30

gabyx added 2 commits May 18, 2022 01:02

fix: Key function

fc6a54c

- Merge only last layer onto `currentImage`.

fix: Remove old obsolete cacheHasher

d37d622

gabyx force-pushed the bugfix/correct-flatten-paths branch from 11bab2f to d37d622 Compare May 17, 2022 23:21

imjasonh approved these changes May 17, 2022

View reviewed changes

pkg/snapshot/layered_map.go Show resolved Hide resolved

imjasonh merged commit 323e616 into GoogleContainerTools:main May 18, 2022

gabyx deleted the bugfix/correct-flatten-paths branch May 18, 2022 06:09

gabyx added a commit to gabyx/kaniko that referenced this pull request May 18, 2022

fix: Regression test for GoogleContainerTools#2066

4455329

- Test `Dockerfile_test_issue_2066` fails on main@28432d3c

This was referenced May 18, 2022

fix: Regression test for #2066 #2096

Merged

COPY to symlink destination behaviour differs from docker build #1658

Closed

imjasonh pushed a commit that referenced this pull request May 18, 2022

fix: Regression test for #2066 (#2096)

2789b97

* fix: Regression test for #2066 - Test `Dockerfile_test_issue_2066` fails on main@28432d3c * np: Comment

gabyx added a commit to gabyx/kaniko that referenced this pull request May 31, 2022

fix: Add test for issue GoogleContainerTools#2049

8a0e8f9

- This missing files problem should be fixed by PR GoogleContainerTools#2066.

gabyx mentioned this pull request May 31, 2022

fix: Add test for issue #2049 #2114

Merged

4 tasks

imjasonh pushed a commit that referenced this pull request Jun 1, 2022

fix: Add test for issue #2049 (#2114)

c13f9b6

- This missing files problem should be fixed by PR #2066.

imjasonh mentioned this pull request Jun 1, 2022

Release v1.9.0 #2115

Closed

chuangw6 mentioned this pull request Jun 1, 2022

Release v1.9.0 #2117

Merged

4 tasks

tgaddair mentioned this pull request Aug 5, 2022

Image pull error: not a directory: unknown containerd/containerd#4659

Closed

gabyx mentioned this pull request Mar 28, 2023

when replacing a folder by symlink : i get corrupt images when pulling : failed to mknod("somefile", S_IFCHR, 0): no such file or directory #2308

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Refactor `LayersMap` to correct old strange code behavior #2066

fix: Refactor `LayersMap` to correct old strange code behavior #2066

gabyx commented Apr 28, 2022 •

edited

Loading

gabyx commented May 10, 2022

gabyx commented May 10, 2022

gabyx commented May 16, 2022 •

edited

Loading

gabyx commented May 17, 2022

imjasonh left a comment

gabyx commented May 18, 2022

fix: Refactor LayersMap to correct old strange code behavior #2066

fix: Refactor LayersMap to correct old strange code behavior #2066

Conversation

gabyx commented Apr 28, 2022 • edited Loading

gabyx commented May 10, 2022

gabyx commented May 10, 2022

gabyx commented May 16, 2022 • edited Loading

gabyx commented May 17, 2022

imjasonh left a comment

Choose a reason for hiding this comment

gabyx commented May 18, 2022

fix: Refactor `LayersMap` to correct old strange code behavior #2066

fix: Refactor `LayersMap` to correct old strange code behavior #2066

gabyx commented Apr 28, 2022 •

edited

Loading

gabyx commented May 16, 2022 •

edited

Loading