refactor dependency engine to reuse and fit in reconcile loop

[hongchaodeng]

Per discussion with @wonderflow , the previous DAGManager loop has unexpected corner cases like handling orphan-ed AppConfigs and dealing with Workload's traits and scopes.

A better implementation solution would be to base on existing reconcile loop and find the dependencies in each reconcile. If dependency is not satisfied, it would requeue and retry later.

[wonderflow]

I hope to see some test cases to at least cover below cases:

1. component with traits and scopes won't be create if dependency conditions not meet requirement. AppConfig.status will indicate this is processing and what components are pending on.
2. component with traits and scopes all created after dependency conditions meet.
3. trait who need input from component
4. component who need input from trait

[ryanzhang-oss]

IIRC, this code does not support the case of the following scenario
workloadA -> workloadA's trait

and we need more unit tests

[ryanzhang-oss]

Can scope have dependency too?

[hongchaodeng]

Currently there is no plan to support dependency w.r.t. scope. Only workloads and traits.
This is because I do not want to bloat the feature to cover scenarios that do not have real world use cases.

[ryanzhang-oss]

Please add a UT for this function

[ryanzhang-oss]

do we assume it's a primitive type like string or int?

[hongchaodeng]

Currently only string type is supported. This should fix all the use cases as mentioned in the design doc.
We don't see any use case for int yet.

[hongchaodeng]

For complex objects, we are going to provide more high level functions like AutoBinding to fill the gap.

[hongchaodeng]

@wonderflow The AppConfig now will show the dependency status:

  status:
    dependency:
      unsatisfied:
      - from:
          apiVersion: example.com/v1
          kind: Foo
          name: source
          fieldPath: status.key
        to:
          apiVersion: example.com/v1
          kind: Foo
          name: sink
          fieldPaths:
          - spec.key

[hongchaodeng]

The reason why previous implementation just take the whole Workload and its Traits when dependency not met is just to show the prototype.

Now the current implementation has changed that to mark each Workload and Trait whether they are "ready" to apply. This is a more robust solution and easier to debug along with dependency status.

[hongchaodeng]

Added tests:

--- PASS: TestMatchValue (0.00s)
    --- PASS: TestMatchValue/No_conditions_with_nonempty_value_should_match (0.00s)
    --- PASS: TestMatchValue/eq_condition_with_different_value_should_not_match (0.00s)
    --- PASS: TestMatchValue/notEmpty_condition_with_nonempty_value_should_match (0.00s)
    --- PASS: TestMatchValue/eq_condition_with_different_value_from_FieldPath_should_not_match (0.00s)
    --- PASS: TestMatchValue/No_conditions_with_empty_value_should_not_match (0.00s)
    --- PASS: TestMatchValue/eq_condition_with_same_value_should_match (0.00s)
    --- PASS: TestMatchValue/notEq_condition_with_different_value_should_match (0.00s)
    --- PASS: TestMatchValue/notEq_condition_with_same_value_should_not_match (0.00s)
    --- PASS: TestMatchValue/notEmpty_condition_with_empty_value_should_not_match (0.00s)
    --- PASS: TestMatchValue/eq_condition_with_same_value_from_FieldPath_should_match (0.00s)

--- PASS: TestDependency (0.01s)
    --- PASS: TestDependency/DataOutputName_doesn't_exist (0.00s)
    --- PASS: TestDependency/Workload_depends_on_another_Workload_that's_ready (0.00s)
    --- PASS: TestDependency/Workload_depends_on_a_Trait_that's_ready (0.00s)
    --- PASS: TestDependency/Trait_depends_on_a_Workload_that's_ready (0.00s)
    --- PASS: TestDependency/Trait_depends_on_another_unreadyTrait_that's_unready (0.00s)
    --- PASS: TestDependency/Trait_depends_on_another_unreadyTrait_that's_ready (0.00s)
    --- PASS: TestDependency/Workload_depends_on_another_Workload_that's_unready (0.00s)
    --- PASS: TestDependency/Workload_depends_on_a_Trait_that's_unready (0.00s)
    --- PASS: TestDependency/Trait_depends_on_a_Workload_that's_unready (0.00s)

[hongchaodeng]

Since the initial submission the following comments has been addressed:

• Add status to indicate the progress of dependency waiting
• Handle unreadiness of Workload/Trait resources individually instead of bundling them all together.
• Add UT to cover all major cases as shown above

@My-pleasure is trying to write e2e tests to cover dependency user story, which will be up for review early next week. I would like to merge this PR first instead of coupling his commits here.

[wonderflow]

Generally LGTM, some comments hope to be fixed. Thanks @hongchaodeng, really great work!

ping @ryanzhang-oss please also help review it

[wonderflow]

This should not exist at initial time.

[hongchaodeng]

Let me comment it out. Having this without typing every time is very convenient.

[wonderflow]

What if the outside Unready is true while the Unready field here is false? Does it mean workload will apply before all trait is ready?

[hongchaodeng]

This does not have any indication on ordering. It will comply with whatever ordering being applied but skip unready ones.

[wonderflow]

I think we can't apply workload before its trait still unready. They should work as a whole.

[hongchaodeng]

Why do we assume such logic in dependency? What's the reasoning?

[wonderflow]

I think this PR also fixes #89, right? @hongchaodeng

[hongchaodeng]

I think this PR also fixes #89, right?

Yes. The inputs handling logic is AND-ed.

	uds := make([]v1alpha2.UnstaifiedDependency, 0)
	for _, in := range ins {
		s, ok := dag.Sources[in.ValueFrom.DataOutputName]
		val, ready, err := r.checkSourceReady(ctx, s)
		if !ready {
			uds = append(uds, makeUnsatisfiedDependency(obj, s, in))
			return uds, nil
		}
...
	}

[ryanzhang-oss]

a double negative expression like !Unready is usually not easy to read. Why not just called "ready" and flip the boolean value?

[ryanzhang-oss]

the "ready" thing seems to be a good candidate to abstract out so it can applied to any other resources like scope

[hongchaodeng]

The reason is that the default of bool is false.
The default of any Workload/Trait is actually ready (= !unready). The dataInputs field might not exist so it might not go into the dependency logic.

What about change the name to HasDep?

[hongchaodeng]

I have changed this to HasDep. I think having a default false value is still very important in preventing mistaken init place.

[ryanzhang-oss]

Do we not allow dependencies from multiple objects?

[hongchaodeng]

It is allowed.
The status will show each dependency for the same object in individual item:

status:
  dependencies:
  - from:
      name: source-a
    to:
      name: sink-a
  - from:
      name: source-b
    to:
      name: sink-a

But actually in current implementation, the runtime will save computing power and not print the second one as long as it finds any one dependency for a sink not satisfied.

[ryanzhang-oss]

so this means that a "ready" source can be fliped to be "unready" due to network or some other errors. I think we at least should put a TODO here to fine-tune that user experience in the next PR.

[hongchaodeng]

This is the reason we put the logic into reconcile and let controller runtime deal with error handling.
If such transient error happens, the AppConfig status will show it and from users' point of view they should be able to see it and wait for the network hiccup to pass away.

[ryanzhang-oss]

I don't seem to see a Unit test here, maybe we can ask the student to add it?

[hongchaodeng]

Let me answer all questions about the UT here.
All of the mentioned methods are covered in TestDependency. I have used go tool cover to check that.

[My-pleasure]

LGTM, I have verified this and will follow up with a e2e test PR shortly.