v0.11 refactoring

[tonistiigi]

A lot of new code has been added in the last release to ship new features as soon as possible. Now that the v0.10 release is out we should take a step back and solidify the codebase to make sure we have a good foundation for future developments.

Rather than some local cleanups and renames, I'm more interested if the scope and dependencies for packages make sense. Should we have a new package or interface to have a clear separation of responsibilities between components?

Some problem areas that have grown a lot:

• Cache/blobs/mergeop/remote management
  • Lazy refs
  • Cacheopts
  • Blob reference management
  • Interaction with snapshot layer
  • Do these designs make sense looking back at them now?
• Dockerfile
  • I think dispatch/dockerfile2llb logic needs reorganization to support more new features like warnings, named contexts, buildinfo, frontend capabilities.

Other approaches:

• Check dependencies for a package. Does the list make sense? A config package should not depend on 20 other packages. cache package should not depend on solver like it does now.
• Look for functions that takes 5+ parameters as every new feature has added a new one. These should be combined into logical structures.
• Check gocyclo. There is no goal to have everything under certain complexity, but it can point to some troublesome areas.
• Check code that has been marked deprecated a long time ago and should be removed.

Action plan:

We need to avoid big PRs and prefer many smaller PRs with well-defined small scope, so they don't get stuck in review. There are many ways to write the same functionality, and we should avoid changes where the improvement is not clear.

Before working on something, make a special issue for it or comment here. In some cases, you need to try an improvement before you can see if it really helps but at least proposed package/interface level changes should have a design proposal first.

Please post other problem areas or ideas in the comments.

@sipsma @ktock @crazy-max

[sipsma]

One thing I was thinking about during merge+diff is that it would be nice if laziness was implemented entirely on the solver level rather than in the cache package. Basic idea being:

• Change GetByBlob, Merge and Diff methods of cacheManager to always create their snapshots immediately. Stargz mode would still not have to pull all the data down locally, but it would create the snapshot right away. Other snapshotters would actually pull the image layers, do the merge, etc.
• Add a new "lazy" state for vertexes in the solver. The solver will defer Exec'ing these vertexes until a descendent vertex requires unlazying, at which point it will unlazy all necessary ancestors.

The advantages of this approach would be

1. The solver would be better at scheduling the execution of unlazies, which would make it easier to address open issues like unlazying a merge as soon as you know you'll need to rather than actually deferring it until a mount is requested
2. Lots of the ugliness involved with handling lazy refs like DescHandler, CacheOpts and various progress stuff would be minimized or potentially eliminated entirely. All that can just be centralized in the solver logic rather than strewn over the whole codebase.

Hard parts:

1. Pretty big lift from the current state and would involve lots of changes to the solver, which is delicate and can have lots of inadvertent side effects. This can be alleviated by lots of tests of course, but would be a large project.
2. Would require significant refactoring of how exports are handled. I.e. right now you can export a lazy merge ref by just creating the ref and then calling GetRemote, but with the change proposed here that would no longer be possible because merge refs would always be unlazy. You would need to have a way of exporting the result of a vertex without actually necessary having to calculate that result.

This is obviously a vague idea still, but let me know if there's any thoughts on it.

[tonistiigi]

@sipsma I'm not sure I fully get it. Do you mean it should be handled in ops implementation or you want to change the CacheMap+Exec contract? I think the solver itself needs to remain generic and not know about what vertex implementations do. That's a good separation of concerns. I'm open to changing the Remote/Ref interface though. Also thinking about the potential changes needed for the multiple workers on different machines with this design.

which would make it easier to address open issues like unlazying a merge as soon as you know you'll need to rather than actually deferring it until a mount is requested

We already have the CacheMap.PreprocessFunc exposed to the solver that should allow controlling such things.

Lots of the ugliness involved with handling lazy refs like DescHandler, CacheOpts, and various progress stuff would be minimized or potentially eliminated entirely. All that can just be centralized in the solver logic rather than strewn over the whole codebase.

I'm not that happy with this part as well. The fact that we store cache records for items that we can't load without external things in context does not feel very solid. Recently this came up on discussing stargz cache import from local/github storage. This doesn't work atm because (iiuc) stargz is externally connected to registry reference. The way it should work is that we should provide stargz a Remote for pulling the data when it needs it. And when we load cache from a half-complete ref there should be a way to connect it with a remote cache importer (if one is available) so it can finish the pull. I'm not sure if the current DescHandler design can handle this.

[tonistiigi]

Another refactoring issue. Some files are very big. client_test and dockerfile_test have 6k lines making it hard to manage. Should try to split them into some logical chunks.

[crazy-max]

I'm not sure if it's in the scope of the refactoring but related to https://github.com/moby/buildkit/issues/163, I think it would be nice to also consider moving the dockerfile frontend to another repo as at this time it's quite painful to track changes, specially for the dockerfile parser or the frontend itself but also find relative docs about the syntax.

Pros:

• enhance visibility and collaboration
• reduce dependencies overhead for the builder and parser (dedicated go modules)
• ship faster within its own pipeline

Cons:

• changes made on buildkit might also be needed in dockerfile repo and vice versa
• cross pipeline integration tests Dockerfile frontend <> BuildKit
  • https://github.com/crazy-max/buildkit-nofrontend/pull/1/files#diff-bdfdd5a668594d305e92b02444c54a81e9dc9b54f79e827d1364b7b5316ff20aR61-R63
  • https://github.com/crazy-max/dockerfile/blob/cf7ae7136304816b12fcde1a1ea9a08804ee0239/hack/buildkit-it-base#L15-L17

I have already made an initial implem in https://github.com/crazy-max/dockerfile. Here is the current status: https://github.com/crazy-max/dockerfile/issues/8 and here is the repo of buildkit without the frontend https://github.com/crazy-max/buildkit-nofrontend. See also https://github.com/crazy-max/buildkit-nofrontend/pull/1 for changes.

[sipsma]

@tonistiigi The thought process behind my suggestion is basically:

1. Laziness seems like a decision about scheduling the execution of work, which feels like it would fit better on the solver+llbsolver level than the cache level.
2. The ugliness of DescHandler, CacheOpts, etc. comes down to what you said: "we store cache records for items that we can't load without external things in context". If instead the laziness was handled entirely in solver+llbsolver, we would significantly limit the exposure of "external things" and thus eliminate a lot of that ugliness.

I'm not sure I fully get it. Do you mean it should be handled in ops implementation or you want to change the CacheMap+Exec contract? I think the solver itself needs to remain generic and not know about what vertex implementations do.

Here's a sketch of one possible approach (obviously open to suggestions though):

1. The generic solver has a new notion of "laziness" which just means that a vertex's result does not need to be loaded immediately. Instead, the result will only be loaded once a non-lazy vertex needs it, at which time cache will be checked, followed by an exec if there's a cache miss.
2. Whether a vertex is lazy or not can be set by the CacheMap func for each op in llbsolver, which keeps the generic solver agnostic to llbop-specific details.
3. Cache refs would now just all be entirely synchronous, creating their snapshots when the ref is created. Everything that's currently in the DescHandler can just be handled on the op level instead of being passed to the cache manager and handled in there. We may also want to extend the Op.Exec method with a progress writer or something too in order to avoid passing stuff through ctx, but that's quite easy.

One part that's missing is how to deal with the fact that when MergeOp has a DiffOp input, it actually doesn't need to unlazy the DiffOp result (it can just look at the lower and upper of the DiffOp and apply it directly to the merge rather than create a snapshot for the diff and then apply it to the merged snapshot). But I think that might be fine, multiple potential ways of dealing with it that I can sketch out if we end up going this route.

We already have the CacheMap.PreprocessFunc exposed to the solver that should allow controlling such things.

Yeah I guess this approach would eliminate the need for UnlazyResultFunc. The unlazying would now be handled in the solver and have all the additional benefits of simplifying the cache package.

Recently this came up on discussing stargz cache import from local/github storage. This doesn't work atm because (iiuc) stargz is externally connected to registry reference. The way it should work is that we should provide stargz a Remote for pulling the data when it needs it. And when we load cache from a half-complete ref there should be a way to connect it with a remote cache importer (if one is available) so it can finish the pull.

Yeah what I'm proposing wouldn't take care of all the problems that need solving to make that happen, but I think it would at least simplify a lot of things for it. We might need to extend GetByBlob or add a new method to CacheManager to support what you're describing, but at least you would no longer need to deal with setting up a DescHandler and passing that all over the place to set it up, if that makes sense.

[sipsma]

Been thinking about how to clean up lazy refs more. I agree that the fundamental problem is that we store refs that can't be loaded without external context. The general high-level approaches I'm thinking of are:

1. Store all the external context needed to unlazy the ref inside worker and/or cacheManager. No external context needs to be provided other than a session maybe.
   1. This is hard because the context is often ephemeral and tied to client sessions. We would need a way to, for example, say that for a given lazy blob ref there are a list of pullers that could be used to unlazy it, but also that if the session associated with the given puller goes away (client disconnects) it should be removed from the list of available pullers for the lazy ref.
   2. If we can assume that after a client session ends, all the immutableRefs associated with it will be closed (i.e. no higher-level component keeps an ref cached in memory after a solve), this may be more straightforward.
2. Don't store lazy refs at all; only persist them in the solver's cache if/once they are unlazied (and thus don't need external context anymore)
   1. My previous suggestion to make all CacheManager operations synchronous and instead have the solver handle the scheduling of unlazies (through a generic, non-llb-specific mechanism) would accomplish this. However, this is likely an enormous refactor and impacts tons of components.
   2. Maybe a simpler intermediate step is possible, but haven't thought of anything concrete yet.

@tonistiigi Do you have any thoughts on these options or any totally separate ideas?

Right now option 1 seems the most practical to me and I believe it would allow us to get rid of all the ugly CacheOpt stuff. Synchronization of the addition and removal of the context needed to unlazy based on client sessions would be ugly, but it would probably be more concentrated rather than spread all over the codebase. Want to double check with you before I start any experiments with it though.

[tonistiigi]

No external context needs to be provided other than a session maybe.

You will always need a session. And I think the request side would somewhat need to validate that it requested a blob though certain image. Eg. it shouldn't be that I'm building with image A and that hits a lazy record that starts pulling previous image B. Not only for security but image B might already be deleted etc. I also want these changes to solve the stargz cache that doesn't pull from registry case. That is similar but I think the way additional info is required to use the ref would need to be more generic.

I think the main problem in here might not be that we store cache for incomplete refs that are not self-loadable, but the way we load cache for such refs is quite fragile. We expect that loader has added all extra requirements already in the context although there isn't much guarantee that it has happened because it comes from a different component.

If we instead make it clear that some refs have additional requirements, to access them you need to provide the extra requirements and if you don't it behaves like the record does not exist then it could be more stable. The main difference would be that this would involve changes to the cache loading that would ignore the cases where result is incomplete and caller doesn't have enough info to load it. Maybe the data about cache only being available/loadable when additional data is provided needs to be stored in the instruction cache database.

[tonistiigi]

Proposed client integration split:

client_test.go
    testCallDiskUsage,
    testBuildHTTPSource,
    testFrontendImageNaming,
    testLocalSymlinkEscape,
    testLocalSourceDiffer,
    testParallelLocalBuilds,
    testFrontendMetadataReturn,
    testFrontendUseSolveResults,
    testStdinClosed,
    testCallInfo, -> testInfoEndpoint
    testDuplicateWhiteouts,
    testWhiteoutParentDir,
    testMoveParentDir, -> testOverlayMoveParentDir
    testRelativeMountpoint, -> testRuncRelativeMountpoint

state_test.go
    testResolveAndHosts,
    testUser,
    testReadonlyRootFS,
    testProxyEnv,
    testExtraHosts,
    testShmSize,
    testUlimit,
    testCgroupParent,
    testHostnameLookup,
    testHostnameSpecifying,
    testRelativeWorkDir,
    
mounts_test.go
    testBuildMultiMount,
    testSSHMount,
    testMountWithNoSource,
    testCachedMounts,
    testRunCacheWithMounts,
    testSharedCacheMounts,
    testSharedCacheMountsNoScratch,
    testLockedCacheMounts,
    testDuplicateCacheMount,
    testCacheMountNoCache,
    testTmpfsMounts,
    testSecretEnv,
    testSecretMounts,

image_test.go
    testSchema1Image,
    testPullZstdImage,
    testPullWithLayerLimit
    testLazyImagePush,
    testStargzLazyPull,
    testPushByDigest,
    
progress_test.go
    testSourceMap,
    testSourceMapFromRef,
    testWarnings,

export_test.go
    testExporterTargetExists,
    testTarExporterWithSocket,
    testTarExporterWithSocketCopy,
    testTarExporterSymlink,
    testBuildExportZstd,
    testExportBusyboxLocal,
    testBuildPushAndValidate,
    testBuildExportWithUncompressed,
    testInvalidExporter,
    testOCIExporter,
    
remotecache_test.go
    testZstdLocalCacheExport,
    testZstdRegistryCacheImportExport,
    testZstdLocalCacheImportExport,
    testUncompressedLocalCacheImportExport,
    testUncompressedRegistryCacheImportExport,
    testStargzLazyRegistryCacheImportExport,
    testStargzLazyInlineCacheImportExport,
    testMultipleRegistryCacheImportExport,
    testBasicInlineCacheImportExport,
    testBasicRegistryCacheImportExport,
    testBasicLocalCacheImportExport,
    testCacheExportCacheKeyLoop

mergediff_test.go
  testMergeOp,
  testMergeOpCacheInline,
  testMergeOpCacheMin,
  testMergeOpCacheMax,

file_test.go
    testRmSymlink,
    testFileOpInputSwap,
    testFileOpMkdirMkfile,
    testFileOpCopyRm,
    testFileOpCopyIncludeExclude,
    testFileOpRmWildcard,
    testCopyFromEmptyImage,

buildinfo_test.go
    testBuildExportWithForeignLayer,
    testBuildInfoExporter,
    testBuildInfoInline,
    testBuildInfoNoExport,

securitymode_test.go
    testSecurityMode,
    testSecurityModeSysfs,
    testSecurityModeErrors,
    testClientGatewayContainerSecurityMode,

network_test.go
    testNetworkMode,
    testHostNetworking,
    testClientGatewayContainerHostNetworking
    testBridgeNetworking,

gateway_test.go
    testClientGatewaySolve,
    testClientGatewayFailedSolve,
    testClientGatewayEmptySolve,
    testNoBuildID,
    testUnknownBuildID,
    testClientGatewayContainerExecPipe,
    testClientGatewayContainerCancelOnRelease,
    testClientGatewayContainerPID1Fail,
    testClientGatewayContainerPID1Exit,
    testClientGatewayContainerMounts,
    testClientGatewayContainerPID1Tty,
    testClientGatewayContainerExecTty,
    testClientSlowCacheRootfsRef,
    testClientGatewayContainerPlatformPATH,
    testClientGatewayExecError,
    testClientGatewaySlowCacheExecError,
    testClientGatewayExecFileActionError,
    testClientGatewayContainerExtraHosts,
    testClientGatewayContainerSignal,
    testClientGatewayFrontendAttrs,
    

Maybe prefix all with tests_gateway_test.go to the files appear together.

[tonistiigi]

Dockerfile tests

args_test.go
  testGlobalArg,
  testBuiltinArgs,
  testMultiArgs,
  testPlatformArgsImplicit,
  testPlatformArgsExplicit,
  testQuotedMetaArgs,
  testDefaultEnvWithArgs,

contexts_test.go
  testDockerfileDirs, -> testLocalBuildContext
  testDockerignore,
  testDockerignoreInvalid,
  testDockerignoreOverride,
  testDockerfileFromGit,
  testDockerfileFromHTTP,
  testSymlinkedDockerfile,
  testHTTPDockerfile,
  testDockerfileLowercase, -> testLowecaseDockerfile
  testTarContext,
  testTarContextExternalDockerfile,
  testContextChangeDirToFile,
  
commands_test.go
  testDockerfileInvalidCommand, -> testRunInvalidCommand
  testDockerfileScratchConfig, -> testScratchWithMetadata
  testExportedHistory,
  testExposeExpansion,
  testUser,
  testCmdShell,
  testLabels,
  testIgnoreEntrypoint,
  testDockerfileInvalidInstruction,
  testWorkdirCreatesDir,
  testWorkdirUser,
  testWorkdirExists,
  testWorkdirCopyIgnoreRelative,
  testOnBuildCleared,
  testEnvEmptyFormatting,
  testPullScratch,
  testDefaultShellAndPath,
  
copy_test.go
  testAddURLChmod,
  testDockerfileAddChownExpand -> testAddChownExpand
  testDockerfileADDFromURL, -> testAddFromURL
  testDockerfileAddArchive, -> testAddArchive
  testDockerfileAddArchiveWildcard
  testCopySymlinks,
  testCopyChown,
  testCopyChmod,
  testCopyOverrideFiles,
  testCopyVarSubstitution,
  testCopyWildcards,
  testCopyRelative,
  testCopyChownCreateDest,
  testCopyThroughSymlinkContext,
  testCopyThroughSymlinkMultiStage,
  testCopySocket,
  testCopyChownExistingDir,
  testCopyWildcardCache,
  testCopyFollowAllSymlinks,
  testEmptyDestDir, -> testCopyEmptyDestDir
  testEmptyWildcard, -> testCopyEmptyWildcard
  testMultiStageImplicitFrom, -> testCopyMultiStageImplicitFrom
  testMultiStageCaseInsensitive, -> testCopyMultiStageCaseInsensitive
  testSymlinkDestination,

resources_test.go
  testShmSize,
  testUlimit,
  testCgroupParent,
  testDockefileCheckHostname, -> testHostname

named_contexts_test.go
  testNamedImageContext,
  testNamedLocalContext,
  testNamedInputContext,
  testNamedMultiplatformInputContext,

exporter_test.go
  testTarExporter,
  testExportMultiPlatform,
  testReproducibleIDs,

cache_test.go
  testNoCache,
  testCacheReleased,
  testNoSnapshotLeak,
  testCacheImportExport,
  testImportExportReproducibleIDs,
  testCacheMultiPlatformImportExport,
  testExportCacheLoop,
  testWildcardRenameCache, -> testCacheInvalidationforWildcard
  
gateway_test.go
  testFrontendUseForwardedSolveResults, -> testFrontendChainedSolves
  testFrontendInputs,
  testFrontendSubrequests,

sourcemap_test.go
  testErrorsSourceMap,

[JakeCooper]

Is there a timeline for getting v0.11 out?

CC @Ahmedmozaly