Command for generation of EOL annotation data file by NikolaMilosavljevic · Pull Request #1358 · dotnet/docker-tools

NikolaMilosavljevic · 2024-07-01T23:29:29Z

Contributes to #1201

Opening this PR as a draft. There are some areas that could use a little more cleanup and refactoring.

This work adds a new command, with related tests, for generation of EOL annotation data file.

The command is doing the following:

Compares old and new image-info files to determine which digests should be marked for EOL
Utilizes Microsoft.Deployment.DotNet.Releases library to determine EOL dates of .NET products and use them in annotations of images and platforms
Queries Azure event log telemetry to detect any 'dangling' digests - i.e. those that are present in ACR but were never added to image-info files

Pipeline work, to enable annotation of EOL digests is not present in this PR. That work isn't complex, however it is incomplete and untested. Shared in https://github.com/NikolaMilosavljevic/docker-tools/pull/new/pipeline.eol.annotations

ghost · 2024-07-01T23:29:34Z

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.

ghost · 2024-07-01T23:29:35Z

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.

mthalman

I haven't looked at the tests yet but this to be on track with what I was expecting.

src/Microsoft.DotNet.ImageBuilder/src/DotNetReleasesService.cs

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

src/Microsoft.DotNet.ImageBuilder/src/AzureLogService.cs

mthalman · 2024-07-02T19:21:14Z

@dagood - Be aware that we're adding a dependency on Azure Logs Workspace here that contains the ACR events. That would be necessary to configure in order to support EOL annotations.

Co-authored-by: Matt Thalman <mthalman@microsoft.com>

…tationDataCommand.cs Co-authored-by: Matt Thalman <mthalman@microsoft.com>

lbussell · 2024-07-02T19:43:12Z

src/Microsoft.DotNet.ImageBuilder/src/Models/AcrEventEntry.cs

+namespace Microsoft.DotNet.ImageBuilder.Models.Annotations
+{
+    public class AcrEventEntry
+    {
+        // TODO: at the moment we only use 'TimeGenerated' and 'Digest'
+        // We should likely delete the other properties.
+
+        public DateTime TimeGenerated { get; set; }
+        public string OperationName { get; set; }
+        public string Repository { get; set; }
+        public string Tag { get; set; }
+        public string Digest { get; set; }
+
+        public AcrEventEntry()
+        {
+        }
+    }
+}


Suggested change

namespace Microsoft.DotNet.ImageBuilder.Models.Annotations

{

public class AcrEventEntry

{

// TODO: at the moment we only use 'TimeGenerated' and 'Digest'

// We should likely delete the other properties.

public DateTime TimeGenerated { get; set; }

public string OperationName { get; set; }

public string Repository { get; set; }

public string Tag { get; set; }

public string Digest { get; set; }

public AcrEventEntry()

{

}

}

}

namespace Microsoft.DotNet.ImageBuilder.Models.Annotations;

public record AcrEventEntry(DateTime TimeGenerated, string Digest);

lbussell · 2024-07-18T16:37:27Z

src/Microsoft.DotNet.ImageBuilder/src/DotNetReleasesService.cs

+using System.Linq;
+using Microsoft.Deployment.DotNet.Releases;
+using System.Threading.Tasks;
+using Microsoft.DotNet.ImageBuilder.Models.Annotations;
+using Newtonsoft.Json;


Suggested change

using System.Linq;

using Microsoft.Deployment.DotNet.Releases;

using System.Threading.Tasks;

using Microsoft.DotNet.ImageBuilder.Models.Annotations;

using Newtonsoft.Json;

using Microsoft.Deployment.DotNet.Releases;

using System.Threading.Tasks;

Unused imports

lbussell · 2024-07-18T16:41:18Z

src/Microsoft.DotNet.ImageBuilder/src/DotNetReleasesService.cs

+            ProductCollection col = await ProductCollection.GetAsync();
+
+            foreach (Product product in col)


Suggested change

ProductCollection col = await ProductCollection.GetAsync();

foreach (Product product in col)

ProductCollection dotnetProducts = await ProductCollection.GetAsync();

foreach (Product product in dotnetProducts)

Naming could be more clear here.

lbussell · 2024-07-18T16:48:54Z

