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

resource service triggers background tasks before transaction is committed #6955

Closed
sanderr opened this issue Jan 9, 2024 · 2 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@sanderr
Copy link
Contributor

sanderr commented Jan 9, 2024

The orchestration service calls the resource service's mark_deployed method in a transaction context. The latter passes it on to resource_action_update, which adds a background task for ConfigurationModel.mark_done_if_done and for agent.resource_event. These background tasks are added before the transaction is committed.

Consequences (hypothesis):

  1. mark_done_if_done inspects resource state. I believe it now races (and typically loses) with the transaction commit so that it never sees all resources as deployed. The configuration model is therefore never marked as deployed. This impacts e.g. inmanta-cli version list. An easy reproduction is to simply run the quickstart. Export a new version without changes to the resources, then run inmanta-cli --host 172.30.0.3 version list -e SR_Linux. It will remain in deploying until a repair is triggered. The web console does show all resources as deployed.
  2. Does agent.resource_event inspect any database state (through an API call)? Is it safe to be used in this transaction context? We may be sending an event for something that hasn't committed yet. When the agent responds to the event by querying the server for resource state, it may get the wrong state.
  3. There may or may not be other flows that pass a connection object into this method.

I did not go more in-depth at this time, because it is not entirely clear to me what is the best way forward. The mark_done_if_done approach might be no longer relevant, in which case it might be better to drop it and use something else for inmanta-cli version list rather than to address the concurrency problem. More input is needed from the team.

@sanderr sanderr added the bug Something isn't working label Jan 9, 2024
@sanderr sanderr added the discussion There is a discussion happening on this issue label Feb 6, 2024
@sanderr
Copy link
Contributor Author

sanderr commented Feb 6, 2024

  1. Investigate scope of the race condition: which background tasks are affected and which are problematic?
  2. If you can fix it in +- 1 day, fix it. Otherwise report back.

@sanderr sanderr removed the discussion There is a discussion happening on this issue label Feb 6, 2024
@wouterdb wouterdb self-assigned this Feb 19, 2024
@wouterdb
Copy link
Contributor

  1. this is the only place where we start a background task in the scope of a transaction

inmantaci pushed a commit that referenced this issue Feb 20, 2024
# Description

Added transaction finalizer
fixes #6955

# Self Check:

Strike through any lines that are not applicable (`~~line~~`) then check the box

- [x] Attached issue to pull request
- [x] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [ ] End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
- [ ] If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)
inmantaci pushed a commit that referenced this issue Feb 20, 2024
Pull request opened by the merge tool on behalf of #7220
inmantaci pushed a commit that referenced this issue Feb 20, 2024
@sanderr sanderr mentioned this issue Mar 1, 2024
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants