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

PTEUDO-1422: Publish status events for state changes in database #377

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bfabricio
Copy link
Contributor

@bfabricio bfabricio commented Dec 10, 2024

Summary of changes

  1. Expand the DatabaseClaimStatus condition list by adding more conditions.
  2. Refactor the claim status logic and implement corresponding unit tests.
  3. Create unit tests for the mode selector.

Demo

DB claim

---
apiVersion: persistance.atlas.infoblox.com/v1
kind: DatabaseClaim
metadata:
  name: fbottega-sample-database
  namespace: fbottega
spec:
  class: fbottega
  appId: some-app 
  secretName: sample-secret-name
  type: postgres
  userName: sample-user
  databaseName:  sample-db
  dsnName: dsn.txt
  dbVersion: "15.3"
  minStorageGB: 20 
$ make build-images push-images deploy
...

$ k get databaseclaim fbottega-sample-database -o yaml
...
  conditions:
  - lastTransitionTime: "2024-12-16T11:05:07Z"
    message: Database is provisioned.
    observedGeneration: 3
    reason: Available
    status: "True"
    type: Synced
  - lastTransitionTime: "2024-12-16T10:43:59Z"
    message: Database successfully synchronized and ready for use.
    observedGeneration: 3
    reason: Available
    status: "True"
    type: Ready
...

@bfabricio bfabricio added the WIP label Dec 10, 2024
@bfabricio bfabricio marked this pull request as ready for review December 12, 2024 12:03
// reflect the error that occurred. It returns the error that
// should be returned by the Reconcile function.
func manageError(ctx context.Context, cli client.Client, claim *v1.DatabaseClaim, inErr error) (ctrl.Result, error) {
func (m *StatusManager) SetErrorStatus(ctx context.Context, dbClaim *v1.DatabaseClaim, inErr error) (reconcile.Result, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can change the method name to SetError since the struct already has the Status (StatusManager).

m.SetStatusCondition(ctx, dbClaim, ReconcileErrorCondition(inErr))
var wrappedErr *managedErr
if existingErr, isManaged := inErr.(*managedErr); isManaged {
logr.Error(existingErr, "manageError called multiple times for the same error")
Copy link
Contributor

@drewwells drewwells Dec 12, 2024

Choose a reason for hiding this comment

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

At some point, this could be promoted to a panic. So we can identify and fix the duplidate error calls happening

Comment on lines 95 to 98
//if object is getting deleted then call requeue immediately
if !dbClaim.ObjectMeta.DeletionTimestamp.IsZero() {
return ctrl.Result{Requeue: true}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't you check for deleted state before updating the status?

@drewwells
Copy link
Contributor

I think you could move claimstatus out of the pkg/databaseclaim package.

Comment on lines 20 to 25
ConditionReady ConditionType = "Ready"
ConditionProvisioning ConditionType = "Provisioning"
ConditionDeleting ConditionType = "Deleting"
ConditionMigrating ConditionType = "Migrating"
PasswordRotation ConditionType = "PasswordRotation"
ConditionConnectionIssue ConditionType = "ConnectionFailed"
Copy link
Contributor

@drewwells drewwells Dec 12, 2024

Choose a reason for hiding this comment

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

I would collapse these to something similiar to Crossplane ConditionReady, ConditionSync. All of these other condition states could be part of those 2.
ConditionSync would indicate if db-controller is performing any operation provisioning, deleting, migrating. It is True when all state is synced to Crossplane/AWS (requires checking Crossplane CR status). I would also consider that implicit versioning errors in the sync field as NeedsMigrate. That may require a separate ticket and be out of scope for this one.

Ready indicates the database is not just synced but available for user. User credentials are created and within rotation period. If credentials are expired, then ready should be false.

Status ConditionReady ConditionSync
Ready True True
Provisioning True False
Deleting True False
Migrating True False
ConnectionIssue False True

@bfabricio bfabricio removed the WIP label Dec 13, 2024
}

func (m *StatusManager) SetStatusCondition(ctx context.Context, dbClaim *v1.DatabaseClaim, condition metav1.Condition) {
logf := log.FromContext(ctx).WithValues("databaseclaim", dbClaim.Name)

condition.LastTransitionTime = metav1.Now()
Copy link
Contributor

Choose a reason for hiding this comment

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

The LastTransitionTime is set already when you create the condition:

func CreateCondition(condType ConditionType, status metav1.ConditionStatus, reason, message string) metav1.Condition {
now := metav1.NewTime(time.Now())
return metav1.Condition{
Type: string(condType),
Status: status,
Reason: reason,
Message: message,
LastTransitionTime: now,
}
}
.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed from the create condition as it makes more sense to keep it here, closer to the update operation.

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.

3 participants