Fixes #13668 - adds update task.

[ShimShtein]

No description provided.

[tstrachota]

This method is quite difficult to understand without any context. Would you mind adding some tests to make the purpose more obvious?

[ShimShtein]

It's the method that is used by dynflow patch - basically it will be called on deserialization process of dynflow task parameters. First argument (storage) is the serialized task output and the second one is a path to the relevant property in the output of the task.
Let's wait for Ivan's comments on this, if he approves the solution I'll add tests.

[iNecas]

I understand the need for bening able to reference things that are not just hashes and arrays. My concern with the current implementation is however, that it handles deserialization directly in the referencing. I think the input/output proxy objects might be more smart that they are right now (just plain hashes) and I'm willing to give you a coding hand in making the change in Dynflow, as it would make dynflow more usable. The dereferencing part would that became trivial. What do you think about that approach?

[ShimShtein]

@iNecas, basically sounds good. Let me tell you my assumptions:

1. Each task knows how to serialize/deserialize its own objects.
2. Dynflow framework doesn't care how the recipient task will handle the output of the sender task.
3. Dynflow needs to supply a stub for the output object in planning phase in order to understand the dependency between the tasks.
4. Dynflow doesn't want to limit the interface between tasks to specific object types (like hash, array and primitives).

Am I missing something?

[iNecas]

1. the tasks would specify the implementation to be used for the input/output object, to achieve the extendability.
   I assume one could define a default implementation in configuration. For example, the active record serializer/deserializer should be generic enough for majority of the Rails project.
2. I'm not sure I understand this part. What would be an example of dynflow caring about how the recipient task handles the output
3. yes, this part would stay the same as it's right now
4. no, the only constraint would be, that the input/output serializers know how to serialized/deserialize the objects to/from database.

Do this answers fit into you assumptions?

[tstrachota]

I like the idea of having an object that takes care of (de)serialization of actions' inputs/outputs. There's an issue open for that in deployments http://projects.theforeman.org/issues/12172 but it would certainly be cleaner to have it supported directly in dynflow.

Regarding @ShimShtein's thoughts in the latest comment it might be interesting to introduce the concept of transformations. Since OutputReference takes care of dereferencing itself (correct me if I'm wrong) I think we could add support for using transform functions and have the both concepts could live side by side. Action writers can then choose what's more appropriate for them.

The latest "thread-pool-like" idea seems quite against the dynflow's design to me. As @iNecas mentions, planning heeds to happen before the execution.
BTW specifically in the example with the host I think the DeleteHost action should rather be part of the planned rollback mechanism (Dynflow/dynflow#87).

[ShimShtein]

@tstrachota, Transformations are already a necessity. Right now it's pretty limited and implemented implicitly by square brackets (the transform function is a filter transform - you can only choose a property). After introducing custom output objects, it will be impossible to maintain same transformation code in framework, since the input will become unknown.
I am not a big fan of thread pool either and I don't think it's the ultimate solution. Still we have to address at least the problem with passing (transformed) output as a parameter to plan phase of another task. It is possible to finish the plan phase of the envelope task before the plan phase of dependent task starts. The difficulty here is that planning of tasks that rely on other task's output can be done only after the run phase of the dependency task has returned with a result.

The example was a stupid one, I just couldn't think of a good one with a condition. Will this example be better:

  host_creation = plan_action(CreateHost, host_attrs)
  if host_creation.output[:environment] == 'production'
    plan_action(AddServerToSplunkMonitoring, host_creation.output[:ip])
  end

[iNecas]

----- Original Message -----

@@ -7,6 +7,26 @@ def run
results = SerializableArray.new(results)
SearchTaskDefinition.create_output(results, output)
end
+

•    def self.dereference_output(storage, path)
  

OK, the use case with a condition that I have described is indeed very
advanced. We can disallow it (at least for now), but what about the original
problem that I describe in (2)?

I see an unusual task ordering here - plan envelope task -> plan
CreateHostTask -> execute CreateHostTask-> plan DhcpReserve task ->
execute DhcpReserveTask

No, it's still plan CreateHost -> plan DhcpReserve -> execute CreateHost -> execute DhcpReserve
It's not intended to do much with the inputs in the plan phase: the plan phase
is really mainly for planning and validation if possible.

---

Reply to this email directly or view it on GitHub:
https://github.com/theforeman/foreman_deployments/pull/24/files#r55202579

[iNecas]

----- Original Message -----

In app/lib/foreman_deployments/tasks/search_task_definition.rb :

@@ -7,6 +7,26 @@ def run
results = SerializableArray.new(results)
SearchTaskDefinition.create_output(results, output)
end
+

•    def self.dereference_output(storage, path)
  

@tstrachota , Transformations are already a necessity. Right now it's pretty
limited and implemented implicitly by square brackets (the transform
function is a filter transform - you can only choose a property). After
introducing custom output objects, it will be impossible to maintain same
transformation code in framework, since the input will become unknown.
I'm don't follow. Can't be just used the approach you just did, with calling a method on the object, if it responds to it
and it's not hash? I'm sure it will not work for everything, but the 80-20 rule might apply here quite nicely.

I think the transformations are nice idea but not really a blocker here. Let me make it straight: getting
rid of the output sub-chains in planning is no-go here, as it's already being used by Dynflow users.
We can talk about implementing it better way once the transformations are there, but not sooner.

I am not a big fan of thread pool either and I don't think it's the ultimate
solution. Still we have to address at least the problem with passing
(transformed) output as a parameter to plan phase of another task. It is
possible to finish the plan phase of the envelope task before the plan phase
of dependent task starts. The difficulty here is that planning of tasks that
rely on other task's output can be done only after the run phase of the
dependency task has returned with a result.

The example was a stupid one, I just couldn't think of a good one with a
condition. Will this example be better:
host_creation = plan_action( CreateHost , host_attrs) if
host_creation.output[ :environment ] == ' production ' plan_action(
AddServerToSplunkMonitoring , host_creation.output[ :ip ]) end

Again: the contract in planning is, that the output is meant just for the planning purposes (passing as input to others
task). So we are not aiming to support this example. If there is a better name for the output that would
better describe this contract, I'm ok with introducing alias (but keeping the sub-key chaining).

—
Reply to this email directly or view it on GitHub .

[ShimShtein]

No, it's still plan CreateHost -> plan DhcpReserve -> execute CreateHost -> execute DhcpReserve
It's not intended to do much with the inputs in the plan phase: the plan phase
is really mainly for planning and validation if possible.

In this case, DhcpReserve's #plan method will get a TaskReference object as a parameter. It's a bit non-trivial for the implementers, but as long as it's well documented, I'm OK with it.

getting rid of the output sub-chains in planning is no-go here, as it's already being used by Dynflow users.

IMHO we can use them both - we will just use different access points to the output. For example we can leave the #output method as is (with chain recording) and create another method #original_output that will return the deserialized version of the object (without the chaining).

I'm ok with introducing alias (but keeping the sub-key chaining)

That's basically what my patch does. If we move the implementation of self.dereference_output to Actions::Base class, it will become the default behavior, but still be overridable, if the task wishes to. Probably I will break it to two methods - one that deals with root deserializing and one for dealing with call chains. Then we would be able to get rid of the ugly if that I add to ExecutionPlan::OutputReference#dereference.
Basically we will just move the deserialization responsibility to the task itself - which anyway sounds reasonable to me.
What do you say @iNecas, does the last paragraph sound like a good starting point?

[tstrachota]

@ShimShtein thanks, it's an important addition!
I briefly read through the code and asked Ivan for checking the dynflow patch part. I haven't tested it yet though.