Docker Devfile Component Sync and Execution #2922

maysunfaisal · 2020-04-18T16:18:13Z

What type of PR is this?
/kind feature

What does does this PR do / why we need it:
This PR adds functionality to sync project and exec devfile commands for the docker component.

Specifically, this PR:

syncs the project to the project volume /projects for components that has mount sources true
integrates the docker exec client API for devfile command execution
refactors sync and exec code logic for its consumers
integration tests
unit tests

Which issue(s) this PR fixes:

#2717

How to test changes / Special notes to the reviewer:
odo preference set pushtarget docker
odo create <component>
odo push

johnmcollier

Overall changes look good, left some preliminary comments based on your WIP changes for sync, I'll re-review once exec is in too.

Will you be adding unit tests to this PR?

johnmcollier · 2020-04-20T18:48:20Z

pkg/lclient/containers.go

@@ -83,3 +87,80 @@ func (dc *Client) GetContainerConfig(containerID string) (*container.Config, err

 	return containerJSON.Config, nil
 }
+
+//ExecCMDInContainer executes
+func (dc *Client) ExecCMDInContainer(podName string, containerID string, cmd []string, stdout io.Writer, stderr io.Writer, stdin io.Reader, tty bool) error {


I understand why podName string is an arg here (because of the SyncClient interface), but...

We should consider changing the SyncClient interface to take a function like ExecCMDInComponent instead of ExecCMDinContainer, then the client's function (either local or Kube) determines the container (local) or Pod (kube) that needs to be exec'd into. That way we don't need to pass in a (blank) pod name in the Docker client code.

What do you think?

addressed in 162657e

johnmcollier · 2020-04-20T18:49:21Z

pkg/lclient/containers.go

+		AttachStdout: stdout != nil,
+		AttachStderr: stderr != nil,
+		Cmd:          cmd,
+		// WorkingDir:   "/tmp",


Can this commented out param be removed?

johnmcollier · 2020-04-20T18:58:14Z

pkg/sync/sync.go

 				if err != nil {
 					return err
 				}
 			}
 		}
 	} else {
-		return recursiveTar(filepath.Dir(srcPath), filepath.Base(srcPath), filepath.Dir(destPath), filepath.Base(destPath), tarWriter, globExps)
+		return recursiveTar(filepath.Dir(srcPath), filepath.Base(srcPath), filepath.Dir(destPath), "", tarWriter, globExps)


Why did you change the param for destFile from filepath.Base(destPath) to ""?

So odo 1 currently tar'd local projects files on your laptop/workspace to /projects in the container, but it did in a way that the archvie had a top level dir. For example, if your local laptop current dir was someproject where you ran odo push, then tar would create someproject/<files> and did a tar extract with command cmdArr := []string{"tar", "xf", "-", "-C", targetPath, "--strip", "1"} where strip was basically stripping this top level dir created during tar, so that inside the container, files would be in /projects

This was fine with kube exec api, but docker client api CopyToContainer does not handle this. The files are copied to /projects/someproject/<files> and would need to execute another command to handle this.

So instead of creating the destination tar in filepath.Base(destPath), we just do not specify a dir, and pass in "" for this reason and tar extract command has been updated to remove the strip option in kclient/occlient implementation

johnmcollier · 2020-04-20T19:03:21Z

pkg/sync/adapter.go

+)
+
+// New instantiantes a component adapter
+func New(adapterContext common.AdapterContext, client SyncClient) Adapter {


What's the benefit of an adapter here over just adding functions to a SyncClient instance?

I'm fine with either approach here, just curious.

Just thought it was cleaner, but yeah i can remove the adapter and perhaps make them functions like the other packages

johnmcollier · 2020-04-20T19:11:24Z

pkg/sync/adapter.go

+	if !podChanged && !parameters.ForceBuild && len(parameters.WatchFiles) == 0 && len(parameters.WatchDeletedFiles) == 0 {
+		absIgnoreRules := util.GetAbsGlobExps(parameters.Path, parameters.IgnoredFiles)
+
+		spinner := log.NewStatus(log.GetStdout())


Can you use log.Spinner() for the spinner here? To be consistent with the rest of odo. And pair that with a defer s.End(false)

johnmcollier · 2020-04-20T19:39:16Z

pkg/devfile/adapters/docker/utils/utils.go

+// GetSupervisordVolumeLabels returns the label selectors used to retrieve the supervisord volume
+func GetSupervisordVolumeLabels() map[string]string {
+	supervisordLabels := map[string]string{
+		"name": adaptersCommon.SupervisordVolumeName,


Since there be multiple versions of supervisord, could we make one of the labels the version of supervisord? So that old versions of the supervisord volume aren't accidentally used by odo

johnmcollier · 2020-04-20T19:39:59Z

pkg/devfile/adapters/docker/utils/utils.go

+		adaptersCommon.SupervisordMountPath,
+	}
+
+	s := log.Spinner("Pulling image " + image)


Maybe put this log behind a glog.v(3)? Don't think the user needs to see it by default

johnmcollier · 2020-04-20T19:46:35Z

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

-	)
+	// Get a sync adapter. Check if project files have changed and sync accordingly
+	syncAdapter := sync.New(a.AdapterContext, &a.Client)
+	err = syncAdapter.CheckProjectFiles(parameters, pod.GetName(), containerName, podChanged, componentExists)


Can we change the name of CheckProjectFiles to be something more clear? Something like SyncFiles?

johnmcollier · 2020-04-22T01:00:08Z

pkg/devfile/adapters/common/utils.go

+		// if there are two commands, it is the optional build command and the mandatory run command, set buildRequired to true
+		buildRequired = true
+	} else {
+		return false, fmt.Errorf("error executing devfile commands - there should be at least 1 command or at most 2 commands, currently there are %v commands", len(pushDevfileCommands))


Will this error cause issues when we need to add support devfile init commands (or other commads) later for Docker? Would it be better to just set buildRequired = true if len(pushDevfileCommands) >= 2?

dev init PR changed the logic, which i think is better(glancing at it). That will replace this..

johnmcollier · 2020-04-22T01:06:22Z

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

@@ -36,6 +68,29 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) {
 		return errors.Wrap(err, "unable to create or update component")
 	}

+	containers := utils.GetComponentContainers(a.Client, a.ComponentName)
+	// Find at least one pod with the source volume mounted, error out if none can be found


Change pod to container 😉

johnmcollier · 2020-04-22T01:07:04Z

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

+	// Find at least one pod with the source volume mounted, error out if none can be found
+	containerID, err := getFirstContainerWithSourceVolume(containers)
+	if err != nil {
+		return errors.Wrapf(err, "error while retrieving container for component: %s", a.ComponentName)


Can you change the error message to be a little more clear that we couldn't find any volume that had the source volume mounted on it?

johnmcollier · 2020-04-22T01:08:36Z

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

+	syncAdapter := sync.New(a.AdapterContext, &a.Client)
+	// podName is set to empty string on docker
+	// podChanged is set to false, since docker volume is always present even if container goes down
+	isPushRequired, err := syncAdapter.SyncFiles(parameters, "", containerID, false, componentExists)


SyncFiles takes in a number of params, could we merge them into a single struct like we did for the push params? Then we wouldn't have to explicitly set an empty podName for Docker. Thoughts?

Can you change isPushRequired to something else, since part of what SyncDoes is push the changes to the running container. Maybe something like buildRequired or execRequired?

johnmcollier · 2020-04-22T01:12:11Z

pkg/sync/adapter.go

+	common.AdapterContext
+}
+
+// SyncFiles checks whether files have changed in a project


The function description should be updated, as it doesn't just check if the files changed:

If files are passed in from watch, it syncs those files directly

Otherwise, it determines which files changed and then syncs them to the conainer

johnmcollier · 2020-04-22T01:36:52Z

pkg/devfile/adapters/docker/utils/utils.go

+	return supervisordLabels
+}
+
+// CreateAndInitSupervisordVolume creates the supervisord volume and initializes it with odo init


