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

Clean up rsync progress display #1789

Merged
merged 1 commit into from
Jun 16, 2023
Merged

Conversation

frelon
Copy link
Contributor

@frelon frelon commented Jun 16, 2023

The current implementation has a memory leak, use Ticker instead.

Also add --partial, --progress, --human-readable flags to rsync call.

@frelon frelon requested a review from a team as a code owner June 16, 2023 06:56
@frelon frelon force-pushed the fix-upgrade-oom branch from 5472ed6 to e7a9ff0 Compare June 16, 2023 07:00
@frelon frelon enabled auto-merge (squash) June 16, 2023 07:01
The current implementation has a memory leak, use Ticker instead.

Also add `--partial`, `--progress`, `--human-readable` flags to rsync
call.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon force-pushed the fix-upgrade-oom branch from e7a9ff0 to 85de375 Compare June 16, 2023 07:10
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 89.47% and no project coverage change.

Comparison is base (2928cb1) 76.57% compared to head (85de375) 76.58%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1789   +/-   ##
=======================================
  Coverage   76.57%   76.58%           
=======================================
  Files          59       59           
  Lines        5584     5594   +10     
=======================================
+ Hits         4276     4284    +8     
- Misses       1012     1014    +2     
  Partials      296      296           
Impacted Files Coverage Δ
pkg/utils/common.go 80.05% <89.47%> (-0.01%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@frelon frelon merged commit 45aa692 into rancher:main Jun 16, 2023
@frelon frelon deleted the fix-upgrade-oom branch June 16, 2023 07:18
@ldevulder
Copy link
Contributor

@frelon just out of curiosity: how Ticker helps to remove/reduce the memory leak?

@frelon
Copy link
Contributor Author

frelon commented Jun 16, 2023

@ldevulder ran some tests with the different implementation, and it was because I used a break instead of a return in the select, which breaks from the select and causes an infinite loop instead of breaking from the loop:

	stop := make(chan bool)
	go func() {
		for {
			select {
			case <-stop:
				break // <- this should be a return
			case <-time.After(5 * time.Second):
				fmt.Println(message)
			}
		}
	}()

So the Ticker implementation is not needed, but I will keep it since it's already merged!

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.

4 participants