Skip to content

Report as debug log, the time spent waiting for resources#30689

Merged
mattfarina merged 2 commits intohelm:dev-v3from
benoittgt:report-time-waited-v3
Mar 21, 2025
Merged

Report as debug log, the time spent waiting for resources#30689
mattfarina merged 2 commits intohelm:dev-v3from
benoittgt:report-time-waited-v3

Conversation

@benoittgt
Copy link
Copy Markdown
Contributor

@benoittgt benoittgt commented Mar 19, 2025

What this PR does / why we need it:

When trying to improve the speed of a helm upgrade. I think it could be great to see how much time the helm upgrade spent waiting for ressources when you use --wait. So I thought about adding a small log point with the elapsed time in seconds.

Output example

creating 2 resource(s)
preparing upgrade for podinfo
performing update for podinfo
creating upgraded release for podinfo
checking 2 resources for changes
Looks like there are no changes for Service \"podinfo\"
Patch Deployment \"podinfo\" in namespace default
waiting for release podinfo resources (created: 0 updated: 2  deleted: 0)
beginning wait for 2 resources with timeout of 15m0s
Deployment is not ready: default/podinfo. 0 out of 9 expected pods are ready
Deployment is not ready: default/podinfo. 0 out of 9 expected pods are ready
wait for ressources succeeded within 4s

If approved, I can backport it to main/v4.

Similar work: #30685

Special notes for your reviewer:

If applicable:

  • this PR contains user facing changes (the docs needed label should be applied if so)
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 19, 2025
Signed-off-by: Benoit Tigeot <benoit.tigeot@lifen.fr>
@benoittgt benoittgt force-pushed the report-time-waited-v3 branch from 68632cf to 422c58e Compare March 20, 2025 20:44
@TerryHowe
Copy link
Copy Markdown
Contributor

@robertsirc want to look at this since he has some log work going on.

@robertsirc
Copy link
Copy Markdown
Member

So we are moving from log to slog, if we are going to change this I'd rather us convert this over to slog.

Copy link
Copy Markdown
Member

@robertsirc robertsirc left a comment

Choose a reason for hiding this comment

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

thank you for your pull request this will need some attention before we can approve.

@benoittgt
Copy link
Copy Markdown
Contributor Author

Thanks for the review. slog change are for main and helm v4 if I read #30603 ?

My change was more for v3 but I open a new Pr on main that change to slog for this function. #30696

Copy link
Copy Markdown
Member

@robertsirc robertsirc left a comment

Choose a reason for hiding this comment

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

LGTM

@mattfarina mattfarina added this to the 3.18.0 milestone Mar 21, 2025
@mattfarina mattfarina merged commit 78a052c into helm:dev-v3 Mar 21, 2025
5 checks passed
@benoittgt benoittgt mentioned this pull request Mar 25, 2025
3 tasks
@benoittgt
Copy link
Copy Markdown
Contributor Author

@robertsirc is it possible to include it in the next patch version v3.17.4 ?

@robertsirc
Copy link
Copy Markdown
Member

I think it can make the next release which is 3.18.0 is the next minor release and will be on May 14, 2025

@benoittgt benoittgt deleted the report-time-waited-v3 branch April 15, 2025 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants