Fix cluster-local annonation, set knative svc defaults #211
Conversation
This file contains hidden or 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
added 3 commits
June 14, 2022 19:20
krithika369
changed the title
leonlnj
approved these changes
Jun 15, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
In recent versions of Knative, the private resources must be created with the
networking.knative.dev/visibility=cluster-locallabel which was previouslyserving.knative.dev/visibility=cluster-local. See:Without the right label, the Enrichers and Ensemblers will have both sets of hosts mapped (cluster-local and external-ip).
E2e Test Flakiness
In #207, the Knative
net-istiocontroller (used in e2e tests) was upgraded to1.0.0. The same version of the controller is used internally in Gojek, to run e2e tests against a GKE cluster. The tests were proving to be flaky with both the routers (accessed via the ingress gateway) and enrichers/ensemblers (accessed as cluster-local endpoints) intermittently returning 404. This was especially more commonly observed with the ensemblers used in the tests.This PR corrects the service visibility label which seems to fix the intermittent issues. It's not clear (to me) why that is the case - from examining the logic for the
net-istiocontroller (here and here) and observing its logs, the Kingress is marked asLoadBalancerReady, in the same fashion, having received no failed / unknown probe status for the relevant endpoints even in the scenarios where the tests fail (example screenshot below shows the first host for each rule in the Kservice being probed; no corresponding failure logs seen).Nevertheless, the change from this PR has been tested numerous times on the internal CI and the e2e tests consistently pass.
Service Defaults
Inspired by Kserve's implementation, this PR also adds a small change to set the defaults on the KService specs before submitting it for creation / update, to prevent some unnecessary detection of changes during processing.