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

feat: Add cachedimage pulling progress status #402

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aDisplayName
Copy link

@aDisplayName aDisplayName commented Aug 27, 2024

Add following property in CachedImage CRD

  • spec.progress.available
  • spec.progress.total

Update the status.progress.available in CachedImage CRD every 5 seconds during the image pulling in cachedimage controller

This is to replace the PR #400 with all the clutters removed.

This is to implement the feature request #401

The CRD / API version is not changed here, still left as v1alpha1.

@aDisplayName aDisplayName marked this pull request as ready for review August 27, 2024 15:17
@plaffitt
Copy link
Contributor

Thanks for this new PR ! It will be easier for me to review since there are less commits and much less changes too haha

To answer your comment in the previous one (by the way, you could also squash, reword and force-push your old branch instead of creating a new one, but, this way we keep discussions in the same PR, but it's ok for this time don't worry): I think we don't need to bump the version of the CRD, the goal of naming it alpha is to emphasize that it can change over time, moreover you only add fields, so it shouldn't create any friction.

I will review it soon, thansk again for your work!

Copy link
Contributor

@plaffitt plaffitt left a comment

Choose a reason for hiding this comment

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

I tried to compile and run your changes on my dev cluster and I'm a bit confused about the values showed in the status of CachedImages. For instance for quay.io/jetstack/cert-manager-cainjector:v1.13.3, it shows a total of 318 (bytes ?) but docker inspect quay.io/jetstack/cert-manager-cainjector:v1.13.3 | jq '.[0].Size' outputs 42445613. So it doesn't really seems to work. Also showing the total size doesn't seems that useful to know the progression, as I would have to download and inspect the image first, or maybe describe the CachedImage in order to compare the status.progress.available vs status.progress.total. It would be much more interesting to have a percentage of completion instead of a total.

PS : conventional commit requires commits message to not start with an uppercase letter

internal/controller/kuik/cachedimage_controller.go Outdated Show resolved Hide resolved
internal/controller/kuik/cachedimage_controller.go Outdated Show resolved Hide resolved
@aDisplayName
Copy link
Author

aDisplayName commented Aug 28, 2024

I tried to compile and run your changes on my dev cluster and I'm a bit confused about the values showed in the status of CachedImages. For instance for quay.io/jetstack/cert-manager-cainjector:v1.13.3, it shows a total of 318 (bytes ?) but docker inspect quay.io/jetstack/cert-manager-cainjector:v1.13.3 | jq '.[0].Size' outputs 42445613. So it doesn't really seems to work. Also showing the total size doesn't seems that useful to know the progression, as I would have to download and inspect the image first, or maybe describe the CachedImage in order to compare the status.progress.available vs status.progress.total. It would be much more interesting to have a percentage of completion instead of a total.

PS : conventional commit requires commits message to not start with an uppercase letter

I'm also puzzled by this. Here is the experiment I've done (https://mcr.microsoft.com/en-us/product/dotnet/sdk/tags)

image reported total / complete value by remote progress update type
mcr.microsoft.com/dotnet/sdk:8.0.401 318 Manifest List
mcr.microsoft.com/dotnet/sdk:8.0.401-alpine3.19 257371821 Manifest List
mcr.microsoft.com/dotnet/sdk:8.0.401-alpine3.19-amd64 257371503 Docker Image
mcr.microsoft.com/dotnet/sdk:8.0.401-alpine3.20 318 Manifest List
mcr.microsoft.com/dotnet/sdk:8.0.401-alpine3.20-amd64 258143681 Docker Image

Not sure what is happening under the hood from remote.Write and remote.WriteIndex

@aDisplayName aDisplayName requested a review from plaffitt August 28, 2024 20:48
@@ -348,16 +348,11 @@ func (r *CachedImageReconciler) cacheImage(cachedImage *kuikv1alpha1.CachedImage
lastWriteComplete := int64(0)
onUpdated := func(update v1.Update) {
needUpdate := false
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the update, but now the code isn't compiling...

Error: internal/controller/kuik/cachedimage_controller.go:350:3: needUpdate declared and not used

@plaffitt
Copy link
Contributor

Not sure what is happening under the hood from remote.Write and remote.WriteIndex

Sorry but I cannot merge something that don't work as expected and I'm a bit worried that you implemented a feature without understanding what you were doing. If you really need this feature please dig a bit more into the functioning of this in order to get a clear understanding of it and make it work as expected.

@aDisplayName
Copy link
Author

aDisplayName commented Nov 30, 2024

I would believe the smaller size is the size of the new manifest, when the original layer has already existed, thus why the registry routine does not count it as total written size.
I'll try to find out if the total size of the image can be retrieved from the manifest, and update the total size accordingly.
But the current value does reflect how much data the kuik operator has transferred from the network, which are more close to the reality when using that to estimate the network traffic performance

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