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

Fix influxdb e2e test failure. #55412

Merged
merged 1 commit into from
Nov 10, 2017
Merged

Fix influxdb e2e test failure. #55412

merged 1 commit into from
Nov 10, 2017

Conversation

loburm
Copy link
Contributor

@loburm loburm commented Nov 9, 2017 •
edited

In scalability testing influxdb was recently disabled, but we still
trying to execute corresponidng test, as a result it fails all the time.
Skip test if influxdb is disabled.

Fixes #54636

NONE

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 9, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 9, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@loburm loburm force-pushed the fix-infludb-e2e branch 2 times, most recently from ef9bf7c to acc086b Compare November 9, 2017 16:04
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2017
@loburm
Copy link
Contributor Author

loburm commented Nov 9, 2017

/assign @shyamjvs

shyamjvs
shyamjvs reviewed Nov 9, 2017
View reviewed changes
hack/ginkgo-e2e.sh Outdated
@@ -148,6 +148,7 @@ export PATH=$(dirname "${e2e_test}"):"${PATH}"
--network="${KUBE_GCE_NETWORK:-${KUBE_GKE_NETWORK:-e2e}}" \
--node-tag="${NODE_TAG:-}" \
--master-tag="${MASTER_TAG:-}" \
--cluster-monitoring="${KUBE_ENABLE_CLUSTER_MONITORING:-influxdb}" \
Copy link
Member

@shyamjvs shyamjvs Nov 9, 2017 •
edited

Choose a reason for hiding this comment

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

I wouldn't push on it - but cluster-monitoring-mode might be a better name for what the flag is doing.

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.

shyamjvs
shyamjvs reviewed Nov 9, 2017
View reviewed changes
test/e2e/framework/test_context.go Outdated
@@ -103,6 +103,8 @@ type TestContextType struct {
// Node e2e specific test context
NodeTestContextType

// Monitoring solution that is used in current cluster. Values are coming from ENABLE_CLUSTER_MONITORING flag.
Copy link
Member

Choose a reason for hiding this comment

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

You mean 'KUBE_ENABLE_CLUSTER_MONITORING env var'?
I would skip mentioning that, as it's not necessary that the value has to come from there - someone can also just set the flag.

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.

shyamjvs
shyamjvs reviewed Nov 9, 2017
View reviewed changes
test/e2e/framework/test_context.go Outdated
@@ -103,6 +103,8 @@ type TestContextType struct {
// Node e2e specific test context
NodeTestContextType

// Monitoring solution that is used in current cluster. Values are coming from ENABLE_CLUSTER_MONITORING flag.
ClusterMonitoring string
Copy link
Member

Choose a reason for hiding this comment

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

Add a newline before the next line (like it was before). And remove the one above - it's not needed imo.

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.

shyamjvs
shyamjvs reviewed Nov 9, 2017
View reviewed changes
test/e2e/framework/util.go Outdated
@@ -331,6 +331,12 @@ func SkipUnlessProviderIs(supportedProviders ...string) {
}
}

func SkipUnlessClusterMonitoringIs(supportedMonitoring ...string) {
Copy link
Member

Choose a reason for hiding this comment

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

Here also I'd name it sth like SkipUnlessClusterMonitoringModeIs.

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.

@loburm
Fix influxdb e2e test failure.
ba31379 
In scalability testing influxdb was recently disabled, but we still
trying to execute corresponidng test, as a result it fails all the time.
Skip test if influxdb is disabled.
@loburm
Copy link
Contributor Author

loburm commented Nov 10, 2017

Thanks for the review. I have resolved all your comments, PTAL.

@loburm
Copy link
Contributor Author

loburm commented Nov 10, 2017

/test pull-kubernetes-bazel-build
/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-node-e2e

@shyamjvs
Copy link
Member

/lgtm

Thanks a lot for fixing!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2017
@shyamjvs
Copy link
Member

@gmarek Could you approve for the hack/ part?

@gmarek
Copy link
Contributor

gmarek commented Nov 10, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gmarek, loburm, shyamjvs

Associated issue: 54636

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-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 55394, 55412). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 7c04a68 into kubernetes:master Nov 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shyamjvs shyamjvs shyamjvs left review comments

Assignees

@bowei bowei

@shyamjvs shyamjvs

@pwittrock pwittrock

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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@loburm @shyamjvs @gmarek @k8s-github-robot @bowei @pwittrock @k8s-ci-robot