stack/slapos: slapos.cookbook version up (5f5d5102) · Commits · nexedi / slapos

While building Gitlab, instantiation failed systematically (logs). The git bisect (between working state 138cfdb7 and 74d92074 failing instantiation) shows that this commit is the first bad one.

/cc @kirr @jerome

@iv thanks for info about this. Could you please further manually bisect slapos.cookbook beetween its 1.0.25 and 1.0.30 so we can see the exact faulting commit?

/cc @rafael

By looking at log, I think just some external source from rake got broken... Maybe checking diff will lead to nothing meaninfull. But you an try :)

@rafael we already can prove that via changing only slapos.cookbook from 1.0.25 to 1.0.30 it breaks - this was the bisection result and is reliably reproducible. Also I've looked myself a bit some time ago and the breakage seems to be related to the way how wrapper scripts are now generated.

We will hopefully see exactly after @iv finish the bisection in between 1.0.25 and 1.0.30 slapos.cookbook.

The only thing which could apply on this case was a change on slapos/recipe/wrapper.py to workarround linux shebang problem... in case you use such wrapper:

https://lab.nexedi.com/nexedi/slapos/commits/master/slapos/recipe/wrapper.py

Probably yes. I'm suspecting f4e51f77. @iv please just bisect it and we will know for sure what to fix.

@kirr yes, it's building!

@kirr I thought I could manually change the revision number in slapos.cfg file, but it appears that on Pypi, there is no slapos.cookbook version 1.0.26 to 29, leading to the error:

"Error: Couldn't find a source distribution for 'slapos.cookbook==1.0.28'."

when I build...

@iv do you have any idea why exactly f4e51f77 cause problem ?

@iv the eggs are not released for every tag.

@jerome not yet, but I'll investigate.

@iv for finding commit-granularity info about where it broke, it is better to switch to slapos-cookbook-develop and then manually do the binary search with using revision = ... in slapos-cookbook-repository.

@iv any news?

@kirr as you suggested, at f4e51f77, the instanciation of the webrunner started to be impossible. But the error was due to missing parameter (fixed by b171c34f). After this, I have the same error as before (gitlab-rake complaining about 'test' environment).

@iv, if I read you correctly f4e51f77 broke gitlab build, b171c34f amended it a bit, but we still have the failure. If so then regression brought by f4e51f77 is not completely fixed yet. Right?

/cc @jerome, @rafael

@kirr exactly!

@iv thanks for feedback.

@jerome - seems f4e51f77 is not propagating environment properly through wrapper. Maybe any idea how to fix?

Thanks beforehand,
Kirill

@kirr @iv I tried to reproduce a simplified version of what seem to be happening there, with this these scripts that should be equivalent to wrapper generated before and after that change:

$ cat wrapped_script 
#!/usr/bin/python
import os
print os.environ['TEST_VAR']

$ cat wrapper_old.sh
#!/bin/sh
exec wrapped_script

$ cat wrapper_new.sh
#!/bin/bash
COMMAND=./wrapped_script
if [[ -f $COMMAND && x$(head -c2 "$COMMAND") = x"#!" ]]; then
   SHEBANG=$(head -1 "$COMMAND")
   INTERPRETER=( ${SHEBANG#\#!} )
   COMMAND="${INTERPRETER[@]} $COMMAND"
fi
exec $COMMAND

but the behaviour is same:

$ TEST_VAR=X ./wrapper_old.sh 
X
$ TEST_VAR=X ./wrapper_new.sh 
X

is this what you meant by "propagating environment" ? the recipe method also accepts an environment paremeter, so I am a bit confused.

@jerome thanks for feedback. Yes, that's what I was meaning with "propagating enviroment". Gitlab SR sets RAILS_ENV=production when generating gitlab-rake and other wrappers:

https://lab.nexedi.com/nexedi/slapos/blob/cd9faac0/software/gitlab/instance-gitlab.cfg.in#L288

but from behaviour it feels those are not picked up.

I had not looked into details and maybe it is not an environemtn, but after bisection we know behaviour started to change after f4e51f77. So could you please have a look?

Thanks beforehand again,
Kirill

I don't understand why createPythonScript is called with parameter "environment" in wrapper.py, because there is an argument "arguments", that is just later used by zc.buildout.easy_install.scripts. I'm not that familiar with buildout, though.

EDIT: I read too fast, arguments is a set in this case, which value is (command_line, wait_files, environment,).

It looks like environment argument is correct for setting environment variables, isn't it?

Also, after reading the logs again, it looks like environment (via RAILS_ENVIRONMENT environment variable) is set to 'test' (line 144) in a first time, and then to 'production' (line 153) as expected.

before f4e51f77, we had: ~/srv/runner/instance/slappart0/bin/gitlab-rake containing:

...
if __name__ == '__main__':                                                                                                                                                                          
    sys.exit(slapos.recipe.librecipe.execute.generic_exec((['/srv/slapgrid/slappart16/srv/runner/software/fffb3c99781923d3adb8bc53eb6c027a/bin/bundle', 'exec', 'sh', '-c', 'cd /srv/slapgrid/slappart16/srv/runner/instance/slappart0/gitlab-work && rake "$@"', 'rake'], None, {'BUNDLE_GEMFILE': '/srv/slapgrid/slappart16/srv/runner/software/fffb3c99781923d3adb8bc53eb6c027a/parts/gitlab/Gemfile', 'HOME': '/srv/slapgrid/slappart16/srv/runner/instance/slappart0', 'SIDEKIQ_MEMORY_KILLER_MAX_RSS': '1000000', 'RAILS_ENV': 'production'})))

after, ~/srv/runner/instance/slappart0/bin/gitlab-rake contains:

#!/bin/bash                                                                                                                                                                                         
COMMAND=/srv/slapgrid/slappart16/srv/runner/instance/slappart0/bin/gitlab-rake.py                                                                                                                   
                                                                                                                                                                                                    
# If the wrapped command uses a shebang, execute the referenced                                                                                                                                     
# executable passing the script path as first argument.                                                                                                                                             
# This is to workaround the limitation of 127 characters in #!                                                                                                                                      
if [[ -f $COMMAND && x$(head -c2 "$COMMAND") = x"#!" ]]; then                                                                                                                                       
  SHEBANG=$(head -1 "$COMMAND")                                                                                                                                                                     
  INTERPRETER=( ${SHEBANG#\#!} )                                                                                                                                                                    
  COMMAND="${INTERPRETER[@]} $COMMAND"                                                                                                                                                              
fi                                                                                                                                                                                                  
                                                                                                                                                                                                    
exec $COMMAND

which is a wrapper around gitlab-rake.py containing:

...                                                                                                                                                                                               
if __name__ == '__main__':                                                                                                                                                                          
    sys.exit(slapos.recipe.librecipe.execute.generic_exec((['/srv/slapgrid/slappart16/srv/runner/software/fffb3c99781923d3adb8bc53eb6c027a/bin/bundle', 'exec', 'sh', '-c', 'cd /srv/slapgrid/slappart16/srv/runner/instance/slappart0/gitlab-work && rake "$@"', 'rake'], None, {'BUNDLE_GEMFILE': '/srv/slapgrid/slappart16/srv/runner/software/fffb3c99781923d3adb8bc53eb6c027a/parts/gitlab/Gemfile', 'HOME': '/srv/slapgrid/slappart16/srv/runner/instance/slappart0', 'SIDEKIQ_MEMORY_KILLER_MAX_RSS': '1000000', 'RAILS_ENV': 'production'})))

gitlab-rake.py after is same as gitlab-rake before.

This slapos.cookbook:wrapper has an argument parameters-extra which if set to true, propagate command line arguments to the wrapped script. The default value for this parameter is false.

Before f4e51f77, the generated wrapper was also propagating arguments even when parameters-extra was not set, but since this commit, this parameters-extra option is now handled as expected.

This is the reason for this regression. In our case, when we see /srv/slapgrid/slappart16/srv/runner/instance/slappart0/bin/gitlab-rake assets:clean, it just calls rake without arguments.

So a simple patch that fix the problem would be jerome/slapos@d3d05f02 . This way, the generated wrapper becomes:

...
exec $COMMAND $@

and arguments are correctly propagated.

Feel free to cherry-pick that patch for now, but it may be nice to rethink this parameters-extra option, after this debugging session, I believe it should be true by default.

/cc @seb for introducing the parameter in 80bb4305 and @vpelletier for touching this code in e7083872

mentioned in commit 6082d6e9

@jerome thanks for your analysis. I've applied your patch to master and added your description into it as commit message is the proper context for such descriptions (for those of use who use git history to learn about project - gitk, git blame and similar things on web ui).

The patch was applied as 6082d6e9. Thanks again.

@iv please confirm it fixes gitlab SR build for you on latest master.

@jerome thank you for the investigation

@kirr it works fine after applying @jerome's patch

@iv thanks for feedback. @jerome thanks again for the patch.

it may be nice to rethink this parameters-extra option, after this debugging session, I believe it should be true by default.

+1

It should not even be an option at all: if one wants parameters to be propagated, it just works. If one does not want parameters to be propagated, they just don't pass parameters.

@@ -135,7 +135,7 @@ requests = 2.10.0
 setuptools = 19.6.2
 simplejson = 3.8.2
 six = 1.10.0
 slapos.cookbook = 1.0.25
 slapos.cookbook = 1.0.30
 slapos.core = 1.3.15
 slapos.extension.strip = 0.1
 slapos.libnetworkcache = 0.14.5
-...