refactor: Move Data Layer provisioning from Runner to Coordinator #805
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
morgsmccauley
changed the title
morgsmccauley
changed the title
morgsmccauley
changed the title
morgsmccauley
commented
Jun 17, 2024
| @@ -126,6 +126,8 @@ describe('Indexer integration', () => { | |||
| } | |||
| ); | |||
|
|
|||
| await provisioner.provisionUserApi(indexerConfig); | |||
There was a problem hiding this comment.
Need to provision manually since it's no longer called within execute
morgsmccauley
commented
Jun 17, 2024
| if self.redis_client.indexer_states_set_exists().await? { | ||
| return Ok(()); | ||
| } | ||
| pub async fn migrate(&self) -> anyhow::Result<()> { |
There was a problem hiding this comment.
Migration to add provisioned_state enum, defaulting to Provisioned. There is some risk here if an Indexer hasn't actually been provisioned, but I believe it's unlikely and can handle manually if so.
morgsmccauley
changed the title
morgsmccauley
added a commit
that referenced
this pull request
Jun 20, 2024
This PR removes Data Layer resources on Indexer Delete. To achieve this, the following has been added: - `Provisioner.deprovision()` method which removes: schema, cron jobs, and if necessary, Hasura source, database, and role - `DataLayerService.StartDeprovisioningTask` gRPC method - Calling the above from Coordinator within the delete lifecycle hook In addition to the above, I've slightly refactored DataLayerService to make the addition of de-provisioning more accomodating: - `StartDeprovisioningTask` and `StartProvisioningTask` now return opaque IDs rather than using `accountId`/`functionName` - to avoid conflicts with eachother - There is a single `GetTaskStatus` method which is used for both, before it was specific to provisioning As mentioned in #805, the Coordinator implementation is a little awkward due to the shared/non-blocking Control Loop. I'll look to refactor this later and hopefully improve on this.
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR updates Coordinator to handle Data Layer provisioning, removing the implicit step from Runner. Provisioning itself is still completed within Runner, but Coordinator will trigger and monitor it. Functionally, provisioning is the same, but there are some subtle differences around how it is handled:
A
provisioned_stateenum has been added to the Indexer state within Redis. This is used to persist what stage of provisioning the Data Layer is at, as well as ensuring we only provision once.Concerns with current implementation
Overall, I'm not happy with the implementation here, but feel it is the best we can do given the current structure of Coordinator. As to not block this feature I decided to go forward with this approach, and will create a ticket to refactor/update later.
Provisioning is triggered within the "new" handler, and then polled within the "existing" handler, which seems a little awkward. The separated handling is necessary since no operation within the control loop (i.e.
Synchroniser) should block, as that would block synchronisation for all other Indexers. So we need to trigger the provisioning initially, and then poll the completion each subsequent control loop.I feel we have outgrown the current control loop, and am planning to refactor later. Rather than have a single control loop for all Indexers, I'm thinking we can have dedicated loops for each of them. We could spawn a new task for each Indexer, which then manages its own lifecycle. Then each Indexer is free to wait for as long as it wants, without impacting other Indexers. This would allow us to handle the blocking provisioning step much more elegantly.