Workflow Approval Nodes #4264
Workflow Approval Nodes #4264
Conversation
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
beeankha
force-pushed
the
workflow_pause_approve
branch
from
2e7dc5c to
da2f1b9
Compare
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
Can we take some of the spec for the feature from #1206 and put it into the file |
| @@ -600,3 +602,56 @@ def actually_running(self): | |||
| # WorkflowJobs don't _actually_ run anything in the dispatcher, so | |||
| # there's no point in asking the dispatcher if it knows about this task | |||
| return self.status == 'running' | |||
|
|
|||
|
|
|||
| class WorkflowApprovalTemplate(UnifiedJobTemplate): | |||
There was a problem hiding this comment.
I don't yet understand this feature very well, but my personal preference would be to try and avoid subclassing from the unified job template and unified job models. There is a lot of baggage that come along with those.
There was a problem hiding this comment.
@chrismeyersfsu and I spoke at length with @beeankha about this and in this case, we've intentionally decided that we want a lot of this baggage, because these objects will end up behaving a lot like jobs in our system (and we didn't think it made sense to reinvent the wheel with a new set of objects that are so similar). By making these nodes just be UJT/UJ implementations, we get a lot for free, such as:
- No need to change the task manager or DAG logic - from the perspective of how a workflow's state machine works, these are just jobs that transition through various states. When they go into
pendingstate, however, we don't actually launch a real playbook/background task under the hood. The process of transitioning frompendingtosuccessorfailureisn't bound to some background process - it's determined by manual approval by a user (or some API client). - No need to change the workflow node logic (or any of its underlying code) to accommodate a new type of relationship. Approval templates are just UJTs.
- We get some common relationships/models we care about for free (such as M2M notification templates), and the concepts of start/success/failure map very closely to the ideas of "this node needs approval", "this node just got approved", and "this node just got rejected".
One side effect of this that you're probably already pondering is that we'll probably want to be very explicit about what we expose at the serializer level for these new objects (i.e., things like a "launch" endpoint and "execution_node/controller_node" don't really make sense).
There was a problem hiding this comment.
Approval templates are just UJTs.
Then use UJTs.
In [1]: UnifiedJobTemplate.objects.create()
Out[1]: <UnifiedJobTemplate: -20>Whenever you introduce a new sub-model, it piggybacks onto a million places that you don't expect, and it introduces things that we will be cleaning up from for a very long time. Django polymorphic is a den of 🐉s, and adding to the sub-types scales very poorly. If we do this with a new sub-model, then we need very through investigation with a lot of data to see what actual queries are being produced while interacting with the unified models.
We will still have problems creating bare UJTs, because some code is sure to make assumptions that these do not exist. But that is the lesser of the two evils without a doubt. The sub-model is adding things on top of an unstable house of cards.
There was a problem hiding this comment.
Then use UJTs.
cc @matburt @mabashian @beeankha @chrismeyersfsu for opinions
This seems fine until later on down the road when we decide to add behavior to these templates (i.e., "allow a user to specify a timeout on the approval node"). Then you arrive at a place where you really do want a separate table for the polymorphic subclass to hold its particular attributes.
If we do this with a new sub-model, then we need very through investigation with a lot of data to see what actual queries are being produced while interacting with the unified models.
That's fair - it's possible that by introducing a new UJT and UJ implementation, we have bleed-through in places (these approval nodes start showing up in places we don't expect). This is the type of thing I'd hope we would catch through our integration test suite, though.
Most of all, this bothers me because it's a special case. Every other UJT-like thing in our system is a polymorphic subclass of UJT, and when a row exists in main_unifiedjob, it has a value set for polymorphic_ctype_id and is referenced by one of the polymorphic child tables. For this new approval concept, you'd have to just sort of know that these UJT rows represented workflow approvals.
There was a problem hiding this comment.
@AlanCoding another question regarding this approach. How would you list something like this (the UI will want to generate this list):
"List all of the UnifiedJobs which represent ready to be approved/rejected."
In the current version of this PR, this is very straightforward:
/api/v2/workflow_approvals/?status=pending
If we treated these as vanilla UJTs, we'd probably end up adding some new ApprovalConfig model/foreign key on WorkflowJobNode and issuing requests like this:
/api/v2/unified_jobs/?unified_job_node__approval_config__isnull=false&status=pending
There was a problem hiding this comment.
@AlanCoding one open-ended question is how we handle the name field on WorkflowApprovalTemplate. UJT.name is required, but unlike projects, job templates, etc... the notion of a name (unique across the org) for one of these doesn't really make much sense. Would we want to auto-generate names for these? I can't think of any actual name that would make them identifiable, and in fact you wouldn't actually ever look for and select on of these templates from a list; by their very nature, they're adhoc and ephemeral. Whenever you need one, you just make a new one on the fly (they're not re-used).
Do you have any ideas?
There was a problem hiding this comment.
Or maybe we should treat these as reusable instead of anonymous? Maybe people might want to make them and re-use them e.g., "Requires Manager Approval") This might make a lot of sense once we start mixing in notification support - it would be kind of a drag to have to set these up over and over again for every node in a workflow.
@mabashian what if the process of configuring a pause node on a workflow meant you could either choose a prior configuration or make a new one on the fly?
There was a problem hiding this comment.
I vote for having these approval nodes be reusable, since there may be instances (as @ryanpetrello points out) where there is a standard, regularly-occuring ✔️that's required by the same person/group and is done repeatedly for certain types of workflows. They can be uniquely named, just like any job template or workflow, and can be easily identifiable by the user that way.
Just to expand on this point, re-creating a notification every time you want to be notified of something = discouraging, but since notifications are permanent (until deleted) and assignable at will, they're much more useful. Approval nodes could be thought of in a similar way, usage-wise.
There was a problem hiding this comment.
Catching up here....I see the usefulness of having reusable ones. Where did y'all end on that conversation?
There was a problem hiding this comment.
@kdelee current documented design plan is not allowing reuse, and creating them by a related endpoint of WFJT nodes, thereby having them tied to a node.
beeankha
force-pushed
the
workflow_pause_approve
branch
from
da2f1b9 to
9c29161
Compare
|
Build succeeded.
|
|
Build failed.
|
|
Build succeeded.
|
This comment has been minimized.
This comment has been minimized.
|
Build failed.
|
|
Build succeeded.
|
beeankha
force-pushed
the
workflow_pause_approve
branch
from
86281bb to
0dbcf70
Compare
|
Build failed.
|
…d seconds. Remove workflow template badge in approvals drawer.
ryanpetrello
force-pushed
the
workflow_pause_approve
branch
from
d5a7766 to
23f75cf
Compare
|
Build failed.
|
|
Build succeeded.
|
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
…ssh_host Add support for satellite6_want_ansible_ssh_host
SUMMARY
Introducing a new type of pause/approve node in between job/workflow nodes so that a user can give the 👍 or 👎 to proceed with or cancel the workflow.
ISSUE TYPE
COMPONENT NAME
AWX VERSION
ADDITIONAL INFORMATION
Details can be found in AWX Issue #1206