search

dotnet / docker-tools

This is a repo to house some common tools for our various docker repos.
MIT License
124 stars 46 forks source link

Fix build matrix graph generation to include grandparents in calculation #1143

Closed lbussell closed 1 year ago

lbussell commented 1 year ago

Fixes https://github.com/dotnet/docker-tools/issues/1141

I did not test https://github.com/dotnet/docker-tools/pull/1142 enough and it turns out it was still an issue in some cases. In this case, the PlatformVersionedOS matrix graph generation would create multiple legs for the same OS when we needed to build images with a shared cached parent that were not direct siblings. For example, generating a matrix when we have built both aspnet and aspnet-composite would end up creating two separate test graphs - one with [runtime-deps, aspnet-composite] and the other with [runtime, aspnet, sdk] (that's before we filter the images down in a later step).

flowchart TD
    A[runtime-deps] --> B
    A --> E[aspnet-composite]
    B[runtime] --> C
    C[aspnet] --> D[sdk]

The issue is that the CreateNodeList method didn't include parents-of-parents in its calculation. I changed it to be recursive. We don't need to dig deeper into children because our Dockerfiles don't branch beneath runtime-deps, no children have multiple descendants. However I could be convinced to add walking the children graph to CreateNodeList. because parent images that aren't cached imply all their children will also be not cached.

I also added some extra testing to cover this case. I added SDK to the test graph to make it more representative of the actual build but I don't think it makes a difference.

dotnet-issue-labeler[bot] commented 1 year ago

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

lbussell commented 1 year ago

I accidentally created an sdk->sdk cycle in the manifest defined in the test. Fixed that. Glad it wasn't an issue with the actual code - we should never have cycles in our actual Dockerfiles/manifests.