What do you mean by initializes it with odo init?

updated to say supervisord bootstrap, i meant odo init repo earlier

johnmcollier · 2020-04-22T01:38:46Z

pkg/devfile/adapters/docker/utils/utils.go

+	}
+
+	var s *log.Status
+	if log.IsDebug() {


As an alternative, we can do if glog.V(4) { so that it shows up properly when users do odo push --v <level> (see https://github.com/openshift/odo/blob/master/pkg/lclient/images.go#L24)

Hmm, think will stick with log.IsDebug() since it returns true for any -v > 0, then its not just bounded by -v 4

And we also use log.IsDebug() for showing build output from build command

Fair enough, works for me.

johnmcollier · 2020-04-22T01:39:23Z

pkg/devfile/adapters/docker/utils/utils.go

+	AddVolumeToComp(supervisordVolumeName, adaptersCommon.SupervisordMountPath, &hostConfig)
+
+	// Create the docker container
+	if log.IsDebug() {


See my other comment about using if glog.V(4)

johnmcollier · 2020-04-22T01:40:25Z

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

-	err = a.execDevfile(pushDevfileCommands, componentExists, parameters.Show, pod.GetName(), pod.Spec.Containers)
-	if err != nil {
-		return err
+	if isPushRequired {


See other comment about renaming isPushRequired

johnmcollier · 2020-04-22T01:42:36Z

pkg/exec/devfile.go

+
+	err := ExecuteCommand(client, podName, containerName, cmdArr, show)
+	if err != nil {
+		s.End(false)


Because of the defer s.End(false), we don't need the s.End(false) here.

kadel · 2020-04-22T11:18:53Z

/refresh

cdrage · 2020-04-22T18:03:25Z

@maysunfaisal Need to rebase / resolve conflicts. Looks like a lot of code I was going to review is either going to be overridden or changed due to the conflicts.

maysunfaisal · 2020-04-22T19:12:43Z

@cdrage it would be nice if you could look at the sync code changes, they have changed a teeny bit and they dont have merge conflicts. This affects occlient, kclient and lcient - See review discussion - #2922 (comment)

kadel · 2020-04-23T08:22:34Z

/retest

cdrage · 2020-04-23T13:07:42Z

@cdrage it would be nice if you could look at the sync code changes, they have changed a teeny bit and they dont have merge conflicts. This affects occlient, kclient and lcient - See review discussion - #2922 (comment)

There's still conflicts :( I don't think you pushed the changes?

kadel · 2020-04-23T13:09:32Z

I don't see any integrations test that would verify that the commands were actually executed, and the application was build and started :-/

maysunfaisal · 2020-04-23T14:07:53Z

@cdage it was because the delete PR went in early this morning, was rebased with master until then!

@kadel I am working on the tests today since we didnt cut a driver yesterday

kadel · 2020-04-23T14:54:38Z

/approve

openshift-ci-robot · 2020-04-23T14:54:54Z

[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

openshift-bot · 2020-04-27T17:39:07Z

/retest

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

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

cdrage · 2020-04-27T17:56:14Z

/lgtm

cdrage · 2020-04-27T18:41:08Z

/retest

cdrage · 2020-04-27T19:02:59Z

Tests all pass but we're waiting for benchmarks to complete. So merging manually.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation labels Apr 18, 2020

openshift-ci-robot requested review from adisky and mik-dass April 18, 2020 16:20

maysunfaisal force-pushed the 2717-1 branch from b6afb5d to f09c1ae Compare April 19, 2020 19:17

johnmcollier suggested changes Apr 20, 2020

View reviewed changes

openshift-ci-robot assigned johnmcollier Apr 20, 2020

maysunfaisal force-pushed the 2717-1 branch from f09c1ae to 8a9ea68 Compare April 21, 2020 19:15

maysunfaisal marked this pull request as ready for review April 21, 2020 19:15

maysunfaisal changed the title ~~{WIP} Docker Devfile Component Sync and Execution~~ Docker Devfile Component Sync and Execution Apr 21, 2020

openshift-ci-robot 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 Apr 21, 2020

maysunfaisal force-pushed the 2717-1 branch 2 times, most recently from 4a9f018 to 2aed578 Compare April 21, 2020 23:41

johnmcollier suggested changes Apr 22, 2020

View reviewed changes

maysunfaisal force-pushed the 2717-1 branch from 2aed578 to 99bdf3c Compare April 22, 2020 19:49

kadel mentioned this pull request Apr 23, 2020

Devfile sync and exec support for local docker containers #2717

Closed

4 tasks

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. labels Apr 23, 2020

maysunfaisal force-pushed the 2717-1 branch from 162657e to 91fa905 Compare April 24, 2020 01:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Apr 24, 2020

maysunfaisal force-pushed the 2717-1 branch from d8b2cb1 to cfd7394 Compare April 24, 2020 17:14

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

maysunfaisal added 17 commits April 27, 2020 13:46

Docker Exec API draft

759fea2

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Sync logic for docker components

2cd1d4a

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

docker supervisord init container and vol draft

348b32e

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

docker devfile exec draft

0a250da

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

docker exec first pass

63648de

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

do not devfile push, if no file changes - fix regression

9251553

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Fix existing devfile tests for docker exec

597e8b6

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Docker exec feedback 1 and cleanup

e03f2f1

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Further docker exec cleanup, addressing feedback 2

d5c800a

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

rebase, fix url hostconfig & push log info

935efce

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Fix devfile docker container restart bug

c6e1158

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Fix current tests for devfile docker exec sync

3b5a181

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

Docker integration tests - push exec, sync, GET url

671d44b

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

docker exec unit tests 1 & cleanup

6c67d4f

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

docker exec unit tests 2

7663a37

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

temp commit: sync unit tests 1

7acef00

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

devfile exec,sync feedback 3, cleanup, tests

6339e36

Signed-off-by: Maysun J Faisal <maysun.j.faisal@ibm.com>

maysunfaisal force-pushed the 2717-1 branch from c2bf582 to 6339e36 Compare April 27, 2020 17:47

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

johnmcollier mentioned this pull request Apr 27, 2020

[Devfile] Remove namespace flag from odo commands #2909

Merged

cdrage merged commit 10826cc into redhat-developer:master Apr 27, 2020

This was referenced Apr 27, 2020

Write Tests for Sync package with kclient and occlient #3014

Closed

Devfile Adapter Cleanup/Refactor #3015

Closed

johnmcollier mentioned this pull request Apr 27, 2020

Adding init command capability to devfiles #2907

Merged

maysunfaisal mentioned this pull request May 8, 2020

Do not start Docker component containers as root #3131

Closed

2 tasks

Docker Devfile Component Sync and Execution #2922

Docker Devfile Component Sync and Execution #2922

Conversation

maysunfaisal commented Apr 18, 2020 • edited Loading

johnmcollier left a comment • edited Loading

Choose a reason for hiding this comment

johnmcollier Apr 20, 2020 • edited Loading

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

maysunfaisal Apr 21, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maysunfaisal Apr 22, 2020 • edited Loading

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 Apr 22, 2020

cdrage commented Apr 22, 2020 • edited Loading

maysunfaisal commented Apr 22, 2020

kadel commented Apr 23, 2020

cdrage commented Apr 23, 2020

kadel commented Apr 23, 2020

maysunfaisal commented Apr 23, 2020

kadel commented Apr 23, 2020

openshift-ci-robot commented Apr 23, 2020

openshift-bot commented Apr 27, 2020

cdrage commented Apr 27, 2020

cdrage commented Apr 27, 2020

cdrage commented Apr 27, 2020 • edited Loading

maysunfaisal commented Apr 18, 2020 •

edited

Loading

johnmcollier left a comment •

edited

Loading

johnmcollier Apr 20, 2020 •

edited

Loading

maysunfaisal Apr 21, 2020 •

edited

Loading

maysunfaisal Apr 22, 2020 •

edited

Loading

cdrage commented Apr 22, 2020 •

edited

Loading

cdrage commented Apr 27, 2020 •

edited

Loading