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.

Sign up for GitHub

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

Jump to bottom

PTEUDO-1088: changed the hash function, refactoring, working in progress #263

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

PTEUDO-1088: changed the hash function, refactoring, working in progress #263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
213 changes: 133 additions & 80 deletions pkg/databaseclaim/databaseclaim.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ type input struct {
MasterConnInfo v1.DatabaseClaimConnectionInfo
TempSecret string
DbHostIdentifier string
OldDbHostIdentifier string
Copy link
Contributor

Choose a reason for hiding this comment

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

r.Input is a bad idea. Reconcile is a global struct and code is attempting to store per request information in it. What happens if requestA puts a value in MasterConnInfo, then requestB doesn't setup MasterConnInfo and re-uses the one from requestA? Now, we have bugs that are based on order of requests coming in.

I suggest any per-request parameters are stored in the context or simply derived from the ctrl.Request coming in. See this article on how to store things in contexts https://medium.com/@matryer/context-keys-in-go-5312346a868d

HostParams hostparams.HostParams
EnableReplicationRole bool
EnableSuperUser bool
Expand Down Expand Up @@ -323,6 +324,7 @@ func (r *DatabaseClaimReconciler) setReqInfo(ctx context.Context, dbClaim *v1.Da
}

r.Input.DbHostIdentifier = r.getDynamicHostName(dbClaim)
r.Input.OldDbHostIdentifier = r.getOldDynamicHostName(dbClaim)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't agree with this code. We're moving to a new naming scheme that will have many possibilities, probably 5. We wouldn't want 6 variables to keep track of all the possible combinations and also another for the old static naming convention, then update the many many references to these variables to use all 6 variables. I'd remove modifying databaseclaimes for now and just focus on getting the naming scheme right.

If I were writing this, I'd setup logic at the start of reconcile that finds the crossplane CR. For instance, getHostParams. Now all of the methods performing databse operations can rely on a single tested method for discovering databases.

func (r) getHostParams(ctx context.Context, names []string) (*HostParams, error)

}
if r.Config.Viper.GetBool("supportSuperUserElevation") {
r.Input.EnableSuperUser = *dbClaim.Spec.EnableSuperUser
Expand Down Expand Up @@ -531,99 +533,121 @@ func (r *DatabaseClaimReconciler) postMigrationInProgress(ctx context.Context, d
return ctrl.Result{RequeueAfter: time.Minute}, nil
}

// Create, migrate or upgrade database
// executeDbClaimRequest manages the reconciliation logic for different database claim modes.
func (r *DatabaseClaimReconciler) executeDbClaimRequest(ctx context.Context, dbClaim *v1.DatabaseClaim) (ctrl.Result, error) {

logr := log.FromContext(ctx).WithValues("databaseclaim", dbClaim.Namespace+"/"+dbClaim.Name)

if dbClaim.Status.ActiveDB.ConnectionInfo == nil {
dbClaim.Status.ActiveDB.ConnectionInfo = new(v1.DatabaseClaimConnectionInfo)
}
if dbClaim.Status.NewDB.ConnectionInfo == nil {
dbClaim.Status.NewDB.ConnectionInfo = new(v1.DatabaseClaimConnectionInfo)
}
// Ensure connection info is initialized.
ensureConnectionInfoInitialized(&dbClaim.Status.ActiveDB)
ensureConnectionInfoInitialized(&dbClaim.Status.NewDB)

// FIXME: why is a per request value being set in a global variable?
// Set the mode based on the current context and dbClaim
r.mode = r.getMode(ctx, dbClaim)
if r.mode == M_PostMigrationInProgress {

switch r.mode {
case M_PostMigrationInProgress:
return r.postMigrationInProgress(ctx, dbClaim)
case M_UseExistingDB:
return r.handleUseExistingDB(ctx, dbClaim, logr)
case M_MigrateExistingToNewDB:
return r.handleMigrateExistingToNewDB(ctx, dbClaim, logr)
case M_InitiateDBUpgrade:
return r.reconcileMigrateToNewDB(ctx, dbClaim)
case M_MigrationInProgress, M_UpgradeDBInProgress:
return r.reconcileMigrationInProgress(ctx, dbClaim)
case M_UseNewDB:
return r.handleUseNewDB(ctx, dbClaim, logr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like a good refactor, but did you plan on adding tests to make sure you didn't break any of this code? The only migration test we have doesn't use the db-controller at all and just calls code inside pkg/pgctl

default:
logr.Info("unhandled mode")
return r.manageError(ctx, dbClaim, fmt.Errorf("unhandled mode"))
}
if r.mode == M_UseExistingDB {
logr.Info("existing db reconcile started")
err := r.reconcileUseExistingDB(ctx, dbClaim)
if err != nil {
return r.manageError(ctx, dbClaim, err)
}
dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy()
dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}}
}

logr.Info("existing db reconcile complete")
return r.manageSuccess(ctx, dbClaim)
// ensureConnectionInfoInitialized ensures that the ConnectionInfo is initialized.
func ensureConnectionInfoInitialized(status *v1.Status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't a typical naming convention for initializing a pointer. new(type) is the way to do this or a func named NewType ie. v1.NewStatus.

The go convention is to not need any initialization. For example,sync.RWMutex{} just works as is and will even throw go vet errors if you pass it around incorrectly. We can do the same here by removing the unnecessary pointers requiring all this initialization code.

if status.ConnectionInfo == nil {
status.ConnectionInfo = new(v1.DatabaseClaimConnectionInfo)
}
if r.mode == M_MigrateExistingToNewDB {
logr.Info("migrate to new db reconcile started")
//check if existingDB has been already reconciled, else reconcileUseExisitngDB
existing_db_conn, err := v1.ParseUri(dbClaim.Spec.SourceDataFrom.Database.DSN)
if err != nil {
return r.manageError(ctx, dbClaim, err)
}
if (dbClaim.Status.ActiveDB.ConnectionInfo.DatabaseName != existing_db_conn.DatabaseName) ||
(dbClaim.Status.ActiveDB.ConnectionInfo.Host != existing_db_conn.Host) {
}

logr.Info("existing db was not reconciled, calling reconcileUseExisitngDB before reconcileUseExisitngDB")
// handleUseExistingDB handles the M_UseExistingDB mode.
func (r *DatabaseClaimReconciler) handleUseExistingDB(ctx context.Context, dbClaim *v1.DatabaseClaim, logr logr.Logger) (ctrl.Result, error) {
logr.Info("existing db reconcile started")

err := r.reconcileUseExistingDB(ctx, dbClaim)
if err != nil {
return r.manageError(ctx, dbClaim, err)
}
dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy()
dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}}
}

return r.reconcileMigrateToNewDB(ctx, dbClaim)
if err := r.reconcileUseExistingDB(ctx, dbClaim); err != nil {
return r.manageError(ctx, dbClaim, err)
}
if r.mode == M_InitiateDBUpgrade {
logr.Info("upgrade db initiated")

return r.reconcileMigrateToNewDB(ctx, dbClaim)
dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy()
dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}}
Comment on lines +582 to +583
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, the effort to use and make sure ConnectionInfo is setup is intense. There's at least 4 times this code is duplicated and ConnectionInfo is initialized 8 times.

The best I can tell is somebody thought they would make ConnectionInfo a pointer so they can check for nil in a few places. However, ConnectionInfo is never nil due to this https://github.com/infobloxopen/db-controller/blob/main/pkg/databaseclaim/databaseclaim.go#L378-L383. Just change the type to a struct and remove all of this code initializing ConnectionInfo.

logr.Info("existing db reconcile complete")

return r.manageSuccess(ctx, dbClaim)
}

// handleMigrateExistingToNewDB handles the M_MigrateExistingToNewDB mode.
func (r *DatabaseClaimReconciler) handleMigrateExistingToNewDB(ctx context.Context, dbClaim *v1.DatabaseClaim, logr logr.Logger) (ctrl.Result, error) {
logr.Info("migrate to new db reconcile started")

existingDBConn, err := v1.ParseUri(dbClaim.Spec.SourceDataFrom.Database.DSN)
if err != nil {
return r.manageError(ctx, dbClaim, err)
}
if r.mode == M_MigrationInProgress || r.mode == M_UpgradeDBInProgress {
return r.reconcileMigrationInProgress(ctx, dbClaim)
}
if r.mode == M_UseNewDB {

logr.Info("Use new DB")
result, err := r.reconcileNewDB(ctx, dbClaim)
if err != nil {
if !isExistingDBReconciled(dbClaim, existingDBConn) {
logr.Info("existing db was not reconciled, calling reconcileUseExistingDB before reconcileUseExistingDB")
if err := r.reconcileUseExistingDB(ctx, dbClaim); err != nil {
return r.manageError(ctx, dbClaim, err)
}
if result.RequeueAfter > 0 {
logr.Info("requeuing request")
return result, nil
}
if r.Input.TempSecret != "" {
newDBConnInfo := dbClaim.Status.NewDB.ConnectionInfo.DeepCopy()
newDBConnInfo.Password = r.Input.TempSecret

if err := r.createOrUpdateSecret(ctx, dbClaim, newDBConnInfo); err != nil {
return r.manageError(ctx, dbClaim, err)
}
}
dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy()
if r.Input.SharedDBHost {
dbClaim.Status.ActiveDB.DbState = v1.UsingSharedHost
} else {
dbClaim.Status.ActiveDB.DbState = v1.Ready
}
dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}}
}

return r.reconcileMigrateToNewDB(ctx, dbClaim)
}

