Modify API to remove related files on delete of actions/workflows #5304

mahesh-orch · 2021-07-15T15:55:49Z

This change is to remove related files of actions/workflows from disk when API called from CLI and web UI (StackStorm/st2web#898). Currently when this delete API is invoked then this only de-register the actions from database and when user reloads the st2 then these actions get registered into database again.

nzlosh · 2021-07-15T16:26:41Z

What issue does this PR relate to?

This is a big conceptual shift to StackStorm as a whole since the API does not create/modify or delete files in a pack because it is not authoritative, git is. This change would break the fundamental premise of infrastructure as code and would have changes reverted on the next package upgrade creating a lot of confusion for the lay user.

amanda11 · 2021-07-15T16:57:44Z

I think this is related to adding delete functionality into the UI, so the reverse of the workflow composer + that creates a new workflow on disk and in the live pack.
I've not reviewed the code, but I believe that is the context...

However altering the current behaviour of the endpoint might not be the best course of action, as those already using the DELETE endpoint might rely on the fact it deletes from memory and not disk. So I wonder if it should have at least a new parameter to the call that the UI calls, that allows the default behaviour to be unchanged.

The create does at the moment creates files if its passed the data_files parameter - which I presume is sent from the UI.

blag

A few nitpicks that need to be cleaned up.

Also, this code will explode if two different worker processes try to remove the same pack at almost the same time, or if the process cannot remove the files for some reason (eg: permissions issues). Please wrap them in a try/catch block to ignore exceptions where the file is already deleted, and to write a log error if the process doesn't have permission to delete the file.

st2api/st2api/controllers/v1/actions.py

blag · 2021-07-15T20:05:25Z

Just (hopefully) clarify a few points here.

This is a big conceptual shift to StackStorm as a whole since the API does not create/modify or delete files in a pack because it is not authoritative, git is.

This is true, but removing the entire pack directory, .git/ and all, is the ultimate authoritative action.

I think this is related to adding delete functionality into the UI, so the reverse of the workflow composer + that creates a new workflow on disk and in the live pack.

As far as I'm aware, the workflow composer does not write the action metadata and workflow file out to disk, it only stores them in the database.

The overall problem here is that StackStorm allows workflows to be created three different ways:

Installed as part of a pack installed via infrastructure-as-code, like in a build pipeline, Dockerfile, Packer script, etc.
Installed as part of a pack installed by a ST2 worker, as part of the packs.install action.
Created manually in the workflow composer/designer.

File management is one of the integration points that the workflow composer/designer lacks.

amanda11 · 2021-07-15T20:25:35Z

@blag - agree with most of your points.

I am pretty sure it does write to disk on the composer. It certainly updates the files on edit - as have copied the modified files from the "live" file system to my gitrepo before.

There is logic in the modified file to write the data files out on the api create.

st2/st2api/st2api/controllers/v1/actions.py

Lines 129 to 135 in f507d2e

    
           # Write pack data files to disk (if any are provided) 
        
           data_files = getattr(action, "data_files", []) 
        
           written_data_files = [] 
        
           if data_files: 
        
               written_data_files = self._handle_data_files( 
        
                   pack_ref=action.pack, data_files=data_files 
        
               )

Rules - get written directly to database and not to file (by the UI), but I believe workflows get written to disk and to database - which is inconsistent!!!

m4dcoder · 2021-07-15T22:58:32Z

This is related to delete of action/workflows in general. The current delete operation via API (invoked from CLI or UI related to StackStorm/st2web#898) just deletes the database registration. If operator does a reload, the action/workflow that was deleted just gets registered to the database again.

If user manages infra and packs with codes, I don't think this affects or changes that paradigm. They can still manage and load packs and actions thru git, puppet, chef, containers, or which ever way they automate their infra.

The target audiences for this change here are mostly action/workflow developers that is using an instance of StackStorm for development to delete an action/workflow.

For consistency sake, I would suggest making changes so that rule creation also writes content to disk.

m4dcoder

@mahesh-orch

Please update the description. The description should explain why you are making this change and how is the change going to work.
Please make sure there are unit tests that cover the change.
Can we add a confirmation prompt in the CLI to let user know that the operation will delete files and user can OK or back out of the operation?

Co-authored-by: blag <blag@users.noreply.github.com>

mahesh-orch · 2021-07-16T05:31:23Z

A few nitpicks that need to be cleaned up.

Also, this code will explode if two different worker processes try to remove the same pack at almost the same time, or if the process cannot remove the files for some reason (eg: permissions issues). Please wrap them in a try/catch block to ignore exceptions where the file is already deleted, and to write a log error if the process doesn't have permission to delete the file.

Sure, I will do the suggested changes. Thanks.

mahesh-orch · 2021-07-16T05:39:20Z

@m4dcoder Sure, I will write the unit test to cover the made changes. And I will also work on asking for confirmation prompt in the CLI to let user know removal of files to proceed with users agreement or to back out the operation. Thanks

amanda11 · 2021-07-18T10:14:27Z

@m4dcoder if we are going to issue a warning on the CLI and print for confirmation, could we also add a --yes or similar so that you can confirm it on the command line - to make it easier for anyone scripting the cli calls.

Added try except block for handling PermissionError while removing action files from disk. Also, code is added for logging errors.

Added code to add a confirmation prompt in CLI to let user know that the operation will delete files on disk as well. If user says OK then to proceed otherwise to back out the operation.

mahesh-orch · 2021-07-19T14:04:03Z

@blag I have added try except block for handling PermissionError while removing action files from disk. Also, code is added for logging errors. Please review. Thanks.

mahesh-orch · 2021-07-19T14:04:47Z

@m4dcoder I have added code to add a confirmation prompt in CLI to let user know that the operation will delete files on disk as well. If user says OK then to proceed otherwise to back out the operation. Please review. Thanks.

amanda11 · 2021-07-19T15:20:51Z

st2client/st2client/commands/resource.py

@@ -735,16 +736,29 @@ def __init__(self, resource, *args, **kwargs):
    def run(self, args, **kwargs):
        resource_id = getattr(args, self.pk_argument_name, None)
        instance = self.get_resource(resource_id, **kwargs)
-        self.manager.delete(instance, **kwargs)
+        if isinstance(instance, st2client.models.action.Action):


Is it possible to have a cli argument like --yes so that it is possible to delete actions within having to give input. This makes it much easier for anyone using st2 cli in scripts.

I agree, giving the option -y or --yes makes this scriptable.

Also, you should move this changes to the ActionDeleteCommand at https://github.com/StackStorm/st2/blob/master/st2client/st2client/commands/action.py#L280 instead of using if isinstance to check if this resource is an Action.

@amanda11 Sure, I will add functionality to have CLI argument --yes or -y. Thanks.

m4dcoder

Where are the unit tests?

m4dcoder · 2021-07-20T00:46:57Z

st2api/st2api/controllers/v1/actions.py

+        )
+        action_metadata_file_path = os.path.join(
+            BASE_PATH, "packs", pack_name, metadate_file
+        )


The pack base path can be overridden (ref: https://docs.stackstorm.com/packs.html#under-the-hood-pack-basics). Therefore, it is better to use get_pack_base_path @ https://github.com/StackStorm/st2/blob/master/st2common/st2common/content/utils.py#L135 to determine the pack file path.

m4dcoder · 2021-07-20T00:47:05Z

st2api/st2api/controllers/v1/actions.py

+                        action_metadata_file_path,
+                        e,
+                    )
+


Can you create a function at https://github.com/StackStorm/st2/blob/master/st2common/st2common/services/packs.py to delete the action entrypoint and action metadata files? Move the path calculation that you did above into this function as well.

m4dcoder · 2021-07-20T00:48:55Z

st2client/st2client/commands/resource.py

@@ -735,16 +736,29 @@ def __init__(self, resource, *args, **kwargs):
    def run(self, args, **kwargs):
        resource_id = getattr(args, self.pk_argument_name, None)
        instance = self.get_resource(resource_id, **kwargs)
-        self.manager.delete(instance, **kwargs)
+        if isinstance(instance, st2client.models.action.Action):


I agree, giving the option -y or --yes makes this scriptable.

m4dcoder · 2021-07-20T00:53:27Z

st2client/st2client/commands/resource.py

@@ -735,16 +736,29 @@ def __init__(self, resource, *args, **kwargs):
    def run(self, args, **kwargs):
        resource_id = getattr(args, self.pk_argument_name, None)
        instance = self.get_resource(resource_id, **kwargs)
-        self.manager.delete(instance, **kwargs)
+        if isinstance(instance, st2client.models.action.Action):


Also, you should move this changes to the ActionDeleteCommand at https://github.com/StackStorm/st2/blob/master/st2client/st2client/commands/action.py#L280 instead of using if isinstance to check if this resource is an Action.

Moving path calculations and action file remove related code to st2common/services/packs.py

Added `--y` option for action delete command at CLI to make command scriptable

Unit test for newly added code in /st2common/services/packs.py

…pement-changed' section

…pdating message to return in case of failure in deleting action files

… path instead of string concatenation. Adding docstring for test classes to describe specificness.

… to remove duplication.

…nd metadata file

mahesh-orch · 2021-08-11T16:23:08Z

@blag

If file to delete doesn't exist then logging warning message at https://github.com/orchestral-st2/st2/blob/modify_action_delete_api/st2common/st2common/services/packs.py#L257 and L286
In case of no permission to delete file logging full exception and returning proper error message via abort() at https://github.com/orchestral-st2/st2/blob/modify_action_delete_api/st2common/st2common/services/packs.py#L245 and L273 as abort() takes care of returning error message here.
In case of any other exception logging full exception and returning generic message at https://github.com/orchestral-st2/st2/blob/modify_action_delete_api/st2common/st2common/services/packs.py#L255 and L284
The logic in st2client/st2client/commands/action.py has cleaned up to reduce duplication as per your suggestion.
Removed an unnecessary argument for getattr() in st2client/st2client/commands/action.py.
Used os.path.join(....) to prepare path instead of concatenating strings in st2common/tests/unit/services/test_packs.py.
Added assertions for the message of the mocked PermissionError in st2common/tests/unit/services/test_packs.py.
Please review the changes. Thanks.

mahesh-orch · 2021-08-11T16:25:27Z

@amanda11 CHANGELOG for modify action delete API has moved to your suggested location. Please review. Thanks.

mahesh-orch · 2021-08-11T16:32:51Z

@cognifloyd The try/except blocks have been placed one after another instead of nesting as per your suggestion. I had made the changes according to @m4dcoder's suggestions. Docstrings are added in test classes to clear specificness of the unit tests in st2common/tests/unit/services/test_packs.py. Please review. Thanks.

amanda11

Thanks for the updates - my points have all been addressed. Much appreciated.

m4dcoder · 2021-08-12T17:57:03Z

st2common/tests/unit/services/test_packs.py

+
+        # asserting delete_action_files_from_pack function raises exception
+        with self.assertRaises(Exception):
+            delete_action_files_from_pack(TEST_PACK, entry_point, metadata_file)


@mahesh-orch You missed this one I think

blag

I would make some changes, but it's nothing serious.

blag · 2021-08-16T21:21:09Z

st2common/st2common/services/packs.py

+                action_metadata_file_path,
+                e,
+            )
+            if os.path.isfile(action_metadata_file_path):


This check is probably redundant, since if os.remove() fails it will throw some sort of exception.

blag · 2021-08-16T21:21:47Z

st2common/st2common/services/packs.py

+                e,
+            )
+            if os.path.isfile(action_metadata_file_path):
+                msg = 'The action file "{0}" could not be removed from disk, please check the logs or ask your StackStorm administrator to check and delete the actions files manually'.format(


I'm kind of surprised our linting is not catching this long line.

blag · 2021-08-16T21:22:04Z

st2common/st2common/services/packs.py

+                action_entrypoint_file_path,
+                e,
+            )
+            msg = 'The action file "{0}" could not be removed from disk, please check the logs or ask your StackStorm administrator to check and delete the actions files manually'.format(


I'm kind of surprised that our linting is not catching this long line.

…h and fixing long line msg

…d name for error msg assertions

mahesh-orch · 2021-08-17T10:09:22Z

@blag I have done your suggested changes. Also, done similar changes in other files. Please review.

nzlosh · 2021-08-30T13:22:52Z

This PR failed to provide sufficient options to remove a database entry without touching the files on disk

--force assumes yes to delete the file. Yet, if you want to only remove the database entry and not touch the contents on disk, you must do it interactively. The force option must take an argument of force yes/no so that non interactive shell commands can be run.

Going on step further, why break existing behaviour when we don't have to? Remove the --force option, and create the option --remove-files instead. This way existing scripts that expect the database entry to be removed will continue to work. Any new scripts that are developed that would like to delete the file from disk can provide the remove-files flag.

nzlosh · 2021-08-30T15:28:23Z

Digging into this change a little more:

I triggered a corner case that isn't handled correctly. If there is a file access error, the database entry is deleted, leaving St2 in an inconsistent state. This operation should be atomic, if the file was requested to be removed and it fails, the database entry should not be removed.

root@u1804:/opt/stackstorm/packs/mongodb# st2 action delete mongodb.user_actionx
The resource files on disk will be deleted. Do you want to continue? (y/n): y
ERROR: 500 Server Error: Internal Server Error
MESSAGE: No permission to delete "/opt/stackstorm/packs/mongodb/actions/user_actionx.py" file from disk for url: http://127.0.0.1:9101/v1/actions/612cf0f24d30afa26ef56b88

Files user_actionx.py and user_actionx.yaml are still present in the pack.

root@u1804:/opt/stackstorm/packs/mongodb# ls actions/
collection_create.py    collection_delete.yaml  database_create.py    database_delete.yaml  lib              user_actionx.yaml  user_delete.py    user_update.yaml
collection_create.yaml  collection_list.py      database_create.yaml  database_list.py      __pycache__      user_create.py     user_delete.yaml  workflows
collection_delete.py    collection_list.yaml    database_delete.py    database_list.yaml    user_actionx.py  user_create.yaml   user_update.py

However, the action user_actionx is gone from the database.

root@u1804:/opt/stackstorm/packs/mongodb# st2 action list --pack=mongodb
+---------------------------+---------+--------------------------------------------------+
| ref                       | pack    | description                                      |
+---------------------------+---------+--------------------------------------------------+
| mongodb.collection_delete | mongodb | Delete a collection in a database.               |
| mongodb.collection_list   | mongodb | List the available collections for the database. |
| mongodb.database_create   | mongodb | Create a database on a mongo server              |
| mongodb.database_delete   | mongodb | List the available collections for the database. |
| mongodb.database_list     | mongodb | List the available databases in a mongo server   |
| mongodb.user_create       | mongodb | Create a mongodb user account                    |
| mongodb.user_delete       | mongodb | Delete an existing database user.                |
| mongodb.user_update       | mongodb | Update an existing database user account.        |
+---------------------------+---------+--------------------------------------------------+

cognifloyd · 2021-08-30T15:35:28Z

The opposite applies for the default stackstorm-ha install where the packs filesystem is read-only, so deletes should only affect the db.
Maybe we need a system-wide setting in /etc/st2/st2.conf to enable/disable deleting files.

nzlosh · 2021-08-30T15:48:07Z

I assume you mean for kubernetes. I run StackStorm HA on VMs and the packs are not on read-only filesystems. Regardless of which environment St2 is being run on, the code should be able to handle the situation in a generic and coherent way.

nzlosh · 2021-08-31T07:49:22Z

@mahesh-orch @m4dcoder Please open a new PR to address the issues mentioned above. As it stands, this change prevents removing just the database entry.

amanda11 · 2021-08-31T08:37:05Z

Could we get an issue raised and assign it to 3.6 milestone and 3.6 release project?
I think we can't release without a change as this will mean it's impossible to do a st2 delete without an error raised on the Kubernetes HA with it in the current state.

The idea of making it configurable in st2.conf could address most of the issues,and probably requires changes to both api and cli and doc.

Adding code to remove action files on disk

28a3c39

mahesh-orch changed the title ~~Modifying API to remove related files on delete of actions/workflows~~ Modify API to remove related files on delete of actions/workflows Jul 15, 2021

blag suggested changes Jul 15, 2021

View reviewed changes

st2api/st2api/controllers/v1/actions.py Outdated Show resolved Hide resolved

st2api/st2api/controllers/v1/actions.py Outdated Show resolved Hide resolved

m4dcoder requested changes Jul 15, 2021

View reviewed changes

mahesh-orch and others added 3 commits July 16, 2021 10:22

Update st2api/st2api/controllers/v1/actions.py

addd291

Co-authored-by: blag <blag@users.noreply.github.com>

Update st2api/st2api/controllers/v1/actions.py

b70b4a3

Co-authored-by: blag <blag@users.noreply.github.com>

Updating actions.py

f6b91b9

mahesh-orch closed this Jul 16, 2021

mahesh-orch reopened this Jul 16, 2021

mahesh-orch added 2 commits July 19, 2021 16:32

Updating st2api/st2api/controllers/v1/actions.py

ca4d453

Added try except block for handling PermissionError while removing action files from disk. Also, code is added for logging errors.

Updating st2client/commands/resource.py

e7b32b0

Added code to add a confirmation prompt in CLI to let user know that the operation will delete files on disk as well. If user says OK then to proceed otherwise to back out the operation.

amanda11 requested changes Jul 19, 2021

View reviewed changes

m4dcoder requested changes Jul 20, 2021

View reviewed changes

mahesh-orch added 7 commits July 20, 2021 11:40

Merge branch 'StackStorm:master' into modify_action_delete_api

d86d136

Removing whitespaces

7ca0f6c

Updating st2api/controllers/v1/actions.py

0639640

Moving path calculations and action file remove related code to st2common/services/packs.py

Updating /st2common/services/packs.py

c7354c7

Updating /st2common/services/packs.py

b6ed390

Updating /st2client/commands/resource.py

7395008

Added `--y` option for action delete command at CLI to make command scriptable

Adding file test_packs.py

284e130

Unit test for newly added code in /st2common/services/packs.py

mahesh-orch added 6 commits August 11, 2021 20:04

CHANGELOG for modification in action delete API shifted to 'in develo…

b581fc7

…pement-changed' section

Adding log warning in case of file to be removed doesn't exists and u…

24cbc0a

…pdating message to return in case of failure in deleting action files

Adding assertions for error messages. Using os.path.join to prepare…

80788e8

… path instead of string concatenation. Adding docstring for test classes to describe specificness.

Removing an unnecessary argument from getattr() and cleaning up logic…

749c187

… to remove duplication.

Putting try/except blocks one after another instead of nesting

ec59efd

Updating expected_msg variable to take generic path for entry point a…

13eae54

…nd metadata file

amanda11 approved these changes Aug 12, 2021

View reviewed changes

m4dcoder approved these changes Aug 12, 2021

View reviewed changes

cognifloyd approved these changes Aug 13, 2021

View reviewed changes

Merge branch 'master' into modify_action_delete_api

4a0bbdd

blag reviewed Aug 16, 2021

View reviewed changes

m4dcoder mentioned this pull request Aug 16, 2021

Delete action/workflow StackStorm/st2web#898

Merged

mahesh-orch added 3 commits August 17, 2021 15:01

Removing redundant check, adding an appropriate check for file at pat…

63f3d12

…h and fixing long line msg

Minor fixes for long line msgs, comments, typos in comments and metho…

936f618

…d name for error msg assertions

Minor fixes for variable name for msg and length of the msg line

96029a0

mahesh-orch and others added 2 commits August 17, 2021 19:08

Merge branch 'master' into modify_action_delete_api

5fe66c0

Merge branch 'master' into modify_action_delete_api

e76573c

m4dcoder merged commit 854d352 into StackStorm:master Aug 17, 2021

m4dcoder deleted the modify_action_delete_api branch August 17, 2021 19:04

nzlosh mentioned this pull request Sep 1, 2021

API: Rework PR #5304 support for action file deletion. #5348

Closed

Modify API to remove related files on delete of actions/workflows #5304

Modify API to remove related files on delete of actions/workflows #5304

Conversation

mahesh-orch commented Jul 15, 2021 • edited

nzlosh commented Jul 15, 2021

amanda11 commented Jul 15, 2021 • edited

blag left a comment

Choose a reason for hiding this comment

blag commented Jul 15, 2021

amanda11 commented Jul 15, 2021 • edited

m4dcoder commented Jul 15, 2021 • edited

m4dcoder left a comment

Choose a reason for hiding this comment

mahesh-orch commented Jul 16, 2021

mahesh-orch commented Jul 16, 2021

amanda11 commented Jul 18, 2021

mahesh-orch commented Jul 19, 2021

mahesh-orch commented Jul 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahesh-orch Jul 20, 2021 • edited

Choose a reason for hiding this comment

m4dcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahesh-orch commented Aug 11, 2021

mahesh-orch commented Aug 11, 2021 • edited

mahesh-orch commented Aug 11, 2021

amanda11 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mahesh-orch commented Aug 17, 2021

nzlosh commented Aug 30, 2021 • edited

nzlosh commented Aug 30, 2021

cognifloyd commented Aug 30, 2021

nzlosh commented Aug 30, 2021

nzlosh commented Aug 31, 2021

amanda11 commented Aug 31, 2021

mahesh-orch commented Jul 15, 2021 •

edited

amanda11 commented Jul 15, 2021 •

edited

amanda11 commented Jul 15, 2021 •

edited

m4dcoder commented Jul 15, 2021 •

edited

mahesh-orch commented Jul 19, 2021 •

edited

mahesh-orch Jul 20, 2021 •

edited

mahesh-orch commented Aug 11, 2021 •

edited

nzlosh commented Aug 30, 2021 •

edited