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

improve logging in lock generation #338

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

cbgbt
Copy link
Contributor

@cbgbt cbgbt commented Jul 28, 2024

Description of changes:
I had a twoliter update fail and it was difficult to determine why, so I've added additional logging to lockfile generation. I've also done some minor refactoring of the Kit metadata extraction code that threw the error in question.

This (potentially controversially) adds a dependency on the tracing crate. Here's why I like this dependency:

  • Makes it easy to emit trace-level logs for each function call using the instrument macro
  • Makes it easy to add context to log messages with helpful variable values
  • Integrates with the log crate for output, or optionally allows more complex IO via the subscriber concept.

Here's an example:

[2024-07-28T03:18:49Z INFO  twoliter::lock] Resolving project references to create lock file
[2024-07-28T03:18:50Z ERROR twoliter::lock] Mismatched kit metadata in manifest
list canonical_metadata=<ImageMetadata(decoded) [ImageMetadata { name: "bottlerocket-core-kit", version: Version { major: 2, minor: 0, patch: 0 }, sdk: Image {
name: ValidIdentifier("thar-be-beta-sdk"), version: Version { major: 0, minor: 43, patch: 0 }, vendor: ValidIdentifier("bottlerocket-new") }, kits: [] }]> kit_metadata=<ImageMetadata(decoded) [ImageMetadata { name: "bottlerocket-core-kit",
version: Version { major: 2, minor: 0, patch: 0 }, sdk: Image { name: ValidIdentifier("thar-be-beta-sdk"), version: Version { major: 0, minor: 43, patch: 0 }, vendor: ValidIdentifier("bottlerocket") }, kits: [] }]>
Error: Metadata does not match between images in manifest list

Testing done:

  • make build succeeds
  • twoliter update emits helpful logs on failure
  • twoliter update succeeds within existing workspaces

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@cbgbt cbgbt requested review from jmt-lab and sam-berning July 28, 2024 03:31
Comment on lines 605 to 609
let embedded_kit_metadata = stream::iter(manifest_list.manifests)
.map(|manifest| async move {
let image_uri = format!("{}/{}@{}", vendor.registry, image.name, manifest.digest);
EncodedKitMetadata::try_from_image(&image_uri, image_tool).await
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the Docker image tool going to be robust for parallel invocations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to support it (based on my own testing and a lack of warnings found online), but since a pull can result in many parallel layer pulls, 8 is probably too generous of a limit.

I also don't know if the parallel pulls collapse common layers into a single request (I'm guessing not), but the images won't really have layers in common anyways, since they are different architectures.

My real goal here was to separate the kit metadata fetch from the check that the metadata was consistent across images as a clarity change. The parallelism was added as a bonus. I wouldn't be opposed to removing it. For now, I'll just lower it.

@cbgbt cbgbt force-pushed the instrument-lock branch from 56f3081 to a04e0cf Compare August 1, 2024 19:13
@cbgbt cbgbt force-pushed the instrument-lock branch from a04e0cf to 4cd63c9 Compare August 5, 2024 18:48
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 5, 2024

^ Remove concurrent image procedures. Potential instability is probably not worth the potential performance improvement.

@cbgbt cbgbt force-pushed the instrument-lock branch from 4cd63c9 to 53ffe30 Compare August 5, 2024 20:45
@cbgbt
Copy link
Contributor Author

cbgbt commented Aug 5, 2024

^ Force push to rebase

@cbgbt cbgbt force-pushed the instrument-lock branch from 53ffe30 to 1d90850 Compare August 6, 2024 23:39
@cbgbt cbgbt merged commit efd17d9 into bottlerocket-os:develop Aug 8, 2024
1 check passed
@cbgbt cbgbt deleted the instrument-lock branch August 8, 2024 21:51
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.

3 participants