Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for not billing during outages #68

Merged
merged 2 commits into from
May 31, 2024
Merged

Conversation

knikolla
Copy link
Collaborator

Currently hardcoded values for the MGHPCC shutdown during May 2024.

Implemented via subtraction of the runtime of instances during the outage.

Currently hardcoded values for the MGHPCC shutdown during May 2024.

Implemented via subtraction of the runtime of instances during the
outage.
naved001
naved001 previously approved these changes May 30, 2024
@@ -69,6 +69,12 @@ class InstanceRuntime(object):
total_seconds_running: int = 0
total_seconds_stopped: int = 0

def __sub__(self, other):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

learnt something new!

- Split out get_runtime_for_instance to allow easier testing and
  improve legibility.
- Added testing.
- Testing found a bug when calling get_runtime_during multiple times
  due to it changing state in the Instance and InstanceEvent
  objects. Used function variables instead of modifying the object.
@knikolla
Copy link
Collaborator Author

@naved001 thanks for the review! Made an additional commit that made some small refactoring to allow testing and fixed a weird bug that I was encountering in testing.

@knikolla knikolla requested a review from naved001 May 30, 2024 23:15
@@ -115,7 +109,7 @@ def test_instance_no_delete_action():

r = i.get_runtime_during(
datetime(year=1999, month=11, day=1, hour=0, minute=0, second=0),
datetime(year=2000, month=12, day=1, hour=0, minute=0, second=0),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should have been 1999 all along, however it was clamped during a previous call of get_runtime_during therefore it hid the mistake.

@naved001 naved001 dismissed their stale review May 30, 2024 23:17

Will review it again

)
runtime = runtime - excluded_runtime

return runtime

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe I am being paranoid, but should there be a check anywhere to assert that all excluded_intervals fall within the start and end date? If not, it could cause the runtime to be negative

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's good to be paranoid and that seems like a valid concern. I can do that in a follow-up if that's okay. Right now the excluded_intervals are hardcoded in the file and are not user entered.

@knikolla knikolla merged commit c7bd4aa into CCI-MOC:main May 31, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants