Skip to content

Commit 6442127

Browse files
authored
GMC: add UT for reconcile filters (#383)
* add UT for reconcile filters Signed-off-by: KfreeZ <[email protected]> * remove useless code Signed-off-by: KfreeZ <[email protected]> * update tests Signed-off-by: KfreeZ <[email protected]> * debug CI/CD failed Signed-off-by: KfreeZ <[email protected]> * skip use pod_count in e2e's check_gmc_status, the pod_count is not acurrate sometimes when terninating pod is not cleared but counted Signed-off-by: KfreeZ <[email protected]> * better than hardcode Signed-off-by: KfreeZ <[email protected]> * count the pods right Signed-off-by: KfreeZ <[email protected]> --------- Signed-off-by: KfreeZ <[email protected]>
1 parent 0a3e006 commit 6442127

File tree

3 files changed

+319
-149
lines changed

3 files changed

+319
-149
lines changed

.github/workflows/scripts/e2e/gmc_xeon_test.sh

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ function validate_audioqa() {
134134
exit 1
135135
fi
136136

137-
pods_count=$(kubectl get pods -n $AUDIOQA_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
137+
pods_count=$(kubectl get pods -n $AUDIOQA_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
138138
# expected_ready_pods, expected_external_pods, expected_total_pods
139139
# pods_count-1 is to exclude the client pod in this namespace
140140
check_gmc_status $AUDIOQA_NAMESPACE 'audioqa' $((pods_count-1)) 0 7
@@ -187,7 +187,7 @@ function validate_chatqna() {
187187
exit 1
188188
fi
189189

190-
pods_count=$(kubectl get pods -n $CHATQNA_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
190+
pods_count=$(kubectl get pods -n $CHATQNA_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
191191
# expected_ready_pods, expected_external_pods, expected_total_pods
192192
# pods_count-1 is to exclude the client pod in this namespace
193193
check_gmc_status $CHATQNA_NAMESPACE 'chatqa' $((pods_count-1)) 0 9
@@ -257,7 +257,7 @@ function validate_chatqna_with_dataprep() {
257257
exit 1
258258
fi
259259

260-
pods_count=$(kubectl get pods -n $CHATQNA_DATAPREP_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
260+
pods_count=$(kubectl get pods -n $CHATQNA_DATAPREP_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
261261
# expected_ready_pods, expected_external_pods, expected_total_pods
262262
# pods_count-1 is to exclude the client pod in this namespace
263263
check_gmc_status $CHATQNA_DATAPREP_NAMESPACE 'chatqa' $((pods_count-1)) 0 10
@@ -350,7 +350,7 @@ function validate_chatqna_in_switch() {
350350
exit 1
351351
fi
352352

353-
pods_count=$(kubectl get pods -n $CHATQNA_SWITCH_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
353+
pods_count=$(kubectl get pods -n $CHATQNA_SWITCH_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
354354
# expected_ready_pods, expected_external_pods, expected_total_pods
355355
# pods_count-1 is to exclude the client pod in this namespace
356356
check_gmc_status $CHATQNA_SWITCH_NAMESPACE 'switch' $((pods_count-1)) 0 15
@@ -443,8 +443,8 @@ function validate_modify_config() {
443443
exit 1
444444
fi
445445

446-
pods_count=$(kubectl get pods -n $MODIFY_STEP_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
447-
check_gmc_status $MODIFY_STEP_NAMESPACE 'codegen' $pods_count 0 3
446+
pods_count=$(kubectl get pods -n $MODIFY_STEP_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
447+
check_gmc_status $MODIFY_STEP_NAMESPACE 'codegen' $((pods_count-1)) 0 3
448448
if [ $? -ne 0 ]; then
449449
echo "GMC status is not as expected"
450450
exit 1
@@ -461,7 +461,7 @@ function validate_modify_config() {
461461
exit 1
462462
fi
463463

464-
check_gmc_status $MODIFY_STEP_NAMESPACE 'codegen' $pods_count 0 3
464+
check_gmc_status $MODIFY_STEP_NAMESPACE 'codegen' $((pods_count-1)) 0 3
465465
if [ $? -ne 0 ]; then
466466
echo "GMC status is not as expected"
467467
exit 1
@@ -494,8 +494,8 @@ function validate_remove_step() {
494494
exit 1
495495
fi
496496

497-
pods_count=$(kubectl get pods -n $DELETE_STEP_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
498-
check_gmc_status $DELETE_STEP_NAMESPACE 'codegen' $pods_count 0 3
497+
pods_count=$(kubectl get pods -n $DELETE_STEP_NAMESPACE --no-headers | grep -v "Terminating" | wc -l)
498+
check_gmc_status $DELETE_STEP_NAMESPACE 'codegen' $((pods_count-1)) 0 3
499499
if [ $? -ne 0 ]; then
500500
echo "GMC status is not as expected"
501501
exit 1

microservices-connector/internal/controller/gmconnector_controller.go

Lines changed: 74 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -797,100 +797,97 @@ func isMetadataChanged(oldObject, newObject *metav1.ObjectMeta) bool {
797797
return false
798798
}
799799

800-
// SetupWithManager sets up the controller with the Manager.
801-
func (r *GMConnectorReconciler) SetupWithManager(mgr ctrl.Manager) error {
802-
// Predicate to ignore updates to status subresource
803-
ignoreStatusUpdatePredicate := predicate.Funcs{
804-
UpdateFunc: func(e event.UpdateEvent) bool {
805-
// Cast objects to your GMConnector struct
806-
oldObject, ok1 := e.ObjectOld.(*mcv1alpha3.GMConnector)
807-
newObject, ok2 := e.ObjectNew.(*mcv1alpha3.GMConnector)
808-
if !ok1 || !ok2 {
809-
// Not the correct type, allow the event through
810-
return true
811-
}
800+
func isGMCSpecOrMetadataChanged(e event.UpdateEvent) bool {
801+
oldObject, ok1 := e.ObjectOld.(*mcv1alpha3.GMConnector)
802+
newObject, ok2 := e.ObjectNew.(*mcv1alpha3.GMConnector)
803+
if !ok1 || !ok2 {
804+
// Not the correct type, allow the event through
805+
return true
806+
}
812807

813-
specChanged := !reflect.DeepEqual(oldObject.Spec, newObject.Spec)
814-
metadataChanged := isMetadataChanged(&(oldObject.ObjectMeta), &(newObject.ObjectMeta))
808+
specChanged := !reflect.DeepEqual(oldObject.Spec, newObject.Spec)
809+
metadataChanged := isMetadataChanged(&(oldObject.ObjectMeta), &(newObject.ObjectMeta))
815810

816-
fmt.Printf("\n| spec changed %t | meta changed: %t |\n", specChanged, metadataChanged)
811+
fmt.Printf("\n| spec changed %t | meta changed: %t |\n", specChanged, metadataChanged)
817812

818-
// Compare the old and new spec, ignore metadata, status changes
819-
// metadata change: name, namespace, such change should create a new GMC
820-
// status change: depoyment status
821-
return specChanged || metadataChanged
822-
},
823-
// Other funcs like CreateFunc, DeleteFunc, GenericFunc can be left as default
824-
// if you only want to customize the UpdateFunc behavior.
813+
// Compare the old and new spec, ignore metadata, status changes
814+
// metadata change: name, namespace, such change should create a new GMC
815+
// status change: deployment status
816+
return specChanged || metadataChanged
817+
}
818+
819+
func isDeploymentStatusChanged(e event.UpdateEvent) bool {
820+
oldDeployment, ok1 := e.ObjectOld.(*appsv1.Deployment)
821+
newDeployment, ok2 := e.ObjectNew.(*appsv1.Deployment)
822+
if !ok1 || !ok2 {
823+
// Not the correct type, allow the event through
824+
return true
825825
}
826826

827-
// Predicate to only trigger on status changes for Deployment
828-
deploymentStatusChangePredicate := predicate.Funcs{
829-
UpdateFunc: func(e event.UpdateEvent) bool {
830-
oldDeployment, ok1 := e.ObjectOld.(*appsv1.Deployment)
831-
newDeployment, ok2 := e.ObjectNew.(*appsv1.Deployment)
832-
if !ok1 || !ok2 {
833-
// Not the correct type, allow the event through
834-
fmt.Printf("| status missing |\n")
835-
return true
827+
if len(newDeployment.OwnerReferences) == 0 {
828+
// fmt.Printf("| %s:%s: no owner reference |\n", newDeployment.Namespace, newDeployment.Name)
829+
return false
830+
} else {
831+
for _, owner := range newDeployment.OwnerReferences {
832+
if owner.Kind == "GMConnector" {
833+
// fmt.Printf("| %s:%s: owner is GMConnector |\n", newDeployment.Namespace, newDeployment.Name)
834+
break
836835
}
836+
}
837+
}
837838

838-
if len(newDeployment.OwnerReferences) == 0 {
839-
// fmt.Printf("| %s:%s: no owner reference |\n", newDeployment.Namespace, newDeployment.Name)
840-
return false
841-
} else {
842-
for _, owner := range newDeployment.OwnerReferences {
843-
if owner.Kind == "GMConnector" {
844-
// fmt.Printf("| %s:%s: owner is GMConnector |\n", newDeployment.Namespace, newDeployment.Name)
845-
break
846-
}
839+
oldStatus := corev1.ConditionUnknown
840+
newStatus := corev1.ConditionUnknown
841+
if !reflect.DeepEqual(oldDeployment.Status, newDeployment.Status) {
842+
if newDeployment.Status.Conditions != nil {
843+
for _, condition := range newDeployment.Status.Conditions {
844+
if condition.Type == appsv1.DeploymentAvailable {
845+
newStatus = condition.Status
847846
}
848847
}
849-
850-
oldStatus := corev1.ConditionUnknown
851-
newStatus := corev1.ConditionUnknown
852-
if !reflect.DeepEqual(oldDeployment.Status, newDeployment.Status) {
853-
if newDeployment.Status.Conditions != nil {
854-
for _, condition := range newDeployment.Status.Conditions {
855-
if condition.Type == appsv1.DeploymentAvailable {
856-
newStatus = condition.Status
857-
}
858-
}
859-
}
860-
if oldDeployment.Status.Conditions != nil {
861-
for _, condition := range oldDeployment.Status.Conditions {
862-
if condition.Type == appsv1.DeploymentAvailable {
863-
oldStatus = condition.Status
864-
}
865-
}
866-
}
867-
// Only trigger if the status has changed from true to false|unknown or vice versa
868-
if (oldStatus == corev1.ConditionTrue && oldStatus != newStatus) ||
869-
(newStatus == corev1.ConditionTrue && oldStatus != newStatus) {
870-
{
871-
fmt.Printf("| %s:%s: status changed from : %v to %v|\n",
872-
newDeployment.Namespace, newDeployment.Name,
873-
oldStatus, newStatus)
874-
return true
875-
}
848+
}
849+
if oldDeployment.Status.Conditions != nil {
850+
for _, condition := range oldDeployment.Status.Conditions {
851+
if condition.Type == appsv1.DeploymentAvailable {
852+
oldStatus = condition.Status
876853
}
877854
}
878-
return false
879-
},
880-
//ignore create and delete events, otherwise it will trigger the nested reconcile which is meaningless
881-
CreateFunc: func(e event.CreateEvent) bool {
882-
return false
883-
}, DeleteFunc: func(e event.DeleteEvent) bool {
884-
return false
885-
},
855+
}
856+
// Only trigger if the status has changed from true to false|unknown or vice versa
857+
if (oldStatus == corev1.ConditionTrue && oldStatus != newStatus) ||
858+
(newStatus == corev1.ConditionTrue && oldStatus != newStatus) {
859+
{
860+
fmt.Printf("| %s:%s: status changed from : %v to %v|\n",
861+
newDeployment.Namespace, newDeployment.Name,
862+
oldStatus, newStatus)
863+
return true
864+
}
865+
}
866+
}
867+
return false
868+
869+
}
870+
871+
// SetupWithManager sets up the controller with the Manager.
872+
func (r *GMConnectorReconciler) SetupWithManager(mgr ctrl.Manager) error {
873+
// Predicate to ignore updates to status subresource
874+
gmcfilter := predicate.Funcs{
875+
UpdateFunc: isGMCSpecOrMetadataChanged,
876+
// Other funcs like CreateFunc, DeleteFunc, GenericFunc can be left as default
877+
// if you only want to customize the UpdateFunc behavior.
878+
}
879+
880+
// Predicate to only trigger on status changes for Deployment
881+
deploymentFilter := predicate.Funcs{
882+
UpdateFunc: isDeploymentStatusChanged,
886883
}
887884

888885
return ctrl.NewControllerManagedBy(mgr).
889-
For(&mcv1alpha3.GMConnector{}, builder.WithPredicates(ignoreStatusUpdatePredicate)).
886+
For(&mcv1alpha3.GMConnector{}, builder.WithPredicates(gmcfilter)).
890887
Watches(
891888
&appsv1.Deployment{},
892889
&handler.EnqueueRequestForObject{},
893-
builder.WithPredicates(deploymentStatusChangePredicate),
890+
builder.WithPredicates(deploymentFilter),
894891
).
895892
Complete(r)
896893
}

0 commit comments

Comments
 (0)