Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go.mod: github.com/containerd/containerd v1.7.7, containerd/nri v0.4.0, switch to github.com/containerd/log module #598

Merged
merged 2 commits into from
Jun 26, 2024

Conversation

thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 21, 2024

go.mod: github.com/containerd/containerd v1.7.7, containerd/nri v0.4.0

Update to the first release of containerd to alias the log package to the new containerd/log module.

This update of the containerd module also comes with an updated version of github.com/containerd/nr, which has some breaking changes in the interface definitions.

the optimizer-nri-plugin and prefetchfilees-nri-pugin pacakges had to be updated due to containerd/nri@9d86150 changing the interface to require a context to be passed.

Full diffs:

switch to github.com/containerd/log module

containerd 1.7.7 and up alias the log package to the new module,
and deprecate the package.

This patch:

  • replaces existing uses of the log package to use the module
  • replaces some direct uses of logrus to use the log module

There is one change in behavior in the nri-plugins, which now
use the default format as used by the containerd log module.

Comment on lines +242 to +244
// FIXME(thaJeztah): ucontainerd's log does not set "PadLevelText: true"
_ = log.SetFormat(log.TextFormat)
ctx := log.WithLogger(context.Background(), log.L)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently uses a different format than what was used before;

  • Now uses RFC3339NanoFixed format
  • Now uses FullTimestamp: true
  • ❌ does not use PadLevelText: true

https://github.com/containerd/log/blob/v0.1.0/context.go#L153-L158

	case TextFormat:
		L.Logger.SetFormatter(&logrus.TextFormatter{
			TimestampFormat: RFC3339NanoFixed,
			FullTimestamp:   true,
		})
		return nil

Comment on lines +149 to +151
// FIXME(thaJeztah): ucontainerd's log does not set "PadLevelText: true"
_ = log.SetFormat(log.TextFormat)
ctx := log.WithLogger(context.Background(), log.L)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

@thaJeztah thaJeztah changed the title switch to github.com/containerd/log module go.mod: github.com/containerd/containerd v1.7.7, containerd/nri v0.4.0, switch to github.com/containerd/log module Jun 21, 2024
@thaJeztah thaJeztah marked this pull request as ready for review June 21, 2024 16:16
@thaJeztah thaJeztah self-assigned this Jun 21, 2024
Update to the first release of containerd to alias the log package to
the new containerd/log module.

This update of the containerd module also comes with an updated version
of github.com/containerd/nr, which has some breaking changes in the
interface definitions.

the optimizer-nri-plugin and prefetchfilees-nri-pugin pacakges had to be
updated due to [containerd/nri@9d86150] changing the interface to require
a context to be passed.

[containerd/nri@9d86150]: containerd/nri@9d86150

Full diffs:

- https://github.com/containerd/containerd/v1.7.0...v1.7.7
- containerd/nri@v0.3.0...v0.4.0

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
containerd 1.7.7 and up alias the log package to the new module,
and deprecate the package.

This patch:

- replaces existing uses of the log package to use the module
- replaces some direct uses of logrus to use the log module
- enables the depguard linter to prevent accidental use of
  the deprecated packages.

There is one change in behavior in the nri-plugins, which now
use the default format as used by the containerd log module.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah
Copy link
Member Author

@imeoer ptal 🤗

@imeoer
Copy link
Collaborator

imeoer commented Jun 26, 2024

LGTM, thanks for the careful work! :)

@imeoer imeoer merged commit 71ab64a into containerd:main Jun 26, 2024
16 checks passed
@thaJeztah thaJeztah deleted the switch_to_containerd_log branch June 26, 2024 10:11
@thaJeztah
Copy link
Member Author

@imeoer thanks! I also move #599 out of draft, but it looks like there's an issue with OTEL 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants