Make start and stop functions blocking #181

NicolasMahe · 2018-06-01T08:25:36Z

Add WaitForContainerStatus func
It's using a time.Sleep to block the thread. Should I start a go routine that will sleep and on the main block with a waitgroup? I think the time.Sleep is enough
Update Daemon start
Update Daemon stop
Update daemon commands
Update Service start
It's using a WaitGroup to wait for all dependencies to start.
It's also using a mutex to edit serviceIDs array that is shared between the goroutines.
Update Service stop
It's using a WaitGroup to wait for all dependencies to stop.
Update service commands

codecov · 2018-06-01T09:18:52Z

Codecov Report

Merging #181 into dev will increase coverage by 0.35%.
The diff coverage is 75.36%.

@@            Coverage Diff             @@
##              dev     #181      +/-   ##
==========================================
+ Coverage   69.16%   69.51%   +0.35%     
==========================================
  Files          58       58              
  Lines        1216     1263      +47     
==========================================
+ Hits          841      878      +37     
- Misses        303      309       +6     
- Partials       72       76       +4

Impacted Files	Coverage Δ
container/service_options.go	`100% <100%> (ø)`	⬆️
daemon/stop.go	`55.55% <100%> (+5.55%)`	⬆️
daemon/start.go	`66.66% <42.85%> (-5.56%)`	⬇️
container/container.go	`79.48% <80%> (+0.17%)`	⬆️
service/start.go	`77.41% <80.95%> (+2.41%)`	⬆️
service/stop.go	`75.75% <83.33%> (+4.32%)`	⬆️
api/core/stop.go	`66.66% <0%> (-4.77%)`	⬇️
api/service/emit_event.go	`100% <0%> (ø)`	⬆️

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 0005c7c...10be2d1. Read the comment docs.

…r unit test.

…ul for unit test.

antho1404 · 2018-06-01T17:09:36Z

container/container.go

