Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add EC2 spaces test to assess_network_health.py #8165
Conversation
vernhart
added some commits
Dec 1, 2017
nskaggs
added
the
no-test-run
label
Dec 1, 2017
nskaggs
requested a review
from
wupeka
Dec 1, 2017
nskaggs
reviewed
Dec 11, 2017
•
Running this on AWS, I get this error. The test needs to setup anything required.
2017-12-11 17:33:23 ERROR 3 subnets required for spaces assignment.
Traceback (most recent call last):
File "/var/lib/jenkins/juju-ci-tools/utility.py", line 403, in logged_exception
yield
File "/var/lib/jenkins/juju-ci-tools/deploy_stack.py", line 952, in runtime_context
yield
File "/var/lib/jenkins/juju-ci-tools/deploy_stack.py", line 1059, in booted_context
yield machines
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 830, in main
start_test(bs_manager.client, args, None)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 796, in start_test
args.reboot, args.series, maas)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 71, in assess_network_health
self.setup_testing_environment(client, bundle, target_model, series)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 142, in setup_testing_environment
self.assign_spaces(client)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 209, in assign_spaces
raise Exception('3 subnets required for spaces assignment.')
Exception: 3 subnets required for spaces assignment.
On LXD, I get this error. We need to ensure this still runs on all our providers.
2017-12-11 18:08:30 ERROR ('0', 'cannot match subnets to zones: space "space1" not found')
Traceback (most recent call last):
File "/var/lib/jenkins/juju-ci-tools/utility.py", line 403, in logged_exception
yield
File "/var/lib/jenkins/juju-ci-tools/deploy_stack.py", line 952, in runtime_context
yield
File "/var/lib/jenkins/juju-ci-tools/deploy_stack.py", line 1059, in booted_context
yield machines
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 830, in main
start_test(bs_manager.client, args, None)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 796, in start_test
args.reboot, args.series, maas)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 71, in assess_network_health
self.setup_testing_environment(client, bundle, target_model, series)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 143, in setup_testing_environment
self.setup_dummy_deployment(client, series)
File "/var/lib/jenkins/juju-ci-tools/assess_network_health.py", line 366, in setup_dummy_deployment
client.wait_for_started()
File "/var/lib/jenkins/juju-ci-tools/jujupy/client.py", line 1375, in wait_for_started
timeout=timeout, start=start)
File "/var/lib/jenkins/juju-ci-tools/jujupy/client.py", line 1358, in _wait_for_status
status.raise_highest_error(ignore_recoverable=True)
File "/var/lib/jenkins/juju-ci-tools/jujupy/status.py", line 356, in raise_highest_error
raise errors[0]
ProvisioningError: ('0', 'cannot match subnets to zones: space "space1" not found')
| @@ -13,6 +13,7 @@ | ||
| import os | ||
| import socket | ||
| from collections import defaultdict | ||
| +from netaddr import IPNetwork, IPAddress |
nskaggs
Dec 11, 2017
•
Edited 1 time
-
nskaggs
Dec 11, 2017
Owner
-
Dec 11, 2017
We'll want to add netaddr to the list of depends. This needs tweaked, but I would add to requirements.txt to have it pip installed.
vernhart
Dec 19, 2017
I've replaced this with ipaddress, a built-in module in python3. I've added a line to requirements.txt for pre-3 versions. I'm not sure if python 3 resolves this requirement but I would expect it to.
| + | ||
| + :param client: Juju client object with controller | ||
| + """ | ||
| + if client.env.provider == "ec2": |
nskaggs
Dec 11, 2017
Owner
As this is a generic call, can you add log.info("Skipping spaces assignment. Support providers are ...")
nskaggs
Dec 11, 2017
Owner
Also, I assume spaces support will extend beyond aws, but as we include it, we'll update. If so can you make this
if client.env.provider in supported_provider:
Veebers
Dec 11, 2017
Owner
To extend on this, you save an indent if you return early:
if client.env.provider not in supported_providers:
log.info('Skipping. . . ')
return
vernhart
Dec 19, 2017
Where would be the best place to put supported_providers? As a global? In main and then passed as parameters to called functions? As self.supported_providers in the class init?
Does it make sense to rename this supported_spaces_providers to be explicit?
| + for subnet in subnets['subnets'].keys(): | ||
| + # skip subnets with INFAN in the provider-id as they may be | ||
| + # inherited from underlay and therefore we cannot assign a space | ||
| + if 'INFAN' not in subnets['subnets'][subnet]['provider-id']: |
nskaggs
Dec 11, 2017
Owner
Maybe be more verbose here in our logging. Ensure we are logging if we skip or add a space and why.
| + spaces = yaml.safe_load(client.get_juju_output('list-spaces', | ||
| + '--format=yaml'))['spaces'] | ||
| + # Remove fan subnets from spaces list to avoid confusion | ||
| + for space in list(spaces): |
nskaggs
Dec 11, 2017
Owner
This probably is useful outside the method. Consider making a function for clarity and reuse.
| + | ||
| + :param client: Juju client object with controller | ||
| + """ | ||
| + if client.env.provider == "ec2": |
nskaggs
Dec 11, 2017
Owner
As this is a generic call, can you add log.info("Skipping spaces assignment. Support providers are ...")
nskaggs
Dec 11, 2017
Owner
Also, I assume spaces support will extend beyond aws, but as we include it, we'll update. If so can you make this
if client.env.provider in supported_provider:
Veebers
Dec 11, 2017
Owner
To extend on this, you save an indent if you return early:
if client.env.provider not in supported_providers:
log.info('Skipping. . . ')
return
vernhart
Dec 19, 2017
Where would be the best place to put supported_providers? As a global? In main and then passed as parameters to called functions? As self.supported_providers in the class init?
Does it make sense to rename this supported_spaces_providers to be explicit?
| + subnets = yaml.safe_load(client.get_juju_output('list-subnets', | ||
| + '--format=yaml')) | ||
| + if not subnets: | ||
| + raise Exception('No subnets defined in EC2') |
Veebers
Dec 11, 2017
Owner
I would prefer to see a specific Exception used here, is there a failing in juju? JujuAssertionError, is there a configuration failure, use something like ConfigException (note this one doesn't actually exist).
vernhart
Jan 3, 2018
Would it make sense to create a custom exception (such as ConfigException) for cases like this? I browsed through the built-in exceptions and I'm not sure there is one that is perfectly applicable except maybe EnvironmentError: https://docs.python.org/2/library/exceptions.html#exceptions.EnvironmentError
| + '--format=yaml')) | ||
| + if not subnets: | ||
| + raise Exception('No subnets defined in EC2') | ||
| + idx = 0 |
Veebers
Dec 11, 2017
Owner
This isn't really an index, further more it's 'pythonic' to be explicit so 'subnet_count' seems better suited here.
| + idx += 1 | ||
| + client.juju('add-space', ('space{}'.format(idx), subnet)) | ||
| + if idx < 3: | ||
| + raise Exception('3 subnets required for spaces assignment.') |
Veebers
Dec 11, 2017
Owner
See previous comment re: plain exception, where is the error coming from (who's fault, juju, the provider, the script etc.)
| + | ||
| + :param client: Juju client object with machines and spaces | ||
| + """ | ||
| + if client.env.provider == "ec2": |
| + if 'INFAN' in spaces[space][subnet]['provider-id']: | ||
| + del spaces[space][subnet] | ||
| + log.info('SPACES:\n {0}'.format(json.dumps(spaces, indent=4, | ||
| + sort_keys=True))) |
Veebers
Dec 11, 2017
Owner
Please note that the preferred method of breaking up a line is to break at the first paran, bracket etc. This reduces stacking up against the right hand margin.
For instance this would become:
log.info(
'SPACES:\n {0}'.format(
json.dumps(spaces, indent=4, sort_keys=True)))| + machines = yaml.safe_load(client.get_juju_output('list-machines', | ||
| + '--format=yaml'))['machines'] | ||
| + machine_test = self.verify_machine_spaces(client, spaces, machines) | ||
| + log.info('Machines in Expected Spaces ' |
Veebers
Dec 11, 2017
Owner
As this pattern is used 3x here I would consider (but not essential) breaking out to a function:
def log_verify_results(title, results):
log.info('{title}:\n {results}'.format(...))I would also consider having the logging occur within the verify_* functions.
| + 'space:\n {0}'.format(json.dumps(fail_test, indent=4, | ||
| + sort_keys=True))) | ||
| + | ||
| + return self.parse_spaces_results(machine_test, ping_test, fail_test) |
Veebers
Dec 11, 2017
Owner
It would be better to have verify_machine_spaces, verify_spaces_connectivity and add_container_with_wrong_space_errs to just return the error strings (or tuples of strings?) for any errors that occurred.
At any rate, having the error reporting at the source makes sense.
This saves having to attempt to parse the results to generate a meaningful result.
| + results[machine] = False | ||
| + for ip in eth0['ip-addresses']: | ||
| + if self.ip_in_cidr(ip, spaces[expected_space].keys()[0]): | ||
| + results[machine] = True |
Veebers
Dec 11, 2017
Owner
Is it an error if there are multiple ip address on eth0, and the first ip is in the cider but the following aren't?
i.e. do we need to check all the ips on the interface or as soon as we get a match can we break?
| + if self.ip_in_cidr(ip, spaces[expected_space].keys()[0]): | ||
| + results[machine] = True | ||
| + else: | ||
| + log.info('In machine {0}, {1} is not in ' |
Veebers
Dec 11, 2017
Owner
For more complex log strings like this it would be nice to name the placeholders.
'In machine {machine}, {ip} is not in {expected_space}({space_name?})'.format(...)| + return results | ||
| + | ||
| + | ||
| + def machine_can_ping_ip(self, client, machine, ip): |
Veebers
Dec 11, 2017
Owner
There is no need for this to be a member method, it can be a standalone function.
| @@ -325,6 +530,7 @@ def ensure_exposed(self, client, series): | ||
| self.setup_expose_test(client, series, exposed) | ||
| service_results = {} | ||
| + log.info('MACHINE:\n {}'.format(json.dumps(yaml.safe_load(client.get_juju_output('list-machines', '--format=yaml'))['machines'], indent=4, sort_keys=True))) # VWH |
vernhart
Jan 3, 2018
That line was meant for me debugging my code. The line itself is too long and the command gives excessive output. I'll break it up anyway. :)
| @@ -502,6 +708,16 @@ def is_ipv6(self, address): | ||
| return False | ||
| return True | ||
| + def ip_in_cidr(self, address, cidr): |
Veebers
Dec 11, 2017
Owner
This should be a standalone function, there is no need for it to be a member method.
vernhart commentedDec 1, 2017
•
Edited 1 time
-
vernhart
Dec 1, 2017
Please provide the following details to expedite Pull Request review:
Description of change
Add EC2 network spaces tests to assess_network_health.py
QA steps
Part of QA.
Documentation changes
Affects CI workflow.