Fix Docker Supervisord Vol Initialization #3129

maysunfaisal · 2020-05-08T17:47:37Z

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

What type of PR is this?
/kind bug
/kind cleanup

What does does this PR do / why we need it:
Docker adapter uses one supervisord volume for all the odo components. However, if the supervisord volume is not initialized properly(for example, when someone presses Ctrl-C during odo push); future odo push is going to fail because supervisord may not have been copied to the volume

This PR:

copies the odo-init image to the supervisord volume when component does not exist
cleans up init container after vol has been initialized
refactor component project vol logic
spins up a supervisord vol per odo component instead of one for all odo components
delete the supervisord vol on odo delete
updated componentExist logic for docker adapter
clean up and new unit tests

Which issue(s) this PR fixes:

Fixes #3034
Partially Addresses #3015

How to test changes / Special notes to the reviewer:

odo push and press Ctrl-C soon
Make sure there is a supervisord volume docker volume ls
odo push should succeed now

codecov · 2020-05-12T16:37:56Z

Codecov Report

Merging #3129 into master will increase coverage by 0.27%.
The diff coverage is 79.90%.

@@            Coverage Diff             @@
##           master    #3129      +/-   ##
==========================================
+ Coverage   45.57%   45.84%   +0.27%     
==========================================
  Files         111      110       -1     
  Lines       10972    10876      -96     
==========================================
- Hits         5000     4986      -14     
+ Misses       5483     5416      -67     
+ Partials      489      474      -15

Impacted Files	Coverage Δ
pkg/devfile/adapters/docker/component/utils.go	`74.19% <72.13%> (+11.47%)`	⬆️
pkg/devfile/adapters/docker/component/adapter.go	`74.78% <73.33%> (-2.72%)`	⬇️
pkg/lclient/containers.go	`72.41% <81.25%> (+1.74%)`	⬆️
pkg/devfile/adapters/docker/utils/utils.go	`82.82% <100.00%> (+3.84%)`	⬆️
pkg/lclient/fakeclient.go	`77.98% <100.00%> (+11.05%)`	⬆️
pkg/sync/sync.go	`45.54% <0.00%> (-2.98%)`	⬇️
pkg/devfile/adapters/common/utils.go	`52.27% <0.00%> (-2.08%)`	⬇️
...g/devfile/adapters/kubernetes/component/adapter.go	`30.80% <0.00%> (-0.85%)`	⬇️
pkg/envinfo/envinfo.go	`60.65% <0.00%> (-0.59%)`	⬇️
... and 14 more

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 d04fb14...b93d172. Read the comment docs.

johnmcollier · 2020-05-12T17:14:25Z

@maysunfaisal Do we need to copy the supervisord binary on every odo push, or just the odo pushes that require container start/restart?

johnmcollier · 2020-05-12T16:59:48Z

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

+
+	return projectVolumeName, nil
+}
+
 // CreateAndInitSupervisordVolume creates the supervisord volume and initializes
 // it with supervisord bootstrap image - assembly files and supervisord binary


Maybe mention that it returns the name of the volume that was created?

johnmcollier · 2020-05-12T17:06:42Z

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

-		projectVolumeName = projectVols[0].Name
-	} else if len(projectVols) > 1 {
-		return errors.Wrapf(err, "Error, multiple source volumes found for component %s", componentName)
+		return errors.Wrapf(err, "Unable to determine the project source volume for component %s", componentName)


We shouldn't capitalize the beginning of error messages

johnmcollier · 2020-05-12T17:10:06Z

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

-		return componentExists, fmt.Errorf("Unable to find source volume for component %s", componentName)
-	} else if len(projectVols) > 1 {
-		return componentExists, errors.Wrapf(err, "Error, multiple source volumes found for component %s", componentName)
+		return componentExists, errors.Wrapf(err, "Unable to determine the project source volume for component %s", componentName)


error message should be lower case

johnmcollier · 2020-05-12T17:17:44Z

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

-	// Get the supervisord volume
-	supervisordLabels := utils.GetSupervisordVolumeLabels()
-	supervisordVolumes, err := a.Client.GetVolumesByLabel(supervisordLabels)
+	a.supervisordVolumeName, err = utils.CreateAndInitSupervisordVolume(a.Client)


Couple questions around always initializing the supervisord volume:

Can we change it so that it's only on odo pushes that require a container start/restart? If the container is running and we just need to sync the code + exec some command, we shouldn't need to re-init the volume, no?

Does running this on every odo push impact performance?

Regd point 2, it takes only 1s when we init volume every time.

addressed in review commit

johnmcollier · 2020-05-12T17:19:11Z

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

@@ -233,6 +234,37 @@ func UpdateComponentWithSupervisord(comp *common.DevfileComponent, runCommand co
 	}
 }

+// CreateAndGetProjectVolume creates a project volume if absent and returns the
+// name of the created project volume
+func CreateAndGetProjectVolume(client lclient.Client, componentName string) (string, error) {


Nit: Maybe we should change the name since we're not always creating a volume? CreateProjectVolumeIfNeeded? Or something similar

Additionally, the only place where this function is called is in docker/adapters/component/utils.go, could we move this function there and also make it unexported?

do you reckon i should move the supervisord stuff and make it unexported too since its only being used in component utils? 🤔

johnmcollier · 2020-05-12T17:27:17Z

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

-	var projectVolumeName string
-	projectVolumeLabels := utils.GetProjectVolumeLabels(componentName)
-	projectVols, err := a.Client.GetVolumesByLabel(projectVolumeLabels)
+	projectVolumeName, err := utils.CreateAndGetProjectVolume(a.Client, componentName)


I'm wondering (and want your thoughts on this), if CreateAndGetProjectVolume returns an already existing volume (potentially with old/outdated/incomplete source code on it) during first-time push, should it overwrite the contents on the volume?

so if its a first time push, i.e.; componentExists is false, we can force sync if a volume already exists. Yeah i like the idea, it sounds like we should do it

it looks like our current master code already covers this.

If its first time push, componentExists = false, we sync. Doesn't matter if we are reusing volume.

if its update component, componentExists = true, we sync

if any of the container from devfile component is missing

if any of the container needs to be updated

if containers does not require update, files changed, file index present, indexer handles it

if containers does not require update, files changed, file index absent, indexer handles it.

So looks like i might not need to do anything.

johnmcollier · 2020-05-12T17:39:36Z

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

+			return "", errors.Wrapf(err, "unable to create supervisord volume for component")
+		}
+	} else {
+		supervisordVolumeName = supervisordVolumes[0].Name


What happens if we overwrite the supervisord binary on the volume while another process is copying it to the container?

I guess, would it be better to have one supervisord volume per component, rather than for all of docker?

I guess, would it be better to have one supervisord volume per component, rather than for all of docker?

+100

Honestly, I was trying to test this, I had the same concerns when writing this PR. I was trying to do simultaneous odo push at the same time. But I couldn't hit the issue.

I guess we can move it a supervisord volume per odo component, to be safe!

addressed in review commit

johnmcollier · 2020-05-12T17:40:12Z

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

 	if err != nil {
 		return err
 	}
 	if log.IsDebug() {
 		s.End(true)
 	}

+	err = client.RemoveContainer(containerID)


What's the reasoning behind removing the supervisord container?

Everytime we init, the container is left behind..

Even though its exits after initializing the volume, you can see a bunch of init containers when you run docker ps -a

kadel · 2020-05-13T13:33:20Z

Instead of initializing every time could we create some kind of indicator that would tell odo if the initialization was completed or not?
For example, we could just add a file to the volume that would contain the timestamp and odo version. This file would be created as the last step after everything has been copied to the volume.
Odo version can be later used to safely update the content of the supervisord volume if it changes in future odo versions.

maysunfaisal · 2020-05-13T22:58:00Z

@kadel we're creating the supervisord volume with supervisord version label.. so if a new version of supervisord was released, a new volume would be created. - https://github.com/openshift/odo/blob/master/pkg/devfile/adapters/docker/utils/utils.go#L165

As for the timestamp idea, yeah i was thinking maybe we can create a flag in the volume that says if supervisord vol was init or not. But, it looks like if we run init supervisord volume everytime, it takes about 1s for the operation to complete. So I am debating if its worth doing it the other way since currently the supervisord init container logic is same for all odo v1, odo v2 kube and odo v2 docker ie; cp from odo-init image container to supervisord volume(in docker) or pvc(in kubernetes)

But maybe @johnmcollier's idea also sounds fair #3129 (comment), we can initialize whenever we need to start the component only, then we wouldn't need to init everytime.

maysunfaisal · 2020-05-15T00:25:42Z

@johnmcollier @kadel PR is up for re-review, thx

johnmcollier

Mostly nits

johnmcollier · 2020-05-19T15:34:23Z

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

-	componentExists := utils.ComponentExists(a.Client, a.ComponentName)
+	componentExists, err := utils.ComponentExists(a.Client, a.Devfile.Data, a.ComponentName)
+	if err != nil {
+		return errors.Wrapf(err, "unable to determine if component exists for component %s", a.ComponentName)


Nit: unable to determine if component %s exists

johnmcollier · 2020-05-19T15:41:43Z

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

-				if typeVal := vol.Labels["type"]; typeVal == "projects" {
-					vols = append(vols, vol)
-				}
+			} else if typeVal := vol.Labels["type"]; typeVal == "projects" {


projects and supervisord should be constants

johnmcollier · 2020-05-19T15:44:39Z

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

@@ -80,7 +56,7 @@ func (a Adapter) createComponent() (err error) {
 			}
 			dockerVolumeMounts = append(dockerVolumeMounts, volMount)
 		}
-		err = a.pullAndStartContainer(dockerVolumeMounts, projectVolumeName, comp)
+		err = a.pullAndStartContainer(dockerVolumeMounts, a.projectVolumeName, comp)


Nit: If pullAndStartContainer is a function that belongs to the adapter, we don't need to pass in a.projectVolumeName, as we can access it directly.

johnmcollier · 2020-05-19T15:46:59Z

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

@@ -138,7 +101,7 @@ func (a Adapter) updateComponent() (componentExists bool, err error) {
 			log.Infof("\nCreating Docker resources for component %s", a.ComponentName)

 			// Container doesn't exist, so need to pull its image (to be safe) and start a new container
-			err = a.pullAndStartContainer(dockerVolumeMounts, projectVolumeName, comp)
+			err = a.pullAndStartContainer(dockerVolumeMounts, a.projectVolumeName, comp)


Nit: We don't need to pass in a.projectVolumeName, since we can access it as a.projectVolumeName from within pullAndStartContainer too.

johnmcollier · 2020-05-19T15:48:28Z

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

@@ -256,7 +219,7 @@ func (a Adapter) startComponent(mounts []mount.Mount, projectVolumeName string,
 	// Create the docker container
 	s := log.Spinner("Starting container for " + *comp.Image)
 	defer s.End(false)
-	err = a.Client.StartContainer(&containerConfig, &hostConfig, nil)
+	_, err = a.Client.StartContainer(&containerConfig, &hostConfig, nil)


Just checking: We don't need to be doing anything with the container ID that's returned here?

Nope, we just return..

We only had to update this func for container id, so that we removed the supervisord init container after starting it, so that it doesn't clutter up docker ps -a

johnmcollier · 2020-05-19T15:50:53Z

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

-	volume            = "vol"
+	supervisordVolume       = "supervisord"
+	projectsVolume          = "projects"
+	projectSourceVolumeName = "odo-project-source"
 )

 // ComponentExists checks if Docker containers labeled with the specified component name exists


Might want to add a bit to the function description explaining the added component exists logic we discussed on slack. Or a component above the if-block

johnmcollier · 2020-05-19T15:56:30Z

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

+// CreateAndInitSupervisordVolumeIfReqd creates the supervisord volume and initializes
+// it with supervisord bootstrap image - assembly files and supervisord binary
+// returns the name of the supervisord volume and an error if present
+func CreateAndInitSupervisordVolumeIfReqd(client lclient.Client, componentName string, componentExists bool) (string, error) {


Should we move CreateAndInitSupervisordVolumeIfReqd to the docker/component/utils.go file?

My thinking is that since CreateAndInitSupervisordVolumeIfReqd is only called from the component package, we probably don't need to keep it in a separate package. Then we can unexport it as well.

johnmcollier · 2020-05-19T16:00:07Z

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


 	return supervisordVolumeName, nil
 }

 // StartBootstrapSupervisordInitContainer pulls the supervisord bootstrap image, mounts the supervisord
 // volume, starts the bootstrap container and initializes the supervisord volume via its entrypoint
-func StartBootstrapSupervisordInitContainer(client lclient.Client, supervisordVolumeName string) error {
-	supervisordLabels := GetSupervisordVolumeLabels()
+func StartBootstrapSupervisordInitContainer(client lclient.Client, componentName, supervisordVolumeName string) error {


Function can be unexported

maysunfaisal · 2020-05-26T22:33:32Z

/retest

maysunfaisal · 2020-05-26T22:34:23Z

@johnmcollier @kadel Hi 👋 , could you pls take a look at this PR and re-review? I've updated the PR and addressed all the review comments!

maysunfaisal · 2020-05-27T14:46:58Z

/retest

johnmcollier

Changes look good and review comments addressed.

/lgtm

kadel · 2020-05-28T07:13:52Z

/approve

maysunfaisal · 2020-06-12T20:48:17Z

/retest

openshift-bot · 2020-06-13T00:53:31Z

/retest