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: store artifacts in OCI registry #3328

Merged
merged 2 commits into from
Nov 15, 2024

Conversation

stuartwdouglas
Copy link
Collaborator

Initial draft of storing artifacts in the OCI registry, not really ready for review and needs a lot of work:

Basically:

  • Starts a registry container in dev mode that is used to store the deployment content
  • Deletes the artefacts table
  • The controller uploads artifacts to the registry on deployment
  • The runner pulls from the registry to download its artifacts

This was referenced Nov 5, 2024
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 3 times, most recently from 0175633 to c4ff2b2 Compare November 5, 2024 22:33
@stuartwdouglas stuartwdouglas added the skip-proto-breaking PRs with this label will skip the breaking proto check label Nov 5, 2024
@stuartwdouglas stuartwdouglas marked this pull request as ready for review November 5, 2024 22:46
@stuartwdouglas stuartwdouglas requested review from wesbillman and removed request for a team November 5, 2024 22:46
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 2 times, most recently from 69b2ad2 to 81a3df3 Compare November 5, 2024 22:58
@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 Nov 5, 2024
@stuartwdouglas stuartwdouglas requested a review from a team as a code owner November 5, 2024 23:54
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 7 times, most recently from cf14e66 to 5e29180 Compare November 12, 2024 23:57
@@ -280,6 +280,8 @@ jobs:
run: just pnpm-install
- name: Build Language Plugins
run: just build-language-plugins
- name: Pull Registry
run: docker image pull registry:2
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should just use docker compose for this yeah?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For some reason it was failing to pull on CI, and I have no idea why. Explicitly pulling it worked for some reason. I can remove this and try again in case it was a transient failure, but it was very odd.

// in the interim releases and artefacts will continue to be linked via the `deployment_artefacts` table
Handle *libdal.Handle[ContainerService]
db sql.Querier
type OCIArtifactService struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We have been using "artEfact" everywhere else

backend/controller/artefacts/oci_registry.go Outdated Show resolved Hide resolved
HeartbeatJitter time.Duration `help:"Jitter to add to heartbeat period." default:"2s"`
Deployment string `help:"The deployment this runner is for." env:"FTL_DEPLOYMENT"`
DebugPort int `help:"The port to use for debugging." env:"FTL_DEBUG_PORT"`
Registry artefacts.RegistryConfig `embed:""`
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should have prefix:"oci-" otherwise the flags will be difficult to distinguish from other ones in the future.

@@ -41,3 +41,7 @@ provisioner:
istio:
enabled: true

registry:
repository: "ftl-registry:5000/ftl-artefacts"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Possibly not for now, but should we be namespacing these by realm at some point?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just the repository used in the GH Kube tests.

RunnerBase *url.URL `help:"Base bind address for FTL runners." default:"http://127.0.0.1:8893" env:"FTL_RUNNER_BIND"`
Dir string `arg:"" help:"Directory to scan for precompiled modules." default:"."`
ControllerTimeout time.Duration `help:"Timeout for Controller start." default:"30s"`
Registry artefacts.RegistryConfig `embed:""`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Registry artefacts.RegistryConfig `embed:""`
Registry artefacts.RegistryConfig `embed:"" prefix:"oci-"`

Deployment model.DeploymentKey `help:"Deployment to download." arg:""`
Dest string `short:"d" help:"Destination directory." default:"."`
Deployment model.DeploymentKey `help:"Deployment to download." arg:""`
Registry artefacts.RegistryConfig `embed:""`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs a prefix.

I feel like we should be injecting the registry instance itself rather than the config? ie. put the config in the root CLI struct, and add a bind function for this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This and release will be the only commands that directly interact with the registry though, in most other cases we would construct it but not use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It won't construct it unless it's injected

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even so won't it 'pollute' all the other commands with OCI flags that do nothing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In that regard it's similar to the controller endpoint, which is unused by some commands but used in enough places that it warrants being global. This will be used in at least 5 locations that I'm aware of - download, serve/dev, release, deploy, box

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

serve/dev don't use it, as they start their own registry, deploy uploads through the controller so that the local user does not need OCI credentials. Thinking about it I am not sure if we want download / release to actually interact directly with the registry long term either, rather than going through a controller so the controller can gate the interactions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I kind of agree, in which case we should keep the current gRPC interface...

Copy link
Collaborator

Choose a reason for hiding this comment

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

And continue to use it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

(sorry, for downloads - WDYT?)

)

// Artefacts downloads artefacts for a deployment from the Controller.
func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient, key model.DeploymentKey, dest string) error {
func Artefacts(ctx context.Context, client ftlv1connect.ControllerServiceClient, key model.DeploymentKey, dest string, registry artefacts.RegistryConfig) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It'd be cleaner if we injected the artefact client rather than the config, I think. Better for testing etc.

@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 2 times, most recently from fae4665 to fe75727 Compare November 13, 2024 01:43
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 3 times, most recently from 67dd5ac to 2d7a3e7 Compare November 13, 2024 03:32
@stuartwdouglas stuartwdouglas force-pushed the stuartwdouglas/oci-registries branch 4 times, most recently from 06c6e04 to e4711a6 Compare November 13, 2024 10:49
@stuartwdouglas
Copy link
Collaborator Author

l have changed this so download now goes through the controller.

This removes the artefacts table and instead stores all binaries in a
container registry. This means that FTL now requires a registry to operate,
and in dev mode it is spun up automatically in a similar manner to postgres.

Uploads are done via the controller, and the runner pulls content directly
from the registry.
@stuartwdouglas stuartwdouglas enabled auto-merge (squash) November 15, 2024 01:31
@stuartwdouglas stuartwdouglas merged commit b02c014 into main Nov 15, 2024
92 checks passed
@stuartwdouglas stuartwdouglas deleted the stuartwdouglas/oci-registries branch November 15, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run-all A PR with this label will run the full set of CI jobs in the PR rather than in the merge queue 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.

2 participants