resilient: fix parse error bug when parameters are multilines (083c159f) · Commits · nexedi / slapos

Hello,

@alain.takoudjou This commit creates problem w KVM instantiation, described in https://www.erp5.com/group_section/forum/slapos--Problem-while-instantiating-KVM-1CcbVpbI4G

Relevant part of traceback:

2017-03-20 14:45:21 slapos[28509] INFO Traceback (most recent call last):
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/zc.buildout-2.5.2+slapos005-py2.7.egg/zc/buildout/buildout.py", line 2211,
in main
2017-03-20 14:45:21 slapos[28509] INFO     getattr(buildout, command)(args)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/zc.buildout-2.5.2+slapos005-py2.7.egg/zc/buildout/buildout.py", line 637, i
n install
2017-03-20 14:45:21 slapos[28509] INFO     self._install_parts(install_args)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/zc.buildout-2.5.2+slapos005-py2.7.egg/zc/buildout/buildout.py", line 806, i
n _install_parts
2017-03-20 14:45:21 slapos[28509] INFO     installed_files = self[part]._call(recipe.install)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/zc.buildout-2.5.2+slapos005-py2.7.egg/zc/buildout/buildout.py", line 1579,
in _call
2017-03-20 14:45:21 slapos[28509] INFO     return f()
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/slapos.recipe.template-2.9-py2.7.egg/slapos/recipe/template/jinja2_template
.py", line 229, in install
2017-03-20 14:45:21 slapos[28509] INFO     out.write(self.template.render(**self.context)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/Jinja2-2.9.5-py2.7.egg/jinja2/environment.py", line 1008, in render
2017-03-20 14:45:21 slapos[28509] INFO     return self.environment.handle_exception(exc_info, True)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/Jinja2-2.9.5-py2.7.egg/jinja2/environment.py", line 780, in handle_exceptio
n
2017-03-20 14:45:21 slapos[28509] INFO     reraise(exc_type, exc_value, tb)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/parts/template-kvm-resilient/instance-kvm-resilient.cfg.jinja2", line 48, in top
-level template code
2017-03-20 14:45:21 slapos[28509] INFO     {{ replicated.replicate("kvm", backup_amount, "kvm-export", "kvm-import", slapparameter_dict=slapparameter_dict, monitor_parameter_
dict=monitor_dict) }}
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/eggs/Jinja2-2.9.5-py2.7.egg/jinja2/runtime.py", line 551, in _invoke
2017-03-20 14:45:21 slapos[28509] INFO     rv = self._func(*arguments)
2017-03-20 14:45:21 slapos[28509] INFO   File "/opt/slapgrid/13262bc33abba8c27f6b150096e0f0fc/template-replicated.cfg.in", line 45, in template
2017-03-20 14:45:21 slapos[28509] INFO     config-{{parameter_name}} = {{ parameter_value.split('\n') | join('\n  ') }}
2017-03-20 14:45:21 slapos[28509] INFO UndefinedError: 'list object' has no attribute 'split'
2017-03-20 14:45:21 slapos[28509] ERROR Failed to run buildout profile in directory '/srv/slapgrid/slappart18'

thanks for report, I didn't test on KVM. I will reproduce the bug to understand why it fail there. But, from the error message, I should probably check if parameter_value is a string before split. If possible for you add the check and let me know if it fix the issue.

I will give a try with string check.

Hello,

This is my proposed patch:

diff --git a/stack/resilient/buildout.cfg b/stack/resilient/buildout.cfg
index 4703a19..b30b340 100644
--- a/stack/resilient/buildout.cfg
+++ b/stack/resilient/buildout.cfg
@@ -73,7 +73,7 @@ mode = 0644
 [template-replicated]
 recipe = slapos.recipe.build:download
 url = ${:_profile_base_location_}/template-replicated.cfg.in
-md5sum = 4718b969bc9a661f74883217bdf46c7d
+md5sum = f72842a8e94c1752d08210af1b5c39ae
 mode = 0644
 destination = ${buildout:directory}/template-replicated.cfg.in

diff --git a/stack/resilient/template-replicated.cfg.in b/stack/resilient/template-replicated.cfg.in
index a774764..737f247 100644
--- a/stack/resilient/template-replicated.cfg.in
+++ b/stack/resilient/template-replicated.cfg.in
@@ -42,7 +42,9 @@ config-name = {{namebase}}0
 # Bubble up all the instance parameters to the requested export instance.
 {% if slapparameter_dict is defined %}
 {% for parameter_name, parameter_value in slapparameter_dict.items() %}
+{% if parameter_value is string %}
 config-{{parameter_name}} = {{ parameter_value.split('\n') | join('\n  ') }}
+{% endif %}
 {% endfor %}
 {% endif %}
 {% for key, value in monitor_dict.iteritems() -%}

How do you want me to proceed:

commit directly
create branch and create pull-request
you commit this parch

I think you should add the case parameter_value is not a string, you just put as it is to cover all the cases:

{% if parameter_value is string %} config-{{parameter_name}} = {{ parameter_value.split('\n') | join('\n ') }} {% else %} config-{{parameter_name}} = {{ parameter_value }} {% endif %}

The original code before my change was this : https://lab.nexedi.com/nexedi/slapos/blob/4ee4a6d8f620a8d61176c1627467185a50c83588/stack/resilient/template-replicated.cfg.in#L44

    {% if parameter_value is string %}
    config-{{parameter_name}} = {{ parameter_value.split('\n') | join('\n ') }}
    {% else %}
    config-{{parameter_name}} = {{ parameter_value }}
    {% endif %}

Hello,

You're right:

diff --git a/stack/resilient/buildout.cfg b/stack/resilient/buildout.cfg
index 4703a19..e58d454 100644
--- a/stack/resilient/buildout.cfg
+++ b/stack/resilient/buildout.cfg
@@ -73,7 +73,7 @@ mode = 0644
 [template-replicated]
 recipe = slapos.recipe.build:download
 url = ${:_profile_base_location_}/template-replicated.cfg.in
-md5sum = 4718b969bc9a661f74883217bdf46c7d
+md5sum = 75686abd9cb8d6e1f4e15931e280fa56
 mode = 0644
 destination = ${buildout:directory}/template-replicated.cfg.in

diff --git a/stack/resilient/template-replicated.cfg.in b/stack/resilient/template-replicated.cfg.in
index a774764..a37b2c2 100644
--- a/stack/resilient/template-replicated.cfg.in
+++ b/stack/resilient/template-replicated.cfg.in
@@ -42,7 +42,11 @@ config-name = {{namebase}}0
 # Bubble up all the instance parameters to the requested export instance.
 {% if slapparameter_dict is defined %}
 {% for parameter_name, parameter_value in slapparameter_dict.items() %}
+{% if parameter_value is string %}
 config-{{parameter_name}} = {{ parameter_value.split('\n') | join('\n  ') }}
+{% else %}
+config-{{parameter_name}} = {{ parameter_value }}
+{% endif %}
 {% endfor %}
 {% endif %}
 {% for key, value in monitor_dict.iteritems() -%}

Good, it's OK for me if you commit. Thank you for the fix

Simpler fix: just use the dumps callable available to all jinja2 template rendering, which precisely exists to escape all this buildout syntax nonsense.

@vpelletier I do not know the source of the problem you refer "escape all this buildout syntax nonsense". @alain.takoudjou can you please comment on this? Should the logic be rewritten according to @vpelletier comment, it means that my change 84fc84dd was incorrect?

No, what you did is correct but it mean use dumps() was enough.

config-{{parameter_name}} = {{ dumps(parameter_value) }}

I didn't knew that dumps was another solution to this problem, but it seems a better way to do.

@luke

I do not know the source of the problem you refer

If the value contains "${...:...}" literaly, buildout will evaluate it, corrupting the value.

If the value contains " ;" literally, buildout will consider the rest of the line is a comment, corrupting the value. Likewise for "\n#" literally (I believe there are more "comment" sequences accepted by ConfigParser and by extension by buildout, at least buildout 1).

If the value contains "${bla:recipe}\n[bla]\nrecipe = ...\n" literally, it can do a lot of fun stuff.

[EDIT]: About \n, maybe \r could also be used to cause a newline, which wouldn't be caught with proposed code.