Merge lp:~viswesn/juju-ci-tools/ensure_provider_cleanup into lp:juju-ci-tools
- ensure_provider_cleanup
- Merge into trunk
Status: | Merged |
---|---|
Merged at revision: | 1967 |
Proposed branch: | lp:~viswesn/juju-ci-tools/ensure_provider_cleanup |
Merge into: | lp:juju-ci-tools |
Prerequisite: | lp:~viswesn/juju-ci-tools/juju-ci-cleanup |
Diff against target: |
508 lines (+434/-3) 4 files modified
deploy_stack.py (+18/-1) substrate.py (+96/-2) tests/test_deploy_stack.py (+58/-0) tests/test_substrate.py (+262/-0) |
To merge this branch: | bzr merge lp:~viswesn/juju-ci-tools/ensure_provider_cleanup |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Aaron Bentley (community) | Approve | ||
Christopher Lee (community) | Needs Fixing | ||
Review via email: mp+315456@code.launchpad.net |
Commit message
Description of the change
Do ensure cleanup for each of the provider and make sure we need to come with the list of resources that were not cleanup during cleanup activity.
- 1833. By viswesuwara nathan
-
review comments addressed
viswesuwara nathan (viswesn) wrote : | # |
Most of the code remains same for ensure_cleanup at this point in time which invokes terminate the instance. The other cleanup activity specific to security group, firewall, etc., were not addressed here at this point in time because that requires contribution from the providers before supporting invoking them in ensure_cleanup.
ABHAY (ankatare) wrote : | # |
looks good to me, only one phrase correction required.
Aaron Bentley (abentley) wrote : | # |
This branch introduces a bunch of duplicate code. Please avoid that.
I think you probably want an Account-specific list_dirty method and then a non-specific clean_dirty method which operates on that list. If a substrate doesn't support e.g. delete_
Whatever you do, don't repeat yourself. Factor out the duplicate code.
Don't catch Exception. Catch the specific exceptions that you expect to be raised.
When you do catch an exception, don't make it impossible to see what the exception was. We need to know *why* it's failing in order to improve the script in the future. You could print it. You could append a tuple of (resource, exception) to unclean_resources.
Are you certain that this script is not going to delete freshly-created resources? For example, could non_instance_groups contain a group created by a different script that is about to be associated to an instance? Please explain why it's safe to delete specific resource types in the docstring or comments.
You are providing a string to terminate_
viswesuwara nathan (viswesn) wrote : | # |
>> Whatever you do, don't repeat yourself. Factor out the duplicate code.
<<Viswesn>> Currently we see most of the code is duplicated but then once we have list of resources to be cleaned up in coming days for each substrate then ensure_cleanup is going to be different for each substrate.
>> Don't catch Exception. Catch the specific exceptions that you expect to be raised.
<<Viswesn>> Now I need to address this by sending the unclean=[] to the callee function and then append the resource name + exception something like ["instance_id", "instance-001", "Interface failed"]
-------
delete_
if unclean is None:
...
....
except Exception as ex:
-------
Chris, let me know your comments on this. We decided not to do so in our last call but then to catch the exact exception then I need to pass unclean to the callee method rather than having like try..catch.. statement on invoking terminate_instance.
>> You are providing a string to terminate_
<<Viswesn>> Yes, I agree and I will address this issue.
>> Are you certain that this script is not going to delete freshly-created resources?
<<Viswesn>> if ensure_cleanup has visibility to those freshly-created resource then yes but seems like it is not at present and it is up the substrate developer to bring this in to picture as part of ensure_cleanup.
I will start working on this code after getting comments from Chirs and Aaron.
Please let me know on this. Thanks.
Aaron Bentley (abentley) wrote : | # |
> >> Whatever you do, don't repeat yourself. Factor out the duplicate code.
> <<Viswesn>> Currently we see most of the code is duplicated but then once we
> have list of resources to be cleaned up in coming days for each substrate then
> ensure_cleanup is going to be different for each substrate.
The parts that are the same, like terminate_
> >> Don't catch Exception. Catch the specific exceptions that you expect to be
> raised.
> <<Viswesn>> Now I need to address this by sending the unclean=[] to the callee
> function and then append the resource name + exception something like
> ["instance_id", "instance-001", "Interface failed"]
>
> -------
> delete_
> if unclean is None:
> unclean=[]
> ...
> ....
> except Exception as ex:
> unclean.
> ex.args])
> -------
>
> Chris, let me know your comments on this. We decided not to do so in our last
> call but then to catch the exact exception then I need to pass unclean to the
> callee method rather than having like try..catch.. statement on invoking
> terminate_instance.
I don't understand why catching a specific exception would require you to pass unclean to the callee method.
> >> Are you certain that this script is not going to delete freshly-created
> resources?
> <<Viswesn>> if ensure_cleanup has visibility to those freshly-created resource
> then yes but seems like it is not at present and it is up the substrate
> developer to bring this in to picture as part of ensure_cleanup.
ensure_cleanup has visibility to those freshly-created resources. It is your problem, not the substrate developer's.
Aaron Bentley (abentley) wrote : | # |
I have had a chance to look at the design doc: https:/
This feature has two intents:
1. Ensure resources are cleanup so that we have uninterrupted testing
and do not accrue extra costs
2. We want to report a failure when destroy-controller fails to
cleanup.
The way security groups and interfaces are being handled does not meet goal 2. It cannot report that destroy-controller failed to clean up for this test run, because it does not know that the security groups were created by this test run.
The doc even suggests "Match security groups/firewalls by controller uuid in the name or in tags/metadata".
Since the doc was written, goal 2 has become more important than goal 1, because our existing cleanup scripts are doing a good job and juju itself is better-behaved.
- 1834. By viswesuwara nathan
-
Code updated
- 1835. By viswesuwara nathan
-
Review comments addressed
Christopher Lee (veebers) wrote : | # |
A couple of times in discussion I'm stated that we'll concentrate only on AWS for this MP and then do the other providers in follow up MPs.
This MP alters more than just the AWSAccount class, a couple of issues with this:
- This makes the MP even longer. The diff is 548 lines long, this is too long to comfortably review. As mentioned multiple times before we want to have short MPs (less than 300 lines) this makes reviews quicker, easier and less prone to errors.
- There are no tests included for the altered *Account classes. There is no assurance that this branch hasn't broken anything.
- Due to the first points, adding the needed tests would blow the diff count way out.
make lint fails.
Please notice the comments about methods that could be made functions, having functions makes testing a lot easier (no need to create or mock state).
- 1836. By viswesuwara nathan
-
Review comments addressed
- 1837. By viswesuwara nathan
-
Review comments addressed
viswesuwara nathan (viswesn) wrote : | # |
This branch is created long back for juju-ci-cleanup enhancement work and already we integrated the changes specific to most of the cloud providers and that is the reason for seeing code changes for providers like OpenStack, GCE, etc. As discussed in the call today I kept changes only for AWS and other provider changes were removed.
Please review it and let me know your comments on the updated code base. Thanks
Christopher Lee (veebers) wrote : | # |
Some further queries about the data-structures used for storing errors.
I expanded on your question a bit, let me know what you think or if you would like more info.
Some test issues too. Make sure you're mocking the right thing, it's all too easy when mocking to produce false positives in your code that don't match reality when actually being run.
viswesuwara nathan (viswesn) wrote : | # |
Chris, I updated the code by taken up almost all the review comments provided and more over in the current code review I removed the test case "TestGetSecurit
Let us discuss on this on coming Monday call or please let me know by mail how to address "get_all_
- 1838. By viswesuwara nathan
-
Review comments addressed
- 1839. By viswesuwara nathan
-
Review comments updated
Christopher Lee (veebers) wrote : | # |
Hi Vishwa,
Lets talk in our next call re: your query for TestGetSecurity
Please see comments inline.
viswesuwara nathan (viswesn) wrote : | # |
Hi Chris,
I updated the review comments and let us discuss more on TestGetSecurity
- 1840. By viswesuwara nathan
-
Review comments addressed
- 1841. By viswesuwara nathan
-
GetSecurityGroups changes
- 1842. By viswesuwara nathan
-
GetSecurityGroups changes
Aaron Bentley (abentley) wrote : | # |
attempt_
The tests mock out the direct callees, even though there are alternatives. Once consequence is that you didn't notice that attempt_
The tests use assertEqual(0, mock.call_count) where they could use mock.assert_
Also, I don't think it makes sense to reimplement set.issubset as contains_
There are some formatting suggestions, too. See below.
Also, please merge trunk more frequently. I got merge conflicts with one algorithm.
Aaron Bentley (abentley) wrote : | # |
The failure:
http://
- 1843. By viswesuwara nathan
-
Review comments addressed
- 1844. By viswesuwara nathan
-
Merge to trunk
viswesuwara nathan (viswesn) wrote : | # |
1. Most of the review comments were addressed.
2. Code merged with trunk
3. Hitting one failure on having patch for terminate_instances - http://
Please let me know how to resolve this error.
4. Update the comments section on addressing it.
Christopher Lee (veebers) wrote : | # |
responded to comments.
Christopher Lee (veebers) wrote : | # |
clarified some points and pointed out some test issues.
- 1845. By viswesuwara nathan
-
Review comments addressed
Christopher Lee (veebers) wrote : | # |
You haven't answered the questions or given reason for the changes that differ to the requests from the previous review (namely repr vs str and the ability to store better details on why the delete failed). Please respond.
Also you seemed to have ignored the comments surrounding contains_
I've identified concerns with the testing below, namely patching the wrong then and incorrect asserts.
viswesuwara nathan (viswesn) wrote : | # |
I used repr instead of str because when I read couple of articles in the stackoverflow they recommended to use repr than str hence I decided to go for repr than str.
Now coming to use of issubset; I already asked this on March 16th; Please find the same here
"Getting error as - AttributeError: 'list' object has no attribute 'issubset'
Do I miss something? Please guide me on this.
I also addressed the comments given and I too asked questions on few of them. Please
help me in answering those questions. Thanks
- 1846. By viswesuwara nathan
-
Review comments addressed
Christopher Lee (veebers) wrote : | # |
Hi Vishwa,
In the future it would be helpful to mention why you made a change different to what was suggested, it removes any need for guessing and/or assuming that there was a misunderstanding.
Also, please state the actual reason for the decision (i.e. x is better because of y, not someone else said to do it this way).
Regarding your question re: issubset, please make sure you read all the comments as I have already answered this. On March 17th I state:
"""
You're missing that possibly_known_ids is a list object and doesn't have the method issubset. You need a set object for that.
The use of issubset was also mentioned in a previous comment.
Aarons suggestion here is also to get rid of the function and just make the call where it's used. With reducing the call down to use issubset this makes sense.
Originally I suggested to have a separate function to ease testing and readability but this has evolved to the point where that's not needed.
"""
I have answered your question in line (sorry for the wall of text).
Please see here: http://
Hint: it's related to the comment about what get_security_groups actually returns.
Christopher Lee (veebers) wrote : | # |
Updated comment.
viswesuwara nathan (viswesn) wrote : | # |
Hi All,
>> In the future it would be helpful to mention why you made a change different to what was suggested, it removes any need for guessing and/or assuming that there was a misunderstanding.
<<Viswa>> I definitely do this And I used repr instead of str on reading from link
http://
>> Regarding your question re: issubset
<<Viswa>> The function is now modified and it is now single line; I didn't integrated the same in the caller and kept the function contains_
Major changes in the test code were done to make Mock only for lower level function (client) as discussed and mock were not introduced for the functions that were introduced by us.
- 1847. By viswesuwara nathan
-
Code review comments addressed
Christopher Lee (veebers) wrote : | # |
One fix, one question that needs answered.
- 1848. By viswesuwara nathan
-
review comments addressed
viswesuwara nathan (viswesn) wrote : | # |
Fixed and provided answer to your question. Thanks
Aaron Bentley (abentley) wrote : | # |
Some minor issues mentioned inline. Otherwise, I think this is good to merge.
- 1849. By viswesuwara nathan
-
Review comments addressed
- 1850. By viswesuwara nathan
-
fixed lint errors
viswesuwara nathan (viswesn) wrote : | # |
Fixed all the review comments.
Preview Diff
1 | === modified file 'deploy_stack.py' |
2 | --- deploy_stack.py 2017-03-01 20:34:01 +0000 |
3 | +++ deploy_stack.py 2017-03-29 17:54:35 +0000 |
4 | @@ -518,6 +518,22 @@ |
5 | os.environ['SSO_PASSWORD'], client, tear_down_client) |
6 | |
7 | |
8 | +def error_if_unclean(unclean_resources): |
9 | + """List all the resource that were not cleaned up programmatically. |
10 | + |
11 | + :param unclean_resources: List of unclean resources |
12 | + """ |
13 | + if unclean_resources: |
14 | + logging.critical("Following resource requires manual cleanup") |
15 | + for resources in unclean_resources: |
16 | + resource = resources.get("resource") |
17 | + logging.critical(resource) |
18 | + errors = resources.get("errors") |
19 | + for (id, reason) in errors: |
20 | + reason_string = "\t{}: {}".format(id, reason) |
21 | + logging.critical(reason_string) |
22 | + |
23 | + |
24 | class CreateController: |
25 | """A Controller strategy where the controller is created. |
26 | |
27 | @@ -920,7 +936,8 @@ |
28 | if self.has_controller: |
29 | self.collect_resource_details() |
30 | self.tear_down(self.jes_enabled) |
31 | - self.ensure_cleanup() |
32 | + unclean_resources = self.ensure_cleanup() |
33 | + error_if_unclean(unclean_resources) |
34 | |
35 | # GZ 2016-08-11: Should this method be elsewhere to avoid poking backend? |
36 | def _should_dump(self): |
37 | |
38 | === modified file 'substrate.py' |
39 | --- substrate.py 2017-02-13 15:53:00 +0000 |
40 | +++ substrate.py 2017-03-29 17:54:35 +0000 |
41 | @@ -76,6 +76,37 @@ |
42 | subprocess.check_call(command_args, env=environ) |
43 | |
44 | |
45 | +def attempt_terminate_instances(account, instance_ids): |
46 | + """Initiate terminate instance method of specific handler |
47 | + |
48 | + :param account: Substrate account object. |
49 | + :param instance_ids: List of instance_ids to terminate |
50 | + :return: List of instance_ids failed to terminate |
51 | + """ |
52 | + uncleaned_instances = [] |
53 | + for instance_id in instance_ids: |
54 | + try: |
55 | + # We are calling terminate instances for each instances |
56 | + # individually so as to catch any error. |
57 | + account.terminate_instances([instance_id]) |
58 | + except Exception as e: |
59 | + # Using too broad exception here because terminate_instances method |
60 | + # is handlers specific |
61 | + uncleaned_instances.append((instance_id, repr(e))) |
62 | + return uncleaned_instances |
63 | + |
64 | + |
65 | +def contains_only_known_instances(known_instance_ids, possibly_known_ids): |
66 | + """Identify instance_id_list only contains ids we know about. |
67 | + |
68 | + :param known_instance_ids: The list of instance_ids (superset) |
69 | + :param possibly_known_ids: The list of instance_ids (subset) |
70 | + :return: True if known_instance_ids only contains |
71 | + possibly_known_ids |
72 | + """ |
73 | + return set(possibly_known_ids).issubset(set(known_instance_ids)) |
74 | + |
75 | + |
76 | class AWSAccount: |
77 | """Represent the credentials of an AWS account.""" |
78 | |
79 | @@ -160,14 +191,77 @@ |
80 | break |
81 | return unclean |
82 | |
83 | + def cleanup_security_groups(self, instances, secgroups): |
84 | + """Destroy any security groups used only by `instances`. |
85 | + |
86 | + :param instances: The list of instance_ids |
87 | + :param secgroups: dict of security groups |
88 | + :return: list of failed deleted security groups |
89 | + """ |
90 | + failures = [] |
91 | + for sg_id, sg_instances in secgroups: |
92 | + if contains_only_known_instances(instances, sg_instances): |
93 | + try: |
94 | + deleted = self.client.delete_security_group(name=sg_id) |
95 | + if not deleted: |
96 | + failures.append((sg_id, "Failed to delete")) |
97 | + except EC2ResponseError as e: |
98 | + failures.append((sg_id, repr(e))) |
99 | + |
100 | + return failures |
101 | + |
102 | + def get_security_groups(self, instances): |
103 | + """Get AWS configured security group |
104 | + If instances list is specified then get security groups mapped |
105 | + to those instances only. |
106 | + |
107 | + :param instances: list of instance names |
108 | + :return: list containing tuples; where each tuples contains security |
109 | + group id as first element and the list of instances mapped to that |
110 | + security group as second element. [(sg_id, [i_id, id2]), |
111 | + (sg_id2, [i_id1])] |
112 | + """ |
113 | + group_ids = [sg[0] for sg in self.iter_instance_security_groups( |
114 | + instances)] |
115 | + all_groups = self.client.get_all_security_groups( |
116 | + group_ids=group_ids) |
117 | + secgroups = [(sg.id, [id for id in sg.instances()]) |
118 | + for sg in all_groups] |
119 | + return secgroups |
120 | + |
121 | + def terminate_instances(self, instance_ids): |
122 | + """Terminate the specified instances.""" |
123 | + return self.client.terminate_instances(instance_ids=instance_ids) |
124 | + |
125 | def ensure_cleanup(self, resource_details): |
126 | """ |
127 | Do AWS specific clean-up activity. |
128 | :param resource_details: The list of resource to be cleaned up |
129 | :return: list of resources that were not cleaned up |
130 | """ |
131 | - uncleaned_resource = [] |
132 | - return uncleaned_resource |
133 | + uncleaned_resources = [] |
134 | + |
135 | + if not resource_details: |
136 | + return uncleaned_resources |
137 | + |
138 | + security_groups = self.get_security_groups( |
139 | + resource_details.get('instances', [])) |
140 | + |
141 | + uncleaned_instances = attempt_terminate_instances( |
142 | + self, resource_details.get('instances', [])) |
143 | + |
144 | + uncleaned_security_groups = self.cleanup_security_groups( |
145 | + resource_details.get('instances', []), security_groups) |
146 | + |
147 | + if uncleaned_instances: |
148 | + uncleaned_resources.append( |
149 | + {'resource': 'instances', |
150 | + 'errors': uncleaned_instances}) |
151 | + if uncleaned_security_groups: |
152 | + uncleaned_resources.append( |
153 | + {'resource': 'security groups', |
154 | + 'errors': uncleaned_security_groups}) |
155 | + return uncleaned_resources |
156 | |
157 | |
158 | class OpenStackAccount: |
159 | |
160 | === modified file 'tests/test_deploy_stack.py' |
161 | --- tests/test_deploy_stack.py 2017-03-10 00:50:40 +0000 |
162 | +++ tests/test_deploy_stack.py 2017-03-29 17:54:35 +0000 |
163 | @@ -51,6 +51,7 @@ |
164 | retain_config, |
165 | update_env, |
166 | wait_for_state_server_to_shutdown, |
167 | + error_if_unclean, |
168 | ) |
169 | from jujupy import ( |
170 | EnvJujuClient1X, |
171 | @@ -2673,3 +2674,60 @@ |
172 | wfp_mock.assert_called_once_with('example.org', 17070, closed=True, |
173 | timeout=60) |
174 | hni_mock.assert_called_once_with(client.env, 'i-255') |
175 | + |
176 | + |
177 | +class TestErrorIfUnclean(FakeHomeTestCase): |
178 | + def test_empty_unclean_resources(self): |
179 | + uncleaned_resources = [] |
180 | + error_if_unclean(uncleaned_resources) |
181 | + self.assertEquals(self.log_stream.getvalue(), '') |
182 | + |
183 | + def test_contain_unclean_resources(self): |
184 | + uncleaned_resources = [ |
185 | + { |
186 | + 'resource': 'instances', |
187 | + 'errors': [('ifoo', 'err-msg'), ('ibar', 'err-msg')] |
188 | + }, |
189 | + { |
190 | + 'resource': 'security groups', |
191 | + 'errors': [('sg-bar', 'err-msg')] |
192 | + } |
193 | + ] |
194 | + error_if_unclean(uncleaned_resources) |
195 | + self.assertListEqual(self.log_stream.getvalue().splitlines(), [ |
196 | + "CRITICAL Following resource requires manual cleanup", |
197 | + "CRITICAL instances", |
198 | + "CRITICAL \tifoo: err-msg", |
199 | + "CRITICAL \tibar: err-msg", |
200 | + "CRITICAL security groups", |
201 | + "CRITICAL \tsg-bar: err-msg" |
202 | + ]) |
203 | + |
204 | + def test_unclean_resources_without_sg_error(self): |
205 | + uncleaned_resources = [ |
206 | + { |
207 | + 'resource': 'instances', |
208 | + 'errors': [('ifoo', 'err-msg'), ('ibar', 'err-msg')] |
209 | + }, |
210 | + ] |
211 | + error_if_unclean(uncleaned_resources) |
212 | + self.assertListEqual(self.log_stream.getvalue().splitlines(), [ |
213 | + "CRITICAL Following resource requires manual cleanup", |
214 | + "CRITICAL instances", |
215 | + "CRITICAL \tifoo: err-msg", |
216 | + "CRITICAL \tibar: err-msg", |
217 | + ]) |
218 | + |
219 | + def test_unclean_resources_without_instances_error(self): |
220 | + uncleaned_resources = [ |
221 | + { |
222 | + 'resource': 'security groups', |
223 | + 'errors': [('sg-bar', 'err-msg')] |
224 | + } |
225 | + ] |
226 | + error_if_unclean(uncleaned_resources) |
227 | + self.assertListEqual(self.log_stream.getvalue().splitlines(), [ |
228 | + "CRITICAL Following resource requires manual cleanup", |
229 | + "CRITICAL security groups", |
230 | + "CRITICAL \tsg-bar: err-msg" |
231 | + ]) |
232 | |
233 | === modified file 'tests/test_substrate.py' |
234 | --- tests/test_substrate.py 2017-02-13 15:53:00 +0000 |
235 | +++ tests/test_substrate.py 2017-03-29 17:54:35 +0000 |
236 | @@ -51,6 +51,8 @@ |
237 | stop_libvirt_domain, |
238 | terminate_instances, |
239 | verify_libvirt_domain, |
240 | + contains_only_known_instances, |
241 | + attempt_terminate_instances, |
242 | ) |
243 | from tests import ( |
244 | FakeHomeTestCase, |
245 | @@ -1677,3 +1679,263 @@ |
246 | def test_maas_ensure_cleanup(self): |
247 | substrate_account = MAASAccount('profile', 'url', 'oauth') |
248 | self.assertEqual([], substrate_account.ensure_cleanup([])) |
249 | + |
250 | + |
251 | +class FakeSecurityGroup: |
252 | + def __init__(self, id, instances): |
253 | + self.id = id |
254 | + self._instances = instances |
255 | + |
256 | + def instances(self): |
257 | + return self._instances |
258 | + |
259 | + |
260 | +class TestAWSEnsureCleanUp(TestCase): |
261 | + def test_ensure_cleanup_successfully(self): |
262 | + client = MagicMock() |
263 | + resource_details = dict() |
264 | + resource_details['instances'] = ["i_id1", "i_id2"] |
265 | + aws = AWSAccount(None, 'myregion', client) |
266 | + client.get_all_security_groups.return_value = [ |
267 | + FakeSecurityGroup('sg_id1', ['i_id1', 'i_id2'])] |
268 | + client.delete_security_group.return_value = True |
269 | + uncleaned_resources = aws.ensure_cleanup(resource_details) |
270 | + client.delete_security_group.assert_called_once_with(name='sg_id1') |
271 | + self.assertEqual(client.get_all_instances.call_args_list, |
272 | + [call(instance_ids=['i_id1', 'i_id2'])]) |
273 | + self.assertEqual(uncleaned_resources, []) |
274 | + self.assertEqual( |
275 | + aws.client.terminate_instances.call_args_list, |
276 | + [call(instance_ids=['i_id1']), call(instance_ids=['i_id2'])]) |
277 | + |
278 | + def test_ensure_cleanup_with_uncleaned_instances(self): |
279 | + client = MagicMock() |
280 | + resource_details = dict() |
281 | + resource_details['instances'] = ["i_id1", "i_id2"] |
282 | + aws = AWSAccount(None, 'myregion', client) |
283 | + err_msg = 'Instance error' |
284 | + client.terminate_instances.side_effect = [ |
285 | + Exception(err_msg), Exception(err_msg)] |
286 | + client.get_all_security_groups.return_value = [ |
287 | + FakeSecurityGroup('sg_id1', ['i_id1', 'i_id2'])] |
288 | + client.delete_security_group.return_value = True |
289 | + uncleaned_resources = aws.ensure_cleanup(resource_details) |
290 | + self.assertEqual(client.get_all_instances.call_args_list, |
291 | + [call(instance_ids=['i_id1', 'i_id2'])]) |
292 | + self.assertEqual(uncleaned_resources, [ |
293 | + {'errors': [('i_id1', "Exception('Instance error',)"), |
294 | + ('i_id2', "Exception('Instance error',)")], |
295 | + 'resource': 'instances'}]) |
296 | + |
297 | + def test_ensure_cleanup_with_uncleaned_sg(self): |
298 | + client = MagicMock() |
299 | + resource_details = dict() |
300 | + resource_details['instances'] = ["i_id1", "i_id2"] |
301 | + aws = AWSAccount(None, 'myregion', client) |
302 | + client.terminate_instances.side_effect = ["i_id1", "i_id2"] |
303 | + client.get_all_security_groups.return_value = [ |
304 | + FakeSecurityGroup('sg_id1', [])] |
305 | + client.delete_security_group.return_value = False |
306 | + uncleaned_resources = aws.ensure_cleanup(resource_details) |
307 | + self.assertEqual(uncleaned_resources, [ |
308 | + {'errors': [('sg_id1', 'Failed to delete')], |
309 | + 'resource': 'security groups'}]) |
310 | + self.assertEqual(client.get_all_instances.call_args_list, |
311 | + [call(instance_ids=['i_id1', 'i_id2'])]) |
312 | + |
313 | + def test_ensure_cleanup_with_uncleaned_instances_and_sg(self): |
314 | + client = MagicMock() |
315 | + resource_details = dict() |
316 | + resource_details['instances'] = ["i_id1", "i_id2"] |
317 | + aws = AWSAccount(None, 'myregion', client) |
318 | + ati_err_msg = 'Instance not found' |
319 | + client.terminate_instances.side_effect = [ |
320 | + Exception(ati_err_msg), Exception(ati_err_msg)] |
321 | + client.get_all_security_groups.return_value = [ |
322 | + FakeSecurityGroup('sg_id1', ['i_id1', 'i_id2'])] |
323 | + client.delete_security_group.side_effect = EC2ResponseError( |
324 | + 400, "Bad Request", |
325 | + body={ |
326 | + "RequestID": "xxx-yyy-zz", |
327 | + "Error": { |
328 | + "Code": "Security group failed to delete", |
329 | + "Message": "failed" |
330 | + } |
331 | + }) |
332 | + uncleaned_resources = aws.ensure_cleanup(resource_details) |
333 | + self.assertEqual(client.get_all_instances.call_args_list, |
334 | + [call(instance_ids=['i_id1', 'i_id2'])]) |
335 | + self.assertEqual( |
336 | + uncleaned_resources, |
337 | + [{'errors': [ |
338 | + ('i_id1', "Exception('Instance not found',)"), |
339 | + ('i_id2', "Exception('Instance not found',)")], |
340 | + 'resource': 'instances'}, |
341 | + {'errors': [ |
342 | + ('sg_id1', |
343 | + "EC2ResponseError: 400 Bad Request\n{" |
344 | + "'RequestID': 'xxx-yyy-zz', 'Error': {" |
345 | + "'Message': 'failed', " |
346 | + "'Code': 'Security group failed to delete'}}")], |
347 | + 'resource': 'security groups'}]) |
348 | + |
349 | + |
350 | +class TestAWSCleanUpSecurityGroups(TestCase): |
351 | + def test_delete_secgroup_not_in_use(self): |
352 | + secgroup = [("sg-foo", ["foo", "bar"])] |
353 | + instances = ["foo", "bar"] |
354 | + client = MagicMock() |
355 | + aws = AWSAccount(None, 'myregion', client) |
356 | + failures = aws.cleanup_security_groups(instances, secgroup) |
357 | + self.assertEqual(failures, []) |
358 | + self.assertEqual( |
359 | + client.delete_security_group.call_args, call(name='sg-foo')) |
360 | + |
361 | + def test_dont_delete_secgroup_in_use(self): |
362 | + secgroup = [("sg-foo", ["foo", "bar", "baz"])] |
363 | + instances = ["foo", "bar"] |
364 | + client = MagicMock() |
365 | + aws = AWSAccount(None, 'myregion', client) |
366 | + failures = aws.cleanup_security_groups(instances, secgroup) |
367 | + self.assertEqual(client.delete_security_group.call_count, 0) |
368 | + self.assertEqual(failures, []) |
369 | + |
370 | + def test_return_failure_on_exception(self): |
371 | + secgroup = [("sg-foo", ["foo", "bar"]), ("sg-bar", ["foo", "bar"])] |
372 | + instances = ["foo", "bar"] |
373 | + client = MagicMock(spec=["delete_security_group"]) |
374 | + client.delete_security_group.side_effect = EC2ResponseError( |
375 | + 400, "Bad Request", |
376 | + body={ |
377 | + "RequestID": "xxx-yyy-zz", |
378 | + "Error": { |
379 | + "Code": "InvalidSecurityGroup.NotFound", |
380 | + "Message": "failed" |
381 | + } |
382 | + }) |
383 | + aws = AWSAccount(None, 'myregion', client) |
384 | + failures = aws.cleanup_security_groups(instances, secgroup) |
385 | + self.assertEqual(client.delete_security_group.call_args_list, |
386 | + [call(name='sg-foo'), call(name='sg-bar')]) |
387 | + self.assertListEqual(failures, |
388 | + [('sg-foo', |
389 | + "EC2ResponseError: 400 Bad Request\n{" |
390 | + "'RequestID': 'xxx-yyy-zz', 'Error': {" |
391 | + "'Message': 'failed'," |
392 | + " 'Code': 'InvalidSecurityGroup.NotFound'}}"), |
393 | + ('sg-bar', "EC2ResponseError: 400 Bad Request\n{" |
394 | + "'RequestID': 'xxx-yyy-zz', 'Error': {" |
395 | + "'Message': 'failed'," |
396 | + " 'Code': 'InvalidSecurityGroup.NotFound'}}")]) |
397 | + |
398 | + def test_return_mixed_response(self): |
399 | + secgroup = [("sg-foo", ["foo", "bar"]), ("sg-bar", ["fooX", "barX"])] |
400 | + instances = ["foo", "bar", "fooX", "barX"] |
401 | + client = MagicMock(spec=["delete_security_group"]) |
402 | + client.delete_security_group.side_effect = [ |
403 | + True, False] |
404 | + aws = AWSAccount(None, 'myregion', client) |
405 | + failures = aws.cleanup_security_groups(instances, secgroup) |
406 | + self.assertEqual(failures, [('sg-bar', 'Failed to delete')]) |
407 | + self.assertEqual(client.delete_security_group.call_args_list, |
408 | + [call(name='sg-foo'), call(name='sg-bar')]) |
409 | + |
410 | + def test_instance_mapped_to_more_than_one_secgroup(self): |
411 | + # Delete security group only if it has all the mapped instances |
412 | + # specified in the instances list. |
413 | + secgroup = [("sg-foo", ["foo", "bar"]), ("sg-bar", ["foo", "baz"])] |
414 | + instances = ["foo", "bar"] |
415 | + client = MagicMock() |
416 | + aws = AWSAccount(None, 'myregion', client) |
417 | + failures = aws.cleanup_security_groups(instances, secgroup) |
418 | + self.assertEqual(failures, []) |
419 | + self.assertEqual(aws.client.delete_security_group.call_count, 1) |
420 | + self.assertEqual( |
421 | + client.delete_security_group.call_args, call(name='sg-foo')) |
422 | + |
423 | + |
424 | +class TestContainsOnlyKnownInstances(TestCase): |
425 | + def test_return_true_when_all_ids_known(self): |
426 | + instances = ["foo", "bar", "qnx"] |
427 | + sg_list = ["foo", "bar", "qnx"] |
428 | + self.assertEqual( |
429 | + contains_only_known_instances(instances, sg_list), True) |
430 | + |
431 | + def test_return_true_known_ids_are_subset(self): |
432 | + instances = ["foo", "bar", "qnx", "foo1"] |
433 | + sg_list = ["foo", "bar", "qnx"] |
434 | + self.assertEqual( |
435 | + contains_only_known_instances(instances, sg_list), True) |
436 | + |
437 | + def test_return_false_when_some_ids_unknown(self): |
438 | + instances = ["foo", "qnx"] |
439 | + sg_list = ["foo", "bar"] |
440 | + self.assertEqual( |
441 | + contains_only_known_instances(instances, sg_list), |
442 | + False) |
443 | + |
444 | + |
445 | +class TestAttemptTerminateInstances(TestCase): |
446 | + def test_return_error_on_exception(self): |
447 | + client = MagicMock() |
448 | + instances = ["foo", "bar"] |
449 | + err_msg = "Instance not found" |
450 | + aws = AWSAccount(None, 'myregion', client) |
451 | + client.terminate_instances.side_effect = Exception(err_msg) |
452 | + failed = attempt_terminate_instances(aws, instances) |
453 | + self.assertEqual(failed, |
454 | + [('foo', "Exception('{}',)".format(err_msg)), |
455 | + ('bar', "Exception('{}',)".format(err_msg))]) |
456 | + |
457 | + def test_return_with_no_error(self): |
458 | + client = MagicMock() |
459 | + instances = ["foo", "bar"] |
460 | + aws = AWSAccount(None, 'myregion', client) |
461 | + client.terminate_instances.return_value = ["foo", "bar"] |
462 | + failed = attempt_terminate_instances(aws, instances) |
463 | + self.assertEqual(client.terminate_instances.call_args_list, |
464 | + [call(instance_ids=['foo']), |
465 | + call(instance_ids=['bar'])]) |
466 | + self.assertEqual(failed, []) |
467 | + |
468 | + def test_returns_has_some_error(self): |
469 | + client = MagicMock() |
470 | + instances = ["foo", "bar"] |
471 | + err_msg = "Instance not found" |
472 | + aws = AWSAccount(None, 'myregion', client) |
473 | + client.terminate_instances.side_effect = ["foo", Exception(err_msg)] |
474 | + failed = attempt_terminate_instances(aws, instances) |
475 | + self.assertEqual(client.terminate_instances.call_args_list, [ |
476 | + call(instance_ids=['foo']), call(instance_ids=['bar'])]) |
477 | + self.assertEqual(failed, [ |
478 | + ('bar', "Exception('Instance not found',)")]) |
479 | + |
480 | + |
481 | +class TestGetSecurityGroups(TestCase): |
482 | + def test_instance_managed_by_single_security_group(self): |
483 | + client = MagicMock() |
484 | + instance_sec_groups = (('i_id1', 'sg_id1'), ('i_id2', 'sg_id1')) |
485 | + all_sec_groups = [FakeSecurityGroup('sg_id1', ['i_id1', 'i_id2'])] |
486 | + aws = AWSAccount(None, 'myregion', client) |
487 | + client.get_all_security_groups.return_value = all_sec_groups |
488 | + with patch.object( |
489 | + aws, 'iter_instance_security_groups', |
490 | + autospec=True, return_value=instance_sec_groups): |
491 | + sec_groups = aws.get_security_groups(['i_id1', 'i_id2']) |
492 | + self.assertEqual(sec_groups, [('sg_id1', ['i_id1', 'i_id2'])]) |
493 | + |
494 | + def test_instance_managed_by_multiple_security_group(self): |
495 | + client = MagicMock() |
496 | + instance_sec_groups = (('i_id1', 'sg_id1'), ('i_id2', 'sg_id1')) |
497 | + all_sec_groups = [FakeSecurityGroup( |
498 | + 'sg_id1', ['i_id1', 'i_id2']), |
499 | + FakeSecurityGroup('sg_id2', ['i_id1'])] |
500 | + aws = AWSAccount(None, 'myregion', client) |
501 | + client.get_all_security_groups.return_value = all_sec_groups |
502 | + with patch.object( |
503 | + aws, 'iter_instance_security_groups', |
504 | + autospec=True, return_value=instance_sec_groups): |
505 | + sec_groups = aws.get_security_groups(['i_id1', 'i_id2']) |
506 | + self.assertEqual(sec_groups, |
507 | + [('sg_id1', ['i_id1', 'i_id2']), |
508 | + ('sg_id2', ['i_id1'])]) |
I don't think that adding unclean=[] to terminate_instances is the best way to do this. Instead you should make the call to terminate_instances and catch any exception that might occur and log that.
Changing terminate_instances like you have drastically changes how the method responds to error (it use to raise an exception in some places, now it errors silently) which might have a major effect on other parts of existing code that rely on the original behaviour.
Also of note, I don't think having an empty list as a default arg like that is what you want, you might not be aware but it's behaviour might be surprising. docs.python- guide.org/ en/latest/ writing/ gotchas/ #mutable- default- arguments
This link can describe it better than I can: http://
Please also see inline comments too.
I plan to get some other eyes on this in case I miss something.