Introduce Action rollback proposal

[pkosiec]

Description

Changes proposed in this pull request:

• Introduce Action rollback proposal
• Fix minor issues in TypeInstances proposal

[mszostok]

I like the standalone property because it simplifies a lot but at the same time, it makes it more complicated and somehow I have a feeling that we will have a problem with it. We need to define really nice roles for that, could you group all rules in one place for standalone and rollback fields? Currently, those rules are defined in different places across the whole document. It will be good to group them together

[mszostok]

A few additional thoughts after F2F discussion with Paweł:

• describe rollbackForAction (how it will be handled by the engine, which other fields from spec are taken into account during render phase)
• Add information that input parameters are not supported. What do we want to do in the future?
• Add information that in the rollback action you can add custom action e.g. dump some metric (support for the post-, or pre- hooks)
• how rollback can work for the "downgrade" scenario. E.g. we have a scale action, and we want to rollback this action?
• What about automatic rollbacks for workflows that do not produce anything? Do we want to do that or content creator always need to describe it? We have already info about the TypeInstances rollbacks and information about the edges, so we can start from leaves to root and remove those typeInstances. Workflow still can have an option for some post-, or pre- hooks). Right now, the rollback for the workflow seems quite redundant.

[pkosiec]

Here are my notes from the f2f discussion - to merge with comment above

TODO:

• Describe rollbackForAction. In rollback workflow you can use usual actions as well
• Rollback - restore old values? how?
• Parameters from user or optional input for rollback - not supported right now
• Configure Ingress for Confluence
• Automatic rollback workflow -> Rollback only for actual TypeInstances created in given action (excluding dependent Implementations)

[mszostok]

1. I'm not sure about the Rollback CR that duplicates the logic of Action CR. In the New Rollback Controller I mentioned that maybe it should be done as sequentially triggered a set of Actions. So the Rollback CR is more like a trigger for rollback Action(s).

2. What about the data that is created by the given runner, when and how this will be deleted? For example, ArgoWorkflow CR created during install/upgrade. Now it is connected with the Action CR, so the Rollback CR controller should remove the Action CR from the creation phase? Also, when the Rollback CR will be removed? Are we going to keep them with some retention policy as helm does with history?

3. We have the workflows e.g. Jira install that consists of many sub-implementations e.g. PostgreSQL install which produce the output artifacts but because we flat the workflow then the Jira workflow has the last step which uploads the artifacts to the OCH and then they become TypeInstances.

   How do we want to set the rollbackActionRef properly? I see two option, one that the PostgreSQL install needs to return already properly formatted TypeInstance, the second such rollbackActionRef we treat as the metadata, and it’s computed by engine.

[pkosiec]

@mszostok

1. I'm not sure about the Rollback CR that duplicates the logic of Action CR. In the New Rollback Controller I mentioned that maybe it should be done as sequentially triggered a set of Actions. So the Rollback CR is more like a trigger for rollback Action(s).

This sounds as a good idea. I can mention that in the proposal.

2. What about the data that is created by the given runner, when and how this will be deleted? For example, ArgoWorkflow CR created during install/upgrade. Now it is connected with the Action CR, so the Rollback CR controller should remove the Action CR from the creation phase? Also, when the Rollback CR will be removed? Are we going to keep them with some retention policy as helm does with history?

I think for now we can treat the Action CR with install/update workflows as something, which has been run back in time.
Similarly with Rollback CRs - I would keep them indefinitely until user removes them. Based on Action and Rollback CRs we can build and show a basic timeline on UI, if necessary.

Tl;dr I wouldn't do anything about it for now.

3. We have the workflows e.g. Jira install that consists of many sub-implementations e.g. PostgreSQL install which produce the output artifacts but because we flat the workflow then the Jira workflow has the last step which uploads the artifacts to the OCH and then they become TypeInstances.

How do we want to set the rollbackActionRef properly? I see two option, one that the PostgreSQL install needs to return already properly formatted TypeInstance, the second such rollbackActionRef we treat as the metadata, and it’s computed by engine.

As I wrote in the rollbackActionRef property section:

Engine sets the value of the rollbackActionRef property automatically during TypeInstance creation or update.

From my perspective it should be computed by Engine. Based on TypeInstance names, Engine knows which Action produced a given output TypeInstance. So I would assume this would be a part of rendering of the last step in workflow which uploads TypeInstances. Engine could pass the metadata there. What do you think?

[mszostok]

From my perspective it should be computed by Engine. Based on TypeInstance names, Engine knows which Action produced a given output TypeInstance. So I would assume this would be a part of rendering of the last step in workflow which uploads TypeInstances. Engine could pass the metadata there. What do you think?

could be tricky to do, but reduce the boilerplate so +1

I think for now we can treat the Action CR with install/update workflows as something, which has been run back in time.
Similarly with Rollback CRs - I would keep them indefinitely until user removes them. Based on Action and Rollback CRs we can build and show a basic timeline on UI, if necessary.

Tl;dr I wouldn't do anything about it for now.

But in the future we should go back and revisit that decision, also we should probably ask @platten if this is fine. I know that storing a lot of Action CR could decrease our performance as we also have Informers for them. Maybe we can add some labels e.g. "archived" so we could exclude them from watching?

[pkosiec]

But in the future we should go back and revisit that decision, also we should probably ask @platten if this is fine. I know that storing a lot of Action CR could decrease our performance as we also have Informers for them. Maybe we can add some labels e.g. "archived" so we could exclude them from watching?

Agree, I will prepare a new task for that.
EDIT: Done here: https://cshark.atlassian.net/browse/SV-172

[mszostok]

[Trojan295]

LGTM, although in the first reading the word ancestor was a bit counterintuitive, as I implicitly assumed the usedBy relationship in the dependency tree. Could you add a remark, that the dependency tree is built using the uses relationship?

[lukaszo]

It's true also for upgrades or for any other action which modifies already deployed action. Or am I wrong here?

[pkosiec]

For upgrade or modification of any existing TypeInstances the Interfaces usually make sense, as they base on generic TypeInstances. For example, creating database inside PostgreSQL instance can be totally generic. Or creating user in the DB.

Of course there could some cases where we need to define an Interface, which is more tied to a given Implementation output (TypeInstances). But Rollback is always Implementation-specific thing.

[platten]

Reviewed, LGTM. Nice work!

[pkosiec]

I updated the PR with @Trojan295 suggestion to clarify what is descendant and ancestor in TypeInstance tree. Essentially I added just one sentence to the "Remarks" section:

• TypeInstance dependency tree is built based on the TypeInstance's uses relation. If TypeInstance A uses TypeInstance B, the
  TypeInstance A is ancestor of TypeInstance B. TypeInstance B is a descendant of the TypeInstance A.

I also made sure the whole document is well formatted and the ToC is up to date.

As it doesn't have any impact on the proposal itself, I'm merging it without re-review.