Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add "local-with-fallback" externalTrafficPolicy hack #749

Merged
merged 1 commit into from Jun 1, 2021
Merged

Add "local-with-fallback" externalTrafficPolicy hack #749

merged 1 commit into from Jun 1, 2021

Conversation

danwinship
Copy link

@danwinship danwinship commented May 13, 2021 •
edited 

If a service has a
"traffic-policy.network.alpha.openshift.io/local-with-fallback"
annotation, then only treat it as "externalTrafficPolicy: Local" when
there are actually running local pods.

That is, if we receive traffic for such a service after the last local
pod terminates, then forward it to a remote pod rather than dropping
it.

Also, the existing hack for DNS local pods doesn't actually work because it was modifying allEndpoints after the rest of the code had already ditched allEndpoints in favor of readyEndpoints.

/cc @Miciah @dcbw @aojea @smarterclayton

@openshift-ci-robot openshift-ci-robot added the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label May 13, 2021
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 13, 2021
@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021
@danwinship danwinship added backports/validated-commits Indicates that all commits come to merged upstream PRs. and removed backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. labels May 13, 2021
@aojea
Copy link

aojea commented May 13, 2021 •
edited

how do you do to run the unit tests in proxier_test.go?

EDIT
duh, I was looking into openshift/sdn that vendors the file , hence no tests 🙃

aojea
aojea reviewed May 14, 2021
View reviewed changes
pkg/proxy/iptables/proxier.go
}
if hasLocalEndpoints {
klog.V(4).Infof("Found %d local endpoint(s) for service %q; preferring the local endpoint and ignoring %d other endpoint(s)", len(localEndpoints), svcNameString, len(allEndpoints) - len(localEndpoints))
readyEndpoints = localEndpoints
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the future us, this is "InternalTrafficPolicy: preferLocal" , right?

@aojea
Copy link

aojea commented May 14, 2021

LGTM

@smarterclayton smarterclayton mentioned this pull request May 14, 2021
Closed
@danwinship danwinship mentioned this pull request May 22, 2021
Merged
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2021
@openshift-ci-robot openshift-ci-robot added the backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. label May 25, 2021
@danwinship danwinship changed the title WIP Add "prefer-local" externalTrafficPolicy hack Add "local-with-fallback" externalTrafficPolicy hack May 25, 2021
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2021
@openshift-ci-robot openshift-ci-robot removed the backports/validated-commits Indicates that all commits come to merged upstream PRs. label May 25, 2021
@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@danwinship
UPSTREAM: <carry>: implement "local-with-fallback" external traffic p…
01ee4a5 
…olicy

If a service has a
"traffic-policy.network.alpha.openshift.io/local-with-fallback"
annotation, then only treat it as "externalTrafficPolicy: Local" when
there are actually running local pods.

That is, if we receive traffic for such a service after the last local
pod terminates, then forward it to a remote pod rather than dropping
it.
@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

5 similar comments
@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

1 similar comment
@openshift-ci-robot
Copy link

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

aojea
aojea reviewed May 26, 2021
View reviewed changes
pkg/proxy/iptables/proxier.go
}

const localWithFallbackAnnotation = "traffic-policy.network.alpha.openshift.io/local-with-fallback"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is going to be an "official" annotation, service.kubernetes.io/topology-aware-routing
https://github.com/kubernetes/enhancements/tree/master/keps/sig-network/2433-topology-aware-hints#configuration

do we want to make it more official?

service.kubernetes.io/topology-aware-routing = local-with-fallback

@aojea
Copy link

aojea commented May 26, 2021 •
edited

/lgtm
only bike shedding comments :)

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 26, 2021
@openshift-ci
Copy link

openshift-ci bot commented May 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, danwinship

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@danwinship danwinship merged commit fbaadca into openshift:sdn-4.8-kubernetes-1.21.1 Jun 1, 2021
@danwinship danwinship deleted the prefer-local branch June 1, 2021 18:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aojea aojea aojea left review comments

@dcbw dcbw Awaiting requested review from dcbw

@Miciah Miciah Awaiting requested review from Miciah

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@aojea aojea

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. backports/unvalidated-commits Indicates that not all commits come to merged upstream PRs. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@danwinship @openshift-ci-robot @aojea