PTEUDO-1088: changed the hash function, refactoring, working in progress #263
Conversation
leandrorichardtoledo
commented
Jul 18, 2024
leandrorichardtoledo
commented
- This is a working in progress of the ticket: PTEUDO-1165;
There was a problem hiding this comment.
I'd back out of databaseclaim changes here and focus on the new naming convention with robust testing around it. You have setup one index 0, but we need support for more, 5 indexes.
Name is a []string with several possibilities. We may only ever use the first index -0 but the code should be robust to be returning. The logic to find the current database needs to also consider the old static engine version, then all of the possible new names.
As for the old name, you use these variables to define it, nothing else. They are immutable now until we move everybody to new naming scheme. https://github.com/infobloxopen/db-controller/blob/main/pkg/hostparams/hostparams.go#L25-L30
| @@ -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 { | |||
There was a problem hiding this comment.
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)
| if r.Input.FragmentKey == "" { | ||
| return prefix + dbClaim.Name + suffix | ||
| } |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
Add unit tests for new and old naming patterns. We need to ensure these are correct.
| dbClaim.Status.ActiveDB = *dbClaim.Status.NewDB.DeepCopy() | ||
| dbClaim.Status.NewDB = v1.Status{ConnectionInfo: &v1.DatabaseClaimConnectionInfo{}} |
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
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)
| case M_MigrationInProgress, M_UpgradeDBInProgress: | ||
| return r.reconcileMigrationInProgress(ctx, dbClaim) | ||
| case M_UseNewDB: | ||
| return r.handleUseNewDB(ctx, dbClaim, logr) |
There was a problem hiding this comment.
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
| logr.Info("existing db reconcile complete") | ||
| return r.manageSuccess(ctx, dbClaim) | ||
| // ensureConnectionInfoInitialized ensures that the ConnectionInfo is initialized. | ||
| func ensureConnectionInfoInitialized(status *v1.Status) { |
There was a problem hiding this comment.
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.
| @@ -77,6 +77,7 @@ type input struct { | |||
| MasterConnInfo v1.DatabaseClaimConnectionInfo | |||
| TempSecret string | |||
| DbHostIdentifier string | |||
| OldDbHostIdentifier string | |||
There was a problem hiding this comment.
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