+	for {
+		currentStatus, err := ContainerStatus(namespace)
+		if err != nil {
+			return err


I would use some break here in order to make sure that we always exit the for loop, it's a bit easier to test

Because of the new context of the for and the creation of currentStatus, err is also created. So if I use break, I'm pretty sure the err will not be assign to the return err.
If I write, the following code, go says err is shadowed during return on the return.

for { currentStatus, err := ContainerStatus(namespace) if err != nil { return

That's why I think if I use the break, the error will also be shadowed.

So there are 2 solutions.
The first one is to use return err as it's already the case.
The second is to created currentStatus and then assign the value:

var currentStatus StatusType currentStatus, err = ContainerStatus(namespace) if err != nil { break }

@antho1404 What solution do you think it's the best?

I personally use the second solution all the time, we should definitely avoid shadowed variable it's the kind of things that can create bugs easily.

antho1404 · 2018-06-01T17:11:31Z

container/service_options.go

@@ -71,7 +74,12 @@ func (options *ServiceOptions) swarmPorts() (ports []swarm.PortConfig) {
 	return
 }

-func (options *ServiceOptions) swarmMounts() (mounts []mount.Mount) {
+func (options *ServiceOptions) swarmMounts(force bool) (mounts []mount.Mount) {
+	// hack for preventing mount when in CircleCI


A bit hacky, I would add a comment TOFIX just to make sure that we can find this again when the CI allow to mount directories

Comment updated

antho1404 · 2018-06-01T17:18:55Z

container/type.go

@@ -8,3 +12,13 @@ const (
 	STOPPED StatusType = 1
 	RUNNING StatusType = 2
 )
+
+// TimeoutError represents an error of timeout


I think this is not a really good way to have errors I like the fact that we have the details in the error message but i'm sure we can do it better, I would just use something like that directly

errors.New(Timeout reached after " + e.duration.String() + " for ressource " + e.name)

or maybe have a function

func timeoutError(duration, resource) error { return errors.New(Timeout reached after " + e.duration.String() + " for ressource " + e.name) }

or just a constant with generic error message but like that we can compare this error in the tests for example.

const TimeoutError = errors.New("Timeout error on container action")

This is a bit too much and confusing because you don't even return an error

It's the classic way to create new errors in go and be able to compare. Eg with type casting:

_, ok := err.(*TimeoutError)

https://blog.golang.org/error-handling-and-go

On the contrary, I think we should start creating custom error than using the default one with just a custom description. It will be way easier to debug.

In the case of the TimeoutError, we can imagine than the CLI could decide to do a retry and then wait again.

it just feels like a lot of code just for a simple error that could be done in one constant but ok, let's follow the guidelines

antho1404 · 2018-06-01T17:21:29Z

service/start.go

+		wg.Add(1)
+		go func(service *Service, d dependencyDetails, name string, i int) {
+			serviceID, errStart := dependency.Start(service, d, networkID)
+			mutex.Lock()


I'm not sure a mutex is needed here because in this code we have different goroutine that access different index so there is no reason to have conflicts but still is not something bad to do

antho1404 · 2018-06-01T17:24:53Z

service/start.go

+	if err != nil {
+		return
+	}
+	err = container.WaitForContainerStatus(namespace, container.RUNNING, time.Minute) //TODO: be careful with timeout


a minute is really long. If it was in the cli it might be ok but for an internal function I think it's definitely too long

What timeout you think is ok?

Otherwise, we could pass the timeout duration as a parameter of the Start and Stop function.

antho1404 · 2018-06-01T17:25:23Z

service/stop.go

+			if errStop != nil && err == nil {
+				err = errStop
+			}
+			mutex.Unlock()


same remark here about using defer

also here i don't see why there is a mutex, in the map it makes sense because it's not thread safe if two threads want to write on the same index but with the error I don't think there is any problems

Updated to use defer.

The reason for the mutex is the err comparison in the if:

if .... err == nil {

antho1404 · 2018-06-01T17:27:58Z

service/stop.go

 		return
 	}
-	err = container.StopService([]string{namespace, dependencyName})
+	err = container.WaitForContainerStatus(namespace, container.STOPPED, time.Minute) //TODO: be careful with timeout


again probably too long one minute

antho1404 · 2018-06-02T17:22:56Z

There is no update on the changelog we should add a line on how this will change the user experience

…-start-stop # Conflicts: # container/container_test.go

NicolasMahe · 2018-06-04T12:49:00Z

Changelog updated.
Please review my comments of your comments.

antho1404 · 2018-06-04T15:45:57Z

still have some errors from codeclimate

NicolasMahe · 2018-06-05T06:57:32Z

I'm merging even with the codeclimate issues. I don't know how to fix it in a simple way..

NicolasMahe added 6 commits June 1, 2018 14:07

Add function WaitForContainerStatus

36bdb8b

Make daemon start and stop blockable

e865379

Update command daemon start and stop

a74d343

Make service start and stop blocking

ead3fd9

Improve service command

030cd72

Improve command text

67afce5

NicolasMahe self-assigned this Jun 1, 2018

NicolasMahe added 3 commits June 1, 2018 15:32

Increase test timeout

d335451

pull nginx in test

70c33a4

No service mount with circleCI

001719a

NicolasMahe added 5 commits June 1, 2018 16:19

Prevent mount when running in CircleCI

59f01fc

Remove useless print

e79c20e

Merge branch 'container-spec' into blocking-start-stop

173812b

add option to force service options mount even in CircleCI. useful fo…

03eebc8

…r unit test.

Fix: add option to force service options mount even in CircleCI. usef…

38271a2

…ul for unit test.

NicolasMahe mentioned this pull request Jun 1, 2018

[WIP] Make service start/stop blocking #165

Closed

NicolasMahe requested a review from antho1404 June 1, 2018 09:46

NicolasMahe and others added 2 commits June 1, 2018 17:04

Merge branch 'container-spec' into blocking-start-stop

132b66f

Merge branch 'dev' into blocking-start-stop

116687d

antho1404 reviewed Jun 1, 2018

View reviewed changes

antho1404 added enhancement New feature or request application / service labels Jun 4, 2018

NicolasMahe added 4 commits June 4, 2018 19:16

Update comment on the mount hack for CircleCI

704a0cc

Use defer for mutex.Unlock() on service start and stop

60ea1b4

Update changelog

d22f278

Merge commit '0005c7ca56184bf9414cc437ef40fd967a5c0b6a' into blocking…

9676ed9

…-start-stop # Conflicts: # container/container_test.go

NicolasMahe added 6 commits June 5, 2018 10:40

use defer for waitGroup done in service

e2adc19

Remove timeout from container.WaitForContainerStatus function

61f2da0

Remove return in WaitForContainerStatus

5e404ae

Reorganize daemon start to fix CodeClimate error

50a6576

Refacto Spinner in CLI to fix CodeClimate

7f78f01

Fix daemon start test

10be2d1

NicolasMahe merged commit 20fbb11 into dev Jun 5, 2018

NicolasMahe deleted the blocking-start-stop branch June 5, 2018 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make start and stop functions blocking #181

Make start and stop functions blocking #181

NicolasMahe commented Jun 1, 2018 •

edited

Loading

codecov bot commented Jun 1, 2018 •

edited

Loading

antho1404 Jun 1, 2018

NicolasMahe Jun 4, 2018 •

edited

Loading

antho1404 Jun 4, 2018

antho1404 Jun 1, 2018

NicolasMahe Jun 4, 2018

antho1404 Jun 1, 2018

NicolasMahe Jun 4, 2018

NicolasMahe Jun 4, 2018 •

edited

Loading

antho1404 Jun 4, 2018

antho1404 Jun 1, 2018

antho1404 Jun 1, 2018

NicolasMahe Jun 4, 2018

antho1404 Jun 1, 2018

antho1404 Jun 1, 2018

NicolasMahe Jun 4, 2018

NicolasMahe Jun 4, 2018

antho1404 Jun 1, 2018

antho1404 commented Jun 2, 2018

NicolasMahe commented Jun 4, 2018

antho1404 commented Jun 4, 2018

NicolasMahe commented Jun 5, 2018

Make start and stop functions blocking #181

Make start and stop functions blocking #181

Conversation

NicolasMahe commented Jun 1, 2018 • edited Loading

codecov bot commented Jun 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

NicolasMahe Jun 4, 2018 • 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

NicolasMahe Jun 4, 2018 • 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

antho1404 commented Jun 2, 2018

NicolasMahe commented Jun 4, 2018

antho1404 commented Jun 4, 2018

NicolasMahe commented Jun 5, 2018

NicolasMahe commented Jun 1, 2018 •

edited

Loading

codecov bot commented Jun 1, 2018 •

edited

Loading

NicolasMahe Jun 4, 2018 •

edited

Loading

NicolasMahe Jun 4, 2018 •

edited

Loading