Replace platforms.Format with platforms.FormatAll in user-visible messages and logs. by ctalledo · Pull Request #49973 · moby/moby

ctalledo · 2025-05-14T00:43:39Z

fixes c8d: review uses of platforms.Format vs platforms.FormatAll #48659.

- What I did

Use platforms.FormatAll (instead of platforms.Format) in user-visible messages and logs, since it includes the image's platform OS version (when set).

thaJeztah · 2025-05-14T08:29:55Z

We should probably not mark the ticket as fixed until we reviewed the remaining uses (I changed the "fixes" to "addresses"); we can do those separately (changes in this PR look good AFAICS), but make sure that we're not accidentally dropping os.version in places where it shouldn't be dropped.

I had a quick peek (if my IDE is not lying to me), and outside of vendor code, and the good news is that the list is not too long, so probably doable in a follow-up. I found the remaining uses here;

moby/builder/builder-next/adapters/containerimage/pull.go

Lines 184 to 185 in 294f0c3

    
           // key is used to synchronize resolutions that can happen in parallel when doing multi-stage. 
        
           key := "getconfig::" + ref + "::" + platforms.Format(p)

moby/builder/builder-next/worker/worker.go

Lines 163 to 166 in 294f0c3

    
           	pm[platforms.Format(p)] = struct{}{} 
        
           } 
        
           for _, p := range archutil.SupportedPlatforms(noCache) { 
        
           	if _, ok := pm[platforms.Format(p)]; !ok {

moby/daemon/containerd/image_pull.go

Lines 79 to 82 in 294f0c3

    
           func (i *ImageService) pullTag(ctx context.Context, ref reference.Named, platform *ocispec.Platform, metaHeaders map[string][]string, authConfig *registrytypes.AuthConfig, out progress.Output) error { 
        
           	var opts []containerd.RemoteOpt 
        
           	if platform != nil { 
        
           		opts = append(opts, containerd.WithPlatform(platforms.Format(*platform)))

moby/internal/testutils/specialimage/multiplatform.go

Lines 18 to 20 in 294f0c3

    
           for _, platform := range imagePlatforms { 
        
           	ps := platforms.Format(platform) 
        
           	manifestDesc, _, err := oneLayerPlatformManifest(dir, platform, FileInLayer{Path: "bash", Content: []byte("layer-" + ps)})

moby/internal/testutils/specialimage/partial.go

Lines 28 to 30 in 294f0c3

    
           for _, platform := range opts.Stored { 
        
           	ps := platforms.Format(platform) 
        
           	manifestDesc, _, err := oneLayerPlatformManifest(dir, platform, FileInLayer{Path: "bash", Content: []byte("layer-" + ps)})

thaJeztah · 2025-05-14T08:33:43Z

 		platform = *pp
 	}
-	log.G(ctx).Debugf("%s resolved to a manifestList object with %d entries; looking for a %s match", ref, len(mfstList.Manifests), platforms.Format(platform))
+	log.G(ctx).Debugf("%s resolved to a manifestList object with %d entries; looking for a %s match", ref, len(mfstList.Manifests), platforms.FormatAll(platform))


Not for this PR, but we should use structured logs more for entries like these; ideally we would review the names we used for fields in those logs (as I think we're not always consistent); having more structured logs allows easier filtering / querying of logs (more so if they carry consistent fields, which allows correlating logs).

thaJeztah

LGTM

ctalledo · 2025-05-14T16:34:16Z

I had a quick peek (if my IDE is not lying to me), and outside of vendor code, and the good news is that the list is not too long, so probably doable in a follow-up. I found the remaining uses here;

Thanks @thaJeztah; yes I saw those remaining instances too, but wasn't sure if the change should apply to them because they are functional changes (as opposed to logging / messaging changes).

I'll take a closer look and do another commit on this PR.

ctalledo · 2025-05-14T17:26:57Z

I'll take a closer look and do another commit on this PR.

Done with this, I've added a second commit to this PR; see 7a57e73.

thaJeztah · 2025-05-16T11:19:08Z

 	// key is used to synchronize resolutions that can happen in parallel when doing multi-stage.
-	key := "getconfig::" + ref + "::" + platforms.Format(p)
+	key := "getconfig::" + ref + "::" + platforms.FormatAll(p)


Could use some extra eyes on this one if changing this one is OK to do

@tonistiigi: does this PR look OK (in particular the use of FormatAll() above).

tonistiigi

This is ok in user messages but may not be fine in places where platform string is passed as input.

tonistiigi · 2025-05-16T22:12:25Z

 		pm := make(map[string]struct{}, len(w.Opt.Platforms))
 		for _, p := range w.Opt.Platforms {
-			pm[platforms.Format(p)] = struct{}{}
+			pm[platforms.FormatAll(p)] = struct{}{}


This should be updated like https://github.com/moby/buildkit/blob/master/worker/base/worker.go#L264

Thanks; I get what you are recommending, but that seems outside the scope of this PR (i.e., you are suggesting a refactoring of the entire function, where as this PR is simply about replacing Format() with FormatAll()).

Can we do what you are suggesting in a follow-up PR?

Then don't make changes to this part of the code.

Just so I understand: is that because the change I am proposing incorrect? If so where, I don't see the error (and CI is green).

The old code is incorrect and the new code is incorrect as well. OSVersion would be incorrect in here in any case.

Got it, thanks, I've reverted that part of the change.

I assume the change in builder/builder-next/adapters/containerimage/pull.go looks good to you?

Looks like that code was originally added in dbffbe8 (#41601); was this incorrect all this time? 😓

Oh, never mind; I looked at the wrong part of the diff 🙈 ignore me.

vvoland

CI is red

thaJeztah · 2025-05-19T15:08:51Z

We could probably move the second commit to a separate PR if that one still needs looking at (assuming the first one was green)

…sages and logs. Use FormatAll in user-visible messages and logs, since it includes the image's platform OS version (when set). Fixes moby#48659. Signed-off-by: Cesar Talledo <[email protected]>

ctalledo · 2025-05-19T17:25:28Z

We could probably move the second commit to a separate PR if that one still needs looking at (assuming the first one was green)

No need, CI is green now (I rebased from master).

Signed-off-by: Cesar Talledo <[email protected]>

thaJeztah

LGTM, thanks!

@ctalledo could you open a tracking ticket to outline the work that's needed as commented by Tonis? Trying to make sure we don't lose track of that.

thaJeztah · 2025-05-20T18:52:34Z

Also @vvoland PTAL (you have a "request changes" pending)

ctalledo · 2025-05-20T23:00:02Z

@ctalledo could you open a tracking ticket to outline the work that's needed as commented by Tonis? Trying to make sure we don't lose track of that.

Sure: #50037

ctalledo self-assigned this May 14, 2025

ctalledo requested a review from thaJeztah May 14, 2025 00:44

ctalledo mentioned this pull request May 14, 2025

c8d: review uses of platforms.Format vs platforms.FormatAll #48659

Closed

thaJeztah added this to the 28.2.0 milestone May 14, 2025

thaJeztah added the status/2-code-review label May 14, 2025

thaJeztah reviewed May 14, 2025

View reviewed changes

thaJeztah approved these changes May 14, 2025

View reviewed changes

ctalledo requested a review from tonistiigi as a code owner May 14, 2025 17:24

thaJeztah reviewed May 16, 2025

View reviewed changes

tonistiigi reviewed May 16, 2025

View reviewed changes

vvoland requested changes May 19, 2025

View reviewed changes

Replace platforms.Format with platforms.FormatAll in user-visible mes…

cf11cd1

…sages and logs. Use FormatAll in user-visible messages and logs, since it includes the image's platform OS version (when set). Fixes moby#48659. Signed-off-by: Cesar Talledo <[email protected]>

ctalledo force-pushed the fix-for-48659 branch from 7a57e73 to 6d39506 Compare May 19, 2025 15:47

ctalledo requested a review from vvoland May 19, 2025 17:25

Replace platforms.Format with platforms.FormatAll in functional code.

bb41e5a

Signed-off-by: Cesar Talledo <[email protected]>

ctalledo force-pushed the fix-for-48659 branch from 6d39506 to bb41e5a Compare May 19, 2025 18:07

tonistiigi approved these changes May 20, 2025

View reviewed changes

thaJeztah approved these changes May 20, 2025

View reviewed changes

ctalledo mentioned this pull request May 20, 2025

Update worker.Platforms() in builder/builder-next/worker/worker.go #50037

Closed

vvoland approved these changes May 21, 2025

View reviewed changes

vvoland merged commit a3bee41 into moby:master May 21, 2025
144 checks passed

ctalledo deleted the fix-for-48659 branch May 21, 2025 18:18

Conversation

ctalledo commented May 14, 2025 • edited by thaJeztah Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

ctalledo commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctalledo commented May 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi 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!

Choose a reason for hiding this comment

Uh oh!

ctalledo May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonistiigi May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

vvoland left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 19, 2025

Uh oh!

ctalledo commented May 19, 2025

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented May 20, 2025

Uh oh!

ctalledo commented May 20, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ctalledo commented May 14, 2025 •

edited by thaJeztah

Loading

ctalledo commented May 14, 2025 •

edited

Loading

ctalledo May 19, 2025 •

edited

Loading

tonistiigi May 19, 2025 •

edited

Loading