Ability to specify a version of a pack resource (action) to use when running an action execution #3997
Conversation
work tree directories.
|
Will help with #3870 |
|
You mention using containers instead of virtualenvs... I stopped using docker because it was too complex to duplicate my esoteric network setup from host into the container. Some of my actions need not only system packages, but system network setup (vpn, ssh sessions with local forwarded ports as a system service, access to listen to or use the broadcast address of a particular host link, access to particular usb hardware, ... I'm not using all of that in ST2 right now, but I would like to). Containers make that more difficult. |
|
@cognifloyd That's a valid point. What I really just wanted to convey with the container possibility as a solution is that it's a complex problem and no solution is 100% and we are just trying to solve the "version of the code which is used for the execution" problem. @nmaludy What do you think, will this help enough with #3870 (of course it's not a whole solution, but a small part of it :))? |
|
Talked with @enykeev about the approach on Slack. We decided to go with the "worktree per execution" approach for now. This way we get the most isolation and we simply don't have enough hard data to make a decision and everything else would be speculation. If it turns out that it's too slow at some point in the future, we can change it and go with the worktree per revision / version approach (where the worktree for a particular version is shared among different executions). |
runners. Create git work tree directory inside the runner pre_run() method.
script and Python runner (aka runners where it makes sense to support that argument).
support git worktree functionality. This functionality only makes sense with runners which operate / use local files from pack directory (e.g. local script runner and Python runner).
inside the git worktree directory.
git worktree directory.
When this parameter is set to True, git worktree directory is not cleaned up in post_run() to aid with debugging / troubleshooting.
|
@m4dcoder Test case added in 4c02af3. As mentioned above, it indeed works correctly because the directory where the script which is executed is located gets automatically and implicitly added to The thing did get me thinking about other scenarios and I caught that - 16b88dd. Will add test case for that shortly. |
|
Test case for pack commons libs path in 7d2818a. |
|
As far as support for action chain and Mistral goes - I do agree that eventually it would be good to have support for that as well, but initial goal was only support for Python runner. Only reason I also added support for local shell script runner is because a lot of code can be re-used between Python and Local shell script runner and the change was relatively straight forward. And not doing it for only a single runner resulted in a better code which is not "biased" against Python runner (better abstraction and separation of concerns). The change for Mistral is more involved so I would like to postpone for the future. |
|
And to expand on that and to clarify further - I think that's the right model for releasing / shipping pretty much any kind of bigger "experimental" feature - in a small incremental manner. Easier to change / adopt / etc things based on the feedback, compared to just throwing out one big change (agile! 😂). Having said that, in cases like that, it's also good that the initial implementation contains implementation for more than one runner to make sure it's not biased against a single runner which results in a better abstraction, etc. |
|
@lakshmi-kannan @bigmstone ^can you two please also have a look. @m4dcoder and I have different ideas on how to proceed with this :) |
There was a problem hiding this comment.
Why wouldn't we implement this in the st2actionrunner instead of the runners themselves. There's plenty of reason this could be relevant to even runners you say they aren't needed (revisioning API changes to the local runner etc). I wouldn't call this a blocker, but I'd like to know the answer before I can 👍.
|
I feel like the team is a little bit overzealous about using "request for change" without actualy mentioning a change they want to be made. @bigmstone if you look closer at the code, you'll see that the only purpose of the worker is to dispatch a container with the liveaction and the actual work of reading the entrypoint code happening in the runner itself. Also, it seems to me that the runner knows better how to handle the specifics of its own execution. In case of python runner, for example, it should be able to also handle switching venv, a function the rest of the runners should not care about. @m4dcoder If we're agreeing that it is runners responsibility to handle its own code versioning, then we should be able to do it slow and implement it runner by runner. I see no reason to block this PR simply on a basis that it didn't cover all the possible scenario. Following your logic, I don't see why cloudslang runner should be exempt. Do anyone of the team feel confident of implementing this change there? |
|
@enykeev if we're checking out a specific version of an |
|
Ok, then I'm asking @StackStorm/team a counter-question: can someone explain to me in simple words the distinction between actionrunner worker, container and runner? |
|
@bigmstone Good question and @enykeev good explanation. That's also why I said we should "formally" document each service and component responsibility - a lot of those decisions were made early on in the code, but there are not formally documented (in a code comment / docs / similar). Sometimes it's hard when building out new features, because you can always argue that X belongs in Y where Y can be pretty much any component. In short, what @enykeev has said. Entry point and other manipulation happens inside the runner class. Another reason is that some of the code is runner specific (e.g. making sure common libs path which only applies to Python runner correctly resolves to git worktree directory, etc.). So while I could put the code in the action runner container (which I believe is what you mean when you said action runner), but I think it would result in a bad abstraction. We would need to do |
|
Yes, container is where I envisioned this going. In hindsight making the distinction of I don't want to get to a point where we're branching to check runners as @Kami mentioned, but checking out a version of |
bigmstone
dismissed
their stale review
DGAF (<- This a joke...only meant to elicit a small chuckle)
|
Sounds like the runners need a common at of utilities to handle this. So,
when the next runner needs this, say actionchain, or Mistral, refactor the
common bits into some runner git utilities. But until then, get feedback on
the implementation with these two runners.
The exact placement of the code is not critical right now, but the
functionality is awesome. Learn first, then refactor. The user interface
(selecting the entry point version to use) stays the same no matter where
the code is out how often it's duplicated before it gets de-duplicated.
(These 2 cents come from the peanut gallery)
…On Fri, Mar 2, 2018, 11:09 Matthew Stone ***@***.***> wrote:
Yes, container is where I envisioned this going. In hindsight making the
distinction of st2actionrunner doesn't make any since sense the action
runner, container, and runner all execute in that process.
I don't want to get to a point where we're branching to check runners, but
checking out a version of entrypoint in the contain and doing runner
specific stuff in the runner seems reasonable to me. I'm not going to die
on that hill though. If we want to replicate calling the checkout code in
each runner it's not a blocker to me - even though I think that's better
suited in the container.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3997 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABfIPqDh0MuOLp0JfkbuBXaE6gA_sMZzks5taXyvgaJpZM4SDI27>
.
|
|
@cognifloyd Agree - current implementation does this by having runner inherit from new class that can handle this. So net-new runners could just start by inheriting from this class from the onset. |
|
@bigmstone @m4dcoder I'm still missing "formal" approval of this PR so I can finally merge it into master :) |
Description
This pull request starts an implementation which allows user to specify a version (either a git revision / branch / tag) of a resource to use for a particular action execution.
Ability to use a specific version of a pack resource comes handy in many scenarios. Notably when running a distributed deployment and you want to ensure a specific version of pack resource is used.
That is something @enykeev and I and others have discussed in the past.
The code is far from finished and I just opened the PR so we can get everyone on the same page and agree on the implementation.
Usage
User can specify which version of pack content to use by specifying
content_versionglobal runner parameter.For example:
st2 run mypack.myaction arg=value content_version=v0.5.0Limitations
Users can still use packs which are not git repositories, it simply means they can't leverage this functionality.
This would be simply too complex and add a lot of overhead for a little value.
Imo, the right way to handle that is containers. Virtualenvs are complex and Python packages can also depend on C libraries and system dependencies so just versioning virtualenv directories wouldn't solve the whole problem.
Implementation
The implementation leverages
git worktreefunctionality. This functionality was suggested by @enykeev and I agree it's a good idea.git worktree simply makes a bare copy of a git repository directory and sets HEAD to the specified revision.
Before the action is executed a new git worktree directory is created for a specified revision and this directory is used when executing an action.
This directory is ephemeral (specific to an execution) and it's removed once the execution completes.
If we assume that git resources are immutable (that's indeed the case for commit revisions, but not for branches and tags) we could use a fixed directory for each revision (content_version). This way we would avoid directory churn since different executions with same
content_versioncould re-use the same git worktree directory.This approach would also be more efficient and faster because creating a new worktree directory for each action execution adds some overhead.
If we went with this approach we would probably still need some kind of garbage collection service which deletes pack git worktree directories which haven't been accessed for X days or similar.
TODO