Fix reconciliation on Endpoints's subset changes
#1674
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
abaguas
requested review from
donovanmuller,
k0da,
kuritka,
ytsarev and
jkremser
as code owners
abaguas
changed the title
Endpoints updatesEndpoints's subset changes
abaguas
force-pushed
the
fix/degradation
branch
6 times, most recently
from
d0bc6f7
to
f3c225e
Compare
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 k8gb-io#1652. Signed-off-by: Andre Baptista Aguas <andre.aguas@protonmail.com>
abaguas
commented
Jul 31, 2024
| @@ -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) | |||
There was a problem hiding this comment.
Debug statement that was forgotten from a previous commit
| @@ -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 | |||
There was a problem hiding this comment.
The lack of this return was leading to duplicated log lines
| @@ -27,8 +27,6 @@ import ( | |||
| ) | |||
|
|
|||
| func TestWeightsExistsInLocalDNSEndpoint(t *testing.T) { | |||
| t.Parallel() | |||
There was a problem hiding this comment.
There were some random failures in this test, I had to remove parallelism to have it stable. One cluster was taking too long discovering the DNS entries of the other cluster, but I am not sure why.
Before:
--- FAIL: TestWeightsExistsInLocalDNSEndpoint (337.85s)
--- FAIL: TestWeightsExistsInLocalDNSEndpoint/with_gslb_only (245.38s)
--- PASS: TestWeightsExistsInLocalDNSEndpoint/with_ref (92.48s)
After:
--- PASS: TestWeightsExistsInLocalDNSEndpoint (195.55s)
--- PASS: TestWeightsExistsInLocalDNSEndpoint/with_gslb_only (128.23s)
--- PASS: TestWeightsExistsInLocalDNSEndpoint/with_ref (67.32s)
kuritka
approved these changes
Jul 31, 2024
Hopefully when we try to revamp the tests we can migrate them one by one to the new framework and can get to the bottom of these parallelism issues |
This was referenced Jul 31, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The idea was to still trigger reconciliation cycles if resources changes by checking if there was a
generationupdate. https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#metadataHowever, the change caused a degradation watching
Endpointsresources. These resources don't have agenerationbecause they don't have astatus. As a result, a reconciliation was only triggered on creation and deletion, but not on subset update. This caused the controller to become too slow reacting toEndpointschanges, thus leading to slow failovers and failing e2e tests if the GSLB reconcile timer didn't fall during the expected window.The solution proposed in this PR is to always trigger the
endpointMapHandlerfunction when anEndpointresource changes. This is the same behavior as before #1652.In addition, the bind image
internetsystemsconsortium/bind9:9.16pulled from theedgednscluster was no longer available, so it is updated to version9.18: