Move copyfile function to client-go & remove all ocpath related code #503

surajnarwade · 2018-06-04T09:47:03Z

Resolves #443

This PR will remove dependency of oc binary for copyfiles function which copies file to component while odo push

mik-dass · 2018-06-04T12:58:17Z

Works but if I am inside a directory not containing the war file, then push fails

[mdas@localhost odo]$ ./odo update wildly --binary "~/Downloads/sample (1).war"
The component wildly was updated successfully
[mdas@localhost odo]$ ./odo push
Pushing changes to component: wildly
stat /home/mdas/go/src/github.com/redhat-developer/odo/~/Downloads/sample (1).war: no such file or directory

surajnarwade · 2018-06-07T05:02:30Z

@mik-dass , check once again, your path seems to be messed up

/home/mdas/go/src/github.com/redhat-developer/odo/~/Downloads/sample (1).war

mik-dass · 2018-06-07T06:36:59Z

@surajnarwade Yeah sorry my bad, the problem was the ~ is not working in the "", so I guess the user will have to write the full path in that case

mik-dass

It's working for me though just the comments have to be removed

mik-dass · 2018-06-07T09:02:20Z

pkg/occlient/occlient.go

-	data   *string
-	format string
-}
+//func getOcBinary() (string, error) {


@surajnarwade Are these comments required?

mik-dass · 2018-06-07T09:03:03Z

pkg/occlient/occlient.go

-// RsyncPath copies local directory to directory in running Pod.
-func (c *Client) RsyncPath(localPath string, targetPodName string, targetPath string) (string, error) {
-	log.Debugf("Syncing %s to pod %s:%s", localPath, targetPodName, targetPath)
+//// RsyncPath copies local directory to directory in running Pod.


@surajnarwade Are these comments required too?

mik-dass · 2018-06-07T09:03:30Z

pkg/occlient/occlient_test.go

-		})
-	}
-}
+//func TestGetOcBinary(t *testing.T) {


@surajnarwade And here also, are they required?

mik-dass

works for me, LGTM :)

kadel · 2018-06-08T16:05:10Z

pkg/occlient/occlient.go

 	servicecatalogclienset "github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/typed/servicecatalog/v1beta1"
+	appsv1 "github.com/openshift/api/apps/v1"
+	buildv1 "github.com/openshift/api/build/v1"
+	imagev1 "github.com/openshift/api/image/v1"


why have you grouped everything together?
api objects an clients were separated to make logical groups that are easier to search in

it wasn't me :(
it was my stupid IDE and auto go imports
corrected to original one

kadel · 2018-06-08T16:53:53Z

When I run odo watch or odo push after I change something in the files, I'm seeing

E0608 18:53:10.537466   89235 v2.go:163] io: read/write on closed pipe

kadel · 2018-06-08T17:04:05Z

It looks like that when running odo watch it pushes content of the whole directory instead of just changed files.

kadel · 2018-06-08T17:20:02Z

When my repository contains bigger file (100M) it fails

▶ odo -v push
DEBU[0000] Trying to connect to server 192.168.99.100:8443 - %!v(MISSING)
DEBU[0000] Server https://192.168.99.100:8443 is up
DEBU[0000] isLoggedIn err:  <nil>
 output: "developer"
DEBU[0000] No component name passed, assuming current component
Pushing changes to component: python
DEBU[0000] Getting BuildConfig: python
DEBU[0000] Source for component python is file:///Users/tomas/tmp/odo-examples/blog-django-py (local)
DEBU[0000] Waiting for deploymentconfig=python pod
DEBU[0000] Status of python-1-hv54p pod is Running
DEBU[0000] Pod python-1-hv54p is running.
DEBU[0000] Copying file /Users/tomas/tmp/odo-examples/blog-django-py/ to pod python-1-hv54p:/opt/app-root/src
E0608 19:19:36.436075   96528 v2.go:163] io: read/write on closed pipe
DEBU[0002] Error:
unable push files to pod: error while streaming command: command terminated with exit code 2
failed to push component: python

surajnarwade · 2018-06-11T08:36:46Z

For more than 100M file,

error is coming from fsnotify

error watching filesystem for changes: fsnotify queue overflow 
Error while trying to watch /home/snarwade/odo-example/nodejs-ex

Reference:

ABOUT PUSHING the content of WHOLE DIRECTORY

