Skip to content

Commit

Permalink
Refactor E2E script, reduce excess reconciling of resources
Browse files Browse the repository at this point in the history 
Signed-off-by: Jonathan West <[email protected]>
  • Loading branch information
@jgwest
jgwest committed Oct 1, 2024
1 parent 135d0d1 commit 1c1b4c7
Show file tree
Hide file tree
Showing 5 changed files with 136 additions and 57 deletions.
3 changes: 3 additions & 0 deletions controllers/configmap.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro
TrafficRouterPluginConfigMapKey: string(pluginBytes),
}

log.Info("Updating Rollouts ConfigMap due to detected difference")
return r.Client.Update(ctx, actualConfigMap)
} else {
// Plugin URL is the same, nothing to do
Expand All @@ -94,5 +95,7 @@ func (r *RolloutManagerReconciler) reconcileConfigMap(ctx context.Context, cr ro

actualConfigMap.Data[TrafficRouterPluginConfigMapKey] = string(pluginString)

log.Info("Updating Rollouts ConfigMap")

return r.Client.Update(ctx, actualConfigMap)
}
22 changes: 12 additions & 10 deletions controllers/resources.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsServiceAccount(ctx context.C
normalizedLiveServiceAccount := liveServiceAccount.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveServiceAccount.ObjectMeta, cr)

if !reflect.DeepEqual(normalizedLiveServiceAccount.Labels, expectedServiceAccount.Labels) || !reflect.DeepEqual(normalizedLiveServiceAccount.Annotations, expectedServiceAccount.Annotations) {
if !areStringMapsEqual(normalizedLiveServiceAccount.Labels, expectedServiceAccount.Labels) || !areStringMapsEqual(normalizedLiveServiceAccount.Annotations, expectedServiceAccount.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of ServiceAccount %s do not match the expected state, hence updating it", liveServiceAccount.Name))

Expand Down Expand Up @@ -103,7 +103,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsRole(ctx context.Context, cr

removeUserLabelsAndAnnotations(&normalizedLiveRole.ObjectMeta, cr)

if !reflect.DeepEqual(normalizedLiveRole.Labels, expectedRole.Labels) || !reflect.DeepEqual(normalizedLiveRole.Annotations, expectedRole.Annotations) {
if !areStringMapsEqual(normalizedLiveRole.Labels, expectedRole.Labels) || !areStringMapsEqual(normalizedLiveRole.Annotations, expectedRole.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of Role %s do not match the expected state, hence updating it", liveRole.Name))

Expand Down Expand Up @@ -151,7 +151,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsClusterRole(ctx context.Cont
normalizedLiveClusterRole := liveClusterRole.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)

if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of Role %s do not match the expected state, hence updating it", liveClusterRole.Name))

Expand Down Expand Up @@ -226,7 +226,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsRoleBinding(ctx context.Cont

normalizedLiveRoleBinding := liveRoleBinding.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveRoleBinding.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveRoleBinding.Labels, expectedRoleBinding.Labels) || !reflect.DeepEqual(normalizedLiveRoleBinding.Annotations, expectedRoleBinding.Annotations) {
if !areStringMapsEqual(normalizedLiveRoleBinding.Labels, expectedRoleBinding.Labels) || !areStringMapsEqual(normalizedLiveRoleBinding.Annotations, expectedRoleBinding.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of RoleBinding %s do not match the expected state, hence updating it", liveRoleBinding.Name))

Expand Down Expand Up @@ -297,7 +297,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsClusterRoleBinding(ctx conte

normalizedLiveClusterRoleBinding := liveClusterRoleBinding.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveClusterRoleBinding.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveClusterRoleBinding.Labels, expectedClusterRoleBinding.Labels) || !reflect.DeepEqual(normalizedLiveClusterRoleBinding.Annotations, expectedClusterRoleBinding.Annotations) {
if !areStringMapsEqual(normalizedLiveClusterRoleBinding.Labels, expectedClusterRoleBinding.Labels) || !areStringMapsEqual(normalizedLiveClusterRoleBinding.Annotations, expectedClusterRoleBinding.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of ClusterRoleBinding %s do not match the expected state, hence updating it", liveClusterRoleBinding.Name))

Expand Down Expand Up @@ -430,7 +430,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToAdminClusterRole(

normalizedLiveClusterRole := liveClusterRole.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))

Expand Down Expand Up @@ -482,7 +482,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToEditClusterRole(c

normalizedLiveClusterRole := liveClusterRole.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))

Expand Down Expand Up @@ -534,7 +534,8 @@ func (r *RolloutManagerReconciler) reconcileRolloutsAggregateToViewClusterRole(c

normalizedLiveClusterRole := liveClusterRole.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveClusterRole.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !reflect.DeepEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {
if !areStringMapsEqual(normalizedLiveClusterRole.Labels, expectedClusterRole.Labels) || !areStringMapsEqual(normalizedLiveClusterRole.Annotations, expectedClusterRole.Annotations) {

updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of aggregated ClusterRole %s do not match the expected state, hence updating it", liveClusterRole.Name))

Expand Down Expand Up @@ -672,7 +673,8 @@ func (r *RolloutManagerReconciler) reconcileRolloutsMetricsService(ctx context.C

normalizedLiveService := liveService.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveService.ObjectMeta, cr)
if !reflect.DeepEqual(normalizedLiveService.Labels, expectedSvc.Labels) || !reflect.DeepEqual(normalizedLiveService.Annotations, expectedSvc.Annotations) {
if !areStringMapsEqual(normalizedLiveService.Labels, expectedSvc.Labels) || !areStringMapsEqual(normalizedLiveService.Annotations, expectedSvc.Annotations) {

updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of metrics Service %s do not match the expected state, hence updating it", liveService.Name))

Expand Down Expand Up @@ -748,7 +750,7 @@ func (r *RolloutManagerReconciler) reconcileRolloutsSecrets(ctx context.Context,
normalizedLiveSecret := liveSecret.DeepCopy()
removeUserLabelsAndAnnotations(&normalizedLiveSecret.ObjectMeta, cr)

if !reflect.DeepEqual(normalizedLiveSecret.Labels, expectedSecret.Labels) || !reflect.DeepEqual(normalizedLiveSecret.Annotations, expectedSecret.Annotations) {
if !areStringMapsEqual(normalizedLiveSecret.Labels, expectedSecret.Labels) || !areStringMapsEqual(normalizedLiveSecret.Annotations, expectedSecret.Annotations) {
updateNeeded = true
log.Info(fmt.Sprintf("Labels/Annotations of Secret %s do not match the expected state, hence updating it", liveSecret.Name))

Expand Down
30 changes: 30 additions & 0 deletions controllers/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"os"
"reflect"
"sort"
"strings"

Expand Down Expand Up @@ -85,6 +86,30 @@ func appendStringMap(src map[string]string, add map[string]string) map[string]st
return res
}

// In go, reflect.DeepEqual(a, b), on maps a and b, will return false even when len(a) == 0 and len(b) == 0. This is due to a nil map being considered different from an empty map.
// This function handles this case better: if maps both have a length of 0, we consider them always equal.
func areStringMapsEqual(one map[string]string, two map[string]string) bool {

isOneEmpty := false

isTwoEmpty := false

if len(one) == 0 {
isOneEmpty = true
}

if len(two) == 0 {
isTwoEmpty = true
}

if isOneEmpty && isTwoEmpty {
return true
}

return reflect.DeepEqual(one, two)

}

// combineStringMaps will combine multiple maps: maps defined earlier in the 'maps' slice may have their values overriden by maps defined later in the 'maps' slice.
func combineStringMaps(maps ...map[string]string) map[string]string {

Expand Down Expand Up @@ -410,6 +435,11 @@ func removeUserLabelsAndAnnotations(obj *metav1.ObjectMeta, cr rolloutsmanagerv1

for objectLabelKey := range obj.Labels {

// We add this label to the aggregated cluster roles we generate, so we should not process it as a user label.
if strings.Contains(objectLabelKey, "rbac.authorization.k8s.io/") && strings.Contains(objectLabelKey, "aggregate") {
continue
}

existsInDefault := false

for defaultLabelKey := range defaultLabelsAndAnnotations.Labels {
Expand Down
61 changes: 43 additions & 18 deletions controllers/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -425,42 +425,36 @@ var _ = Describe("removeUserLabelsAndAnnotations tests", func() {
}

Expect(k8sClient.Create(ctx, &cr)).To(Succeed())
setRolloutsLabelsAndAnnotations(&obj)

removeUserLabelsAndAnnotations(&obj, cr)

Expect(obj.Labels).To(Equal(expectedLabels))
Expect(obj.Annotations).To(Equal(expectedAnnotations))
},
Entry("when no user-defined labels or annotations exist",
map[string]string{}, map[string]string{},
map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{},
map[string]string{},
map[string]string{},
map[string]string{},
),
Entry("when user-defined labels and annotations are present",
map[string]string{"user-label": "value"}, map[string]string{"user-annotation": "value"},
map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{"user-label": "value"},
map[string]string{"user-annotation": "value"},
map[string]string{},
map[string]string{},
),
Entry("when user-defined labels are present and annotations are empty",
map[string]string{"user-label": "value"}, map[string]string{},
map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{"user-label": "value"},
map[string]string{},
map[string]string{},
map[string]string{},
),
Entry("when user-defined labels are empty and annotations are present",
map[string]string{}, map[string]string{"user-annotation": "value"},
map[string]string{"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{},
map[string]string{"user-annotation": "value"},
map[string]string{},
map[string]string{},
),

Entry("when both user and non-user-defined labels are present, and annotations are empty",
map[string]string{"user-label": "value",
"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
Expand All @@ -472,6 +466,20 @@ var _ = Describe("removeUserLabelsAndAnnotations tests", func() {
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{},
),
Entry("rbac aggregate-to- labels should not be removed, as we add these to ClusterRoles",
map[string]string{
"rbac.authorization.k8s.io/aggregate-to-admin": "true",
"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{},
map[string]string{
"rbac.authorization.k8s.io/aggregate-to-admin": "true",
"app.kubernetes.io/name": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/part-of": DefaultArgoRolloutsResourceName,
"app.kubernetes.io/component": DefaultArgoRolloutsResourceName},
map[string]string{},
),
)
})

Expand Down Expand Up @@ -656,6 +664,23 @@ var _ = Describe("setAdditionalRolloutsLabelsAndAnnotationsToObject tests", func
})
})

var _ = Describe("areStringMapsEqual tests", func() {
DescribeTable("areStringMapsEqual should match reflect.DeepEqual(), except when comparing nil with empty map", func(one map[string]string, two map[string]string, expectedRes bool) {

res := areStringMapsEqual(one, two)
Expect(res).To(Equal(expectedRes))
},
Entry("both nil", nil, nil, true),
Entry("one nil, one empty", map[string]string{}, nil, true),
Entry("one empty, one nil", nil, map[string]string{}, true),
Entry("both empty", map[string]string{}, map[string]string{}, true),
Entry("one empty, one has value", map[string]string{}, map[string]string{"key": "value"}, false),
Entry("one has value, one nil", map[string]string{"key": "value"}, nil, false),
Entry("equal values", map[string]string{"key": "value"}, map[string]string{"key": "value"}, true),
)

})

var _ = Describe("envMerge tests", func() {
DescribeTable("merges two slices of EnvVar entries",
func(existing, merge, expected []corev1.EnvVar, override bool) {
Expand Down
77 changes: 48 additions & 29 deletions hack/run-rollouts-manager-e2e-tests.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,31 @@ SCRIPTPATH="$(
pwd -P
)"

function cleanup {
echo "* Cleaning up"
killall main || true
killall go || true
}

trap cleanup EXIT


# Treat undefined variables as errors
set -u

extract_metrics_data() {

TMP_DIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir')

while true; do
curl http://localhost:8080/metrics -o "$TMP_DIR/rollouts-metric-endpoint-output.txt"
if [ "$?" == "0" ]; then
break
fi
echo "* Waiting for Metrics endpoint to become available"
sleep 3
done

# 1) Extract REST client get/put/post metrics

# Example: the metrics from /metric endpoint look like this:
Expand All @@ -20,9 +38,10 @@ extract_metrics_data() {
# rest_client_requests_total{code="201",host="api.pgqqd-novoo-oqu.pa43.p3.openshiftapps.com:443",method="POST"} 110

curl http://localhost:8080/metrics -o "$TMP_DIR/rollouts-metric-endpoint-output.txt"
GET_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "GET" | rev | cut -d' ' -f1`
PUT_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "PUT" | rev | cut -d' ' -f1`
POST_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "POST" | rev | cut -d' ' -f1`

GET_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "GET" | rev | cut -d' ' -f1 | rev`
PUT_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "PUT" | grep -v "409" | rev | cut -d' ' -f1 | rev`
POST_REQUESTS=`cat "$TMP_DIR/rollouts-metric-endpoint-output.txt" | grep "rest_client_requests_total" | grep "POST" | rev | cut -d' ' -f1 | rev`

if [[ "$GET_REQUESTS" == "" ]]; then
GET_REQUESTS=0
Expand Down Expand Up @@ -91,28 +110,6 @@ else

fi

set +e

# If the output from the E2E operator is available, then check it for errors
if [ -f "/tmp/e2e-operator-run.log" ]; then

# Wait for the controller to flush to the file, before killing the controller
sleep 10
killall main
sleep 5

# Grep the log for unexpected errors
# - Ignore errors that are expected to occur

UNEXPECTED_ERRORS_FOUND_TEXT=`cat /tmp/e2e-operator-run.log | grep "ERROR" | grep -v "because it is being terminated" | grep -v "the object has been modified; please apply your changes to the latest version and try again" | grep -v "unable to fetch" | grep -v "StorageError"` | grep -v "client rate limiter Wait returned an error: context canceled"
UNEXPECTED_ERRORS_COUNT=`echo $UNEXPECTED_ERRORS_FOUND_TEXT | grep "ERROR" | wc -l`

if [ "$UNEXPECTED_ERRORS_COUNT" != "0" ]; then
echo "Unexpected errors found: $UNEXPECTED_ERRORS_FOUND_TEXT"
exit 1
fi
fi

# Sanity test the behaviour of the operator during the tests:
# - We check the (prometheus) metrics coming from the 'localhost:8080/metrics' endpoint of the operator.
# - For example, if the reported # of Reconcile calls was unusually high, this might mean that the operator was stuck in a Reconcile loop
Expand Down Expand Up @@ -148,19 +145,19 @@ sanity_test_metrics_data() {
if [[ "`expr $PUT_REQUEST_PERCENT \> 40`" == "1" ]]; then
# This value is arbitrary, and should be updated if at any point it becomes inaccurate (but first audit the test/code to make sure it is not an actual product/test issue, before increasing)

echo "Put request %$PUT_REQUEST_PERCENT was greater than the expected value"
echo "Put request was %$PUT_REQUEST_PERCENT greater than the expected value"
exit 1

fi

if [[ "`expr $DELTA_ERROR_RECONCILES \> 20`" == "1" ]]; then
if [[ "`expr $DELTA_ERROR_RECONCILES \> 30`" == "1" ]]; then
# This value is arbitrary, and should be updated if at any point it becomes inaccurate (but first audit the test/code to make sure it is not an actual product/test issue, before increasing)
echo "Number of Reconcile calls that returned an error $DELTA_ERROR_RECONCILES was greater than the expected value"
exit 1

fi

if [[ "`expr $DELTA_SUCCESS_RECONCILES \> 200`" == "1" ]]; then
if [[ "`expr $DELTA_SUCCESS_RECONCILES \> 250`" == "1" ]]; then
# This value is arbitrary, and should be updated if at any point it becomes inaccurate (but first audit the test/code to make sure it is not an actual product/test issue, before increasing)

echo "Number of Reconcile calls that returned success $DELTA_SUCCESS_RECONCILES was greater than the expected value"
Expand All @@ -169,4 +166,26 @@ sanity_test_metrics_data() {
fi
}

sanity_test_metrics_data
sanity_test_metrics_data

set +e

# If the output from the E2E operator is available, then check it for errors
if [ -f "/tmp/e2e-operator-run.log" ]; then

# Wait for the controller to flush to the file, before killing the controller
sleep 10
killall main
sleep 5

# Grep the log for unexpected errors
# - Ignore errors that are expected to occur

UNEXPECTED_ERRORS_FOUND_TEXT=`cat /tmp/e2e-operator-run.log | grep "ERROR" | grep -v "because it is being terminated" | grep -v "the object has been modified; please apply your changes to the latest version and try again" | grep -v "unable to fetch" | grep -v "StorageError"` | grep -v "client rate limiter Wait returned an error: context canceled"
UNEXPECTED_ERRORS_COUNT=`echo $UNEXPECTED_ERRORS_FOUND_TEXT | grep "ERROR" | wc -l`

if [ "$UNEXPECTED_ERRORS_COUNT" != "0" ]; then
echo "Unexpected errors found: $UNEXPECTED_ERRORS_FOUND_TEXT"
exit 1
fi
fi

0 comments on commit 1c1b4c7

Please sign in to comment.