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

Improve the upgrade test for ingress. #58412

Merged

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented Jan 17, 2018

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.

None

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 17, 2018
@rramkumar1
Copy link
Contributor Author

rramkumar1 commented Jan 17, 2018

cc @nicksardo @MrHohn @bowei
For review

@MrHohn
Copy link
Member

MrHohn commented Jan 17, 2018

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 17, 2018
@rramkumar1 rramkumar1 force-pushed the ingress-upgrade-testing branch 7 times, most recently from cb13725 to 6138099 Compare January 17, 2018 22:59
Copy link
Member

@MrHohn MrHohn left a 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.
Copy link
Member

Choose a reason for hiding this comment

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

double with

Copy link
Contributor Author

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"}
Copy link
Member

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"?

Copy link
Contributor Author

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"}

Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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")
Copy link
Member

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

if len(umList[0].HostRules) > 0 {
return true, nil
} else {
return false, nil
Copy link
Member

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.

Copy link
Contributor Author

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"}
Copy link
Member

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, done.

@rramkumar1 rramkumar1 force-pushed the ingress-upgrade-testing branch 3 times, most recently from 19ebf41 to 349d32c Compare January 18, 2018 16:55
@@ -0,0 +1,16 @@
apiVersion: v1
kind: ReplicationController
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See issue #58640

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)
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@foxish foxish removed their assignment Jan 18, 2018
@@ -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 {
Copy link
Contributor

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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed triggerSync.

})
})
// Wait for update to sync with cloud.
t.jig.WaitForIngress(false)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, done.

@nicksardo
Copy link
Contributor

Minor notes, but LG overall.

Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

/lgtm

Paths: []extensions.HTTPIngressPath{
{
Path: "/test",
// Note: Dependant on using "static-ip" manifest.
Copy link
Member

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@MrHohn
Copy link
Member

MrHohn commented Jan 19, 2018

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2018
@rramkumar1 rramkumar1 force-pushed the ingress-upgrade-testing branch from 0e04e9f to c65633e Compare January 19, 2018 20:02
@rramkumar1
Copy link
Contributor Author

/retest

@rramkumar1 rramkumar1 force-pushed the ingress-upgrade-testing branch from c65633e to 312f36f Compare January 22, 2018 23:26
Copy link
Member

@bowei bowei left a 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

l, err := gceCloud.ListGlobalForwardingRules()
Expect(err).NotTo(HaveOccurred())
for _, fwd := range l {
if cont.canDelete(fwd.Name, fwd.CreationTimestamp, false) {
Copy link
Member

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?

Copy link
Contributor Author

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")
Copy link
Member

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...)

Copy link
Contributor Author

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

@bowei
Copy link
Member

bowei commented Jan 23, 2018

Dicussed: will put ability to inject own image for testing in a follow-on PR.

/lgtm
/approve no-issue

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018
@k8s-ci-robot
Copy link
Contributor

[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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2018
@rramkumar1 rramkumar1 force-pushed the ingress-upgrade-testing branch from 312f36f to 2941c4b Compare January 23, 2018 00:53
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018
@bowei
Copy link
Member

bowei commented Jan 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@rramkumar1
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce

@rramkumar1
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

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.

@k8s-github-robot k8s-github-robot merged commit a00b92b into kubernetes:master Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants