DOCD-1685 Set resource requests/limits from fiaas.yml on bootstrapping pod #37
Conversation
|
lgtm remove the wip label unless there are additional things to be addressed? |
There are a few more testcases on the way |
oyvindio
force-pushed
the
DOCD-1685-set-resources
branch
from
d63e016
to
fb98a8f
Compare
Can't merge list and tuple
+ minor modifications for Python 3.x compatibility
Avoid breaking tests if we change the default fiaas.yml
oyvindio
force-pushed
the
DOCD-1685-set-resources
branch
from
fb98a8f
to
bb3ee3d
Compare
fiaas_skipper/deploy/bootstrap.py
Outdated
| @@ -17,38 +18,40 @@ | |||
|
|
|||
|
|
|||
| class BarePodBootstrapper(object): | |||
| def __init__(self, cmd_args=()): | |||
| def __init__(self, cmd_args=[]): | |||
There was a problem hiding this comment.
Using list here is a bad choice. Mutable objects should not be the default value in a parameter-list, it sets the stage for hard to understand bugs later. If this really needs to be a list, it should default to None, with initializing in the __init__-method.
There was a problem hiding this comment.
You're right, good catch! It is used in the command field of the bootstrapping pod/spec/container, which is a ListField, so it needs to be a list at that point. I'll set the argument default to None and initialize it in the init method.
This only worked because the production code paths that use this always set cmd_args (to a list), and it started breaking in the tests I wrote when I didn't.
setup.cfg
Outdated
| @@ -9,3 +9,8 @@ directory=build/reports/coverage | |||
|
|
|||
| [coverage:xml] | |||
| output=build/reports/coverage.xml | |||
|
|
|||
| [flake8] | |||
There was a problem hiding this comment.
This is already configured in .prospector.yaml, and you are setting a different max-line-length.
|
@mortenlj PTAL |
|
Codacy issues seem to stem from the prospector version used not understanding Python 3 or something like that |
Supposed to support Python 3.3 and up. Might be worth a bug report to Prospector? PyPI says 3.3-3.5, but these features aren't new in 3.6 are they? |
We were relying on the default resource requests/limits enforced by the limitrange in most namespaces, which worked by coincidence untill we hit a namespace that didn't have defaults but did have a resourcequota denying pods with BestEffort QoS. This caused the bootstrapping pod to not be scheduled. Now skipper will use there resources specified in fiaas-deploy-daemon's fiaas.yml when creating the bootstrapping pod as well, unless there is a resourcequota enforcing BestEffort QoS pods only.