remove logs from platforms

[akhilerm]

remove github.com/containerd/containerd/log package from the platforms package ;so the platforms package is not dependent on the containerd repository.

This will also help with the containerd API extraction that is planned.

Ref :[1]

Signed-off-by: Akhil Mohan akhilerm@gmail.com

[k8s-ci-robot]

Hi @akhilerm. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[akhilerm]

/cc @cpuguy83

[fangn2]

@akhilerm What's the plan for re-placing the Platforms package?
I think more deps need to be removed, i.e containerd/errdefs.
@cpuguy83 I checked the commit history to the platforms, roughly they were updated monthly, does it still make sense to move them to a separate repo? I know currently buildkit, moby consume the platforms packages by depending on contained.

[AkihiroSuda]

panic seems too harsh here. Maybe just use stdlib log? (If we are really going to remove containerd/log here)

[akhilerm]

If we are not able to reliably get the CPU variant, does containerd features like fetch, run etc work as expected?

[cpuguy83]

Likely not. I think it would be very strange for this to fail ever unless it's running in qemu-user or something?

[akhilerm]

@AkihiroSuda Changed to standard log package. PTAL

[mxpv]

IMO It's really odd to use log package in one of containerd packages.
If we really want to isolate dependency, let's introduce a func or interface that can be overwritten. Something like:

And have platforms.Printf = log.L.Error somewhere during initialization to preserve existing behavior.

[akhilerm]

Should this initialization be made part of all the binaries which are in cmd/ directory? Not able to get one location that serves all.

[samuelkarp]

What about changing this to return an error rather than print a log?

[akhilerm]

@samuelkarp , changing the function to return an error also involves refactoring it in quite a number of places where the cpuVariant() function is used. Thats why i was thinking, if the other solution suggested above of using the interface is much better.

[mxpv]

Could you pls rebase against main branch to includes latest CI fixes for windows?

[akhilerm]

What's the plan for re-placing the Platforms package?

Its not about replacing platforms package. It will be available inside the containerd/container repo itself. But we can have it published into a repository like containerd/api or something else. So anyone who wants to use platforms outside the containerd organisation can refer to that.

This is something that is already done in kubernetes repository. We are trying to do the similar for containerd/containerd and a new repo containerd/api.

[dims]

/ok-to-test

[k8s-ci-robot]

@dims: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[samuelkarp]

/ok-to-test

[github-actions]

This PR is stale because it has been open 90 days with no activity. This PR will be closed in 7 days unless new comments are made or the stale label is removed.

[thaJeztah]

looks like still relevant

[akhilerm]

Closing as the changes in this PR has been achieved via #9086, so platforms is no more dependent on the containerd/containerd repo for logging.

[thaJeztah]

• oh! I recall I was looking at that import indeed, while working on platforms: isolate from errdefs and api dependencies #9095 (but it slipped my mind that this PR existed TBH)

I definitely think that the platforms package could make a good candidate to become its own little module, so that it can be versioned independently (and used in release branches, as well as external consumers)

[akhilerm]

@thaJeztah In that case, should we still decouple platforms from containerd/log, so that platforms does not have any dependency on the log package?

[thaJeztah]

The containerd/log package is a lot more minimal, but ideally it wouldn't be there; just not sure if switching to log is the best solution, as using that would mean that this package (when used as part of containerd) would use a different logger from everything else (so logs may end up elsewhere from all other logs). For library packages, the ideal should always be to have no logging at all, or more specifically: have a way for the consumer of the library to control logging (which could be either through passing a logger, or being able to configure a logger through other means).

containerd's "context-logger" is an elegant solution for that, but due to the context-key being strong-typed, still means that it's coupled to the containerd (log) codebase. My hope was that the Golang maintainers would officially endorse this approach (and for a while it looked like they did; the slog package had a way to pass a logger through a context), but later they removed that part (:cry:).

So.. looking at options here to see if we can surface the information that's logged to callers (instead of logging);

• however, the cpuVariant function signature does not have an error return (leaving "logging" the only solution). We could fix that (as it's not an exported function), but looking at how it's used, also doesn't bring much (all callers don't have an error-return)
• if we would memorize the error and return it (callers to be either handling it or logging it), we would produce a lot more noise (although maybe some of that would be warranted).

As mentioned above; passing the logger through the context.Context wouldn't help, as we'd still need the log package to do so, so perhaps some way to set the logger?

Which could be, just an io.Writer that can be set;

var DebugLogger io.Writer

(and used to print to)

Or some ad-hoc minimum interface that must be satisfied (not sure if that should be a Printf, Print (no formatting) or Errorf;

var DebugLogger interface {
	Printf(format string, v ...any)
}

[thaJeztah]

LOL, that was a bit more lengthy than anticipated; input / suggestions from others more than welcome 😃

[akhilerm]

That was a rather elaborate description of what we want to have.

If passing the logger is an option, then what about the approach described by @mxpv here #8072 (comment)

[thaJeztah]

LOL, looks like I didn't read that earlier comment properly, it's indeed quite similar.

I'd be ok with a var Printf = log.Printf, or perhaps a var Logger func(format string, a ...interface{}) (which can be nil by default), or something along those lines.