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.

Sign up for GitHub

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

Jump to bottom

Fixing spark e2e test failures. #36548

Merged
merged 1 commit into from
Nov 11, 2016
Merged

Fixing spark e2e test failures. #36548

merged 1 commit into from
Nov 11, 2016

Conversation

foxish
Copy link
Contributor

@foxish foxish commented Nov 10, 2016 •
edited

What this PR does / why we need it: Fixes e2e test failures in the spark tests. These failures were caused by an update to the examples in #33604

Which issue this PR fixes : fixes #36102

Release note:

NONE

cc @calebamiles @elsonrodriguez @saad-ali

This change is Reviewable

@foxish foxish added area/stateful-apps priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. labels Nov 10, 2016
@foxish foxish added this to the v1.5 milestone Nov 10, 2016
@foxish
Copy link
Contributor Author

foxish commented Nov 10, 2016

@jimmycuadra @elsonrodriguez PTAL

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 10, 2016
@foxish foxish mentioned this pull request Nov 10, 2016
Closed
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 81dde54c17f88484c44eeb1da03d45f62e2ab2f3. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@foxish
Copy link
Contributor Author

foxish commented Nov 10, 2016

@k8s-bot cvm gke e2e test this

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 81dde54c17f88484c44eeb1da03d45f62e2ab2f3. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jimmycuadra
Copy link
Contributor

Did you mean to assign me on this one? I don't have any context for this code.

@foxish
Copy link
Contributor Author

foxish commented Nov 10, 2016

@k8s-bot kubemark e2e test this

@foxish
Copy link
Contributor Author

foxish commented Nov 10, 2016

@jimmycuadra Unassigned. I assigned you as you dealt with the e2e tests for cassandra recently.

@jimmycuadra
Copy link
Contributor

Ah. That was just a follow up to a previous PR I made finishing some of the petset rename stuff.

@foxish foxish assigned chrislovecnm and unassigned jimmycuadra Nov 10, 2016
@elsonrodriguez
Copy link
Contributor

@foxish looks good, but next time don't shy away from asking for an example fix if it makes sense.

@foxish
Copy link
Contributor Author

foxish commented Nov 10, 2016

@elsonrodriguez I felt that the way the examples are written right now makes sense, with the namespace. Perhaps we could have kept the namespace from the example YAMLs, and just changed the instructions to specify --namespace= with each kubectl command. I'm not sure which I prefer.

chrislovecnm
chrislovecnm suggested changes Nov 10, 2016
View reviewed changes
Copy link
Contributor

@chrislovecnm chrislovecnm left a comment

Choose a reason for hiding this comment

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

A couple of recommendation. Thank for addressing this!!

test/e2e/examples.go
nsSpark := "spark-cluster" // TODO: find a way to infer this from the yaml.
AfterEach(func() {
// clean up the spark namespace.
err := c.Core().Namespaces().Delete(nsSpark, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

We just throwing a pain here if the build framework is borked?

Copy link
Contributor

Choose a reason for hiding this comment

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

Panic not pain ...

test/e2e/examples.go
// TODO: Add Zepplin and Web UI to this example.
serviceYaml := mkpath("spark-master-service.yaml")
masterYaml := mkpath("spark-master-controller.yaml")
workerControllerYaml := mkpath("spark-worker-controller.yaml")
nsFlag := fmt.Sprintf("--namespace=%v", ns)
nsYaml := mkpath("namespace-spark-cluster.yaml")
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we check that this file exists. This bit me in a C* e2e test.

@dims dims mentioned this pull request Nov 11, 2016
Closed
@janetkuo
Copy link
Member

janetkuo commented Nov 11, 2016 •
edited

Review status: 0 of 2 files reviewed at latest revision, 4 unresolved discussions.

test/e2e/examples.go, line 145 at r1 (raw file):

  framework.KubeDescribe("Spark", func() {
      nsSpark := "spark-cluster" // TODO: find a way to infer this from the yaml.

Can we just remove the namespace in all spark yaml files? In the examples, the namespace is already set to spark-cluster, therefore, specifying namespace in the yaml isn't necessary. Plus, the namespace used in test should be unique.

test/e2e/examples.go, line 148 at r1 (raw file):

      AfterEach(func() {
          // clean up the spark namespace.
          err := c.Core().Namespaces().Delete(nsSpark, nil)

Need to change this if removing nsSpark

Comments from Reviewable

@foxish
Copy link
Contributor Author

foxish commented Nov 11, 2016

@janetkuo I misunderstood how the example worked. You're right, this change would be unnecessary; and removing the namespace from the example YAMLs should suffice. Will update. Thanks!

@foxish
Copy link
Contributor Author

foxish commented Nov 11, 2016 •
edited

@janetkuo PTAL. Updated. The e2e test also passes locally.

@k8s-github-robot k8s-github-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 11, 2016
@k8s-github-robot k8s-github-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 11, 2016
@janetkuo
Copy link
Member

/lgtm

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

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3509419 into kubernetes:master Nov 11, 2016
@foxish foxish deleted the fix-spark-e2e branch November 11, 2016 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chrislovecnm chrislovecnm chrislovecnm requested changes

Assignees

@chrislovecnm chrislovecnm

@janetkuo janetkuo

Labels
area/stateful-apps lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

[k8s.io] [Feature:Example] [k8s.io] Spark should start spark master, driver and workers {Kubernetes e2e suite}
8 participants
@foxish @k8s-ci-robot @jimmycuadra @elsonrodriguez @janetkuo @k8s-github-robot @chrislovecnm @googlebot