-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
Improve the upgrade test for ingress. #58412
Improve the upgrade test for ingress. #58412
Conversation
cc @nicksardo @MrHohn @bowei |
/ok-to-test |
cb13725
to
6138099
Compare
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.
Thanks, looks good overall.
@@ -72,6 +72,11 @@ var kubeProxyDowngradeTests = []upgrades.Test{ | |||
&upgrades.IngressUpgradeTest{}, | |||
} | |||
|
|||
// Upgrade ingress with with custom image. |
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.
double with
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.
Done
framework.ExpectNoError(err) | ||
|
||
testSuite := &junit.TestSuite{Name: "ingress upgrade"} | ||
ingressTest := &junit.TestCase{Name: "[sig-networking] ingress", Classname: "upgrade_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.
Probably name it "[sig-networking] ingress-upgrade"?
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.
Done
|
||
testSuite := &junit.TestSuite{Name: "ingress upgrade"} | ||
ingressTest := &junit.TestCase{Name: "[sig-networking] ingress", Classname: "upgrade_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.
Missing testSuite.TestCases = append(testSuite.TestCases, ingressTest)
here?
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.
Done
test/e2e/upgrades/ingress.go
Outdated
return nil | ||
} else { | ||
// TODO(rramkumar): Make this error message more descriptive. | ||
return fmt.Errorf("forced update of UrlMap did not result in proper sync to cloud") |
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.
What about return without the else block?
if ... {
return 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.
Done
test/e2e/upgrades/ingress.go
Outdated
if !reflect.DeepEqual(i1, i2) { | ||
return spew.Errorf("resources after ingress upgrade were different:\n Pre-Upgrade: %#v\n Post-Upgrade: %#v", i1, i2) | ||
} else { | ||
return 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.
return without the else block.
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.
Done
test/e2e/upgrades/ingress.go
Outdated
if len(umList[0].HostRules) > 0 { | ||
return true, nil | ||
} else { | ||
return false, 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.
return without the else block.
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.
Done
out.txt
Outdated
@@ -0,0 +1,146 @@ | |||
Client Version: version.Info{Major:"1", Minor:"10+", GitVersion:"v1.10.0-alpha.1.610+179eef88ec27bc-dirty", GitCommit:"179eef88ec27bc79002445658281998c21008332", GitTreeState:"dirty", BuildDate:"2018-01-12T18:12:48Z", GoVersion:"go1.9.2", Compiler:"gc", Platform:"linux/amd64"} |
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.
out.txt was included unexpectedly...
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.
Oops, done.
19ebf41
to
349d32c
Compare
@@ -0,0 +1,16 @@ | |||
apiVersion: v1 | |||
kind: ReplicationController |
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.
Should we upgrade this to deployment?
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 can also change the rc to a deployment for static-ip/rc.yaml. I'll make that change in a separate PR.
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.
See issue #58629
@@ -0,0 +1,11 @@ | |||
apiVersion: extensions/v1beta1 | |||
kind: Ingress |
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.
nginx
seems to have its own folder under test/e2e/testing-manifests
. Should GCE do the same since we'll be testing annotations specific to this controller?
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.
Will make that change in a separate PR.
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.
See issue #58640
test/e2e/upgrades/ingress.go
Outdated
default: | ||
// Currently ingress gets disrupted across node upgrade, because endpoints | ||
// get killed and we don't have any guarantees that 2 nodes don't overlap | ||
// their upgrades (even on cloud platforms like GCE, because VM level | ||
// rolling upgrades are not Kubernetes aware). | ||
t.verify(f, done, false) | ||
t.verify(f, done, false, false) |
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 we're okay with always triggering a sync regardless of being a master or ingress upgrade.
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 can enable for master upgrades later if you prefer.
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.
Removed triggerSync bool so its enabled for master and ingress upgrade.
test/e2e/upgrades/ingress.go
Outdated
@@ -122,4 +153,81 @@ func (t *IngressUpgradeTest) verify(f *framework.Framework, done <-chan struct{} | |||
} | |||
By("hitting the Ingress IP " + t.ip) | |||
framework.ExpectNoError(framework.PollURL(fmt.Sprintf("https://%v/", t.ip), "", framework.LoadBalancerPollTimeout, t.jig.PollInterval, t.httpClient, false)) | |||
|
|||
if triggerSync { |
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.
If you're going to keep triggerSync
, recommend returning early.
if !triggerSync {
return
}
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.
Removed triggerSync.
test/e2e/upgrades/ingress.go
Outdated
}) | ||
}) | ||
// Wait for update to sync with cloud. | ||
t.jig.WaitForIngress(false) |
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.
Can you mention in this comment that WaitForIngress will test that all paths are pinged which is how we know it's synced.
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.
Good call, done.
Minor notes, but LG overall. |
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.
/lgtm
test/e2e/upgrades/ingress.go
Outdated
Paths: []extensions.HTTPIngressPath{ | ||
{ | ||
Path: "/test", | ||
// Note: Dependant on using "static-ip" manifest. |
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.
minor: now it is static-ip-2?
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.
Done.
/test pull-kubernetes-e2e-kops-aws |
0e04e9f
to
c65633e
Compare
/retest |
c65633e
to
312f36f
Compare
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.
mostly nits, other wise looks good
test/e2e/framework/ingress_utils.go
Outdated
l, err := gceCloud.ListGlobalForwardingRules() | ||
Expect(err).NotTo(HaveOccurred()) | ||
for _, fwd := range l { | ||
if cont.canDelete(fwd.Name, fwd.CreationTimestamp, false) { |
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.
canDelete seems to be a strange name; should this actually be isOwned?
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.
Done. Added an isOwned function which is just a wrapper for canDelete.
func ingressUpgradeGCE() error { | ||
// Flip glbc image from latest release image to HEAD to simulate an upgrade. | ||
// Kubelet should restart glbc automatically. | ||
sshResult, err := NodeExec(GetMasterHost(), "sudo sed -i -re 's/(image:)(.*)/\\1 gcr.io\\/e2e-ingress-gce\\/ingress-gce-e2e-glbc-amd64:latest/' /etc/kubernetes/manifests/glbc.manifest") |
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.
Should the name of the container be more unique? Isn't this going to end up sharing the same image across all invocations of the test (say on my personal machine...)
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.
Ack. Issue to track that change is #6381
Dicussed: will put ability to inject own image for testing in a follow-on PR. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, MrHohn, rramkumar1 Associated issue requirement bypassed by: bowei The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
312f36f
to
2941c4b
Compare
/lgtm |
/test all [submit-queue is verifying that this PR is safe to merge] |
/test pull-kubernetes-e2e-gce |
/test pull-kubernetes-e2e-kops-aws |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 58412, 56132, 58506, 58542, 58394). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
This PR improves the existing upgrade e2e test for ingress-gce. Specifically, we add a test which upgrades ingress with a image built from HEAD of the ingress-gce repo.