feat(dqlite): model status service layer #18393

ycliuhw · 2024-11-20T06:55:36Z

This PR introduces the a new service method in the model domain package for the model status feature:

Status - the model status will be computed in the service layer and returned as:
- destroying if the model is being destroyed;
- suspended if the model has an invalid cloud credential;
- busy if the model is being migrated;
- in other cases, it returns available;

Drive-by:

after discussion, we decided to compute all model statuses, so the model status DDL has been removed.

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
~~Integration tests, with comments saying what you're testing~~
~~doc.go added or updated in changed packages~~

QA steps

unit tests.

Documentation changes

No

Links

Jira card: JUJU-7099

tlm · 2024-11-21T01:58:53Z

Hey @ycliuhw,

Cheers for the PR. Are we able to move this implementation into the model service (https://github.com/juju/juju/blob/main/domain/model/service/modelservice.go) ?

tlm

Cheers @ycliuhw, Ignore any comments I have made that you are already on top of. I will get cracking on the state layer.

domain/modelstatus/service/service.go

domain/model/errors/errors.go

domain/model/service/modelservice.go

tlm · 2024-11-24T21:11:32Z

domain/model/service/modelservice.go

+	return s.modelSt.Delete(ctx, s.modelID)
+}
+
+// Status returns the status of the model.


this should include the word "current" I think. For example "Status returns the current status of the model".

We should also document the possible error returns that could result from this function.

For example model not found is a likely scenario even if small.

The state does not exist yet. We could probably assume the state should return a not found error at least.

domain/model/service/modelservice.go

tlm · 2024-11-24T21:38:03Z

domain/model/service/modelservice.go

+	if modelState.Destroying {
+		return model.StatusInfo{
+			Status:  corestatus.Destroying,
+			Message: "the model is being destroyed",


We don't seem to be setting the time field on any off these computed status values.

No, because we don't know the time of the status change.
If you can figure out where we can find it, feel free to include the time in the result returned from the state.

We'll need to ensure the timestamp for the underlying source is modelled. Until then, the caller can interpret zero time as "unknown". We should make sure to update things like juju status to avoid printing junk.

Are you sure that is what we want to do? Should we just not set time.Now() at least that has meaning over time.Zero().

If we tell the client to start doing is == time.Zero it means we have changed the contract of the api and we should technically bump the apiserver facade version.

Keep it simple I say.

domain/model/service/modelservice.go

tlm

looking good. Just a couple of small things.

domain/model/service/modelservice.go

domain/model/service/modelservice_test.go

domain/model/types.go

wallyworld

We need to remove "busy" as a status that can be set

wallyworld · 2024-11-25T06:26:31Z

domain/schema/model/sql/0025-model-status.sql

@@ -8,8 +8,7 @@ INSERT INTO model_status_value VALUES
 (0, 'error'),
 (1, 'available'),
 -- We set the model status to busy when the model is being upgraded.
-(2, 'busy'),
-(3, 'suspended');
+(2, 'busy');


We are going to derive this from whether a model is migrating, right?

We still use busy for upgrade.

domain/model/state/state.go

domain/model/service/modelservice.go

domain/model/service/modelservice_test.go

wallyworld

Can we please change timestamp to time.Time rather than a pointer. I originally gave a +1 to make progress but we should make this change before landing.

domain/model/types.go

wallyworld

Until timestamp is modelled, we'll need to ensure things like juju status do not print junk when the timestamp is zero.

wallyworld · 2024-11-26T01:51:41Z

domain/model/service/modelservice.go

+	if modelState.Destroying {
+		return model.StatusInfo{
+			Status:  corestatus.Destroying,
+			Message: "the model is being destroyed",


We'll need to ensure the timestamp for the underlying source is modelled. Until then, the caller can interpret zero time as "unknown". We should make sure to update things like juju status to avoid printing junk.

domain/model/errors/errors.go

tlm

Ta @ycliuhw, just a few quick comments.

domain/model/service/modelservice.go

tlm · 2024-11-26T02:39:33Z

domain/model/service/modelservice.go

+	if modelState.Destroying {
+		return model.StatusInfo{
+			Status:  corestatus.Destroying,
+			Message: "the model is being destroyed",


Are you sure that is what we want to do? Should we just not set time.Now() at least that has meaning over time.Zero().

If we tell the client to start doing is == time.Zero it means we have changed the contract of the api and we should technically bump the apiserver facade version.

Keep it simple I say.

domain/model/state/modelstate.go

ycliuhw · 2024-11-26T05:43:28Z

/merge

#18437 This PR introduces several clean up tasks to the model state around moving over to internal errors and also better defining some of the types we use on the database. The main goal of this PR is to introduce a new state method for fetching a model's "state". That is the various state various parts of the model are in at any given time so that a model status can be derived from data. It has been decided to implement model status in this way instead of a specific table in the database to track the status of a model. With this approach we are able to calculate the status of model at a given time and not have the complex logic needed to control status setting for a model from several places in the code base. This PR was done in conjunction with #18393 ## Checklist - [x] Code style: imports ordered, good names, simple structure, etc - [x] Comments saying why design decisions were made - [x] Go unit tests, with comments saying what you're testing - ~[ ] [Integration tests](https://github.com/juju/juju/tree/main/tests), with comments saying what you're testing~ - ~[ ] [doc.go](https://discourse.charmhub.io/t/readme-in-packages/451) added or updated in changed packages~ ## QA steps Unit tests are enough for now. ## Documentation changes N/A ## Links **Jira card:** JUJU-7100

manadart

@tlm, @ycliuhw Can we do a follow up to this one please?

// ModelState is the model state required by this service. This is the model
// database state, not the controller state.
type ModelState interface {
	// Create creates a new model with all of its associated metadata.
	Create(context.Context, model.ReadOnlyModelCreationArgs) error

	// Delete deletes a model.
	Delete(context.Context, coremodel.UUID) error

	// Model returns the read only model information set in the database.
	Model(context.Context) (coremodel.ReadOnlyModel, error)
}

// ControllerState is the controller state required by this service. This is the
// controller database, not the model state.
type ControllerState interface {
	// GetModel returns the model with the given UUID.
	GetModel(context.Context, coremodel.UUID) (coremodel.Model, error)

	// GetModelState returns the model state for the given model.
	// It returns [modelerrors.NotFound] if the model does not exist for the given UUID.
	GetModelState(context.Context, coremodel.UUID) (model.ModelState, error)
}

This is semantically clunky. We have 2 things with the same name, but with entirely different meanings, used within a paragraph of code. I suggest ModelStatus.

GetStatus is returning a derived StatusInfo, but we will need access to the non-derived status via the service. Can we:

Change GetStatus to GetStatusInfo.
Implement a GetStatus that returns the ModelState (proposed as ModelStatus above).
Include Importing in the struct above and the v_model_state view.

hpidcock assigned ycliuhw Nov 20, 2024

hpidcock added the 4.0 label Nov 20, 2024

ycliuhw requested review from tlm and wallyworld November 20, 2024 06:58

ycliuhw force-pushed the model-status-service-layer branch from 65f0ed0 to a0d1a46 Compare November 21, 2024 01:26

feat: add placeholders for model status state methods;

c9b8f3a

ycliuhw force-pushed the model-status-service-layer branch 2 times, most recently from ed99cde to d9547a2 Compare November 22, 2024 05:51

feat: implement model status service layer;

2ec3f7f

ycliuhw force-pushed the model-status-service-layer branch from d9547a2 to 2ec3f7f Compare November 22, 2024 05:54

feat: remove model status suspended;

62a5c35

ycliuhw force-pushed the model-status-service-layer branch from 0650424 to 62a5c35 Compare November 22, 2024 06:08

tlm reviewed Nov 24, 2024

View reviewed changes

ycliuhw force-pushed the model-status-service-layer branch from 4779d10 to b54f522 Compare November 25, 2024 05:18

tlm approved these changes Nov 25, 2024

View reviewed changes

domain/model/service/modelservice.go Outdated Show resolved Hide resolved

domain/model/service/modelservice.go Outdated Show resolved Hide resolved

domain/model/service/modelservice_test.go Outdated Show resolved Hide resolved

domain/model/types.go Outdated Show resolved Hide resolved

feat: status service method returns busy if the model is being migrated;

89bc0b8

ycliuhw force-pushed the model-status-service-layer branch from b54f522 to 89bc0b8 Compare November 25, 2024 06:12

wallyworld approved these changes Nov 25, 2024

View reviewed changes

wallyworld requested changes Nov 25, 2024

View reviewed changes

domain/model/types.go Outdated Show resolved Hide resolved

ycliuhw added 2 commits November 26, 2024 09:13

feat: change since timestamp to non pointer;

c6d06a6

feat: compute all model status;

85c9764

wallyworld approved these changes Nov 26, 2024

View reviewed changes

ycliuhw force-pushed the model-status-service-layer branch from 5b3ea7d to 9ace4ef Compare November 26, 2024 02:19

tlm approved these changes Nov 26, 2024

View reviewed changes

feat: remove the model status ddl;

5d26091

ycliuhw force-pushed the model-status-service-layer branch from 9ace4ef to 5d26091 Compare November 26, 2024 04:50

jujubot merged commit 2c0a80f into juju:main Nov 26, 2024
20 of 21 checks passed

tlm mentioned this pull request Nov 26, 2024

feat: model status state layer #18437

Merged

3 tasks

manadart reviewed Jan 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dqlite): model status service layer #18393

feat(dqlite): model status service layer #18393

ycliuhw commented Nov 20, 2024 •

edited

Loading

tlm commented Nov 21, 2024

tlm left a comment

tlm Nov 24, 2024

ycliuhw Nov 25, 2024

tlm Nov 24, 2024

ycliuhw Nov 24, 2024 •

edited

Loading

wallyworld Nov 26, 2024

tlm Nov 26, 2024

tlm left a comment

wallyworld left a comment

wallyworld Nov 25, 2024

ycliuhw Nov 25, 2024

wallyworld left a comment

wallyworld left a comment

wallyworld Nov 26, 2024

tlm left a comment

tlm Nov 26, 2024

ycliuhw commented Nov 26, 2024

manadart left a comment

feat(dqlite): model status service layer #18393

feat(dqlite): model status service layer #18393

Conversation

ycliuhw commented Nov 20, 2024 • edited Loading

Checklist

QA steps

Documentation changes

Links

tlm commented Nov 21, 2024

tlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycliuhw Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm left a comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

wallyworld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ycliuhw commented Nov 26, 2024

manadart left a comment

Choose a reason for hiding this comment

ycliuhw commented Nov 20, 2024 •

edited

Loading

ycliuhw Nov 24, 2024 •

edited

Loading