File sync for devfiles #2681

johnmcollier · 2020-03-05T02:05:03Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind feature

What does does this PR do / why we need it:
This PR implements file syncing for devfile components, using the kubernetes client library. The push code is based on the existing push/sync code used in classic odo, with only a few small tweaks required.

Overall this PR does the following:

It updates the Devfile deployment code to create a shared emptyDir volume (called odo-projects) and mount it under /projects in all of the devfile components that set mountSources: true
- This is in line with what Che does for mounting project source
Creates a Push() function in the devfile adapters to handle file syncing
- If syncing to the pod for the first time (or if --force is set), it will sync all of files in the source directory using the sync package
- If syncing to an already running component:
  - it syncs only the files that changed (by checking odo-file-index.json)
  - If any files were deleted from the source directory, it deletes from the remote pod.
Creates a DoesComponentExist() function in the devfile adapters
- Returns true if a component with the specified label already exists
- Wrapper for utils.ComponentExists
Moved IsEmpty into the pkg/utils package from pkg/component
- Needed for both odo "classic" and "devfile" components, so needed to move it to a neutral location.
Tests
- Unit tests for new functions
  - Can't really unit test Push(), as it can't be mocked with the fake Kube client (e.g. requires exec'ing to a running pod)
- Integration tests to cover push and sync scenarios

Which issue(s) this PR fixes:

Fixes #2473, #2673

How to test changes / Special notes to the reviewer:

Build my branch and run make
Run make test to verify unit tests pass
Enable experimental mode
Run odo push verify that the deployment starts and files get synced
Change/add/remove files, verify the proper changes get reflected on the container.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

amitkrout · 2020-03-05T12:37:55Z

/retest

johnmcollier · 2020-03-05T16:05:57Z

/retest

codecov · 2020-03-05T18:22:40Z

Codecov Report

Merging #2681 into master will decrease coverage by 0.45%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##           master    #2681      +/-   ##
==========================================
- Coverage   43.92%   43.46%   -0.46%     
==========================================
  Files          86       86              
  Lines        7757     7907     +150     
==========================================
+ Hits         3407     3437      +30     
- Misses       4015     4134     +119     
- Partials      335      336       +1

Impacted Files	Coverage Δ
pkg/testingutil/devfile.go	`0% <0%> (ø)`	⬆️
pkg/devfile/adapters/helper.go	`16.66% <0%> (-6.42%)`	⬇️
pkg/component/component.go	`24.3% <0%> (+0.27%)`	⬆️
pkg/kclient/pods.go	`41.42% <0%> (ø)`	⬆️
pkg/kclient/generators.go	`91.25% <100%> (+0.46%)`	⬆️
...g/devfile/adapters/kubernetes/component/adapter.go	`33.93% <18.51%> (-35.42%)`	⬇️
pkg/util/util.go	`65.73% <53.57%> (-1.03%)`	⬇️
pkg/component/watch.go	`71.33% <0%> (-1.34%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4852786...18ac435. Read the comment docs.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-03-06T02:42:47Z

Updated to include:

--namespace flag on odo push
- Unrelated to file syncing, but was a requirement to implement integration tests
integration tests 🎉
- tests to verify both odo push and syncing works
- Adds two simple devfiles, both nodejs. One single container, one multi container
- Just a few simple tests right now as there isn’t a complete end to end scenario.

johnmcollier · 2020-03-06T04:45:49Z

/retest

amitkrout · 2020-03-06T06:29:33Z

.travis.yml

@@ -98,6 +98,7 @@ jobs:
        - travis_wait make test-cmd-app
        - travis_wait make test-cmd-push
        - travis_wait make test-cmd-project
+        - travis_wait make test-cmd-devfile-push


+1,
FYI - Travis job runs test on 3.11 cluster

Update the name: field accordingly. For example

name: "watch, storage, app, project, push and devfile push command integration tests"

Good catch, updated!

amitkrout · 2020-03-06T06:29:45Z

Makefile

@@ -190,6 +190,11 @@ test-cmd-pref-config:
 test-cmd-push:
 	ginkgo $(GINKGO_FLAGS) -focus="odo push command tests" tests/integration/

+# Run odo push command tests


Run odo push command tests -> Run odo push devfile command tests

Thanks, updated!

amitkrout · 2020-03-06T06:41:53Z

.travis.yml

@@ -98,6 +98,7 @@ jobs:
        - travis_wait make test-cmd-app
        - travis_wait make test-cmd-push
        - travis_wait make test-cmd-project
+        - travis_wait make test-cmd-devfile-push


Update the name: field accordingly. For example

name: "watch, storage, app, project, push and devfile push command integration tests"

amitkrout · 2020-03-06T07:21:45Z

tests/integration/cmd_devfile_push_test.go

+		context = helper.CreateNewContext()
+		currentWorkingDirectory = helper.Getwd()
+		helper.Chdir(context)
+		os.Setenv("ODO_EXPERIMENTAL", "true")


Do not use it like this because odo test spec (It) runs on two ginkgo test node in parallel, so it may cause race condition. To avoid such situation we have a env variable which can control preference path.

You can use

os.Setenv("GLOBALODOCONFIG", filepath.Join(context, "config.yaml"))

and in the spec i mean in the It block you can update the preference value. For example

helper.CmdShouldPass("odo", "preference", "set", "Experimental", "true")

I was wondering about the tests running in parallel and whether or not that would cause problems, good to know! I'll update my PR to include that.

amitkrout · 2020-03-06T07:25:29Z

tests/integration/cmd_devfile_push_test.go

+		helper.DeleteProject(project)
+		helper.DeleteDir(context)
+		helper.Chdir(currentWorkingDirectory)
+		os.Unsetenv("ODO_EXPERIMENTAL")


As per https://github.com/openshift/odo/pull/2681/files#r388746348 use os.Unsetenv("GLOBALODOCONFIG") instead.

amitkrout · 2020-03-06T07:41:43Z

tests/integration/cmd_devfile_push_test.go

+			output := helper.CmdShouldPass("odo", "push", "--devfile", "devfile.yaml", "--namespace", project)
+			Expect(output).To(ContainSubstring("✓  Waiting for component to start"))
+			Expect(output).To(ContainSubstring("✓  Push devfile component"))
+			Expect(output).To(ContainSubstring("✓  Changes successfully pushed to component"))


One assertion that confirms the devfile push is successful would be enough. So Expect(output).To(ContainSubstring("Changes successfully pushed to component")) would be the proper fit imo. Do not to pass character like ✓ in assert as it may behave differently on terminal like PowerShell.

Ah, right good point.

amitkrout · 2020-03-06T07:58:36Z

tests/integration/cmd_devfile_push_test.go

+	// This is run after every Spec (It)
+	var _ = AfterEach(func() {
+		helper.DeleteProject(project)
+		helper.DeleteDir(context)


It will throw error on windows platform. Rearranging the line with the below one will fix the problem

helper.Chdir(currentWorkingDirectory) helper.DeleteDir(context)

Ahh, that's a good point about Windows. Updated.

amitkrout · 2020-03-06T08:02:39Z

tests/integration/cmd_devfile_push_test.go

+			Expect(output).To(ContainSubstring("No file changes detected, skipping build"))
+			helper.ReplaceString(filepath.Join(context, "server.js"), "node listening on", "UPDATED!")
+
+			helper.CmdShouldPass("odo", "push", "--context", context+"/nodejs-ex", "--namespace", project)


Use filepath.Join(context, "nodejs-ex") instead of context+"/nodejs-ex".

Updated, good catch!

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-03-06T17:50:37Z

@amitkrout Thanks for reviewing! I've addressed your review comments in a new commit.

johnmcollier · 2020-03-11T19:07:52Z

@kadel @rajivnathan The changes to support pushing to /projects/<projectName> when exactly one project is defined was pretty straightforward, so I've pushed up a commit that does the following:

Before syncing the source code, odo checks how many projects were defined in the devfile
- If exactly one, it sets the "sync folder" to /projects/<projectName>, ensures /projects/<projectName> exists in the container and pushes the files to that folder
- If either no projects are defined, or multiple projects are defined, odo syncs to /projects

As Rajiv notes, we'll probably need to do some more more work later to properly iron out support for multiple projects, but this is a fine start at the very least.

rajivnathan

Changes look good and verified that it syncs to the project folder now. Thanks for making the changes! Just one comment left

rajivnathan · 2020-03-11T19:29:28Z

pkg/devfile/adapters/kubernetes/component/adapter.go

@@ -246,6 +248,22 @@ func (a Adapter) pushLocal(path string, files []string, delFiles []string, isFor
 	s := log.Spinner("Syncing files to the component")
 	defer s.End(false)

+	// If there's only one project defined in the devfile, sync to `/projects/project-name`, otherwise sync to /projects
+	projects := a.Devfile.Data.GetProjects()
+	fmt.Println(len(projects))


Looks like an errant println slipped through

That was an errant debugging line that slipped through, removed.

Signed-off-by: John Collier <John.J.Collier@ibm.com>

rajivnathan · 2020-03-11T19:38:55Z

/lgtm opened #2705 for follow up discussion on how best to handle multiple projects

rajivnathan

/lgtm

girishramnani · 2020-03-12T07:35:17Z

pkg/devfile/adapters/kubernetes/component/adapter.go

+}
+
+// Push syncs source code from the user's disk to the component
+func (a Adapter) pushLocal(path string, files []string, delFiles []string, isForcePush bool, globExps []string) error {


this function is doing alot of things, lets try to extract certain portions to make unit testing better.
two suggestions would be be -
1 - https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R254-R264 can be extracted into a function
2 - https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R268-R277 can be extracted into a function

Also if the function has more then 5 args please try to create a struct to pass on data or consider moving it as part of the method's struct if it doesn't change through out the lifecycle of the struct.
e.g. path and ignoredFiles fields might not change throughout the life of Adapter so maybe it can be places as part of the Adapter's struct? - this is just an example.

@girishramnani I'm not sure moving those sections into separate functions would make unit testing easier - they both involve exec'ing into an already running container and manipulating the files that exist there, and that can't be mocked with the fake kube client.

we should be able to mock it 🤔 ( I think kube/openshift would have a mock clientset for whatever function exec is using ) and it will definitely make unit testing pushLocal easier because when you would write test for pushLocal you would mock the https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R254-R264 and https://github.com/openshift/odo/pull/2681/files#diff-825a1a88d75e789899c58e89a79eb767R268-R277 functions.

I think what makes it different is that we're interacting with the kube client differently than we do for other resources. We're interacting directly with the rest client, not calling an existing exec function, and I think that doesn't work well with the mock Kube client:

req := c.KubeClient.CoreV1().RESTClient(). Post(). Namespace(c.Namespace). Resource("pods"). Name(podName). SubResource("exec"). VersionedParams(&podExecOptions, scheme.ParameterCodec)

and

exec, err := remotecommand.NewSPDYExecutor(config, "POST", req.URL()) if err != nil { return errors.Wrapf(err, "unable execute command via SPDY") } // initialize the transport of the standard shell streams err = exec.Stream(remotecommand.StreamOptions{ Stdin: stdin, Stdout: stdout, Stderr: stderr, Tty: tty, })

(see https://github.com/openshift/odo/blob/master/pkg/kclient/pods.go#L93)

As a compromise, rather than putting those sections with the exec into separate functions, what about moving everything before the exec into their own functions (maybe getCommandToCreateFolder and getCommandToDelete)? Then we could still write unit tests.

(Note when moving ExecCMDInContainer over from occlient, I couldn't find any references in the existing odo code that had unit tests for it, or unit tests for functions that called it, so other parts of odo have had this problem too.)

(Note when moving ExecCMDInContainer over from occlient, I couldn't find any references in the existing odo code that had unit tests for it, or unit tests for functions that called it, so other parts of odo have had this problem too.)

and that is the reason that we should unit test these as we have the opportunity to re-write/replace this functionality

As a compromise, rather than putting those sections with the exec into separate functions, what about moving everything before the exec into their own functions (maybe getCommandToCreateFolder and getCommandToDelete)? Then we could still write unit tests.

I keep that upto you, I just wanted to break this function down into smaller pieces to make it more unit testable

@girishramnani I'll go with approach for now (and see if there are any other pieces we can break up). I'll also address your comment regarding the number of parameters.

Since unit tests for the ExecCmdInContainer function will require more work / intevestigation / design, I'll address it as part of #2652 (which is also dealing with unit-testing exec as part of sync)

kadel · 2020-03-12T15:11:28Z

▶ odo push
 •  Push devfile component spring-petclinic  ...
 ✓  Checking file changes for pushing [12ms]
 ✓  Waiting for component to start [121ms]
 ✓  Syncing files to the component [2s]
 ✓  Push devfile component spring-petclinic [3s]
 ✓  Changes successfully pushed to component

▶ odo push
 •  Push devfile component spring-petclinic  ...
 ✓  Checking file changes for pushing [8ms]
 ✓  Waiting for component to start [114ms]
 ✗  Syncing files to the component [30s]
 ✗  Failed to start component with name spring-petclinic.
Error: Failed to create the component: Failed to sync to component with name spring-petclinic: error while streaming command: Internal error occurred: error executing command in container: container is not created or running

For the second push, I changed the environment variable in devfile.
I guess it's because of this #2661

johnmcollier · 2020-03-12T15:14:17Z

@kadel Yup, looks like it, since changing the environment variable would have restarted the deployment.

kadel · 2020-03-12T15:19:56Z

@kadel Yup, looks like it, since changing the environment variable would have restarted the deployment.

Push command needs to detect this and wait, till the pod is ready again.
This can be addressed in a separate PR, but we need to fix it.

kadel · 2020-03-12T15:35:03Z

/approve

openshift-ci-robot · 2020-03-12T15:35:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kadel

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [kadel]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier · 2020-03-12T17:09:09Z

Separated out pushLocal into 3 more functions and added unit tests for them. Going to refactor ExecCMDInContainer in a separate PR to make it:

Take fewer params (probably a struct of some kind in the sync package). Will also update pushlocal and push to take fewer params too.
Make it more unit-testable.

I'll refactor both ExecCMDInContainer in kclient and occlient (as the syncclient interface requires clients to implement this function)

Signed-off-by: John Collier <John.J.Collier@ibm.com>

elsony · 2020-03-12T19:19:29Z

Confirmed with @johnmcollier this sync support do incremental sync to only sync the changed file.

rajivnathan

lgtm

openshift-bot · 2020-03-12T21:20:43Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-03-12T21:35:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

johnmcollier added 6 commits March 4, 2020 15:42

Implement devfile syncing on odo push

c6485ff

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Add test for GetRemoteFilesMarkedForDeletion

98f4c23

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Properly retrieve sourcePath

0888d70

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Don't push if not needed to

19f0359

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Remove duplicate WaitAndGetPod

c017d17

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Use odo volume constants

49a620a

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added the size/XL label Mar 5, 2020

openshift-ci-robot requested review from elsony and rajivnathan March 5, 2020 02:06

johnmcollier added 2 commits March 4, 2020 21:24

Fix race condition (oops)

b33cea8

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Fix gosec errors

fe7ba17

Signed-off-by: John Collier <John.J.Collier@ibm.com>

johnmcollier added 2 commits March 5, 2020 20:23

Add namespace flag to odo push

a49eadf

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Implement integration tests for odo push

48999ab

Signed-off-by: John Collier <John.J.Collier@ibm.com>

amitkrout reviewed Mar 6, 2020

View reviewed changes

amitkrout suggested changes Mar 6, 2020

View reviewed changes

openshift-ci-robot assigned amitkrout Mar 6, 2020

amitkrout suggested changes Mar 6, 2020

View reviewed changes

Address review comments on tests

9e593e5

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Mar 6, 2020

openshift-ci-robot added size/XXL and removed size/XL labels Mar 11, 2020

rajivnathan suggested changes Mar 11, 2020

View reviewed changes

Remove debugging line

c0005c0

Signed-off-by: John Collier <John.J.Collier@ibm.com>

rajivnathan mentioned this pull request Mar 11, 2020

Determine how odo should handle multiple projects in a devfile #2705

Closed

2 tasks

rajivnathan approved these changes Mar 11, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 11, 2020

girishramnani reviewed Mar 12, 2020

View reviewed changes

GeekArthur mentioned this pull request Mar 12, 2020

Devfile support for odo create #2699

Merged

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. label Mar 12, 2020

johnmcollier added 2 commits March 12, 2020 13:05

Add more unit tests for functions in pushlocal

a010475

Signed-off-by: John Collier <John.J.Collier@ibm.com>

Add comments

f0fedb9

Signed-off-by: John Collier <John.J.Collier@ibm.com>

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 12, 2020

Fix test on Windows

18ac435

Signed-off-by: John Collier <John.J.Collier@ibm.com>

rajivnathan approved these changes Mar 12, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Mar 12, 2020

openshift-merge-robot merged commit 3a9981f into redhat-developer:master Mar 12, 2020

johnmcollier mentioned this pull request May 28, 2020

Add Support for Devfile V2 #3216

Merged

rm3l added the estimated-size/XXL (60 - ??) Rough sizing for Epics. More than 3 sprints of work for one person label Jun 18, 2023

File sync for devfiles #2681

File sync for devfiles #2681

Conversation

johnmcollier commented Mar 5, 2020 • edited Loading

amitkrout commented Mar 5, 2020

johnmcollier commented Mar 5, 2020

codecov bot commented Mar 5, 2020 • edited Loading

Codecov Report

johnmcollier commented Mar 6, 2020 • edited Loading

johnmcollier commented Mar 6, 2020

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

johnmcollier commented Mar 6, 2020

johnmcollier commented Mar 11, 2020

rajivnathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajivnathan commented Mar 11, 2020

rajivnathan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girishramnani Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

johnmcollier Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

johnmcollier Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

girishramnani Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmcollier Mar 12, 2020 • edited Loading

Choose a reason for hiding this comment

kadel commented Mar 12, 2020

johnmcollier commented Mar 12, 2020 • edited Loading

kadel commented Mar 12, 2020

kadel commented Mar 12, 2020

openshift-ci-robot commented Mar 12, 2020

johnmcollier commented Mar 12, 2020

elsony commented Mar 12, 2020

rajivnathan left a comment

Choose a reason for hiding this comment

openshift-bot commented Mar 12, 2020

openshift-bot commented Mar 12, 2020

johnmcollier commented Mar 5, 2020 •

edited

Loading

codecov bot commented Mar 5, 2020 •

edited

Loading

johnmcollier commented Mar 6, 2020 •

edited

Loading

girishramnani Mar 12, 2020 •

edited

Loading

johnmcollier Mar 12, 2020 •

edited

Loading

johnmcollier Mar 12, 2020 •

edited

Loading

girishramnani Mar 12, 2020 •

edited

Loading

johnmcollier Mar 12, 2020 •

edited

Loading

johnmcollier commented Mar 12, 2020 •

edited

Loading