Add oc rsync command

[csrwng]

Refactors existing rsh command to match upstream structure. Adds rsync command that will use 'rsync' if available and on Linux, otherwise will fall back to 'tar'.

In windows, it will use 'tar' and not try rsync.

[csrwng]

@bparees @fabianofranz @smarterclayton

[smarterclayton]

What is this used for?

[csrwng]

To pipe tar output from local to remote exec

[smarterclayton]

And os.Pipe can't do that for you?

[csrwng]

The main reason was convenience. There was less setup required with this library. Not a big deal to go back to os.Pipe.

[smarterclayton]

Would prefer not pulling in a pipe library if os.Pipe() works

On Thu, Sep 24, 2015 at 11:21 AM, Cesar Wong [email protected]
wrote:

In pkg/cmd/cli/cmd/rsync.go
#4787 (comment):

@@ -0,0 +1,381 @@
+package cmd
+
+import (

• "bytes"
• "fmt"
• "io"
• "os"
• "os/exec"
• "path/filepath"
• "strings"

• "github.com/golang/glog"
• "github.com/spf13/cobra"
• "gopkg.in/pipe.v2"

The main reason was convenience. There was less setup required with this
library. Not a big deal to go back to os.Pipe.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4787/files#r40331561.

[csrwng]

@bparees also suggested adding a --delete flag so that we can delete any files in the destination directory that are not in the source directory. Will work on adding that.

[bparees]

Synchronize

[smarterclayton]

Re: --delete - check to see whether rsync already has a name for this. Also... should we be able to pass arguments to rsync?

[liggitt]

not sure I like calling it rsync, then behaving differently than rsync. I'd rather call it just "sync" or something and support a couple sane options

[csrwng]

We already have rsh

[smarterclayton]

Sync is reserved for the controller stuff. Rsync is what it should be
doing - tar is really a fallback id like to go away. I would imagine
extending this to be more rsync like in the future rather than less

On Sep 25, 2015, at 9:01 PM, Jordan Liggitt [email protected]
wrote:

not sure I like calling it rsync, then behaving differently than rsync. I'd
rather call it just "sync" or something and support a couple sane options

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

There is a --delete flag on rsync as well. I would think we can pass flags we don't recognize through to rsync.

[smarterclayton]

In the future potentially. Definitely trying to get the simple path in
place. We can add inotify style watches later

On Sep 26, 2015, at 12:58 AM, Cesar Wong [email protected] wrote:

There is a --delete flag on rsync as well. I would think we can pass flags
we don't recognize through to rsync.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

Updated to use go's tar library for the tar fallback, addressed other comments above. However, there's still some issues with tar on windows. Working on a fix.

[csrwng]

Still requires an update from s2i --> openshift/source-to-image#295
Now using a temporary file for tar instead of a pipe. Writing to a pipeline writer in windows will not block until a reader is available.

[csrwng]

ok, this should be ready for a final review. In windows, simply opening files with standard descriptors is not yet doing the trick for rsync. Needs further investigation and a follow-up pull.

@smarterclayton @bparees

[smarterclayton]

Return

[smarterclayton]

Needs more tests - tar path and rsync path. I'd like rsync path in e2e

[smarterclayton]

I'm not a fan of this check - three execs vs one is expensive. What's the time it adds to the call.

[csrwng]

It takes 100ms on average. We could also check the return code (14 is IPC error) and error output of the local rsync call. If we can determine that it's because rsync is not present in the container, then fallback to tar.

[smarterclayton]

Plus latency, so up to 300ms for long distance. Can't we simply look at
stderr on fallback? I'd prefer to assume rsync in the container and walk
back to something useful. Or if the command fails, only then check rsync.

On Oct 2, 2015, at 1:39 AM, Cesar Wong [email protected] wrote:

In pkg/cmd/cli/cmd/rsync.go
#4787 (comment):

• err := e.Execute(cmd, nil, w, w)
• glog.V(4).Infof("%s", w.String())
• glog.V(4).Infof("error: %v", err)
• return err
  +}
  +
  +func (o *RsyncOptions) checkLocalRsync() error {
• _, err := exec.LookPath("rsync")
• if err != nil {

• glog.V(3).Infof("local rsync not found: %v", err)
  

• }
• return err
  +}
  +
  +func (o *RsyncOptions) checkRemoteRsync() error {
• err := executeWithLogging(o.RemoteExecutor, testRsyncCommand)

It takes 100ms on average. We could also check the return code (14 is IPC
error) and error output of the local rsync call. If we can determine that
it's because rsync is not present in the container, then fallback to tar.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/4787/files#r40979787.

[bparees]

lgtm aside from needing tests.

[csrwng]

@mfojtik thx. I updated the s2i bump to the latest commit and addressed comments.
[testonlyextended][extended:core(gitauth)]

[openshift-bot]

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5577/)

[mfojtik]

LGTM

[csrwng]

ha, just realized I tagged the wrong test!
[testonlyextended][extended:core(rsync)]

@smarterclayton if you get a chance, please take a look as well. Your comments should be addressed.

[openshift-bot]

continuous-integration/openshift-jenkins/testonlyextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5602/)

[openshift-bot]

Evaluated for origin test up to f705724

[openshift-bot]

continuous-integration/openshift-jenkins/testonlyextended Waiting: Determining build queue position

[openshift-bot]

Evaluated for origin testonlyextended up to f705724

[bparees]

lgtm [merge]

if @smarterclayton has further concerns they can be handled in a follow up.

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3554/) (Image: devenv-fedora_2454)

[openshift-bot]

Evaluated for origin merge up to f705724

[smarterclayton]

Can we make sure we have a card to cover the Windows STDIN issues? I want
rsync on windows to not get forgotten.

On Fri, Oct 9, 2015 at 1:16 PM, Ben Parees [email protected] wrote:

lgtm [merge]

if @smarterclayton https://github.com/smarterclayton has further
concerns they can be handled in a follow up.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

Trello card here: https://trello.com/c/bsAQVg0E/718-support-windows-rsync-with-oc-rsync

[smarterclayton]

Do you have a plan to resolve this before 3.1?

On Fri, Oct 9, 2015 at 3:22 PM, Cesar Wong [email protected] wrote:

Trello card here:
https://trello.com/c/bsAQVg0E/718-support-windows-rsync-with-oc-rsync

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

@smarterclayton I did a little bit more digging in this area. The magic that the cygnative code does is that it's using both native win32 library i/o and i/o functions provided by cygwin. It pipes from one to the other. Usually it's either/or for a program. Go by default is using the win32 native calls. It's really not hard to dynamically load the cygwin1.dll and invoke functions on it from go (https://github.com/golang/go/wiki/WindowsDLLs).

However, the issue is what we're willing to require for our code to function. The free version of cwRsync comes only with the 32-bit version of cygwin1.dll. In order for us to load it and use it, our go code needs to be compiled for i386. Alternatively, we could require you to download the 64-bit version of cygwin and add it to your path.

So we have the following options:

1. Write a separate binary that's 32-bit windows that will basically perform the function of cygnative. This will only require you to download the free version of cwRsync and you're set. Pros - less requirement on you as a user. Cons - more headache for us to carry a separate binary.

1b- ish) Simply say the separate binary is the cygnative exe and we tell you where to download it. Pros: can be done now, Cons: really bad experience. Not sure if we can do it legally.

2. Embed the code that pipes from one library to the other in our 64-bit binary and require you to download cygwin 64-bit and add it to your path. Pros: no additional binaries for us to package. Cons: more work for our users, more chance for things to not be setup properly.

3. Make our windows distribution 32-bit only instead of 64-bit? And do something like Add alpha API documentation #2 above. Not sure if this can even be considered.

[smarterclayton]

Why do we need to use cygwin1.dll? Why does #2 require cgywin 64 bit?
Pipes are across process boundaries and shouldn't' be specific to arch of
whatever cgywin.

On Tue, Oct 13, 2015 at 9:21 AM, Cesar Wong [email protected]
wrote:

@smarterclayton https://github.com/smarterclayton I did a little bit
more digging in this area. The magic that the cygnative code does is that
it's using both native win32 library i/o and i/o functions provided by
cygwin. It pipes from one to the other. Usually it's either/or for a
program. Go by default is using the win32 native calls. It's really not
hard to dynamically load the cygwin1.dll and invoke functions on it from go
(https://github.com/golang/go/wiki/WindowsDLLs).

However, the issue is what we're willing to require for our code to
function. The free version of cwRsync comes only with the 32-bit version of
cygwin1.dll. In order for us to load it and use it, our go code needs to be
compiled for i386. Alternatively, we could require you to download the
64-bit version of cygwin and add it to your path.

So we have the following options:

1. Write a separate binary that's 32-bit windows that will basically
   perform the function of cygnative. This will only require you to download
   the free version of cwRsync and you're set. Pros - less requirement on you
   as a user. Cons - more headache for us to carry a separate binary.

1b- ish) Simply say the separate binary is the cygnative exe and we tell
you where to download it. Pros: can be done now, Cons: really bad
experience. Not sure if we can do it legally.

2. Embed the code that pipes from one library to the other in our 64-bit
   binary and require you to download cygwin 64-bit and add it to your path.
   Pros: no additional binaries for us to package. Cons: more work for our
   users, more chance for things to not be setup properly.

3. Make our windows distribution 32-bit only instead of 64-bit? And do
   something like Add alpha API documentation #2 Add alpha API documentation #2 above. Not
   sure if this can even be considered.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

For a go-lang app to use a dll it needs to match the architecture of the dll. Yes, a 64-bit process can spawn a 32-bit and vice versa and they can pipe i/o. That's not the issue. You need the cygwin1.dll because the way that cygwin handles console I/O is very different than native windows. We're trying to pipe I/O from a native cygwin program (cwrsync) to a native windows program.

[smarterclayton]

It's just bytes on a file descriptor? Can you describe the special format
that cygwin is providing IO in?

On Tue, Oct 13, 2015 at 11:15 AM, Cesar Wong [email protected]
wrote:

For a go-lang app to use a dll it needs to match the architecture of the
dll. Yes, a 64-bit process can spawn a 32-bit and vice versa and they can
pipe i/o. That's not the issue. You need the cygwin1.dll because the way
that cygwin handles console I/O is very different than native windows.
We're trying to pipe I/O from a native cygwin program (cwrsync) to a native
windows program.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

I'm researching to give you an intelligent answer :-) I don't believe it is
the format of the I/O, but rather how handle inheritance is setup from the
calling process (the cygwin rsync) to its child process (our oc). Apart
from cygwin source, there's very little to go on in terms of
doc/discussions.

So far I only understand what cygnative is doing to work around the problem
and that's to use the cygwin dll to work with the streams from the cygwin
app.

On Tue, Oct 13, 2015 at 1:45 PM, Clayton Coleman [email protected]
wrote:

It's just bytes on a file descriptor? Can you describe the special format
that cygwin is providing IO in?

On Tue, Oct 13, 2015 at 11:15 AM, Cesar Wong [email protected]
wrote:

For a go-lang app to use a dll it needs to match the architecture of the
dll. Yes, a 64-bit process can spawn a 32-bit and vice versa and they can
pipe i/o. That's not the issue. You need the cygwin1.dll because the way
that cygwin handles console I/O is very different than native windows.
We're trying to pipe I/O from a native cygwin program (cwrsync) to a
native
windows program.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[smarterclayton]

But cgywin rsync is sending it to cygwin native in exactly the same way.
So the way cygwin native reads it has nothing to do with the architecture
of cygwin rsync.

On Tue, Oct 13, 2015 at 2:40 PM, Cesar Wong [email protected]
wrote:

I'm researching to give you an intelligent answer :-) I don't believe it is
the format of the I/O, but rather how handle inheritance is setup from the
calling process (the cygwin rsync) to its child process (our oc). Apart
from cygwin source, there's very little to go on in terms of
doc/discussions.

So far I only understand what cygnative is doing to work around the problem
and that's to use the cygwin dll to work with the streams from the cygwin
app.

On Tue, Oct 13, 2015 at 1:45 PM, Clayton Coleman <[email protected]

wrote:

It's just bytes on a file descriptor? Can you describe the special format
that cygwin is providing IO in?

On Tue, Oct 13, 2015 at 11:15 AM, Cesar Wong [email protected]
wrote:

For a go-lang app to use a dll it needs to match the architecture of
the
dll. Yes, a 64-bit process can spawn a 32-bit and vice versa and they
can
pipe i/o. That's not the issue. You need the cygwin1.dll because the
way
that cygwin handles console I/O is very different than native windows.
We're trying to pipe I/O from a native cygwin program (cwrsync) to a
native
windows program.

—
Reply to this email directly or view it on GitHub
<#4787 (comment)
.

—
Reply to this email directly or view it on GitHub
#4787 (comment).

—
Reply to this email directly or view it on GitHub
#4787 (comment).

[csrwng]

@smarterclayton I chatted with someone from the cygwin list yesterday and he explained that cygwin fd's != Windows file handles. They point to different things in memory that do not interop. It is made worse in this case because cwrsync (the parent process) is creating an anonymous pipe and using its fds to replace standard fds on the child process's file table. Only cygwin knows how to write to it... native windows APIs can't work with its in-memory representation.

[csrwng]

@smarterclayton Just wanted to run an idea by you that wouldn't require us to try to find a cygwin dll. Basically we use rsh to start an rsync daemon on the pod container, use port-forward to forward it to a local port on the windows machine and then tell rsync to use the rsync protocol on the local port. Do you think it would be worth pursuing?

[smarterclayton]

Good thought. Two exec sessions def more expensive than one, but if it
works...

On Oct 16, 2015, at 10:08 AM, Cesar Wong [email protected] wrote:

@smarterclayton https://github.com/smarterclayton Just wanted to run an
idea by you that wouldn't require us to try to find a cygwin dll. Basically
we use rsh to start an rsync daemon on the pod container, use port-forward
to forward it to a local port on the windows machine and then tell rsync to
use the rsync protocol on the local port. Do you think it would be worth
pursuing?

—
Reply to this email directly or view it on GitHub
#4787 (comment).