Proposal: Use depends_on for container startup order

[davidkopp]

This PR tries to resolve #567.

It uses depends_on to determine a better startup order, and it checks if the dependent containers are running before starting the respective container.

There are some open questions that have to be discussed. Therefore, this PR is currently more like a proposal.

• Would this PR be problematic for the current measurement model of the boot phase?
• What is the best way to check if the dependent containers are ready? The check if their state is "running" is actually not sufficient. A health check as proposed in Way to check if containers have booted #423 would be better
• What should be configurable regarding the waiting time for dependent containers?
• The Compose Specification of depends_on states, that it also removes the services in dependency order. I think this is not relevant for GMT. Or is it relevant to have a proper measurement of the removal phase?

[ArneTR]

That PR looks excellent!

The logic looks sound and from just looking at it should do exactly what depends_on advertises.

Having the healthcheck logic also integrated would be a dream, but I would argue it is more reasonable to merge this first and then we keep the other issue open for people interesting to open a PR on this.

I will give it a more detailed look tomorrow and make a proper review.

[ArneTR]

I gave the PR a closer look. The approach is great!

I put some comments and remarks. Please give it a look and share your thoughts.

[davidkopp]

@ArneTR Thanks for the review and your helpful comments! I'm a quite inexperienced (Python) programmer, so it is also good for me for learning 😊

I think I have now made all the requested changes.
In addition, I have added one configuration for the waiting loop:

boot:
  wait_time_dependencies: 20

Does this make sense for you?
To be able to execute the tests, this config has to be added to test-config.yml.

[ArneTR]

The key in the config is aptly named. Can you put it under "measurement" though. I think this makes more sense, as we already have config vars there that influence measurements.

So hierarchy would be measurement -> boot -> wait_time_dependencies

2. Check for long syntax looks good.

3. I believe the cycle check is not doing what it should. It will find if a resource is lisitng itself, but it does not find a real cycle.
   Imagine a setup of 6 containers that always list the next one, and the last then listing the first again. This will lead to a cycle also and is currently not checked.

Let me know if you need a sample yml.

[davidkopp]

The config looks now as follows:

measurement:
   idle-time-start: 10
   idle-time-end: 5
   flow-process-runtime: 3800
   phase-transition-time: 1
   boot:
     wait_time_dependencies: 20
   metric-providers:

Is it the way you had in mind?

The faulty implementation of the cycle detection is a little embarrassing. Should be fixed now.

[ArneTR]

The position of the config option is exactly how I had it in mind, thanks for moving that!

@ribalba can you have a look at the cycle protection implementation and how David implemented it. Am a bit short in time today and tomorrow.
@ribalba Please also create a more complex test case with a bigger hierarchy of the containers with more than one link between containers just be on the safe side.

[ribalba]

Hey, thank you for this great PR again. Arne and me just talked about the code and while this code is fully functional we see that it might be hard to maintain in the future as you carry the whole list order_of_service_names or order_array into every sub iteration. While this is fine for now we see that this might be the source of errors in a few years time as it is harder to comprehend. Also this pattern can be the source of errors when doing more complex things. When calling recursive functions it might be better to only pass in the context of the recursion and then handle the building of the list in the parent. Not something we need to fix right now but just as a remark.

[davidkopp]

Based on this feedback I created an alternative PR with some refactorings:
#593

[ribalba]

As soon as this is merged I will add a more complex test. As I can't write to @davidkopp repo.

[davidkopp]

As soon as this is merged I will add a more complex test. As I can't write to @davidkopp repo.

Is it really the case that you don't have the permissions?

The option "Allowing edits by maintainers" is enabled.

If checked, users with write access to green-coding-berlin/green-metrics-tool can add new commits to your 567-depends-on-order branch.

[ribalba]

This was merged here #593 (comment)