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

Error in "ksync create" on windows #177

Closed
gadavivi opened this Issue May 30, 2018 · 21 comments

Comments

Projects
None yet
3 participants
@gadavivi
Copy link

gadavivi commented May 30, 2018

when i am running:

ksync create --selector=app=app $(pwd)\ksync /code

i am getting this error:

FATA[0000] remote path must be absolute
i thinks because i am running on windows ksync does not recognize /code as a valid path...

so to overcome this i ran this command:

ksync create --selector=app=app $(pwd)\ksync c:\code and it worked!

but now i needed to change the remotepath key manually in the ksync.yaml file to:

...
localpath: C:\ksync
remotepath: **/code** (from c:\code)
...

after the change i saw syncthing logged the change i made in the ksync.yaml file and it's also create a log every time i make a change in the C:\ksync directory, but it doesn't sync the /code directory...
i also tried to do ksync get , but still the folders haven't synced.

any idea how to make the folder to sync, maybe i need to modify the remotepath key differently?

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented May 30, 2018

@gadavivi You'll likely be finding a few of these, we largely rely on the standard lib for cross platform functionality.

Have you tried specifying the paths as non-interpolated strings?

ksync create -lapp=some-app 'local\path\' '/remote/path'

You can just edit the yaml directly, but you won't be getting any validation. You'll also need to restart the syncthing process (stop and rerun watch) to have it pick up the changes.

The validation that is being checked is here: https://github.com/vapor-ware/ksync/blob/master/pkg/input/sync_path.go#L52

@timfallmk timfallmk added the windows label May 30, 2018

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented May 30, 2018

if i am specifying non-interpolated strings i am getting FATA[0000] remote path must be absolute

i tryed to changed the yaml file as below:

apikey: ksync
context: ""
log-level: info
namespace: dok
port: 40322
spec:
- name: loved-tapir
  containername: ""
  pod: ""
  selector: app=app
  namespace: dok
  localpath: C:\ksync
  remotepath: /code
  reload: true
  localreadonly: false
  remotereadonly: false
syncthing-port: 8384

i restarted the syncthing process after changing the .yaml file, but still the sync is not happing (i am not getting an error, everything is running, but the sync isn't happening)

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented May 30, 2018

That's odd. Here's a snippet from my ksync.yaml:

spec:
- name: eternal-gar
  containername: ""
  pod: ""
  selector: app=test
  namespace: default
  localpath: /tmp/ksync4
  remotepath: /code
  reload: true
  localreadonly: false
  remotereadonly: false

I suspect it's a problem with the validator.

@gadavivi Actually I think it might be your shell. I'm not all that familiar with Window's shells but all that check does is: https://golang.org/src/path/filepath/path_unix.go?s=313:341#L2

func IsAbs(path string) bool {
	return strings.HasPrefix(path, "/")
}

Edit #3: Actually, I'm an idiot. The windows isAbs function is different than the *nix one. Since that binary is being complied for Windows, it's attempting to validate both as Windows paths. It's definitely a bug, I'm just not sure what the best way to fix it is.

You've probably gotten yourself in a strange state with the sync. Here's what I would do. Copy the spec section of your yaml, then remove it and save. Start watch and let it cleanup any unused syncthing configs (you may want to go to localhost:8384 and see that there are no matching folders there). Once it has calmed down, stop watch, add the spec section back to the yaml, then start watch. It should notice the change and cleanly add the config to syncthing.

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented May 30, 2018

the error is here

in a windows machine the filepath.IsAbs acts differently...

but i still have no idea why after changing the yaml file manually, the sync doesn't happen...

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented May 30, 2018

@gadavivi Try the steps I outlined above.

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented May 31, 2018

i tried your steps, but still syncthing doesn't sync my remote folder with my container folder...

The weird thing is that it seems like everything is working (i added some logs)

 $ ksync get
       NAME                LOCAL           REMOTE    STATUS            POD           CONTAINER
+-----------------+----------------------+--------+----------+---------------------+-----------+
  unified-oarfish   Users\gadavivi\ksync   /code
                                                    watching   app-c6dfd4655-ffkfm
$ ksync watch
INFO[0000] new pod detected                              pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0000] listening                                     bind=127.0.0.1 port=40322
INFO[0003] updating                                      pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0003] updating                                      pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0003] folder sync running                           pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0007] updating                                      pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0007] update complete                               pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0007] issuing reload                                pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0007] reloaded                                      pod=app-c6dfd4655-ffkfm spec=unified-oarfish
INFO[0077] finished unary call with code OK              grpc.code=OK grpc.method=GetSpecList grpc.service=proto.ksync.Ksync grpc.start_time="2018-05-31T16:10:53+03:00" grpc.time_ns=976400 peer.address="127.0.0.1:51908" span.kind=server system=grpc
INFO[0091] finished unary call with code OK              grpc.code=OK grpc.method=GetSpecList grpc.service=proto.ksync.Ksync grpc.start_time="2018-05-31T16:11:07+03:00" grpc.time_ns=0 peer.address="127.0.0.1:51911" span.kind=server system=grpc
INFO[0098] finished unary call with code OK              grpc.code=OK grpc.method=GetSpecList grpc.service=proto.ksync.Ksync grpc.start_time="2018-05-31T16:11:14+03:00" grpc.time_ns=0 peer.address="127.0.0.1:51912" span.kind=server system=grpc
 $ kubectl exec -it app-c6dfd4655-ffkfm ls
Dockerfile        __pycache__       requirements.txt
Makefile          app.yaml          server.py

but my "C:\Users\gadavivi\ksync" is empty :(
logs.zip

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented May 31, 2018

i did a try to with an ubuntu machine (trough virtualbox) against a kubernetes cluster in aws and everything worked fine, so for sure there is some problems with windows :(

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented May 31, 2018

@gadavivi That's unfortunate. Unfortunately, we haven't been able to do user testing (or any specific testing) for Windows since none of us use it, so you're a bit of guinea pig on this one, sorry about that.

I'll try and get a Windows box going and track down what's going on. For whatever reason, the spec isn't getting processed into syncthing's config.

ping @grampelberg

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented Jun 1, 2018

Ok thanks!
by the way do you have a guess where might be the problem? (i will try to dig in on my spare time)

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 1, 2018

@gadavivi I can't look at the code to verify this, but my guess that assumptions were made in writing the config.xml file that syncthing uses (i.e. it uses some string paths that don't work in Windows).

It could be in the path writing itself, or the serialization in grpc. I suspect the latter, but again this is just a guess. That code is probably in pkg/ksync/syncthing/server.go.

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented Jun 4, 2018

ok,
i did some digging and i found the problem (but i don't know where it exactly in the code):

because i am running on windows the go library that deals with paths translate paths to this\is\my\path, but when running on linux the go paths library are translate correctly to this/is/my/path.
this cause ksync to do a wrong mapping in the syncthing container when running on windows.

I found this problem when in the ksync example i logged to the syncthing container:
/ # ls

\var\lib\docker\overlay2\ec73f389284c39ee852cd9045cd1ffda802e62a9f07c2a4fa1fe6fea78068558\merged\code  root
bin                                                                                                    run
dev                                                                                                    sbin
etc                                                                                                    srv
home                                                                                                   syncthing
lib                                                                                                    sys
media                                                                                                  tmp
mnt                                                                                                    usr
proc                                                                                                   var

as you can see because ksync uses the windows mapping, the syncthing container got a bad interpenetration (see the first path above from the ls command) to the folder that it need to sync (it also created the folder because of course it doesn't exits).
so because that folder does not really map to the correct /code directory in the desired container, my files didn't synced.

P.S: When i created a file in this bad directory, my local machine got sync immediately :), so when this bad path interpenetration will get fix, everything is supposed to work.

may you point where this problem in the code?

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 4, 2018

@gadavivi As expected. This is the problem I outlined earlier. You can see (from our CI config) that we compile for each platform/arch in turn. When go builds for a specific arch, it substitutes any different libraries in at compile time based on the GOOS. This means that in the case of windows, the compiler uses the windows version of the path library. That means that all paths evaluated by the client-side ksync binary will use this library.

So the problem is this: When using a different client platform from that of the cluster both the remote path and the local path get evaluated by the same client binary. We'll maybe need to figure out a way to differentiate.

It's possible we should just do the remote evaluation on the remote, since I don't think you could run Windows in a cluster remotely.

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 4, 2018

@gadavivi Can you test f578a2b to see if that solved the create problem for you?

Windows x64 binary ksync

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented Jun 4, 2018

The create work , but the sync doesn't...
i still have the same problem, that in the synchthing container the config.xml doesn't contain a valid linux path...

i added the xml file from the syncthing container (you will see that the path element is from a windows format)
config.xml.txt

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 5, 2018

Damn. It seems they fixed the "bug" I was hoping to exploit. On to the next idea.

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 5, 2018

@gadavivi From the same run, did the spec (in the ksync config) show the correct paths for local and remote?

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 5, 2018

@gadavivi Ok, try this. Sorry to keep making you try things, but I don't have a way to test Windows binaries ☹️
https://738-105306631-gh.circle-artifacts.com/0/binaries/ksync_windows_amd64.exe

@gadavivi

This comment has been minimized.

Copy link
Author

gadavivi commented Jun 5, 2018

it's ok, i am more then happy to help, also really appreciate your help :)

by the way, the above binary is good, now the sync and the create command are working :)

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Jun 5, 2018

Great! 👍 Thanks for testing. I'll get this merged in.

@timfallmk timfallmk closed this Jun 5, 2018

@timfallmk timfallmk reopened this Jun 5, 2018

@pietervogelaar

This comment has been minimized.

Copy link

pietervogelaar commented Mar 12, 2019

I tried with ksync version 0.3.5, but I'm getting a similar problem.

With single quotes around local path:

$ ksync create --name my-app --namespace default -l app=my-app --reload=false --force C:\Users\pieter\minikube\sources\my-app /var/www/app
time="2019-03-12T17:30:01+01:00" level=fatal msg="local path must be absolute"

Without single quotes around local path:

$ ksync create --name my-app --namespace default -l app=my-app --reload=false --force 'C:\Users\pieter\minikube\sources\my-app' /var/www/app
time="2019-03-12T17:30:12+01:00" level=fatal msg="remote path must be absolute"

Is there some kind of regression?

@timfallmk

This comment has been minimized.

Copy link
Collaborator

timfallmk commented Mar 14, 2019

@pietervogelaar Did you paste those backwards?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.