-
Notifications
You must be signed in to change notification settings - Fork 283
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
[BUG] apache.vhosts.standard (and potentially others) generate MemoryError #297
Comments
Thanks for the report, @ixs. I add that the entries under the apache-formula/apache/config/vhosts/standard.sls Lines 22 to 26 in e2e1be1
Meaning: - context:
apache: {{ apache|json }}
id: {{ id|json }}
... @noelmcloughlin Could you have a look at this, please? |
How did I become a code owner ;-) We need to encourage issue raisers to
contribute PRs too. Okay, the value `apache: {{ apache|json }}` was added a
few years ago, I can raise a PR.
…On Tue, Dec 8, 2020 at 6:51 PM Imran Iqbal ***@***.***> wrote:
Thanks for the report, @ixs <https://github.com/ixs>. I add that the
entries under the context block need to be indented by another 2 spaces
as well:
https://github.com/saltstack-formulas/apache-formula/blob/e2e1be18e093dca2d603b68075b388d0f8b61d3d/apache/config/vhosts/standard.sls#L22-L26
Meaning:
- context:
apache: {{ apache|json }}
id: {{ id|json }}
...
------------------------------
@noelmcloughlin <https://github.com/noelmcloughlin> Could you have a look
at this, please?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#297 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADFUUQQCG4Q4PRI4JFZELCLSTZYUTANCNFSM4USF6TKA>
.
|
No need. I have something locally which I'm testing already as PR. |
@noelmcloughlin Well, I usually check the blame to see who last changed the section which has caused the regression and then ping accordingly. It's nothing personal! However, you're probably going to get more pings when there's wholescale changes made to formulas! |
My name is in CODEOWNERS so it's fine. I moved the file I think. I'll
watch the thread and if needed can jump in I opened a local branch and
started prepraring PR but I'll hold offf.
…On Tue, Dec 8, 2020 at 7:49 PM Imran Iqbal ***@***.***> wrote:
How did I become a code owner ;-) We need to encourage issue raisers to
contribute PRs too. Okay, the value apache: {{ apache|json }} was added a
few years ago, I can raise a PR.
@noelmcloughlin <https://github.com/noelmcloughlin> Well, I usually check
the blame to see who last changed the section which has caused the
regression and then ping accordingly. It's nothing personal! However,
you're probably going to get more pings when there's wholescale changes
made to formulas!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#297 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADFUUQTY7HO6K2F3PLMPXGDSTZ7MLANCNFSM4USF6TKA>
.
|
#298 is the PR... Works fine locally. We're still passing way too much information per templating call but at least we're not using exponentially more memory... |
PR merged. closing. Quick turnaround, thanks guys. |
Thanks for the issue. Lets keep it open for a while. I raised #299 as follow-up. There may be more but this PR was obvious. |
👍🏻 There are more cases where full dicts are passed,but the bad ones are where it's done per iteration like in vhosts. |
I have a setup here that has about 200 vhosts using the apache.vhosts.standard state.
state.apply calls fail with a MemoryError and slsutil.renderer generates about 1GB of json...
This seems due to the full apache variable being passed multiple times as context thus exponentially increasing memory consumption.
apache-formula/apache/config/vhosts/standard.sls
Line 23 in e2e1be1
apache-formula/apache/config/vhosts/standard.sls
Line 26 in e2e1be1
Is an example where the full apache variable is passed in twice...
The text was updated successfully, but these errors were encountered: