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

Script to cache metadata requests on the jenkins master #24131

Merged
merged 1 commit into from
Apr 26, 2016
Merged

Script to cache metadata requests on the jenkins master #24131

merged 1 commit into from
Apr 26, 2016

Conversation

fejta
Copy link
Contributor

@fejta fejta commented Apr 12, 2016

Fixes #23545

Create an http server which caches most requests to the metadata server. Use special logic to cache access tokens such that the expires_on json field is correct. Add a script to simplify enabling/disabling the cache by editing /etc/hosts

@fejta fejta added e2e-not-required release-note-none Denotes a PR that doesn't merit a release note. labels Apr 12, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 12, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 12, 2016

GCE e2e build/test passed for commit 4f30fd72e7994561cfbe92c4a6dd70ad7d578105.

@fejta fejta assigned spxtr and unassigned ixdy Apr 13, 2016
ixdy
ixdy reviewed Apr 13, 2016
View reviewed changes
hack/jenkins/metadata-cache/metadata-cache.py
if uri not in _global_cache:
with LOCK:
if uri not in _global_cache:
r2, ok = proxy_request(uri)
Copy link
Member

Choose a reason for hiding this comment

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

do we want to hold the lock while running the request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is simple to understand and since we make each request once the impact is essentially zero.

@k8s-bot
Copy link

k8s-bot commented Apr 14, 2016

GCE e2e build/test passed for commit c70387ad59cd76dc4151fee18ac25d26b5bc2002.

@k8s-bot
Copy link

k8s-bot commented Apr 14, 2016

GCE e2e build/test passed for commit 511dbdf1439ac2b197256687b47631f1654a4b6e.

@ixdy ixdy mentioned this pull request Apr 14, 2016
Merged
@k8s-bot
Copy link

k8s-bot commented Apr 15, 2016

GCE e2e build/test passed for commit 78ed17a328a9bbbe77bfa83b7db18b7fa2de78de.

k8s-github-robot pushed a commit that referenced this pull request Apr 15, 2016
Merge pull request #24281 from ixdy/dockerized-e2e
82458d8 
Automatic merge from submit-queue

Set metadata.google.internal IP in dockerized e2e based on /etc/hosts

Support the metadata cacher from #24131 inside dockerized e2e runs.

cc @fejta
ixdy
ixdy reviewed Apr 15, 2016
View reviewed changes
hack/jenkins/metadata-cache/metadata-cache-control.sh
MGI='metadata.google.internal'
TARGET="/etc/hosts"

grep "$MGI" "$TARGET" || fail "$MGI missing from $TARGET"
Copy link
Member

Choose a reason for hiding this comment

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

nit still not fixed (${MGI}, ${TARGET}, etc)

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

@fejta
Copy link
Contributor Author

fejta commented Apr 20, 2016

PTAL

@fejta fejta mentioned this pull request Apr 20, 2016
Closed
@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 34a5a47243f06e55aa0804542b059e85ce21d7b5.

@spxtr
Copy link
Contributor

spxtr commented Apr 20, 2016 •
edited
Loading

Are we planning on running this even after the underlying problem is fixed? (I'd be totally fine with that, but I wasn't sure if that's the plan)

spxtr
spxtr reviewed Apr 20, 2016
View reviewed changes
hack/jenkins/metadata-cache/metadata-cache.py


def cache_request(uri):
if uri not in _global_cache:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why check this outside the lock if we're just going to recheck once we lock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it is already in the cache I don't want to bother waiting to acquire the lock.

See double-checked locking: https://en.wikipedia.org/wiki/Double-checked_locking

@fejta
Copy link
Contributor Author

fejta commented Apr 20, 2016

Yes, I think we should use the cache until we figure out whether the metadata server has an SLO and if that SLO meets our needs.

@spxtr
Copy link
Contributor

spxtr commented Apr 25, 2016

For some reason the metadata server cache crashed on jenkins-builder-2 today. I think it's fine to merge this after squash, but we should figure out what went wrong ASAP, since when this breaks all e2e jobs start to fail.

@ixdy
Copy link
Member

ixdy commented Apr 25, 2016

Appears to have crashed on jenkins-master too. I'm pretty sure that's why kubemark has been failing continuously for the last few days.

@ixdy
Copy link
Member

ixdy commented Apr 25, 2016

Issue seems to be that both jenkins-master and jenkins-builder-2 restarted for some reason (probably a kernel crash), and so the cache was no longer running as a result. We need to have the cache server started on boot.

@fejta
Copy link
Contributor Author

fejta commented Apr 26, 2016

Let's get this merged. I can work on getting this started automatically on boot thereafter.

@spxtr
Copy link
Contributor

spxtr commented Apr 26, 2016 •
edited
Loading

SGTM. Probably a good idea to squash.

@spxtr spxtr added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 26, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

GCE e2e build/test passed for commit 34a5a47243f06e55aa0804542b059e85ce21d7b5.

@fejta fejta added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

GCE e2e build/test passed for commit 7605ff0.

@fejta
Copy link
Contributor Author

fejta commented Apr 26, 2016

@k8s-bot test this issue #24782

@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

GCE e2e build/test passed for commit 7605ff0.

@fejta
Copy link
Contributor Author

fejta commented Apr 26, 2016

@k8s-bot test this issue #24782

@fejta
Copy link
Contributor Author

fejta commented Apr 26, 2016

#24661 is actually the cause of the previous flake.

@k8s-bot
Copy link

k8s-bot commented Apr 26, 2016

GCE e2e build/test passed for commit 7605ff0.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit b8131fe into kubernetes:master Apr 26, 2016
@fejta fejta deleted the cron branch April 26, 2016 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@spxtr spxtr

Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@fejta @k8s-bot @spxtr @ixdy @k8s-github-robot @k8s-teamcity-mesosphere @googlebot