ec2: add cleanup for VPC #105
Conversation
Codecov Report
@@ Coverage Diff @@
## master #105 +/- ##
==========================================
+ Coverage 78.79% 81.42% +2.62%
==========================================
Files 27 27
Lines 2028 2315 +287
==========================================
+ Hits 1598 1885 +287
Misses 430 430
Continue to review full report at Codecov.
|
asmorodskyi
force-pushed
the
vpc
branch
2 times, most recently
from
054254f
to
cc637df
Compare
asmorodskyi
force-pushed
the
vpc
branch
6 times, most recently
from
7acec00
to
01b741f
Compare
asmorodskyi
force-pushed
the
vpc
branch
2 times, most recently
from
968d063
to
58ee38f
Compare
asmorodskyi
force-pushed
the
vpc
branch
14 times, most recently
from
1fd7e59
to
b02b99b
Compare
ocw/lib/EC2.py
Outdated
| self.cleanup_uploader_vpcs() | ||
|
|
||
| def delete_vpc(self, region, vpcId): | ||
| if PCWConfig.getBoolean('cleanup/vpc_only_notify', self._namespace): |
There was a problem hiding this comment.
I think, most of the variables in the configuration use - as separator. And maybe cleanup/no-vpc
There was a problem hiding this comment.
agree with separator change . but I find cleanup/no-vpc less informative . why you don't like cleanup/vpc-notify-only ?
There was a problem hiding this comment.
I just saw that you also have cleanup/vpc_cleanup and then no-vpc doesn't say anything...
But I'm not sure about it. as you have some kind of tree in these options. Cause vpc_only_notify doesn't work if vpc_cleanup=false.
Maybe we can change the code in that manner, that we have as default
cleanup/vpc-cleanup=True # Find and delete left over VPC's
cleanup/vpc-notifiy=True # Find and notify for left over VPC's
and you can set one or the other and get the expected result.
Another thing is, that we talk only about ec2-vpc's. But at least GCE has VPC as well. Don't know if you wanna reflect that somehow, or we just say "vpc-cleanup where ever it is implemented"
ocw/lib/EC2.py
Outdated
| self.log_info('{} in {} looks like uploader leftover. (OwnerId={}).', vpc['VpcId'], region, | ||
| vpc['OwnerId']) | ||
| if not self.delete_vpc(region, vpc['VpcId']) and not self.dry_run: | ||
| send_mail('VPC deletion locked by running VMs', |
There was a problem hiding this comment.
Maybe send the mail in line 230 https://github.com/SUSE/pcw/pull/105/files#diff-c5779e6ba06607a1398b8f08e0bbbec9563650ba6688e2d3cc6569eddbc7e89eR230
There was a problem hiding this comment.
your comment makes me realize that this approach with delete_vpc returns False in several unrelated cases was bad decision . will redo this part.
| @@ -259,8 +259,16 @@ def has(setting: str) -> bool: | |||
| return False | |||
|
|
|||
| @staticmethod | |||
| def getBoolean(config_path: str, default=False) -> bool: | |||
| value = ConfigFile().get(config_path, default) | |||
| def getBoolean(config_path: str, namespace: str = None, default=False) -> bool: | |||
There was a problem hiding this comment.
I wonder, why we do not change get_feature_property() so it handle bool types as well.
And then drop this getBoolean().
There was a problem hiding this comment.
after spending another half-day on this PR I realized that this is enough :) while generally agree with you I would like to move this into separate PR . I create issue to not forget about this #108
| except Exception as e: | ||
| self.log_err("{} on VPC deletion. {}", type(e).__name__, traceback.format_exc()) | ||
| send_mail('{} on VPC deletion in [{}]'.format(type(e).__name__, self._namespace), traceback.format_exc()) |
There was a problem hiding this comment.
This looks like a rather bad style. In general one should not catch "Exception" itself except for very rare cases. And also as the "try" covers the complete body of the method I wonder why you even handle the exception on this level and not on caller level
There was a problem hiding this comment.
deleting VPC is quite complex thing which includes several step each one can fail in several different way ( network issues , vault/password problems , various exceptions from AWS API ) idea is that any issues with deleting one VPC should not interrupt process of deleting others .
I am aware about the fact that catching everything is bad practice , but I don't think it is applicable here . Keep in mind that even if I will remove try/catch here same function will be called again in 1 hour https://github.com/SUSE/pcw/blob/master/ocw/lib/cleanup.py#L45 so from some perspective everything is wrapped in try/catch Exception anyway :).
No description provided.