Print the progress for container Image Pull during cluster-up #1425
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-bot
added
the
dco-signoff: no
Reviewer's Guide by SourceryThis PR enhances the No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Thanks for your pull request. Before we can look at it, you'll need to add a 'DCO signoff' to your commits. 📝 Please follow instructions in the contributing guide to update your commits with the DCO Full details of the Developer Certificate of Origin can be found at developercertificate.org. The list of commits missing DCO signoff:
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-sigs/prow repository. I understand the commands that are listed here. |
|
Hi @harshitgupta1337. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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-sigs/prow repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @harshitgupta1337 - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider extracting the logic for parsing and reporting progress into a separate function for better readability.
- It might be helpful to add some comments explaining the logic behind the 10% progress update condition.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return err | ||
| } | ||
| fmt.Print(".") | ||
|
|
||
| lastStatus, ok := lastReportedState[pullStatus.Id] |
There was a problem hiding this comment.
issue (complexity): Consider extracting the pull status reporting logic into a helper function to reduce nesting and improve clarity of the code.
Consider extracting the pull‑status reporting logic into a helper function to reduce nesting and improve clarity. For example, you could create a function like this:
```go
func shouldReport(current, last PullStatus) bool {
if current.Status != last.Status {
return true
}
lastProgress := float64(last.ProgressDetail.Current) / float64(last.ProgressDetail.Total)
currProgress := float64(current.ProgressDetail.Current) / float64(current.ProgressDetail.Total)
return currProgress-lastProgress >= 0.1
}Then your scanner loop in the non‑terminal branch can become:
scanner := bufio.NewScanner(progressReader)
// Map to store the last state printed for each container layer.
lastReportedState := make(map[string]PullStatus)
for scanner.Scan() {
line := scanner.Text()
if line == "" {
continue
}
pullStatus := &PullStatus{}
if err := parseAndCheckForError(line, pullStatus); err != nil {
return err
}
toReport := false
if last, ok := lastReportedState[pullStatus.Id]; ok {
toReport = shouldReport(*pullStatus, last)
} else {
toReport = true
}
if toReport {
fmt.Fprintf(writer, "%s\t%s\t%s\n", pullStatus.Id, pullStatus.Status, pullStatus.Progress)
lastReportedState[pullStatus.Id] = *pullStatus
}
}This refactoring isolates the decision logic, reduces nested conditionals, and improves overall readability without changing the functionality.
|
/cc @iholder101 |
|
Thank you very much @harshitgupta1337 for trying to address this annoying issue!
It's unfortunate that we can't just redirect the output to the terminal for it to print in a nicely fashion. However, I'm not sure if or how to do so. @dhiller what are your thoughts on this? is this better than the dots being printed? is there a way to better redirect to terminal? |
What this PR does / why we need it:
Currently, when the
cluster-provisionscripts download a container image (such as thequay.io/kubevirtci/k8s-1.31image), the exact progress of the download is not displayed. This PR addresses this limitation by printing the image pull progress in a manner finds a middle-ground between high verbosity (passing the entirety of the docker image pull output to a non-terminal stdout) and not tracking image download/extraction progress (only showing state changes, e.g.,Downloading --> Verifying Checksum).In the proposed approach, we report the pull progress for each image layer independently. Image pull status for a given layer is printed only when either one of the following 2 conditions is met.
Downloadingstate toVerifying Checksumstate.10%progress in the current state. For example, if a layer is currently inDownloadingstate and it has downloaded more than 10% of the layer since the last time its progress was printed, then the progress of that layer will be printed.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes kubevirt/kubevirt#14242
Special notes for your reviewer:
Checklist
This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.
Release note: