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

feat: Language plugins choose the runner image #3268

Merged
merged 10 commits into from
Nov 7, 2024
Merged

feat: Language plugins choose the runner image #3268

merged 10 commits into from
Nov 7, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Oct 31, 2024

Language plugins can now declare the runner image to use by including a ModuleRuntime in the Module schema.
Image names do not include the tag to use, instead FTL will use the same tag that the rest of FTL is using.
If a language plugin does not choose a runner image, it is defaulted toftl0/ftl-runner.

@ftl-robot ftl-robot mentioned this pull request Oct 31, 2024
@stuartwdouglas stuartwdouglas added the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Oct 31, 2024
@matt2e matt2e added the skip-proto-breaking PRs with this label will skip the breaking proto check label Nov 3, 2024
@matt2e matt2e force-pushed the matt2e/docker branch 5 times, most recently from 09336f9 to 9c1bfbf Compare November 6, 2024 01:31
@matt2e
Copy link
Collaborator Author

matt2e commented Nov 6, 2024

Previously, the language plugin protocol had a separate field to indicate the runner image. The approach has changed to using ModuleRuntime within the schema as that felt like a natural place to store it and it already part of the schema.Module that is being returned by plugins.
LMK if ModuleRuntime is not a good fit.

@matt2e matt2e marked this pull request as ready for review November 6, 2024 03:07
@matt2e matt2e requested review from a team and alecthomas as code owners November 6, 2024 03:07
@matt2e matt2e requested review from worstell and wesbillman and removed request for a team November 6, 2024 03:07
@@ -23,6 +23,17 @@ jobs:
name: docker-runner-artifact
path: artifacts/ftl-runner/ftl-runner.tar
retention-days: 1
- name: Build JVM Runner
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could/should this be a matrix instead? Compute the matrix using something like ftl language list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in the future yes, but we need to have ftl-runner available before building ftl-runner-jvm.
Once theres more than one image built on top of ftl-runner (python super soon), then those can be put in a matrix.

@@ -0,0 +1,22 @@
# Huge hack, we want the JVM and same runtime environment as the runner
Copy link
Collaborator

Choose a reason for hiding this comment

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

The runner image doesn't need the JVM though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need the JVM to run the java module bytecode though right?
(This hack is from before this PR btw, it's just been moved out of the normal runner to the jvm runner)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know what the intermediate state is, but the end state is that the base runner image knows nothing about the JVM, Python, or any runtime, and each runtime-specific image layers on top of the runner image.

Re. this specific comment, I don't quite understand why the base runner would still contain the JVM, but maybe it's an intermediate step?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahhh yes yes. It's pulling from ftl0/ftl-runner which as of this PR contains the JVM, and ftl0/ftl-runner-jvm has not yet been published.
Once this PR makes it to release I'll move it to pulling from the jvm runner.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was a hack to make the image build faster and to prevent the GHA runners running out of disk space.

I don't know what the intermediate state is, but the end state is that the base runner image knows nothing about the JVM, Python, or any runtime, and each runtime-specific image layers on top of the runner image.

@matt2e we should probably change the builds to use FROM instead of basically building a new base image each time. This will reduce that amount of network traffic pulling images as they will share a base layer.

internal/schema/module.go Outdated Show resolved Hide resolved
# Conflicts:
#	backend/protos/xyz/block/ftl/v1/schema/schema.pb.go
#	internal/buildengine/languageplugin/external_plugin.go
# Conflicts:
#	backend/protos/xyz/block/ftl/v1/language/language.pb.go
# Conflicts:
#	jvm-runtime/plugin/common/jvmcommon.go
@matt2e matt2e removed the run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue label Nov 7, 2024
@matt2e matt2e force-pushed the matt2e/docker branch 2 times, most recently from f0df19a to ced8165 Compare November 7, 2024 00:41
@matt2e matt2e merged commit ab91f25 into main Nov 7, 2024
183 of 187 checks passed
@matt2e matt2e deleted the matt2e/docker branch November 7, 2024 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-proto-breaking PRs with this label will skip the breaking proto check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants