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

Set ephemeral storage to match cache #2953

Merged
merged 5 commits into from
Jul 21, 2021
Merged

Conversation

daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented May 11, 2021

Related to #2369

Fixes https://github.com/sourcegraph/sourcegraph/issues/20869

If these values differ, the pod will be evicted for exceeding its storage allotment

Checklist

@daxmc99 daxmc99 requested a review from a team May 11, 2021 17:08
@daxmc99
Copy link
Contributor Author

daxmc99 commented May 11, 2021

Could @sourcegraph/search-core take a look at this to see if this resource reduction is ok?

Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

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

TIL ephemeral-storage limits! Finally! I see there is a whole bunch of stuff related to ephemeral-storage in k8s now, not just emptyDir. Would love to use GenericEphemeralVolume so we can get rid of the dir watcher daemonset stuff. I see that it is behind a feature flag though :(

@@ -30,7 +30,7 @@ spec:
- name: searcher
env:
- name: SEARCHER_CACHE_SIZE_MB
value: "100000"
value: "50000"
Copy link
Member

Choose a reason for hiding this comment

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

Looks good. Maybe we should avoid duplication here, and instead use something like

valueFrom:
  fieldRef:
    fieldPath: spec.resources.requests.ephemeral-storage

That will require us updating the cache parsing code to understand kubernetes resource limit specifications, but that would be a good thing. Not for this PR, but what do you think?

@davejrt
Copy link
Contributor

davejrt commented May 12, 2021

Thanks for this, I had to adjust this for a customer but never got back around to fixing the root cause.

@daxmc99 daxmc99 enabled auto-merge (squash) May 13, 2021 00:33
@daxmc99 daxmc99 disabled auto-merge May 13, 2021 00:35
@daxmc99 daxmc99 changed the title Set ephermeral storage to match cache Set ephemeral storage to match cache May 13, 2021
@daxmc99 daxmc99 enabled auto-merge (squash) May 13, 2021 00:35
@daxmc99
Copy link
Contributor Author

daxmc99 commented May 19, 2021

I was incorrect with my first attempt at this. This requires using resourceFieldRef to specify the value.

@daxmc99 daxmc99 force-pushed the dax/searcher_ephermal_storage branch from 7165b69 to e63fb0d Compare July 20, 2021 22:10
@daxmc99 daxmc99 merged commit 5c45d8e into master Jul 21, 2021
@daxmc99 daxmc99 deleted the dax/searcher_ephermal_storage branch July 21, 2021 00:48
daxmc99 added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Jul 21, 2021
Update CHANGELOG for ephemeral storage change [#2953](sourcegraph/deploy-sourcegraph#2953)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants