Python3 support

[xiaket]

Hi everyone,

This is at this stage an EARLY EXPERIMENTAL PR for python3 support. All unit tests passed locally both in Python 2.7.14 and 3.6.5.

I've utilized future do the stupid changes, and it's part of the stacker code base now. In future, if we want to drop python2 support, we can simply remove it from our code base.

To make it easier to run the tests in 2 environments, I've created two virtual environments and they are managed by a bash script embedded in this PR. Here's a little help for the script(manage) here:

To run a linting test for python2: ./manage lint2 (or ./manage l2 for short)
To run a nose test for python3: ./manage nose3 (or ./manage n3 for short)
To run everything: ./manage run

It's a handy tool created to make the development easier. Technically, it should not be a part of this PR, but for now, I decide to let it in so you do not have to set up a test environment.

This is a huge PR, I've tried my best to make each commit atomic, and I hope I've explained the rationale for the change in the commit message. If you have any questions, just leave a message here or shoot me a message on Slack. Thanks.

[xiaket]

All tests will fail, because I left Makefile unchanged, and essentially all code will import future and cause an error. I hope that's not an issue for now.

[ejholmes]

I may not get a chance to review this for a while, but just wanted to say this is awesome and thanks for opening the PR @xiaket.

For what it's worth, I definitely think we should start getting python3 tested (maybe just running the full test suite against python3 on the master branch), so that we can get it green, and then keep it green.

/cc @phobologic

[phobologic]

This is awesome - thanks so much @xiaket - I'll try to get a look soon!

[xiaket]

FYI, I've done another change and hopefully it will make all the tests pass in CircleCI.

One other thing I want to report is I can see a 100% speed boost when we run the tests in Python 3:

# running unit test in python2
Ran 343 tests in 22.320s
# running unit test in python3
Ran 343 tests in 11.238s

[danielkza]

@xiaket I have good experience with tox, if you need any support you can just ask on the stacker Slack channel and I'll do my best to help :)

[troyready]

@xiaket it needs a small change to make formic2 work, sorry I didn't catch that before:

diff --git a/setup.py b/setup.py
index 7b04581..88dbd5a 100644
--- a/setup.py
+++ b/setup.py
@@ -11,7 +11,7 @@ install_requires = [
     "boto3>=1.3.1",
     "PyYAML~=3.12",
     "awacs>=0.6.0",
-    "formic~=0.9b",
+    "formic2~=1.0",
     "gitpython~=2.0",
     "schematics~=2.0.1",
 ]
diff --git a/stacker/hooks/aws_lambda.py b/stacker/hooks/aws_lambda.py
index 245d1bf..b96c4db 100644
--- a/stacker/hooks/aws_lambda.py
+++ b/stacker/hooks/aws_lambda.py
@@ -6,7 +6,6 @@ import hashlib
 from StringIO import StringIO
 from zipfile import ZipFile, ZIP_DEFLATED
 import botocore
-from functools import partial
 import formic
 from troposphere.awslambda import Code
 from stacker.session_cache import get_session
@@ -119,9 +118,7 @@ def _find_files(root, includes, excludes, follow_symlinks):
 
     root = os.path.abspath(root)
     file_set = formic.FileSet(directory=root, include=includes,
-                              exclude=excludes,
-                              walk=partial(
-                                  os.walk, followlinks=follow_symlinks))
+                              exclude=excludes, symlinks=follow_symlinks)
 
     for filename in file_set.qualified_files(absolute=False):
         yield filename

[xiaket]

Hey @ejholmes, this PR has been here for quite a while, can you please squeeze some time to review some of the DAG related changes? Thanks!

[phobologic]

Hey @xiaket - sorry this has taken so long, changing jobs as well as a vacation followed by sickness has left me with very little time.

I'm gonna give @ejholmes till Wednesday this week to chime in, but I think this is looking good from everything I can tell.

Thanks again for all your patience with this - really appreciate all the work you've done!

[phobologic]

Ok, I'm tempted to accept this - @ejholmes wrote plenty of tests for this, and they are passing - though just one more question: Was there a reason we needed to make this change related to python3, or was this meant to change it for some other reason @xiaket? Do you have any idea if the performance is any different between the two?

[phobologic]

Ok, I created a quick script to test if memory usage changed after removing the deepcopy:

import sys
import resource

from stacker.context import Context
from stacker.config import Config, Stack


def generate_context(stacks, conf):
    stack_names = [d["name"] for d in stacks]
    stack_objs = [Stack(d) for d in stacks]

    conf.stacks = stack_objs
    conf.validate()
    return Context(
        environment={},
        stack_names=stack_names,
        config=conf,
    )


conf = Config()
conf.namespace = "test-namespace"

stacks = []
STACK_COUNT = int(sys.argv[1])

for i in range(STACK_COUNT):
    stacks.append({"name": "vpc%s" % i, "class_path": "blueprints.VPC"})


start_mem = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
for i in range(len(stacks)):
    ii = i + 1
    s = stacks[:ii]
    ctx = generate_context(s, conf)

stacks_created = ctx.get_stacks()

end_mem = resource.getrusage(resource.RUSAGE_SELF).ru_maxrss
total_mem = end_mem - start_mem

print total_mem,

And then ran it repeatedly, increasing the config size by one stack each time with this shell script, and dumped the output into a csv format file:

#!/bin/bash

for i in $(seq 100)
do
    echo "$i,$(python ./test_stacker_mem.py $i)"
done

That resulted in this graph:

So it looks like in 1.3 we regressed and started using more memory once more, and this should help fix it. @ejholmes unless you can think of a reason this might be bad, I'm going to go ahead and go witht his too.

(BTW, here's the google spreadsheet with my run's results: https://docs.google.com/spreadsheets/d/15rP-RPYg7DtFQIwB_lK6oaJwPQHG6tz1NfxsvUKVJeM/edit?usp=sharing)

[phobologic]

Hey @xiaket - I went ahead and created a branch in the cloudtools/stacker repo to kick off functional tests, and it looks like we're running into issues with 2.7:

https://circleci.com/gh/cloudtools/stacker/1992

(here's the fake PR I made to kick off the functional tests: #610)

Any ideas?

[phobologic]

Ok, after some creative debugging with print statements (and dealing with the OS X systems integrity protection, ugh), I think I've tracked it down to urllib in python2.7 having issues with byte strings:

   KeyError: 49
   [2018-06-24T11:20:55] vpc: failed (49)
   [2018-06-24T11:20:55] The following steps failed: vpc
### The following are debug print statements
DEBUG: urllib.quote: s arg: b'10.128.0.0/16'
DEBUG: urllib.quote: QUOTER var: <built-in method __getitem__ of dict object at 0x1099f3168>
   
DEBUG: botocore.vendored.requests.models._encode_params: data arg: {'Parameters.member.2.ParameterKey': 'PublicSubnets', 'Parameters.member.5.ParameterValue': 'NAT', 'StackName': u'stacker-test-namespace-vpc', 'Parameters.member.6.ParameterValue': 'false', 'Version': u'2010-05-15', 'Capabilities.member.1': 'CAPABILITY_NAMED_IAM', 'Parameters.member.5.ParameterKey': 'ImageName', 'Parameters.member.4.ParameterKey': 'BaseDomain', 'Parameters.member.7.ParameterValue': '10.128.0.0/16', 'Parameters.member.2.ParameterValue': '10.128.0.0/24,10.128.1.0/24,10.128.2.0/24,10.128.3.0/24', 'Parameters.member.1.ParameterValue': '', 'TemplateURL': u'https://s3.amazonaws.com/stacker-stacker-test-namespace/stack_templates/stacker-test-namespace-vpc/vpc-829da93c.json', 'Parameters.member.7.ParameterKey': 'CidrBlock', 'Tags.member.1.Value': u'stacker-test-namespace', 'Parameters.member.1.ParameterKey': 'InternalDomain', 'Parameters.member.3.ParameterValue': '10.128.8.0/22,10.128.12.0/22,10.128.16.0/22,10.128.20.0/22', 'Action': u'UpdateStack', 'Parameters.member.3.ParameterKey': 'PrivateSubnets', 'Tags.member.1.Key': 'stacker_namespace', 'Parameters.member.6.ParameterKey': 'UseNatGateway', 'Parameters.member.4.ParameterValue': '', 'Parameters.member.8.ParameterKey': 'InstanceType', 'Parameters.member.8.ParameterValue': 'm3.medium'}
   

DEBUG: botocore.vendored.requests.models._encode_params: RESULT var: [('Parameters.member.2.ParameterKey', 'PublicSubnets'), ('Parameters.member.5.ParameterValue', b'NAT'), ('StackName', 'stacker-test-namespace-vpc'), ('Parameters.member.6.ParameterValue', b'false'), ('Version', '2010-05-15'), ('Capabilities.member.1', 'CAPABILITY_NAMED_IAM'), ('Parameters.member.5.ParameterKey', 'ImageName'), ('Parameters.member.4.ParameterKey', 'BaseDomain'), ('Parameters.member.7.ParameterValue', b'10.128.0.0/16'), ('Parameters.member.2.ParameterValue', b'10.128.0.0/24,10.128.1.0/24,10.128.2.0/24,10.128.3.0/24'), ('Parameters.member.1.ParameterValue', b''), ('TemplateURL', 'https://s3.amazonaws.com/stacker-stacker-test-namespace/stack_templates/stacker-test-namespace-vpc/vpc-829da93c.json'), ('Parameters.member.7.ParameterKey', 'CidrBlock'), ('Tags.member.1.Value', 'stacker-test-namespace'), ('Parameters.member.1.ParameterKey', 'InternalDomain'), ('Parameters.member.3.ParameterValue', b'10.128.8.0/22,10.128.12.0/22,10.128.16.0/22,10.128.20.0/22'), ('Action', 'UpdateStack'), ('Parameters.member.3.ParameterKey', 'PrivateSubnets'), ('Tags.member.1.Key', 'stacker_namespace'), ('Parameters.member.6.ParameterKey', 'UseNatGateway'), ('Parameters.member.4.ParameterValue', b''), ('Parameters.member.8.ParameterKey', 'InstanceType'), ('Parameters.member.8.ParameterValue', b'm3.medium')]

It's specifically urllib.quote having an issue with the byte-string b"10.128.0.0/16" - it may have trouble with others as well, this is just the first byte string it's dealing with.

[phobologic]

Hmm, nevermind, i was wrong about the address being the first byte string - there are others before it.

[phobologic]

Ok, running into a little bit of a roadblock of lack of knowledge around strings/bytestrings in the python2/3 world. I have to work on some other stuff, but I'll be around most of the day to chat (here or on Slack) if anyone gets a chance to take a look!

[xiaket]

@phobologic Sorry for the late reply, I took the last two days fighting this (properly setup an environment alone took me one day) and I already have a dirty hack that can fix this issue, I'm looking for a clean solution ATM. I'll keep you updated about my progress.

[phobologic]

Does the removal of importing the str method cause any other issues? That doesn't look super dirty to me, though maybe I don't understand the implications for python3. Thanks @xiaket!

[xiaket]

@phobologic I believe the failure in dc10db4 is unrelated(throttling of API). I updated README and forced another round of tests, it's all green now.

[phobologic]

@xiaket unfortunately the functional-tests in non-cloudtools/stacker based branches don't actually run (for security reasons). The output shows each of the tests as skipped in circleci.

The good news is that it looks like after forcing retries (to get around the Throttling) it looks like the functional tests passed. I'm going to merge this now! Thanks!