oc rsync has 3 stratergies: rsync, rsync-daemon, or tar

according to documentation,

The tar copy method does not provide the same functionality as rsync. For example, rsync creates the destination directory if it does not exist and will only send files that are different between the source and the destination.

To avoid rsync, I tried to use Tar here, make sense ?

@kadel , can you please give me link of python example that you are trying ?

kadel · 2018-06-11T10:08:36Z

ABOUT PUSHING the content of WHOLE DIRECTORY

We can avoid that, just modify watch so it doesn't push the whole directory, but just a single file that has changed, even better tar all the files that changed in the watch loop

For more than 100M file,
large files are working with old implementation

the error

unable push files to pod: error while streaming command: command terminated with exit code 2

is not from fsnotify library but it looks like it is from the tar command in pod

kadel · 2018-06-11T10:34:04Z

Hmm, it looks like the 100M problem is away. I can't reproduce it anymore.

kadel · 2018-06-13T14:06:59Z

pkg/occlient/occlient.go

+		//watchTar
+		for _, fileName := range changedFiles {
+			if checkFileExist(fileName) {
+				fmt.Println("sending file", fileName)


this probably shouldn't print to stdout. Also, the message is confusing it is not actually sending a file, or is it?

oh that was my debug line :P

kadel · 2018-06-13T14:10:43Z

pkg/occlient/occlient.go

-func (c *Client) RsyncPath(localPath string, targetPodName string, targetPath string) (string, error) {
-	log.Debugf("Syncing %s to pod %s:%s", localPath, targetPodName, targetPath)
+// CopyFile copies single local file to the directory in running Pod.
+func (c *Client) CopyFile(asFile bool, localFile string, targetPodName string, targetPath string, changedFiles []string) error {


We need a comment explaining what the parameters are and fix names.
for example localFile doesn't have to be always a file, it can be a path to a directory when asFiles is false.
changedFiles is also not a good name.
What happens when I set both localFile and changedFiles?

I'll add comments, you can't set both localFile & changedFiles at once, changedFiles will only set if we are using odo watch

kadel · 2018-06-13T14:14:04Z

pkg/occlient/occlient.go

+}
+
+// watchTar will be used to tar files using odo watch
+func watchTar(tw *taro.Writer, fileName string, destFile string) error {


watchTar it looks like you are watching tars :-)
Isn't this function just adding a single file to the taro.Writer?

kadel · 2018-06-13T14:14:51Z

pkg/occlient/occlient.go

@@ -1,15 +1,17 @@
 package occlient

 import (
+	taro "archive/tar"


why is this taro and not just tar?

because there is conflict of name tar, we are using tar from S2I as well,
https://github.com/surajnarwade/ocdev/blob/28c3a6c56db39ad593a0cc79b099eea9d1d24535/pkg/occlient/occlient.go#L24

kadel

It is broken for binary components!!!

When you try to push on a binary component it looks like it is pushing the whole directory.

We need to fix tests first!!!
This should be caught by tests!

kadel · 2018-06-27T13:06:10Z

pkg/component/component.go

@@ -166,13 +166,14 @@ func GetCurrent(client *occlient.Client, applicationName string, projectName str

 // PushLocal push local code to the cluster and trigger build there.
 // asFile indicates if it is a binary component or not


this needs to be updated

kadel · 2018-06-27T13:06:24Z

pkg/component/component.go

-		// We need to make sure that there is a '/' at the end, otherwise rsync will sync files to wrong directory
-		path = fmt.Sprintf("%s/", path)
-	}
+	//if !asFile {


can we delete this?

kadel · 2018-06-27T13:08:18Z

cmd/push.go

-			if sourceType == "binary" {
-				asFile = true
+			var path string
+			if sourceType == "local" {


this looks odd, what will happen with path when sourceType is binary?

shouldn't we call PushLocal where the last argument is slice with one file and that is the binary?

that was my stupid mistake :( and was breaking the binary thing

kadel · 2018-06-27T13:14:34Z

pkg/occlient/occlient.go

-		"--exclude", ".git",
-		"--no-perms",
+// CopyFile copies localFile to the directory in running Pod.
+// if asFile is true, localFile will be file otherwise it is path to a directory


needs to be updated

kadel · 2018-06-27T13:15:36Z

pkg/occlient/occlient.go

+// if asFile is true, localFile will be file otherwise it is path to a directory
+// copyFiles is list of changed files captured during `odo watch`
+// localFile is ignored if copyFiles is set
+func (c *Client) CopyFile(localFile string, targetPodName string, targetPath string, copyFiles []string) error {


localFile should be named localPath as it will indicate that we want to copy the whole directory.

it can be binary as well, so I kept localFile only

no, first argument should always be a directory. If single file copy is required the last argument should be used

kadel · 2018-06-27T13:15:51Z

pkg/occlient/occlient.go

+	if len(copyFiles) != 0 {
+		log.Debugf("Copying files %s to pod %s:%s", copyFiles, targetPodName, targetPath)
+	} else {
+		log.Debugf("Copying file %s to pod %s:%s", localFile, targetPodName, targetPath)


this should say that it is copying directory not files

kadel · 2018-06-27T13:21:29Z

@surajnarwade I tried those commands and they are working for me

Are you sure @mik-dass? Can you please try it again for binary components?

surajnarwade · 2018-06-29T12:33:23Z

@kadel , there was my stupid mistake, Now binary components also works :)

cdrage · 2018-07-10T18:31:11Z

cmd/watch.go

@@ -46,7 +46,7 @@ var watchCmd = &cobra.Command{
 			componentName = args[0]
 		}

-		sourceType, sourcePath, err := component.GetComponentSource(client, componentName, applicationName, projectName)
+		_, sourcePath, err := component.GetComponentSource(client, componentName, applicationName, projectName)


why are we no longer retrieving the sourceType?

cdrage · 2018-07-10T18:50:02Z

pkg/occlient/occlient.go

-	log.Debugf("isLoggedIn err:  %#v \n output: %#v", err, string(output))
+// isLoggedIn checks whether user is logged in or not and returns boolean output
+func (c *Client) isLoggedIn() bool {
+	output, err := c.userClient.Users().Get("~", metav1.GetOptions{})


What's the purpose of the constant "~"? May have to add a comment here (since I don't understand either)

Home maybe?

it gives us current user

cdrage · 2018-07-10T18:51:04Z

pkg/occlient/occlient.go

+	dest := targetPath + "/" + path.Base(localPath)
+	reader, writer := io.Pipe()
+
+	go func() {


Add comment here. This section creates a go routine that creates the tar in the background, correct? Should we add input / any debug information here / progress bar?

cdrage · 2018-07-10T18:51:22Z

pkg/occlient/occlient.go

+		defer writer.Close()
+		err := makeTar(localPath, dest, writer, copyFiles)
+		if err != nil {
+			os.Exit(-1)


Exists with -1... shouldn't we add an error out here / error message?

cdrage · 2018-07-10T18:51:44Z

pkg/occlient/occlient.go

 	}
+	return nil


I don't see a sync check, for this function, should we wait until the go routine is finished before proceeding?

I am confused little bit, here;s the reference: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp.go#L235

@cdrage go routine is writing to Pipe via writer. ExecCMDInContainer is then streaming it to the command's stdin via reader. There is no need to sync anything. ExecCMDInContainer waits for the command to finish, the command will finish only after it receives all the data to stdin.

cdrage · 2018-07-10T18:52:38Z

pkg/occlient/occlient.go

+	var cmdArr []string
+
+	if len(copyFiles) != 0 || string(localPath[len(localPath)-1]) == "/" {
+		cmdArr = []string{"tar", "xf", "-", "-C", targetPath, "--strip", "1"}


we're using tar commands here from the OS. even though it's unlikely, the OS may not have tar (windows users, maybe Mac), could we use the tar library that you imported instead

this tar command runs inside container, so we don't need to worry about it, we are already using tar library here

cdrage · 2018-07-10T18:52:53Z

pkg/occlient/occlient.go

+	return true
+}
+
+// makeTar function is copied from https://github.com/kubernetes/kubernetes/blob/master/pkg/kubectl/cmd/cp.go#L309


cdrage · 2018-07-10T18:53:09Z

pkg/occlient/occlient.go

-func (c *Client) CopyFile(localFile string, targetPodName string, targetPath string) (string, error) {
-	log.Debugf("Copying file %s to pod %s:%s", localFile, targetPodName, targetPath)
+// Tar will be used to tar files using odo watch
+func tar(tw *taro.Writer, fileName string, destFile string) error {


is this function also copied from somewhere else?

Updated with reference link

kadel

during odo watch on binary component I get following message:

DEBU[0009] Copying directory [/Users/tomas/tmp/odo-examples/backend/target/ROOT.war] to pod wildfly-app-1-qfnn5:/opt/app-root/src
Please wait, building component....

It should say "Copying files" not "directory"

Plus it didn't work correctly for some reason.
New war file wasn't used. Component continued to use the old one. Only after I run odo push the new war file was actually used.
Copy either didn't happen during odo watch or it was copied to the wrong place.

kadel · 2018-07-12T12:35:47Z

When doing push on the local component:

[0000] Source for component wildfly is file:///Users/tomas/tmp/odo-examples/backend (local)
DEBU[0000] Waiting for deploymentconfig=wildfly-app pod
DEBU[0000] Status of wildfly-app-1-2gnw7 pod is Running
DEBU[0000] Pod wildfly-app-1-2gnw7 is running.
DEBU[0000] Copying directory [] to pod wildfly-app-1-2gnw7:/opt/app-root/src

empty [] looks weird, something is wrong there

kadel · 2018-07-12T12:59:39Z

pkg/occlient/occlient.go

+// copyFiles is list of changed files captured during `odo watch`
+// localFile is ignored if copyFiles is set
+func (c *Client) CopyFile(localPath string, targetPodName string, targetPath string, copyFiles []string) error {
+	if len(copyFiles) != 0 || string(localPath[len(localPath)-1]) == "/" {


why is it checking if there is / at the end?

surajnarwade · 2018-07-17T09:38:00Z

@kadel , I tried to refactor as per your suggestions. But that's not working since it will need path variable as well since we have to define filename in tar header.

SO I made couple changes in the code and it seems that everything working fine in all 4 scenarios:

--local while odo push
--binary while odo push
--local while odo watch
--binary while odo watch

Examples used:

For binary: https://github.com/wildfly/quickstart/tree/master/helloworld-html5
for local: https://github.com/kadel/nodejs-ex

surajnarwade · 2018-07-18T06:34:43Z

@kadel @cdrage modified as per changes, code and logic is pretty straight forward (thanks to @kadel ) now.
Please review :)

kadel

Comments need to be more descriptive.
Code looks good.

kadel · 2018-07-18T07:15:47Z

pkg/component/component.go

@@ -165,14 +165,10 @@ func GetCurrent(client *occlient.Client, applicationName string, projectName str
 }

 // PushLocal push local code to the cluster and trigger build there.
-// asFile indicates if it is a binary component or not
-func PushLocal(client *occlient.Client, componentName string, applicationName string, path string, asFile bool, out io.Writer) error {
+// files is list of changed files captured during `odo watch` or binary file


The comment should also describe what path is, and how it relates to files

kadel · 2018-07-18T07:18:24Z

pkg/occlient/occlient.go

-func (c *Client) RsyncPath(localPath string, targetPodName string, targetPath string) (string, error) {
-	log.Debugf("Syncing %s to pod %s:%s", localPath, targetPodName, targetPath)
+// CopyFile copies localPath directory or list of files in copyFiles list to the directory in running Pod.
+// copyFiles is list of changed files captured during `odo watch` as well as binary file path


The comment should also describe how localPath relates to copyFiles. This is really important and not obvious.

kadel · 2018-07-18T09:30:10Z

pkg/occlient/occlient.go

 	}
+	return nil


@cdrage go routine is writing to Pipe via writer. ExecCMDInContainer is then streaming it to the command's stdin via reader. There is no need to sync anything. ExecCMDInContainer waits for the command to finish, the command will finish only after it receives all the data to stdin.

cdrage

Other than @kadel 's comment, code LGTM and testing works.

Resolves redhat-developer#443 This PR will remove dependency of oc binary for `copyfiles` function which copies file to component while `odo push`

surajnarwade · 2018-07-24T10:20:14Z

@kadel @cdrage Update with comments

cdrage · 2018-07-24T13:24:13Z

LGTM.

surajnarwade added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 4, 2018

surajnarwade force-pushed the copy-go branch 2 times, most recently from c587bc1 to 8c972d8 Compare June 7, 2018 05:01

surajnarwade force-pushed the copy-go branch from 8c972d8 to add8f0f Compare June 7, 2018 05:27

surajnarwade force-pushed the copy-go branch from add8f0f to f203129 Compare June 7, 2018 08:08

mik-dass reviewed Jun 7, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch 2 times, most recently from 8bef6c8 to 36da172 Compare June 8, 2018 05:11

surajnarwade changed the title ~~[WIP]Move copyfiles function to client-go~~ Move copyfile function to client-go & remove all ocpath related code Jun 8, 2018

surajnarwade removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 8, 2018

surajnarwade requested review from cdrage and kadel June 8, 2018 05:32

mik-dass approved these changes Jun 8, 2018

View reviewed changes

kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 8, 2018

kadel reviewed Jun 8, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch from 36da172 to 0d5d676 Compare June 11, 2018 06:02

surajnarwade removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Jun 11, 2018

surajnarwade force-pushed the copy-go branch 2 times, most recently from 1deedf6 to 28c3a6c Compare June 13, 2018 06:34

kadel reviewed Jun 14, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch from 28c3a6c to 533f5e2 Compare June 14, 2018 08:49

kadel requested changes Jun 27, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch 2 times, most recently from 417b6f6 to ee14e02 Compare June 29, 2018 12:29

surajnarwade force-pushed the copy-go branch from ee14e02 to 721ff2a Compare July 4, 2018 08:31

cdrage suggested changes Jul 10, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch from 721ff2a to f06eb5f Compare July 11, 2018 09:44

kadel requested changes Jul 12, 2018

View reviewed changes

kadel reviewed Jul 12, 2018

View reviewed changes

surajnarwade force-pushed the copy-go branch from f06eb5f to 4303317 Compare July 17, 2018 08:27

surajnarwade force-pushed the copy-go branch 3 times, most recently from 844ddf3 to 2c3e544 Compare July 17, 2018 14:56

kadel reviewed Jul 18, 2018

View reviewed changes

cdrage approved these changes Jul 19, 2018

View reviewed changes

Move copyfiles function to client-go

a237d5d

Resolves redhat-developer#443 This PR will remove dependency of oc binary for `copyfiles` function which copies file to component while `odo push`

surajnarwade force-pushed the copy-go branch from 2c3e544 to a237d5d Compare July 23, 2018 07:03

kadel approved these changes Jul 24, 2018

View reviewed changes

cdrage merged commit e8e223b into redhat-developer:master Jul 24, 2018

snyk-bot mentioned this pull request Aug 13, 2021

[Snyk] Upgrade css-what from 5.0.0 to 5.0.1 adamkeinan/odo#2

Open

This was referenced Sep 30, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#50

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#51

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#52

Open

adamkeinan mentioned this pull request Oct 7, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#53

Open

		@@ -166,13 +166,14 @@ func GetCurrent(client *occlient.Client, applicationName string, projectName str

		// PushLocal push local code to the cluster and trigger build there.
		// asFile indicates if it is a binary component or not

Move copyfile function to client-go & remove all ocpath related code #503

Move copyfile function to client-go & remove all ocpath related code #503

Conversation

surajnarwade commented Jun 4, 2018

mik-dass commented Jun 4, 2018 • edited

surajnarwade commented Jun 7, 2018

mik-dass commented Jun 7, 2018

mik-dass left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

surajnarwade Jun 11, 2018 • edited

Choose a reason for hiding this comment

kadel commented Jun 8, 2018

kadel commented Jun 8, 2018

kadel commented Jun 8, 2018

surajnarwade commented Jun 11, 2018 • edited

For more than 100M file,

ABOUT PUSHING the content of WHOLE DIRECTORY

kadel commented Jun 11, 2018 • edited

kadel commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Jun 27, 2018 • edited

surajnarwade commented Jun 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel left a comment • edited

Choose a reason for hiding this comment

kadel commented Jul 12, 2018

Choose a reason for hiding this comment

surajnarwade commented Jul 17, 2018

surajnarwade commented Jul 18, 2018

kadel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

surajnarwade commented Jul 24, 2018

cdrage commented Jul 24, 2018

mik-dass commented Jun 4, 2018 •

edited

mik-dass left a comment •

edited

surajnarwade Jun 11, 2018 •

edited

surajnarwade commented Jun 11, 2018 •

edited

kadel commented Jun 11, 2018 •

edited

kadel commented Jun 27, 2018 •

edited

kadel left a comment •

edited