saltstack-formulas / apache-formula

Set up and configure the Apache HTTP server
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
50 stars 285 forks source link

[BUG] apache.vhosts.standard (and potentially others) generate MemoryError #297

Open ixs opened 3 years ago

ixs commented 3 years ago

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.

https://github.com/saltstack-formulas/apache-formula/blob/e2e1be18e093dca2d603b68075b388d0f8b61d3d/apache/config/vhosts/standard.sls#L23 https://github.com/saltstack-formulas/apache-formula/blob/e2e1be18e093dca2d603b68075b388d0f8b61d3d/apache/config/vhosts/standard.sls#L26

Is an example where the full apache variable is passed in twice...

myii commented 3 years ago

Thanks for the report, @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 Could you have a look at this, please?

noelmcloughlin commented 3 years ago

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 notifications@github.com 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 https://github.com/saltstack-formulas/apache-formula/issues/297#issuecomment-740843694, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQQCG4Q4PRI4JFZELCLSTZYUTANCNFSM4USF6TKA .

ixs commented 3 years ago

No need. I have something locally which I'm testing already as PR.

myii commented 3 years ago

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 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!

noelmcloughlin commented 3 years ago

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 notifications@github.com 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 https://github.com/saltstack-formulas/apache-formula/issues/297#issuecomment-740929638, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADFUUQTY7HO6K2F3PLMPXGDSTZ7MLANCNFSM4USF6TKA .

ixs commented 3 years ago

https://github.com/saltstack-formulas/apache-formula/pull/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...

ixs commented 3 years ago

PR merged. closing. Quick turnaround, thanks guys.

noelmcloughlin commented 3 years ago

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.

ixs commented 3 years ago

👍🏻

There are more cases where full dicts are passed,but the bad ones are where it's done per iteration like in vhosts.