-
Notifications
You must be signed in to change notification settings - Fork 93
Port Service E2E tests from core #115
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
Conversation
@owainlewis Note dependency change |
0c039a2
to
8841eac
Compare
docs/development.md
Outdated
go test -timeout 45m -v ./test/integration/loadbalancer | ||
``` | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Defunct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did an initial pass. I'm guessing you got most of these tests from core? If so i'm not gonna review them in depth. Did you change any?
) | ||
|
||
for _, subnet = range nodeSubnets { | ||
err := wait.ExponentialBackoff(retry.DefaultBackoff, func() (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add backoff here? We should let the controller handle it that way other services have a chance to be updated if the current one errors out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially added it in to make the integration tests pass again. Will remove now focusing on E2E tests.
return fmt.Errorf("get security list for subnet `%s`: %v", subnet.ID, err) | ||
} | ||
var ( | ||
lastErr error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see you don't need lastErr
if err := wait.PollImmediate(Poll, 30*time.Second, func() (bool, error) { | ||
var err error | ||
got, err = f.ClientSet.CoreV1().Namespaces().Create(namespaceObj) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will never exit if the namespace is created. You should check for already exists error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to get an already exists error when using GenerateName
?
test/e2e/framework/framework.go
Outdated
func (f *Framework) DeleteNamespace(namespace string, timeout time.Duration) error { | ||
startTime := time.Now() | ||
if err := f.ClientSet.CoreV1().Namespaces().Delete(namespace, nil); err != nil { | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should return nil here if the namespace wasn't found.
test/e2e/framework/framework.go
Outdated
for _, ns := range f.namespacesToDelete { | ||
By(fmt.Sprintf("Destroying namespace %q for this suite.", ns.Name)) | ||
if err := f.DeleteNamespace(ns.Name, 5*time.Minute); err != nil { | ||
if !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning nil in DeleteNamespace when it's NotFound cleans this up.
|
||
ipPort := net.JoinHostPort(ip, strconv.Itoa(port)) | ||
url := fmt.Sprintf("http://%s%s", ipPort, request) | ||
if ip == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: you should check these errors before you make the url
return false, nil | ||
} | ||
|
||
func TestReachableUDP(ip string, port int, request string, expect string) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used since OCI doesn't support UDP?
con, err := net.Dial("udp", ipPort) | ||
if err != nil { | ||
return false, fmt.Errorf("Failed to dial %s: %v", ipPort, err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're missing defer con.Close()
. This goes for all these tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. I copied this over from core; probably should be fixed there too.
test/e2e/load_balancer_test.go
Outdated
By("checking the TCP LoadBalancer is closed") | ||
jig.TestNotReachableHTTP(tcpIngressIP, svcPort, loadBalancerLagTimeout) | ||
}) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create a ticket to track testing preserving the source ip once OCI supports tcp proxy protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return nil | ||
} | ||
} | ||
return fmt.Errorf("Security list %q does not contain egress rule %+v", secList.ID, rule) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
S should be lowercase
Basic test using modified subset of ServiceTestJig from core's e2e test suite.
dc5c0a4
to
a8af622
Compare
Implements a stripped down version of the Service mutability test from the Kubernetes core E2E test suite.
@jhorwit2 Yes, sorry I should have given you some more context before asking you to take a look. It's still pretty rough around the edges but what I've done is copy parts of the core E2E test framework and Service tests out that apply to the subset of Service type=LoadBalancer functionality that OCI supports. |
@jhorwit2 yes; I got started on it today but got distracted. I’m hoping that gives us enough coverage LB wise to swith to the new OCI SDK. What I’m less clear on is what to do about instances if you have any ideas? |
Although OCI LBaaS is not a pass-through load balancer (and thus does not support ESIPP) this ports the tests covering support for node-local vs cluster-wide routing (i.e. the avoidance of a second hop in the node-local case at the risk of potentially imbalanced traffic spreading.
|
extensionsinternal "k8s.io/kubernetes/pkg/apis/extensions" | ||
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset" | ||
"k8s.io/kubernetes/pkg/controller" | ||
"k8s.io/kubernetes/pkg/kubectl" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This adds the dependency on docker and is only used for the reaping of RC and associated pods (via kubectl.Reaper
). Might be worth ditching the dependency?
26f9891
to
d6d7975
Compare
d6d7975
to
bf05471
Compare
…27-18-43-52 to github (#115) Releasing version V1.8.0
…m fix/oc5-IMDS-lookup to internal * commit 'e02af62049505fef765ec720a5c990e0e78ef33d': Enable IMDS server lookup
Implements a stripped down version of the Service mutability test from the Kubernetes core E2E test suite using a modified subset of the test machinery from core.