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: object store services #18108

Merged
merged 7 commits into from
Sep 23, 2024
Merged

Conversation

SimonRichardson
Copy link
Member

@SimonRichardson SimonRichardson commented Sep 17, 2024

This introduces 2 concepts:

  1. It provides a subset of services to drive the object store.
  2. The model object store is now available in the service factory.

Using these two concepts, we can start interning all the object store usages at the API server level into the domain services. The work to do this was pre-seeded by the provider tracker, so applying it to the object store wasn't a huge lift. The only surprising aspect to this, one that I had forgotten was that we required the controller config. This was easy to implement in the end.

We need to understand if the service factory depending on the upgrade database flag/gate is a wise idea. We can easily change it in the future, but originally was left off because the service factory is lazy evaluated.

Checklist

  • Code style: imports ordered, good names, simple structure, etc
  • Comments saying why design decisions were made
  • Go unit tests, with comments saying what you're testing

QA steps

Ensure that we can still deploy charms and agents. There shouldn't be any real concerns, as the services still exist and those haven't changed. Just that they come from a different worker.

$ juju bootstrap lxd test
$ juju add-model default
$ juju deploy ubuntu

Links

Jira card: JUJU-6549

@SimonRichardson SimonRichardson self-assigned this Sep 17, 2024
@SimonRichardson SimonRichardson changed the title Objectstore services feat: object store services Sep 17, 2024
Comment on lines -1061 to -1068
"provider-tracker",
"is-controller-flag",
"lease-manager",
"object-store-s3-caller",
"provider-service-factory",
"object-store-services",
"query-logger",
"s3-http-client",
"service-factory",
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer require the full-blown service factory for the object store. This means we can drop the provider tracker as well!

@SimonRichardson SimonRichardson force-pushed the objectstore-services branch 2 times, most recently from dfc725d to b1316e7 Compare September 19, 2024 16:11
@SimonRichardson SimonRichardson marked this pull request as ready for review September 19, 2024 16:11
Copy link
Member

@manadart manadart left a comment

Choose a reason for hiding this comment

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

Code is fine, as is QA.

The "singular" naming though. Is it a model object store getter, a singleton store getter? I think we can land on a better name.

core/objectstore/objectstore.go Outdated Show resolved Hide resolved
core/objectstore/objectstore.go Outdated Show resolved Hide resolved
internal/worker/objectstoreservices/worker.go Outdated Show resolved Hide resolved
The objectstore can not be embedded into the service factory because
the objectstore metadata services are inside the service factory. The
DAG of the dependencies prevent this, so to move forward, we can copy
what we've done with the provider tracker worker.

This introduces the concept for the workers. Adding it to the agent
manifolds and pushing it through the API is still required.
The objectstore dependencies have been changed to the objectstore
services (old terminology: object store service factory). This will
allow us to inject the objectstore into the service factory.

There is still a lot to fix, but according to the manifold tests,
we can clearly see that it's possible. So given that signal, I'm
going to press on and complete this.
The controller also has a object store, that also needs to be
migrated across to the new object store services.

The service factory (soon to be renamed) is becoming lighter and
is only offering the services that are serviable by the API
server.

The domain services are in fact real services, not infrastructure.
This is end game.

If this works, it means that we can access the blob store as a
dependency as a first class citizen. This has already shown real
value when it was done for the provider tracker.
The following wires up all the service factory dependencies correctly.
It's now possible to have the object store in the service factory.

There is a question around if the service factory should have a
deprendency on the upgrade database. Originally it didn't because
it's lazy evaluated. Now it's been introduced by the objectstore.

I wonder if that's going to cause us issues.
Now that objectstore has been dropped from the service factory, we
need to regenerate the mocks.
Model is much more discriptive than singular and it gives context
to the resulting expectation.
Copy link
Member

@nvinuesa nvinuesa left a comment

Choose a reason for hiding this comment

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

LGTM and QA passes, thanks!

@SimonRichardson
Copy link
Member Author

/merge

@jujubot jujubot merged commit 9422f01 into juju:main Sep 23, 2024
18 of 20 checks passed
@SimonRichardson SimonRichardson deleted the objectstore-services branch September 23, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants