Is rsync retry logic correct?

[alex-vmw]

Describe the bug For me retries means all tries after the initial try. That said, the current logic of until [ "$n" -ge "$retries" ] in https://github.com/utkuozdemir/pv-migrate/blob/master/helm/pv-migrate/templates/rsync/job.yaml seems flawed, since if n=0 and retries=0 the initial rsync run will never execute.

To Reproduce Steps to reproduce the behavior:

1. Run pv-migrate with --helm-set rsync.maxRetries=0
2. Observe a failure. Looking at rsync pod logs will indicate that initial rsync run never executed.

Expected behavior For me retries means all tries after the initial try, so the expected behavior is to always run rsync once even when --helm-set rsync.maxRetries=0 is specified.

Console output

+ n=0
+ rc=1
+ retries=0
+ period=5
+ basename /tmp/id_ed25519
+ privateKeyFilename=id_ed25519
+ mkdir -p /root/.ssh
+ chmod 700 /root/.ssh
+ cp -v /tmp/id_ed25519 /root/.ssh/
'/tmp/id_ed25519' -> '/root/.ssh/id_ed25519'
+ chmod 400 /root/.ssh/id_ed25519
+ '[' 0 -ge 0 ]
+ '[' 1 -ne 0 ]
+ echo 'rsync job failed after 0 retries'
rsync job failed after 0 retries
+ exit 1

Version

• pv-migrate version and architecture [v1.6.1 - darwin_x86_64]
• Installation method [homebrew]

Additional context I think we should change until [ "$n" -ge "$retries" ] to while [ "$n" -le "$retries" ] that way rsync will always execute once even when --helm-set rsync.maxRetries=0 is specified.

[utkuozdemir]

I agree, bad naming. It should have been attempts probably. But since 0 attempts doesn't make sense, and not to break the compatibility, I agree that we should change it the way you suggest - essentially to do...while semantics.