Skip to content

attempt to handle rose rsync can_pull errors better#2892

Merged
wxtim merged 5 commits intometomi:2.6.xfrom
wxtim:fix.more-logging-of-file-handlers
Oct 23, 2025
Merged

attempt to handle rose rsync can_pull errors better#2892
wxtim merged 5 commits intometomi:2.6.xfrom
wxtim:fix.more-logging-of-file-handlers

Conversation

@wxtim
Copy link
Contributor

@wxtim wxtim commented Apr 24, 2025

Fixes problem not recorded in an issue:

Issue

Rose has a routine to work out which file_loc_handler should be used based on the content of the source= field. However, the function called by the rsync loc handler will attempt to run

# loc handler = foo:bar
ssh -n foo test -e bar

As part of the method used to decide if the source is an Rsync handler. This fails in a difficult to debug way, as it's unclear whether the rsync check routine failed because the host was unreachable, or the file didn't exist, or the source was not meant to be a location reached by rsync, or was just garbage.

This PR

  • Ensures that Rsync is always the last handler tried.
  • Allows the rsync handler to raise more informative errors.
  • Give the user more useful detail if the loc handler cannot be identified because this ssh check fails.

@wxtim wxtim self-assigned this Apr 24, 2025
@oliver-sanders
Copy link
Member

Did you manage to work out why the command stderr was not logged?

@wxtim
Copy link
Contributor Author

wxtim commented Apr 25, 2025

Did you manage to work out why the command stderr was not logged?

Because no one made Rose log it.

When I was testing it in front of you and there was no output, that was because test -e nonexistant doesn't return any stdout/err.

On consideration, I should probably have a specific error message for this outcome!

@oliver-sanders
Copy link
Member

oliver-sanders commented Apr 28, 2025

Because no one made Rose log it.

I would expect there to be some standard way of doing this (else we would have to do this for every single command individually).

From a quick code skim, it looks like this is the centralised logic:

class RosePopenError(Exception):
"""An error raised when a shell command call fails."""
def __init__(self, command, ret_code, stdout, stderr, stdin=None):
self.command = command
self.ret_code = ret_code
self.stdout = stdout
self.stderr = stderr
self.stdin = stdin
Exception.__init__(self, command, ret_code, stdout, stderr)
def __str__(self):
cmd_str = str(RosePopenEvent(self.command, self.stdin))
tail = ""
if self.stderr:
tail = ", stderr=\n%s" % self.stderr
return "%s # return-code=%d%s" % (cmd_str, self.ret_code, tail)

So, I think the issue here, is that this command swallows the RosePopenError rather than allowing it to bubble up and be reported as intended?

@wxtim wxtim force-pushed the fix.more-logging-of-file-handlers branch from 54fa62e to 6609cfb Compare May 1, 2025 08:05
@wxtim wxtim requested a review from MetRonnie May 8, 2025 10:36
@wxtim wxtim closed this May 8, 2025
@wxtim wxtim reopened this May 8, 2025
@wxtim wxtim force-pushed the fix.more-logging-of-file-handlers branch 2 times, most recently from 4b72a72 to 52e10d5 Compare May 8, 2025 12:53
@wxtim wxtim requested a review from MetRonnie May 9, 2025 08:45
@wxtim wxtim force-pushed the fix.more-logging-of-file-handlers branch 2 times, most recently from ff9dba2 to 29067ef Compare May 12, 2025 10:03
@MetRonnie MetRonnie removed their request for review July 14, 2025 10:21
wxtim added 2 commits July 22, 2025 10:31
handle no file error with useful error message

Apply suggestions from code review

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>

Response to review

move error messaging into error class
@wxtim wxtim force-pushed the fix.more-logging-of-file-handlers branch from c1f1a7a to a9eac4a Compare July 22, 2025 09:32
@wxtim wxtim requested a review from MetRonnie July 22, 2025 10:21
@wxtim wxtim requested a review from MetRonnie July 28, 2025 10:21
@wxtim wxtim requested a review from MetRonnie August 14, 2025 14:07
@wxtim
Copy link
Contributor Author

wxtim commented Oct 16, 2025

@oliver-sanders Poke

@oliver-sanders
Copy link
Member

You may want to consider changing to a bugfix branch.

Plz squash merge.

@wxtim wxtim changed the base branch from master to 2.6.x October 23, 2025 12:28
@wxtim wxtim merged commit 9cfcccd into metomi:2.6.x Oct 23, 2025
16 of 17 checks passed
@wxtim wxtim deleted the fix.more-logging-of-file-handlers branch October 23, 2025 12:28
@oliver-sanders oliver-sanders added this to the 2.5.x milestone Oct 23, 2025
@oliver-sanders oliver-sanders modified the milestones: 2.5.x, 2.6.x Nov 3, 2025
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.

3 participants