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

Abstract storage error handling #18163

Merged
merged 1 commit into from
Dec 10, 2015
Merged

Abstract storage error handling #18163

merged 1 commit into from
Dec 10, 2015

Conversation

timothysc
Copy link
Member

Shuffle error handling through abstraction layer to allow for multiple back-ends KV stores.

/cc @wojtek-t for review.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@timothysc timothysc force-pushed the storage_error_handling branch from 2b4b7e1 to f69c604 Compare December 3, 2015 19:42
@googlebot
Copy link

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Dec 3, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 3, 2015
@wojtek-t wojtek-t assigned wojtek-t and unassigned bgrant0607 Dec 3, 2015
@wojtek-t
Copy link
Member

wojtek-t commented Dec 3, 2015

@timothysc - will take a look tomorrow

@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit 2b4b7e1cd9ec7caa3f1c5008250a7336f070cee2.

@k8s-bot
Copy link

k8s-bot commented Dec 3, 2015

GCE e2e test build/test passed for commit f69c6047c7f17ce8f17c248dae81eee64f3ff1ab.

@timothysc timothysc mentioned this pull request Dec 3, 2015
Closed
wojtek-t
wojtek-t reviewed Dec 4, 2015
View reviewed changes
pkg/storage/errors.go
return etcdutil.IsEtcdTestFailed(err)
}

//TODO: Add any further storage errors that may require special handling.
Copy link
Member

Choose a reason for hiding this comment

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

Can we remove this TODO now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@timothysc timothysc force-pushed the storage_error_handling branch from f69c604 to 216825e Compare December 4, 2015 15:13
@wojtek-t
Copy link
Member

wojtek-t commented Dec 4, 2015

LGTM - thanks!

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 4, 2015
@k8s-bot
Copy link

k8s-bot commented Dec 4, 2015

GCE e2e test build/test passed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 6, 2015

GCE e2e build/test failed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@timothysc
Copy link
Member Author

@wojtek-t umm I think the bot is loosing it's mind here.

@wojtek-t
Copy link
Member

wojtek-t commented Dec 7, 2015

@timothysc - no; the problem is that we have extreeeeeeemly huge problems with our tests - tests are extremely flaky and we have backlog of more than 40 PRs that are ready to merge. It may take some time... (sorry)

@wojtek-t
Copy link
Member

wojtek-t commented Dec 7, 2015

@k8s-bot e2e test this please

@k8s-bot
Copy link

k8s-bot commented Dec 7, 2015

GCE e2e test build/test passed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 8, 2015

GCE e2e build/test failed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@timothysc timothysc closed this Dec 8, 2015
@timothysc timothysc reopened this Dec 8, 2015
@timothysc
Copy link
Member Author

So I re-ran tests locally, all green.

@wojtek-t
Copy link
Member

wojtek-t commented Dec 9, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e build/test failed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@wojtek-t
Copy link
Member

wojtek-t commented Dec 9, 2015

@k8s-bot e2e test this please

@timothysc
Copy link
Member Author

So it appears the Pods e2e.test is now timing out, when it passes locally and on previous run.

• [SLOW TEST:257.275 seconds]
Pods
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/pods.go:1177
should have their auto-restart back-off timer reset on image update
/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/pods.go:898

@wojtek-t
Copy link
Member

wojtek-t commented Dec 9, 2015

@timothysc - this failure:
"Pods should not back-off restarting a container on LivenessProbe failure"
is a known issue - #18293
I just moved it to flaky suite so it shouldn't cause more problems

@timothysc
Copy link
Member Author

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e build/test failed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@k8s-bot
Copy link

k8s-bot commented Dec 9, 2015

GCE e2e test build/test passed for commit 216825e7aa55b4b35afa98fc0d9343cfe3aa4e03.

@timothysc timothysc mentioned this pull request Dec 9, 2015
Closed
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 9, 2015
@timothysc timothysc force-pushed the storage_error_handling branch from 216825e to a428246 Compare December 10, 2015 14:10
@k8s-github-robot
Copy link

PR changed after LGTM, removing LGTM.

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 10, 2015
@timothysc
Copy link
Member Author

@wojtek-t needs re-apply, due to rebase, due to testing circus.

@wojtek-t wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2015
@wojtek-t
Copy link
Member

Sure - LGTM

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit a428246.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Dec 10, 2015

GCE e2e test build/test passed for commit a428246.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 10, 2015
Merge pull request #18163 from timothysc/storage_error_handling
0127c45 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 0127c45 into kubernetes:master Dec 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@wojtek-t wojtek-t

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@timothysc @googlebot @k8s-github-robot @wojtek-t @k8s-bot @k8s-teamcity-mesosphere @bgrant0607