// isExistingDBReconciled checks if the existing DB has been reconciled.
func isExistingDBReconciled(dbClaim *v1.DatabaseClaim, existingDBConn *v1.DatabaseClaimConnectionInfo) bool {
return dbClaim.Status.ActiveDB.ConnectionInfo.DatabaseName == existingDBConn.DatabaseName &&
dbClaim.Status.ActiveDB.ConnectionInfo.Host == existingDBConn.Host
}

// handleUseNewDB handles the M_UseNewDB mode.
func (r *DatabaseClaimReconciler) handleUseNewDB(ctx context.Context, dbClaim *v1.DatabaseClaim, logr logr.Logger) (ctrl.Result, error) {
logr.Info("use new db")
result, err := r.reconcileNewDB(ctx, dbClaim)
if err != nil {
return r.manageError(ctx, dbClaim, err)
}

return r.manageSuccess(ctx, dbClaim)
if result.RequeueAfter > 0 {
logr.Info("requeuing request")
return result, nil
}

if r.Input.TempSecret != "" {
newDBConnInfo := dbClaim.Status.NewDB.ConnectionInfo.DeepCopy()
newDBConnInfo.Password = r.Input.TempSecret
if err := r.createOrUpdateSecret(ctx, dbClaim, newDBConnInfo); err != nil {
return r.manageError(ctx, dbClaim, err)
}
}

logr.Info("unhandled mode")
return r.manageError(ctx, dbClaim, fmt.Errorf("unhandled mode"))
dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy()
dbClaim.Status.ActiveDB.DbState = getDBState(r.Input.SharedDBHost)
dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}}

return r.manageSuccess(ctx, dbClaim)
}

// getDBState returns the appropriate DB state based on whether the DB is shared.
func getDBState(sharedDBHost bool) v1.DbState {
if sharedDBHost {
return v1.UsingSharedHost
}
return v1.Ready
}

func (r *DatabaseClaimReconciler) reconcileUseExistingDB(ctx context.Context, dbClaim *v1.DatabaseClaim) error {
Expand Down Expand Up @@ -775,10 +799,10 @@ func (r *DatabaseClaimReconciler) reconcileNewDB(ctx context.Context, dbClaim *v
return ctrl.Result{}, nil
}

func (r *DatabaseClaimReconciler) reconcileMigrateToNewDB(ctx context.Context,
dbClaim *v1.DatabaseClaim) (ctrl.Result, error) {
func (r *DatabaseClaimReconciler) reconcileMigrateToNewDB(ctx context.Context, dbClaim *v1.DatabaseClaim) (ctrl.Result, error) {

logr := log.FromContext(ctx)
logr.Info("upgrade db initiated")

if dbClaim.Status.MigrationState == "" {
dbClaim.Status.MigrationState = pgctl.S_Initial.String()
Expand Down Expand Up @@ -1578,6 +1602,22 @@ func (r *DatabaseClaimReconciler) getDynamicHostName(dbClaim *v1.DatabaseClaim)
return prefix + r.Input.FragmentKey + suffix
}

// getDynamicHostName is used to name the crossplane
// dbinstance CRs
func (r *DatabaseClaimReconciler) getOldDynamicHostName(dbClaim *v1.DatabaseClaim) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this entire func belongs in the HostParams package, not here. Something like this. The EngineName needs to be a separate special CLI argument that is defined as a required program parameter and specified in the helm chart so we can have per env override. This variable on eu-com-1 can never change until all databases there are recreated with new names. It will be static forever since it's impossible to change it due to current databases are named with it.

Here's a possible name for this special method

func (h *HostParams) DeprecatedDBHostName(envName string, staticEngineVersion string, dbClaimName string)

var prefix string
suffix := "-" + r.Input.HostParams.OldHash()

if r.Config.DbIdentifierPrefix != "" {
prefix = r.Config.DbIdentifierPrefix + "-"
}
if r.Input.FragmentKey == "" {
return prefix + dbClaim.Name + suffix
}
Comment on lines +1614 to +1616
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove all references to FragmentKey that you find. It is not used at all in db-controller. It's only used for an old and broken piece of software renamed to db-controller-legacy, we don't need to support it.


return prefix + r.Input.FragmentKey + suffix
}

func (r *DatabaseClaimReconciler) getParameterGroupName(dbClaim *v1.DatabaseClaim) string {
hostName := r.getDynamicHostName(dbClaim)
params := &r.Input.HostParams
Expand Down Expand Up @@ -1970,7 +2010,6 @@ func (r *DatabaseClaimReconciler) managePostgresDBInstance(ctx context.Context,
Name: r.getProviderConfig(),
}
restoreFromSource := defaultRestoreFromSource
dbInstance := &crossplanerds.DBInstance{}

params := &r.Input.HostParams
ms64 := int64(params.MinStorageGB)
Expand All @@ -1987,16 +2026,31 @@ func (r *DatabaseClaimReconciler) managePostgresDBInstance(ctx context.Context,
maxStorageVal = &params.MaxStorageGB
}

err = r.Client.Get(ctx, client.ObjectKey{
Name: dbHostName,
}, dbInstance)
var dbInstance crossplanerds.DBInstance
oldKey := client.ObjectKey{
Name: r.Input.OldDbHostIdentifier,
}
err = r.Client.Get(ctx, oldKey, &dbInstance)
if err != nil {
if errors.IsNotFound(err) {
if !errors.IsNotFound(err) {
return false, fmt.Errorf("error getting db instance key: %s: %w", oldKey.Name, err)
}

key := client.ObjectKey{
Name: dbHostName,
}
err = r.Client.Get(ctx, key, &dbInstance)
if err != nil {
if !errors.IsNotFound(err) {
return false, fmt.Errorf("error getting db instance key: %s: %w", key.Name, err)
}

validationError := params.CheckEngineVersion()
if validationError != nil {
return false, validationError
}
dbInstance = &crossplanerds.DBInstance{

dbInstance = crossplanerds.DBInstance{
ObjectMeta: metav1.ObjectMeta{
Name: dbHostName,
// TODO - Figure out the proper labels for resource
Expand Down Expand Up @@ -2048,6 +2102,7 @@ func (r *DatabaseClaimReconciler) managePostgresDBInstance(ctx context.Context,
},
},
}

if r.mode == M_UseNewDB && dbClaim.Spec.RestoreFrom != "" {
snapshotID := dbClaim.Spec.RestoreFrom
dbInstance.Spec.ForProvider.CustomDBInstanceParameters.RestoreFrom = &crossplanerds.RestoreDBInstanceBackupConfiguration{
Expand All @@ -2065,22 +2120,20 @@ func (r *DatabaseClaimReconciler) managePostgresDBInstance(ctx context.Context,
}
//create DBInstance
logr.Info("creating crossplane DBInstance resource", "DBInstance", dbInstance.Name)
if err := r.Client.Create(ctx, dbInstance); err != nil {
if err := r.Client.Create(ctx, &dbInstance); err != nil {
return false, err
}
} else {
//not errors.IsNotFound(err) {
return false, err
}
}

// Deletion is long running task check that is not being deleted.
if !dbInstance.ObjectMeta.DeletionTimestamp.IsZero() {
err = fmt.Errorf("can not create Cloud DB instance %s it is being deleted", dbHostName)
logr.Error(err, "DBInstance", "dbHostIdentifier", dbHostName)
return false, err
}

_, err = r.updateDBInstance(ctx, dbClaim, dbInstance)
_, err = r.updateDBInstance(ctx, dbClaim, &dbInstance)
if err != nil {
return false, err
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/hostparams/hostparams.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,24 @@ type HostParams struct {
}

func (p *HostParams) String() string {
return fmt.Sprintf("%s-%s-%s", p.Engine, p.InstanceClass, p.EngineVersion)
index := 0 // TODO: this should comes from somewhere else.
return fmt.Sprintf("%s-%d", p.InstanceClass, index)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add unit tests for new and old naming patterns. We need to ensure these are correct.

}

func (p *HostParams) Hash() string {
crc32q := crc32.MakeTable(0xD5828281)
return fmt.Sprintf("%08x", crc32.Checksum([]byte(p.String()), crc32q))
}

func (p *HostParams) OldString() string {
return fmt.Sprintf("%s-%s-%s", p.Engine, p.InstanceClass, p.EngineVersion)
}

func (p *HostParams) OldHash() string {
crc32q := crc32.MakeTable(0xD5828281)
return fmt.Sprintf("%08x", crc32.Checksum([]byte(p.OldString()), crc32q))
}

func (p *HostParams) HasShapeChanged(activeShape string) bool {
if p.isDefaultShape {
// request is for a "" shape
Expand Down
Loading