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

[Old] feat: Changesets #4197

Closed
wants to merge 2 commits into from
Closed

[Old] feat: Changesets #4197

wants to merge 2 commits into from

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Jan 28, 2025

https://hackmd.io/n2G9RlgCRrOZo_-M91NMJw?both

Problems / Missing pieces

Changes to provisioner code hasn't started

Provisioner used to be the one to create the deployment, but instead it needs to watch for changesets to provision. Buildengine now points to schema service to create a changeset, whereas it used to point to provisioner to call Create/ReplaceDeployment.
So currently when you run ftl dev it will get stuck:

  • The first batch of modules try getting deployed by calling schemaservice's CreateChangeset
  • ... no provisioning starts. Dead end

Chicken and egg problem with module schema and deployment key

I imagined this would happen:

  • Schema service receives a CreateChangeset call, creates a changeset with modules.
  • Each of those modules would get a deployment key even though they are not fully provisioned / deployed
    • I assumed deployment key would be how the provisioner sent back progress to schemaservice

But I think things are currently set up such that a module schema only gets a deployment key when it also has an endpoint (both are non optional fields of ModuleRuntimeDeployment).

Assumption for now is that most services only care about canonical schema

So no calls will route to a deployment that is within a changeset that is still being provisioned. Same for cron, ingress, etc etc.
But pubsub breaks that assumption. As the runner becomes ready it will immediately start consuming events. Which can then start making calls which can do whatever...
I think for the first go at changesets that will just have to be a known issue that we fix afterwards.

GetSchema hasn't been updated with options for changesets

The idea was that we would make the GetSchema endpoint allow the caller to specify if they want to get a specific changeset in the response (use case: CI wanting to know if a changeset it kicked off was started)

Pull Schema is currently super cumbersome for most use cases

A lot of the callers of PullSchema are currently broken, and need to be reimplemented with the new events.
But I think the new events are overkill for their use cases. Given most services don't care about changesets, they really just want to care about updates to canonical deployments and when a changeset gets committed to canonical. But the problem is that the event for a changeset being commited does not currently include final module schemas for eveything within that module. So instead they would need to keep track of all the ChangesetCreated and DeploymentUpdated (with changeset key) events just to have an accurate view of the changeset when it gets commited.
So yeh... I think this should get simplified if we just send the whole contents of the deployments along with ChangesetCommited events

Other gotchas / half finished thoughts:

  • I expect we want dependant modules to be able to be put into one changeset. For now for simplicity, buildengine is making a changeset per build batch. I'm not sure how long that will last us and when provisioner will need to start understanding when it can start provisioning each module within a changeset.
  • I'm not loving everything around schemaservice.GetDeployment( deploymentKey + changesetKey) and schemaservice.FindDeployment(deploymentKey) (also returns an optional changeset key). Same goes for whether provisioner needs to include changeset key whenever it wants to update the schema as each resource is provisioned.
    • I like that changeset makes things explicit. But then i don't know how a lot of the code at the moment is lazy and calls FindDeployment(). Maybe that's just growing pains of a refactor like this though. But i can see the advantanges of just using deployment key by itself everywhere.
  • We used to have GetActiveDeployments. i think I got a bit ahead of myself and started delineating between Canonical views of the schema and cases where you'd want the Latest view of the schema which replaces any canonical deployments with those that have been successfully deployed within a changeset.
    • It might be that we don't ever need a Latest view of the schema because nothing should be calling those yet.
    • So i think this is something to back out?
  • We used to have weirdness to handle around DeploymentRemoved events, where we didn't know if it was removed and was about to be replaced by a DeploymentCreated event. Changeset Commited events should now bundle those 2 separate events into one, so there should be some code we can simplify. eg: cron service.
  • Juho mentioned how the schemaservice state will get extra large if each committed changeset keeps it's full module schema around. I think that can be considered properly after this initial changeset implementation though.
  • Tests don't even build yet sorry...

@alecthomas alecthomas mentioned this pull request Jan 28, 2025
@matt2e matt2e changed the title feat: Changesets [Old] feat: Changesets Jan 29, 2025
@matt2e matt2e closed this Feb 3, 2025
@matt2e matt2e deleted the matt2e/changesets/initial branch February 5, 2025 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant