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

Can't send to SFTP destinations that don't support FSETSTAT #1189

Open
reidsunderland opened this issue Aug 26, 2024 · 8 comments
Open

Can't send to SFTP destinations that don't support FSETSTAT #1189

reidsunderland opened this issue Aug 26, 2024 · 8 comments
Labels
Discussion_Needed developers should discuss this issue. Priority 5 - Defect Missing and/or broken functionality - do not forget Refactor change implementation of existing functionality. work-around a work-around is provided, mitigating the issue.

Comments

@reidsunderland
Copy link
Member

Sending to SFTP destinations that don't support FSETSTAT causes a failure:

[ERROR] sarracenia.flow send could not send /source/filename to inflight=None sftp://[email protected]/ /destination/filename: FSETSTAT unsupported

Looking at the Paramiko code revealed that FSETSTAT is used for chmod, chown, utime and truncate.

This happens even with timeCopy and permCopy set to False, so we shouldn't be calling chmod, chown or utime.

We do call truncate in 2 places:

  1. in readlocal_write of the Transfer class:
    dst.truncate(rw_length)
    • I think this is the case we're encountering, because length is set to 0. This case is for overwriting files that already exist. We want to ensure that remnants of the old file aren't left behind when the new file is smaller than the old one.
  2. in the SFTP transfer class:
    if length != 0: rfp.truncate(self.fpos)

In the SFTP code, there's already logic to determine if a file already exists on the destination, so we could possibly do something with that to not call truncate in the case where the file doesn't already exist.

We could additionally add a try/except around the truncate call(s), but need to be careful to make sure we're not ignoring failures to truncate in cases where truncation really was needed (when a new file overwrites a larger old file).

The workaround is to use binary acceleration for all file transfers to these destinations.

@reidsunderland reidsunderland added work-around a work-around is provided, mitigating the issue. Priority 5 - Defect Missing and/or broken functionality - do not forget Refactor change implementation of existing functionality. Discussion_Needed developers should discuss this issue. labels Aug 26, 2024
@petersilva
Copy link
Contributor

I think we need to wrap all the calls with try/except and emit a warning instead of having the transfer fail:

try:
truncate/chmod/utime... whatever
except Exception as ex:
logger.warning( "it failed : " ex )

we could also have a credentials setting "noFSETSTAT" as a flag on sftp to not even try...

It's a little project.

@petersilva
Copy link
Contributor

should we have timeout(0) implement a disabling of timeouts as a side-quest on this one?

@reidsunderland
Copy link
Member Author

should we have timeout(0) implement a disabling of timeouts as a side-quest on this one?

I vote yes for this, it should be an easy change to have alarm_set to only set the alarm when timeout > 0

@petersilva
Copy link
Contributor

So I have a branch that is running in testing... and I implemented a the warning thing, and the timeout disabling thing. now with just those changes, the sender will work, but it will spit out a warning for every file sent "warning truncate failed"...

So I implemented a nofsetattr as a keyword in the credentials file, and so when you declare your sftp server credentials you add that keyword, and you will no longer get any warnings.

I'm now having second thoughts... should it be a credentials keyword or an option in the sr3_options... if people use ~/.ssh/config to store their sftp auth setup (as they should) then there is no need for an entry in the credentials.config... except this new flag... ugh... that's why I'm wondering if a normal keyword option is the better way to go.

@petersilva
Copy link
Contributor

another option might be something like:

set sarracenia.transfer.sftp.Sftp.nofsetstat on

declare it as an option in the sftp transfer protocol... hmm..
not sure what is preferable.

@petersilva
Copy link
Contributor

I changed it to an option in the config file. Seems more straight-forward.

@reidsunderland
Copy link
Member Author

I prefer the option in the config file, I think it's easier to find and avoids creating credentials.conf entries that we wouldn't need otherwise.

I like set sarracenia.transfer.sftp.Sftp.nofsetstat on. It's a little bit cumbersome but it's an option we shouldn't need very often and makes it clear that it only applies to SFTP.

@petersilva
Copy link
Contributor

need a new branch for a second try.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion_Needed developers should discuss this issue. Priority 5 - Defect Missing and/or broken functionality - do not forget Refactor change implementation of existing functionality. work-around a work-around is provided, mitigating the issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@petersilva @reidsunderland and others