Redo volumes,disks,images cleanup #171
Conversation
|
I'm all pro for drop of unneded code :D |
asmorodskyi
force-pushed
the
volumes
branch
7 times, most recently
from
c806ea3
to
4bbdc41
Compare
Codecov ReportBase: 80.21% // Head: 67.61% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #171 +/- ##
===========================================
- Coverage 80.21% 67.61% -12.60%
===========================================
Files 25 13 -12
Lines 2072 1056 -1016
===========================================
- Hits 1662 714 -948
+ Misses 410 342 -68
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
ocw/lib/EC2.py
Outdated
| self.cleanup_snapshots(cleanup_ec2_max_snapshot_age_days) | ||
| if cleanup_ec2_max_volumes_age_days >= 0: | ||
| self.cleanup_volumes(cleanup_ec2_max_volumes_age_days) | ||
| cleanup_ec2_max_age_days = PCWConfig.get_feature_property('cleanup', 'ec2-max-age-days', self._namespace) |
There was a problem hiding this comment.
why do we need the extra variable for ec2 here?
There was a problem hiding this comment.
historical reasons , I think we will keep this for next commits ...
ocw/lib/provider.py
Outdated
| """ | ||
| delta_in_hours = PCWConfig.get_feature_property('cleanup', 'max-age-hours', self._namespace) | ||
| max_allowed_age = datetime.now(timezone.utc) - timedelta(hours=delta_in_hours) | ||
| return max_allowed_age > age |
There was a problem hiding this comment.
The name of the method is older_than_max_age_hours but this comparison does exactly the opposite 🤔
There was a problem hiding this comment.
There is a bit of confusion in this method - age is not an age (i.e. a duration) but a timestamp, i.e. a certain point in time. Suggestion:
def is_outdated(self, timestamp):
delta = PCWConfig.get_feature_property('cleanup', 'max-age-hours', self._namespace) # maximum allowed delta
delta = timedelta(delta)
now = datetime.now(timezone.utc)
# Resource is outdated, if the given timestamp plus allowed delta exceeds the current timestamp
return (timestamp + delta) > now
There was a problem hiding this comment.
the code which you suggesting will not give expected behavior . I can show you in example .
Let's say max-age-hours=7 and if I call it with is_outdated("01-01-2022 21-00") it will gives me false . your method may be called is_still_valid because it will return true only for timestamps which didn't cross max-age-hours
There was a problem hiding this comment.
True, the last line should be return now > (timestamp > delta)
asmorodskyi
force-pushed
the
volumes
branch
2 times, most recently
from
2305239
to
6d4c671
Compare
Initially when data storage cleanup logic was created we wanted to make it very flexable from one side and very coutious from another. After more than 2 years of practical usage of the tool we can clearly state that half of implemented features don't have real use case. This commit is dropping not needed functionality keeping only what necessary
Initially when data storage cleanup logic was created
we wanted to make it very flexable from one side
and very coutious from another. After more than 2 years
of practical usage of the tool we can clearly state that
half of implemented features not needed