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

udn, e2e: Add external client to nodeport and loadblanacer services tests #4702

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

qinqon
Copy link
Contributor

@qinqon qinqon commented Sep 5, 2024

What this PR does and why is it needed

Add e2e test to check connectivity from external client to primary UDN nodeport and loadbalancer services

NOTE: This PR pins metallb to a commit since we don't have a release yet, but this can be fix later on.

Tests:

  • external client to nodeport service on UDPN
  • external client to externalIP service on UDPN
  • external client to loadbalancerIP service on UDPN
  • podify client at other node with nodeport
  • podify client at other node with externalIP
  • podify client at other node with LBIP

Which issue(s) this PR fixes

Fixes #4699

Add primary UDN LB and external client services

@qinqon qinqon requested a review from a team as a code owner September 5, 2024 11:40
@qinqon qinqon requested a review from kyrtapz September 5, 2024 11:40
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from eaa9650 to 75b26c9 Compare September 5, 2024 12:24
@qinqon qinqon changed the title udn, e2e: Add external client to nodeport tests udn, e2e: Add external client to nodeport and loadblanacer services tests Sep 5, 2024
@qinqon qinqon closed this Sep 6, 2024
@qinqon qinqon reopened this Sep 6, 2024
@tssurya
Copy link
Contributor

tssurya commented Sep 6, 2024

we need tests for

  1. external client to nodeport service on UDPN
  2. external client to externalIP service on UDPN
  3. external client to loadbalancerIP service on UDPN
  4. other node as client (meaning node that is different from where the backend pod lives) to nodeport/externalIP/LBIP where backend pod is not on the same node as client node

@martinkennelly maybe talk to @qinqon and see if he needs you to help with half of these...?

@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch 3 times, most recently from ceaf04f to b73b9e7 Compare September 9, 2024 10:50
@qinqon qinqon closed this Sep 9, 2024
@qinqon qinqon reopened this Sep 9, 2024
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch 5 times, most recently from 9938c6e to 6e3bc40 Compare September 10, 2024 10:29
@qinqon
Copy link
Contributor Author

qinqon commented Sep 10, 2024

some control-plane CI is failing

Summarizing 3 Failures:
  [FAIL] Load Balancer Service Tests with MetalLB [It] Should ensure load balancer service works with pmtud
  /home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/service.go:1694
  [FAIL] Load Balancer Service Tests with MetalLB [It] Should ensure load balancer service works when ETP=local and session affinity is set
  /home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/service.go:1921
  [FAIL] Load Balancer Service Tests with MetalLB [It] Should ensure load balancer service works when ETP=local and backend pods are also egressIP served pods
  /home/runner/work/ovn-kubernetes/ovn-kubernetes/test/e2e/service.go:2049

@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch 3 times, most recently from 308e286 to d6f508f Compare September 10, 2024 14:07
@github-actions github-actions bot added feature/egress-ip Issues related to EgressIP feature area/unit-testing Issues related to adding/updating unit tests feature/services&endpoints All issues related to the Servces/Endpoints API feature/egress-gateway All issues related to ICNI/APBR labels Sep 10, 2024
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from d6f508f to bcbabb8 Compare September 10, 2024 14:34
@qinqon qinqon closed this Sep 10, 2024
@qinqon qinqon reopened this Sep 10, 2024
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from bcbabb8 to a99cbef Compare September 10, 2024 15:14
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch 3 times, most recently from b3153cb to 5f12a4a Compare November 5, 2024 15:40
@qinqon qinqon closed this Nov 5, 2024
@qinqon qinqon reopened this Nov 5, 2024
@qinqon qinqon closed this Nov 5, 2024
@qinqon qinqon reopened this Nov 5, 2024
@qinqon qinqon closed this Nov 6, 2024
@qinqon qinqon reopened this Nov 6, 2024
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from 5f12a4a to 9e69dd4 Compare November 6, 2024 08:30
kyrtapz
kyrtapz previously approved these changes Nov 6, 2024
Copy link
Contributor

@kyrtapz kyrtapz left a comment

Choose a reason for hiding this comment

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

/lgtm

@tssurya
Copy link
Contributor

tssurya commented Nov 6, 2024

/assign @tssurya

maiqueb
maiqueb previously approved these changes Nov 6, 2024
Copy link
Contributor

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Thank you.

Just left a question to @ricky-rav and a suggestion to @qinqon .

Comment on lines -57 to -58
By("Removing the namespace so all resources get deleted")
err := cs.CoreV1().Namespaces().Delete(context.TODO(), f.Namespace.Name, metav1.DeleteOptions{})
framework.ExpectNoError(err, "Failed to remove the namespace %s %v", f.Namespace.Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ricky-rav is there a reason why weren't relying on the framework's automatic teardown to clean up the namespace ? Just making sure - I couldn't come up with one.

@qinqon maybe we want to preserve the By clause (when deleting the defaultNetNamespace).

@qinqon
Copy link
Contributor Author

qinqon commented Nov 6, 2024

@maiqueb @ricky-rav I understand that the main difference here is that this test suite is create an extra namespace to work with default pod network.

We should just use framework's AddNamespacesToDelete so the framework delete it for us and also keep it with --delete-namespace=true

@qinqon qinqon dismissed stale reviews from maiqueb and kyrtapz via d8bca82 November 6, 2024 15:14
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from 9e69dd4 to d8bca82 Compare November 6, 2024 15:14
@qinqon qinqon closed this Nov 7, 2024
@qinqon qinqon reopened this Nov 7, 2024
@qinqon qinqon closed this Nov 7, 2024
@qinqon qinqon reopened this Nov 7, 2024
Latest metallb repo support dualstack at dev-env let's pin to it.

Signed-off-by: Enrique Llorente <[email protected]>
Add testing for:
- External clients to LB, NodePort and ExternalIPs
- Podify clients to LB and ExternalIPs

Also skipping testing not working yet with local gw mode.

Signed-off-by: Enrique Llorente <[email protected]>
@qinqon qinqon force-pushed the primary-udn-external-to-nodePort-e2e branch from d8bca82 to 8d6a215 Compare November 8, 2024 11:04
@tssurya
Copy link
Contributor

tssurya commented Nov 9, 2024

hmm all the lanes are red? @qinqon can you rebase on latest master?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/e2e-testing area/unit-testing Issues related to adding/updating unit tests feature/egress-gateway All issues related to ICNI/APBR feature/egress-ip Issues related to EgressIP feature feature/services&endpoints All issues related to the Servces/Endpoints API feature/user-defined-network-segmentation All PRs related to User defined network segmentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UDN: Add external -> services e2e
4 participants