CLOUDP-62444: Adopt functional approach for StatefulSet construction #63
Conversation
| // is currently in the ready state | ||
| func (r *ReplicaSetReconciler) isStatefulSetReady(mdb mdbv1.MongoDB, expectedUpdateStrategy appsv1.StatefulSetUpdateStrategyType) (bool, error) { | ||
| // TODO: This sleep will be addressed as part of https://jira.mongodb.org/browse/CLOUDP-62444 | ||
| time.Sleep(time.Second * 5) |
|
|
||
| // buildStatefulSet takes a MongoDB resource and converts it into | ||
| // the corresponding stateful set | ||
| func buildStatefulSet(mdb mdbv1.MongoDB) (appsv1.StatefulSet, error) { |
| func buildStatefulSet(mdb mdbv1.MongoDB) (appsv1.StatefulSet, error) { | ||
| sts := appsv1.StatefulSet{} | ||
| buildStatefulSetModificationFunction(mdb)(&sts) | ||
| return sts, nil |
There was a problem hiding this comment.
if we can never return an error, then we can just get rid of it
| if ready, err := r.isStatefulSetReady(mdb, getUpdateStrategyType(mdb)); err != nil { | ||
| currentSts := appsv1.StatefulSet{} | ||
| if err := r.client.Get(context.TODO(), mdb.NamespacedName(), ¤tSts); err != nil { | ||
| r.log.Warnf("Error getting StatefulSet: %s", err) |
There was a problem hiding this comment.
[nit] do we want to use r.log.WarnF here? (Almost) everywhere else we use r.log.InfoF(except one case in which we use ErrorF) Maybe we should use the same everywhere for consistency?
(btw I would be more in favour of using WarnF instead of InfoF
There was a problem hiding this comment.
Yep we need to make sure we're consistent with our logging levels. 👍
| func (r *ReplicaSetReconciler) createOrUpdateStatefulSet(mdb mdbv1.MongoDB) error { | ||
| sts, err := buildStatefulSet(mdb) | ||
| set := appsv1.StatefulSet{} | ||
| err := r.client.Get(context.TODO(), types.NamespacedName{Name: mdb.Name, Namespace: mdb.Namespace}, &set) |
There was a problem hiding this comment.
Here we can use the new NamespacedName() function you introduced
| @@ -0,0 +1,33 @@ | |||
| package persistantvolumeclaim | |||
There was a problem hiding this comment.
The package is called persistAntvolumeclaim instead of persistEntvolumeclaim, which is the term used in k8s. Is this a typo?
There was a problem hiding this comment.
almost definitely, nice catch!
pkg/kube/statefulset/statefulset.go
Outdated
| func WithVolumeClaim(name string, f func(*corev1.PersistentVolumeClaim)) Modification { | ||
| return func(set *appsv1.StatefulSet) { | ||
| idx := findVolumeClaimIndexByName(name, set.Spec.VolumeClaimTemplates) | ||
| if idx == -1 { |
There was a problem hiding this comment.
We could have a not_found constant as in podspec_template here.
bznein
left a comment
bznein
left a comment
There was a problem hiding this comment.
LGTM with a few minor comments
antonlisovenko
left a comment
antonlisovenko
left a comment
There was a problem hiding this comment.
love this PR, functional approach suits very well the need to sequentially "enrich" K8s objects - I think this looks clearer and easier to support than the builder pattern
🥇
All Submissions:
This PR replaces the StatefulSet builder with a functional approach.
This allows us to determine if a change is actually taking place without needing to perform an arbitrary time.Sleep (waiting for client cache to clear). We can now use a combination of comparing the before and after of the StatefulSet with out modification function to see if the StatefulSet is ready or not.
(there will be a separate PR to remove the builder and refactor tests if we go ahead with this design)
Side Notes:
statefulset. HaveEqualSpecfunction is no longer required and was removed