Skip to content

Commit

Permalink
fix endpoint reconciliation (#1674)
Browse files Browse the repository at this point in the history
The idea was to still trigger reconciliation cycles if resources changes by checking if there was a `generation` update.
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#metadata
> generation: a sequence number representing a specific generation of the desired state. Set by the system and monotonically increasing, per-resource. May be compared, such as for RAW and WAW consistency.

However, the change caused a degradation in `Endpoints` resources. These resources don't have generation because they don't have a status. As a result, a reconciliation was only triggered on creation and deletion, but not on subset update.
This degradated also the behavior of the k8gb controller. The controller became too slow monitoring the health of applications since `Endpoints` updates no longer triggered a reconcile of the GSLB resource. This lead to slow failovers and failing e2e tests.

The solution proposed in this PR is to always trigger the `endpointMapHandler` function when an `Endpoint` resource changes. This is the same behavior as before #1652.

Signed-off-by: Andre Baptista Aguas <andre.aguas@protonmail.com>
  • Loading branch information
abaguas authored Jul 31, 2024
1 parent dca347d commit 2a6f03f
Show file tree
Hide file tree
Showing 7 changed files with 15 additions and 65 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ terratest: # Run terratest suite
echo -e "$(RED)Make sure you run the tests against at least two running clusters$(NC)" ;\
exit 1;\
fi
cd terratest/test/ && go mod download && CLUSTERS_NUMBER=$(RUNNING_CLUSTERS) go test -v -timeout 20m -parallel=12 --tags=$(TEST_TAGS)
cd terratest/test/ && go mod download && CLUSTERS_NUMBER=$(RUNNING_CLUSTERS) go test -v -timeout 25m -parallel=12 --tags=$(TEST_TAGS)

.PHONY: website
website:
Expand Down
1 change: 0 additions & 1 deletion controllers/gslb_controller_reconciliation.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ func (r *GslbReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
return result.RequeueError(fmt.Errorf("getting GSLB servers (%s)", err))
}
gslb.Status.Servers = servers
fmt.Printf("got servers: %v\n", servers)

loadBalancerExposedIPs, err := refResolver.GetGslbExposedIPs(r.Client, r.Config.EdgeDNSServers)
if err != nil {
Expand Down
12 changes: 12 additions & 0 deletions controllers/gslb_controller_setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
"k8s.io/apimachinery/pkg/types"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
Expand Down Expand Up @@ -98,10 +99,21 @@ func (r *GslbReconciler) SetupWithManager(mgr ctrl.Manager) error {
if e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() {
return true
}

// endpoints don't have state, therefore they don't have a generation
// but when their subsets change they must be be reconciled
gvk, err := apiutil.GVKForObject(e.ObjectOld, r.Scheme)
if err != nil {
log.Warn().Msg("could not fetch GroupVersionKind for object")
} else if gvk.Kind == "Endpoints" {
return true
}

// Ignore reconciliation in case nothing has changed in k8gb annotations
oldAnnotations := e.ObjectOld.GetAnnotations()
newAnnotations := e.ObjectNew.GetAnnotations()
reconcile := !utils.EqualPredefinedAnnotations(oldAnnotations, newAnnotations, k8gbAnnotations...)

return reconcile
},
}).
Expand Down
1 change: 1 addition & 0 deletions controllers/logging/logr.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ func (a *logrSinkAdapter) Info(_ int, msg string, keysAndValues ...interface{})
a.WithValues(keysAndValues)
if a.name != "" {
a.z.Info().Msgf("%s: %s %s", a.name, msg, a.valuesAsJSON())
return
}
a.z.Info().Msgf("%s %s", msg, a.valuesAsJSON())
}
Expand Down
2 changes: 1 addition & 1 deletion deploy/edge/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ spec:
name: zone-volume
containers:
- name: bind
image: internetsystemsconsortium/bind9:9.16
image: internetsystemsconsortium/bind9:9.18
ports:
- containerPort: 1053
protocol: TCP
Expand Down
60 changes: 0 additions & 60 deletions go.sum

Large diffs are not rendered by default.

2 changes: 0 additions & 2 deletions terratest/test/k8gb_weight_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,6 @@ import (
)

func TestWeightsExistsInLocalDNSEndpoint(t *testing.T) {
t.Parallel()

tests := []struct {
gslbPath string
ingressPath string
Expand Down

0 comments on commit 2a6f03f

Please sign in to comment.