search

dotnet / docker-tools

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

Combine duplicate build matrix legs instead of failing the build #1486

Closed lbussell closed 2 weeks ago

lbussell commented 3 weeks ago

Oftentimes when changing the manifest in repos, the build matrix generation may fail with the following error:

Unhandled exception: System.Reflection.TargetInvocationException: Exception has been thrown by the target of an invocation.
 ---> System.InvalidOperationException: Duplicate leg name(s) found in matrix 'linuxAmd64': 9.0-cbl-mariner2.0-distroless-aspnet
   at Microsoft.DotNet.ImageBuilder.Commands.GenerateBuildMatrixCommand.GenerateMatrixInfo() in /image-builder/src/Commands/GenerateBuildMatrixCommand.cs:line 460
   at Microsoft.DotNet.ImageBuilder.Commands.GenerateBuildMatrixCommand.ExecuteAsync() in /image-builder/src/Commands/GenerateBuildMatrixCommand.cs:line 46
   at Microsoft.DotNet.ImageBuilder.Commands.Command`2.<GetCliCommand>b__9_0(TOptions options) in /image-builder/src/Commands/Command.TOptions.cs:line 46
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr)
   --- End of inner exception stack trace ---

This should be addressed by https://github.com/dotnet/docker-tools/issues/1178 which would hopefully eliminate the possibility of duplicate legs being created.

In the meantime, as a mitigation for the above matrix generation issues, I propose that whenever we have duplicate leg names found, we simply combine them so that the build still functions.

In the GenerateBuildMatrix command, that would just mean adding some additional logic here instead of throwing an error:

https://github.com/dotnet/docker-tools/blob/1d6ecd5db81ddce03b88c670750804397122b111/src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateBuildMatrixCommand.cs#L533-L548

dotnet-issue-labeler[bot] commented 3 weeks ago

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

dotnet-issue-labeler[bot] commented 3 weeks ago

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

mthalman commented 3 weeks ago

I have concerns of doing this because it can mask issues in the matrix generation that cause Dockerfiles to be combined into the same leg that we may not want to be combined. Here's an example of such an issue: https://github.com/dotnet/docker-tools/pull/890. As illustrated in the issue, the correct output should have multiple legs, but with unique leg names. If we ended up getting rid of the validation and combining duplicate legs, it wouldn't end up catching a bug like that. In this scenario, it would lead to bad performance in the pipeline since its serializing more of the work that could be done in parallel. That's one thing. But I also wonder whether there are other scenarios that could cause real functional bugs in the output of what we produce.

lbussell commented 2 weeks ago

[Triage] We should spend our time fixing #1178 itself rather than working on band-aid fixes like this. Closing in favor of #1178.