src/Microsoft.DotNet.ImageBuilder/src/Models/Annotations/EolDigestData.cs

    public class EolDigestData
    {
        [JsonProperty(Required = Required.Always)]
-        public string Digest { get; set; }
+        public string Digest { get; set; } = string.Empty;
+
+        // This isn't read from programmatically, but is useful for debugging
+        public string? Tag { get; set; }

        public DateOnly? EolDate { get; set; }


Suggested change

public class EolDigestData

{

[JsonProperty(Required = Required.Always)]

public string Digest { get; set; }

public string Digest { get; set; } = string.Empty;

// This isn't read from programmatically, but is useful for debugging

public string? Tag { get; set; }

public DateOnly? EolDate { get; set; }

public record EolDigestData

{

[JsonProperty(Required = Required.Always)]

public required string Digest { get; init; } = string.Empty;

// This isn't read from programmatically, but is useful for debugging

public string? Tag { get; init; }

public DateOnly? EolDate { get; init; }

These can be init-only

lbussell · 2024-07-18T16:49:57Z

src/Microsoft.DotNet.ImageBuilder/src/AzureScopes.cs

    public const string DefaultAzureManagementScope = "https://management.azure.com" + ScopeSuffix;
    public const string ContainerRegistryScope = "https://containerregistry.azure.net" + ScopeSuffix;
    public const string McrStatusScope = "api://c00053c3-a979-4ee6-b94e-941881e62d8e" + ScopeSuffix;
+    public const string LogAnalyticsScope = "https://api.loganalytics.io" + ScopeSuffix;


Suggested change

public const string LogAnalyticsScope = "https://api.loganalytics.io" + ScopeSuffix;

This is not referenced anywhere.

lbussell · 2024-07-18T16:52:14Z

src/Microsoft.DotNet.ImageBuilder/src/DotNetReleasesService.cs

+    [Export(typeof(IDotNetReleasesService))]
+    public class DotNetReleasesService : IDotNetReleasesService
+    {
+        public async Task<Dictionary<string, DateOnly?>> GetProductEolDatesFromReleasesJson()


Why is DateOnly nullable here? Will DateOnly.FromDateTime below ever return null? Could this be non-nullable and missing dates be represented by being absent from the dictionary?

lbussell · 2024-07-18T17:28:03Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataOptions.cs

+        base.GetCliArguments()
+            .Concat(
+                [
+                    new Argument<string>(nameof(AnnotateEolDigestsOptions.EolDigestsListOutputPath),


Suggested change

new Argument<string>(nameof(AnnotateEolDigestsOptions.EolDigestsListOutputPath),

new Argument<string>(nameof(GenerateEolAnnotationDataOptions.EolDigestsListPath),

I think this one is a bug - it causes an exception later on when you run the code.

lbussell · 2024-07-18T17:52:38Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

+    /// <summary>
+    /// Finds all the digests that are in the registry but not in the supported digests list.
+    /// </summary>
+    private static IEnumerable<EolDigestData> GetUnsupportedDigests(IEnumerable<(string Digest, string? Tag)> registryDigests, IEnumerable<string> supportedDigests)


This method can just be:

registryDigests .Where(registryDigest => !supportedDigests.Contains(registryDigest.Digest)) .Select(registryDigest => new EolDigestData(registryDigest.Digest) { Tag = registryDigest.Tag });

lbussell · 2024-07-18T17:54:03Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

+    private IEnumerable<string> GetSupportedDigests() =>
+        _newImageArtifactDetails.Repos
+            .SelectMany(repo => repo.Images)
+            .SelectMany(image => GetImageDigests(image))


Suggested change

.SelectMany(image => GetImageDigests(image))

.SelectMany(GetImageDigests)

lbussell · 2024-07-18T17:55:55Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

+    private static string? GetLongestTag(IEnumerable<string> tags) =>
+        tags.OrderByDescending(tag => tag.Length).FirstOrDefault();
+
+    private async Task<IEnumerable<(string Digest, string? Tag)>> GetAllDigestsFromRegistry(IEnumerable<string> repoNames)


I have nothing to say besides I tried to speed up this method and I couldn't :)

lbussell · 2024-07-18T17:59:21Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

+        if (image.ProductVersion != null)
+        {
+            string dotnetVersion = Version.Parse(image.ProductVersion).ToString(2);
+            if (_productEolDates != null && _productEolDates.TryGetValue(dotnetVersion, out DateOnly? date))
+            {
+                return GetImageDigests(image).Select(val => new EolDigestData(val.Digest) { Tag = val.Tag, EolDate = date });
+            }
+        }
+
+        return [];


Consider inverting these conditions and returning early?

Suggested change

if (image.ProductVersion != null)

{

string dotnetVersion = Version.Parse(image.ProductVersion).ToString(2);

if (_productEolDates != null && _productEolDates.TryGetValue(dotnetVersion, out DateOnly? date))

{

return GetImageDigests(image).Select(val => new EolDigestData(val.Digest) { Tag = val.Tag, EolDate = date });

}

}

return [];

if (image.ProductVersion == null)

{

return [];

}

string dotnetVersion = Version.Parse(image.ProductVersion).ToString(2);

if (_productEolDates == null || !_productEolDates.TryGetValue(dotnetVersion, out DateOnly? date))

{

return [];

}

return GetImageDigests(image).Select(val => new EolDigestData(val.Digest) { Tag = val.Tag, EolDate = date });

lbussell

Just one question.

lbussell · 2024-07-19T18:34:50Z

src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnnotationDataCommand.cs

+            if (Options.AnnotateEolProducts)
+            {
+                // Annotate images for eol products in new image info
+                foreach (ImageData image in newImageArtifactDetails.Repos.SelectMany(repo => repo.Images))
+                {
+                    digestDataList.AddRange(GetProductEolDigests(image, productEolDates));
+                }
+            }
+        }


Sorry if this has already been discussed. But in what case would we decide to build and ship an image for a product that is already EOL, and mark it as EOL immediately?

For .NET major version EOL. The last patch ships on the EOL date.

Command for generation of EOL annotation data file

5bdf2ee

NikolaMilosavljevic added the do-not-merge A PR that should not be merged yet but needs to remain open label Jul 1, 2024

NikolaMilosavljevic requested a review from a team as a code owner July 1, 2024 23:29

ghost added the untriaged label Jul 1, 2024

mthalman reviewed Jul 2, 2024

View reviewed changes

NikolaMilosavljevic and others added 3 commits July 2, 2024 14:00

Update src/Microsoft.DotNet.ImageBuilder/src/DotNetReleasesService.cs

8d6b1a9

Co-authored-by: Matt Thalman <mthalman@microsoft.com>

Update src/Microsoft.DotNet.ImageBuilder/src/Commands/GenerateEolAnno…

24f2745

…tationDataCommand.cs Co-authored-by: Matt Thalman <mthalman@microsoft.com>

Addressing review comments

13ce63d

lbussell reviewed Jul 2, 2024

View reviewed changes

mthalman added 4 commits July 9, 2024 12:49

Test simplification

a2ed75d

Use different "using" syntax to avoid extra indentation

58254d2

Code review feedback

e07e9a6

Syntax cleanup

5d54a58

lbussell assigned mthalman Jul 9, 2024

mthalman added 8 commits July 9, 2024 13:41

Code cleanup

aa9993a

Code refactoring

f861a7b

Collapse namespace declaration

db73bc2

Options refactoring

9c891f9

Azure log updates

dcbd5cf

Rewrite logic to compare against current registry state

e0d2486

Fix failing CleanAcrImagesCommand tests

bd9f943

Merge branch 'main' into generate.annotations

7dbdeb9

mthalman requested a review from lbussell July 18, 2024 13:03

mthalman changed the title ~~[DRAFT] Command for generation of EOL annotation data file~~ Command for generation of EOL annotation data file Jul 18, 2024

mthalman removed the do-not-merge A PR that should not be merged yet but needs to remain open label Jul 18, 2024

lbussell reviewed Jul 18, 2024

View reviewed changes

Address review comments

01ec82c

mthalman requested a review from lbussell July 18, 2024 20:25

lbussell approved these changes Jul 19, 2024

View reviewed changes

mthalman merged commit 8bbcfec into dotnet:main Jul 22, 2024

This was referenced Jul 22, 2024

Add lifecycle annotations to images pushed to MCR #1201

Closed

[main] Update Image Builder tag reference #1365

Merged

		ProductCollection col = await ProductCollection.GetAsync();

		foreach (Product product in col)

	new Argument<string>(nameof(AnnotateEolDigestsOptions.EolDigestsListOutputPath),
	new Argument<string>(nameof(GenerateEolAnnotationDataOptions.EolDigestsListPath),

	.SelectMany(image => GetImageDigests(image))
	.SelectMany(GetImageDigests)

Conversation

NikolaMilosavljevic commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 1, 2024

Uh oh!

ghost commented Jul 1, 2024

Uh oh!

mthalman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mthalman commented Jul 2, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lbussell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

NikolaMilosavljevic commented Jul 1, 2024 •

edited

Loading