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.

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

Promote e2e "verifying service's sessionAffinity for ClusterIP and NodePort services" to Conformance #76443

Open
wants to merge 1 commit into
base: master
from

Conversation

@mgdevstack
Copy link
Member

commented Apr 11, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
Promotes following e2e to Conformance as they verify important functional behavior of service's session affinity for ClusterIP and NodePort type of services.

  1. [sig-network] Services should have session affinity work for service with type clusterIP
    [Execution Time: ~57 Sec]
  2. [sig-network] Services should be able to switch session affinity for service with type clusterIP
    [Execution Time: ~68 Sec]
  3. [sig-network] Services should have session affinity work for NodePort service
    [Execution Time: ~55 Sec]
  4. [sig-network] Services should be able to switch session affinity for NodePort service
    [Execution Time: ~58 Sec]

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

  • These e2e might fail on Windows as they are using wget -T flag, which is not supported on Windows at this moment.
  • No flakes found on Kind cluster. (Testgrid flake links would be added in comment after few days of observation)

Does this PR introduce a user-facing change?:

Node

/area conformance
/sig testing
@kubernetes/sig-node-pr-reviews
@kubernetes/sig-architecture-pr-reviews
@kubernetes/cncf-conformance-wg

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

/assign

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2019

/priority important-soon

@johnbelamaric johnbelamaric moved this from To Triage to Sorted Backlog in cncf-k8s-conformance-wg Apr 11, 2019

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2019

Tracking this PR under: #76345

@mattjmcnaughton

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2019

Tracking this PR under: #76345

I'm 99% sure this is a goofy question, but just I'm new to sig/node and just trying to wrap my head around things :) How do these conformance tests for service sessionAffinity relate to PodSpec coverage? It seems like the tests being promoted here related more closely to ServiceSpec?

No rush to answer - just trying to sort out my mental model :)

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Good catch @mattjmcnaughton. It should suppose to be tracked under ServiceSpec instead of PodSpec. Once we have ServiceSpec Tracking umbrella issue, we can track over there.
cc @brahmaroutu

@spiffxp

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

/kind cleanup

These e2e might fail on Windows as they are using wget -T flag, which is not supported on Windows at this moment.

Do we mark this as [LinuxOnly] and explain why? Or is there some other command like curl that could be used by ServiceTestJig.CheckAffinity instead?

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

@spiffxp We should fix it. Linux only should be for only if the functionality doesn't work on Windows. We need a consistent way to test on both platforms. That means either finding a common command (e.g., curl as you suggest), or adding a command to the agnhost image that works on both platforms.

@timothysc timothysc moved this from Sorted Backlog to In Progress in cncf-k8s-conformance-wg May 1, 2019

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@mgdevstack ping - any update on finding an OS-neutral way to test this?

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

let me take a look on this and will update asap.

@spiffxp

This comment has been minimized.

Copy link
Member

commented May 6, 2019

/sig windows
Since this may involve agnhost

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 23, 2019

/retest

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

/test pull-kubernetes-conformance-image-test

@aojea

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

/test pull-kubernetes-conformance-kind-ipv6

@aojea

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2019

SGTM all 4 tests pass the IPv6 conformance tests (the failures are unrelated to this PR)

@adelina-t

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

I've just tested and these tests don't work on Windows. They should be marked [LinuxOnly] or skipped for Windows tests.

/cc @PatrickLang

@k8s-ci-robot k8s-ci-robot requested a review from PatrickLang Aug 28, 2019

@aojea

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

/retest

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2019

thanks @adelina-t

@mgdevstack - can you add the [LinuxOnly] tag to this PR before it's merged? We need that to keep the test signals clean for 1.16 release.

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

/test pull-kubernetes-e2e-aks-engine-azure-windows

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

pull-kubernetes-e2e-aks-engine-azure-windows failed to setup cluster [link]

What is causing these e2e to fail on Windows, @adelina-t ?

@adelina-t

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

pull-kubernetes-e2e-aks-engine-azure-windows failed to setup cluster [link]

What is causing these e2e to fail on Windows, @adelina-t ?

Why the setup is failing? Basically this line needs to be removed so that the setup tool knows it's a 1.16 cluster: kubernetes/test-infra#14112

Why are the e2e tests failing? Some logs here: https://paste.ubuntu.com/p/B6MJSy4Smb/ . It seems that with kube-proxy for Windows in kernel mode ( which is what we are using ), there is no implementation for session affinity.

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

/test pull-kubernetes-e2e-aks-engine-azure-windows
the job setup fix merged, this should run now but the new cases added will fail

@johnbelamaric johnbelamaric moved this from Needs Approval to In Progress in cncf-k8s-conformance-wg Aug 29, 2019

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

I pulled this from the "Needs Approval" until this is resolved. @PatrickLang is there no alternative to [LinuxOnly] ?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@mgdevstack: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-aks-engine-azure-windows fc84ff1 link /test pull-kubernetes-e2e-aks-engine-azure-windows

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@PatrickLang PatrickLang moved this from In Progress (v1.16) to In merge queue for 1.16 in SIG-Windows Aug 29, 2019

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

@johnbelamaric - that's the only tag we have available today until we get conformance profiles / feature profiles / ... worked out within the conformance working group. Today we use that tag to exclude tests that rely on Linux-only functionality rather than maintaining a very long regex.

@PatrickLang PatrickLang moved this from In merge queue for 1.16 to In Progress (v1.16) in SIG-Windows Aug 29, 2019

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

Yes, what I meant was, is it possible to support this on Windows?

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2019

No, it's not possible with the current routing rules in Windows that the container networking is based on. I've asked the PMs to put it on a roadmap for a future Windows release. ETA would be 2020 or later

cc @daschott

@johnbelamaric

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Ok, let's tag them then

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

Once #82171 merged. This PR would be rebased to promote e2e for next release cycle.

@mgdevstack

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

/hold

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2019

Tracking number for Windows OS work is 23352509. @daschott or I would be able to look up status with that number if you ping us on that in coming months since that database isn't public

@PatrickLang PatrickLang moved this from In Progress (v1.16) to Backlog in SIG-Windows Sep 3, 2019

@xmudrii

This comment has been minimized.

Copy link
Member

commented Sep 5, 2019

As the code freeze has started, I'm moving it to the 1.17 milestone.

/milestone v1.17

@k8s-ci-robot k8s-ci-robot modified the milestones: v1.16, v1.17 Sep 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.