project-stacker / stacker

Build OCI images natively from a declarative format
https://stackerbuild.io
Apache License 2.0
208 stars 34 forks source link

fix: cache: serialize legacy import field #603

Closed mikemccracken closed 8 months ago

mikemccracken commented 8 months ago

Fixes #592 by serializing the wasLegacyImport field to ensure that the cache records the correct value when an image has it set.

Includes a test based on Serge's description in #592.

The symptom is an error "cache miss because layer definition was changed" when the definition could not have changed since build. This happens during multi file builds in builds with dependencies, and in build-then-publish flows, where the build will be fine but the publish will fail with the same error.

Using --debug to show the cache mismatch shows that somewhere along the line, the layer is being serialized with the wrong default value when writing to the cache.

Needs to change currentCacheVersion to ensure compatibility, see the TestCacheEntryChanged function comment for more info.

codecov[bot] commented 8 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 57.44%. Comparing base (b585bfb) to head (2a8ada6). Report is 1 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #603 +/- ## ========================================== + Coverage 57.38% 57.44% +0.06% ========================================== Files 64 65 +1 Lines 7566 7706 +140 ========================================== + Hits 4342 4427 +85 - Misses 2481 2522 +41 - Partials 743 757 +14 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mikemccracken commented 8 months ago

Some clarification, in part because I wasn't sure until after this was merged, why the field needed to be serialized to fix the build case - it doesn't:

There are two ways this ends up failing: for the single multi file run of stacker build, the cache package makes a copy of the Layer struct when the build checks the cache during the second file's build - copying the Layer out of the cache results in all non-exported fields being set to the zero value, so wasLegacy ends up as false, which conflicts with its original value, which is what it is being compared with in the second stage.

To fix this we only need to make it an exported field.

In the build then publish case, it is going to read in from the cache , and set the field to the zero value because it isn't serialized. To fix that, we also need to add the serialization annotation to write the field out to the cache.

It might be worth having a publish test case that also exercises this- I'll look into it.

mikemccracken commented 8 months ago

My clarification was wrong yesterday. The behavior of copying in go structs with non-exported values is not quite that crazy - there are still some sharp edges there but if you just assign a struct, you will get the identical contents.

My confusion was that I didn't realize that stacker re-read the cache from disk in between files of multi-file builds. So in fact adding the serialization annotation itself is the fix in both cases - and we have to make the field exported to serialize it. So the fix here is correct and minimal, and as a bonus now I can explain it correctly.