Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove st2resultstracker from st2ctl, the development environment and the st2actions setup #5108

Merged
merged 37 commits into from
Jan 23, 2021

Conversation

winem
Copy link
Contributor

@winem winem commented Dec 18, 2020

This removes the references to the st2resultstracker service while keeping the code base of the resultstracker.

close #5070

@pull-request-size pull-request-size bot added the size/L PR that changes 100-499 lines. Requires some effort to review. label Dec 18, 2020
@winem winem added this to the 3.4.0 milestone Dec 18, 2020
@winem
Copy link
Contributor Author

winem commented Dec 18, 2020

It looks like we have a circular dependency between this PR and StackStorm/st2-packages#683 now.

The st2resultstracker is a service managed via the debian/rules file. So the post_install job fails.

In my opinion, it would pass once StackStorm/st2-packages#683 is merged.

I could also try to revert some of the deletions I made as part of this PR and see what it takes to pass the tests but this would lead to an intransparent state of the resultstracker configurations and service definitions in the codebase. So, for now, I'd like to avoid it.

Please let me know what you prefer or if you have another idea.

@winem
Copy link
Contributor Author

winem commented Dec 18, 2020

Agreed with @blag to keep this PR as it is and merge both PRs on monday. The Exchange pack CI builds do have a higher priority and we want them to succeed over the weekend.

See https://stackstorm-community.slack.com/archives/CSBELJ78A/p1608330230290400

@arm4b arm4b requested review from a team and m4dcoder December 21, 2020 14:36
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I left a change request in the st2-packages PR before we can merge it.
Besides of that, we'll need st2docs update https://github.com/StackStorm/st2docs/search?q=st2resultstracker to ensure the full service removal.

@winem
Copy link
Contributor Author

winem commented Dec 23, 2020

Hi, I addressed the comment in the other PR and updated both branches so that they're up-to-date with the corresponding master. I'll take care of st2docs as well.

Edit: Took care of the docs; see StackStorm/st2docs#1045

@arm4b
Copy link
Member

arm4b commented Dec 24, 2020

Looks like https://travis-ci.org/github/StackStorm/st2/jobs/751261368#L1513-L1537 CI is not happy about the st2.conf.sample changes.

The PR needs a bit more work to make sure engine doesn't pick up resultstracker module automatically for st2.conf generation.

@winem
Copy link
Contributor Author

winem commented Jan 16, 2021

Is the querier at all still relevant? To me, it looks like it's only used to check the state asynchronously.

So I would also remove the whole st2common.query module: https://github.com/StackStorm/st2/tree/master/st2common/st2common/query

Please let me know if I miss anything and it's also needed for other cases as well. I couldn't find any use of st2common.query.base that still seems to be relevant.

@winem
Copy link
Contributor Author

winem commented Jan 18, 2021

Ok, removing the whole query module causes many test failures related to actionchains for example which is kind of what I expected. See: https://travis-ci.org/github/StackStorm/st2/jobs/754918090#L1954

So I'm sure I missed something and will check the st2 code again to get an idea how to proceed here. I'm happy about any guidance, sharing the task or contributions.

Edit: Now the tests that failed pass but the ci-unit run fails due to a timeout of 300 seconds. Any idea how to mitigate it? This seems to be due to the poor performance of travis. I increased the timeout for the moment.

I guess I am on a good way and know how to proceed here. But it's still true, that any helping hand is welcome. :)

@winem winem force-pushed the winem-remove-st2resultstracker branch from 0f4fea4 to 03aafce Compare January 18, 2021 22:17
@arm4b arm4b added this to In progress in StackStorm v3.4.0 via automation Jan 21, 2021
@arm4b
Copy link
Member

arm4b commented Jan 21, 2021

Oh, nice! Travis unit/integration tests are now ✔️

Looks like we're good here after fixing the e2e test global issues (irrelevant to this PR).

@blag
Copy link
Contributor

blag commented Jan 21, 2021

Working on fixing the end-to-end tests today.

@winem
Copy link
Contributor Author

winem commented Jan 21, 2021

I did a last review today and would keep it as it is - given that the E2E-tests pass as well.

As a follow up to this topic we might discuss if the code for asynchronous runners (everything around the ActionExecutionState incl the db model, API, etc.) is still needed. But that's out of scope for this one.

And tbh I'm super happy that I got this done. :)

@blag blag marked this pull request as ready for review January 21, 2021 22:58
Copy link
Contributor

@amanda11 amanda11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a query to confirm why a particular test was removed - see comments.

st2actions/tests/unit/test_runner_container.py Outdated Show resolved Hide resolved
@amanda11
Copy link
Contributor

amanda11 commented Jan 22, 2021

LGTM - but I'd appreciate another set of eyes - e.g. @blag / @armab sign off. I presume the agreement was to get rid of the resultstracker code plus the query/callback feature that was previously only used by mistral.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL PR that changes 500-999 lines. Consider splitting work into several ones that easier to review.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

Remove the st2resulstracker service from st2ctl and development env
4 participants