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

control-service: Reset termination status when job is disabled #1405

Merged
merged 5 commits into from
Dec 7, 2022

Conversation

doks5
Copy link
Contributor

@doks5 doks5 commented Dec 5, 2022

Currently, when a data job is deleted, we reset its termination status, so as to avoid issues with monitoring services reporting incorrect status. This, however, is not done in case a data job is disabled, even though the job is no longer active and its termination status should not matter.

This change modifies the DeplymentService, so that if a data job is disabled, the termination status is reset.

Testing Done: Added test.

Signed-off-by: Andon Andonov andonova@vmware.com

@murphp15
Copy link
Collaborator

murphp15 commented Dec 5, 2022

I would prefer that the test is testing the function patchDeployment instead of a level below that.
Because at the moment you aren't really asserting that disabling a job clears the metrics?

@doks5 doks5 force-pushed the person/andonova/disable-clean-metrics branch from aef1c3f to 2f43e15 Compare December 6, 2022 07:35
@doks5
Copy link
Contributor Author

doks5 commented Dec 6, 2022

I would prefer that the test is testing the function patchDeployment instead of a level below that. Because at the moment you aren't really asserting that disabling a job clears the metrics?

Hi, @murphp15
Could you, please, advise where the additional test should go
I searched for a test that checks if the metrics are properly cleared at job deletion, but such does not seem to exist.

UPDATE: I've added two additional tests for the patchDeployment

@antoniivanov
Copy link
Collaborator

Looks in the right direction. I think we need to see what happens when we remove a job deployment as well (I honestly do not know and haven't time to check the code so it's up to you). I am also fine if you decide to handle this case in subsequent PR.

Don't wait for my approval since I am going on vacation :)

@doks5 doks5 force-pushed the person/andonova/disable-clean-metrics branch from 2f43e15 to 1cdc3dc Compare December 6, 2022 16:40
doks5 and others added 3 commits December 7, 2022 10:45
Currently, when a data job is deleted, we reset its termination status, so as to avoid
issues with monitoring services reporting incorrect status. This, however, is not done
in case a data job is disabled, even though the job is no longer active and its termination
status should not matter.

This change modifies the DeplymentService, so that if a data job is disabled, the termination
status is reset.

Testing Done: Added test.

Signed-off-by: Andon Andonov <andonova@vmware.com>
@doks5 doks5 force-pushed the person/andonova/disable-clean-metrics branch from 7a3e738 to 7ac97ed Compare December 7, 2022 08:49
@doks5 doks5 merged commit 2aaac70 into main Dec 7, 2022
@doks5 doks5 deleted the person/andonova/disable-clean-metrics branch December 7, 2022 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants