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

Odsc-56003/fix eval delete #818

Merged
merged 3 commits into from
May 6, 2024

Conversation

mingkang111
Copy link
Member

@mingkang111 mingkang111 commented May 3, 2024

Description

This PR aims to fix status for DELETING/DELETED evaluation:

Changed

  • Improved _get_status to return correct status under different cases.
  • DELETE Eval will return the real status if can be fetched or DELETING if real status not available.
  • DELETE Eval remove target eval from listing cache, so that when UI invoke LIST can get correct status.

TODO

  • UI show banner/or something else that deletion in progress while waiting for backend response.

Test

Screenshot 2024-05-03 at 17 01 26

@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label May 3, 2024
@@ -367,6 +367,9 @@ class AquaEvaluationApp(AquaApp):
_report_cache = TTLCache(maxsize=10, ttl=timedelta(hours=5), timer=datetime.now)
_metrics_cache = TTLCache(maxsize=10, ttl=timedelta(hours=5), timer=datetime.now)
_eval_cache = TTLCache(maxsize=200, ttl=timedelta(hours=10), timer=datetime.now)
_deletion_cache = TTLCache(
Copy link
Member

Choose a reason for hiding this comment

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

any reason we've selected 10 mins, or this is arbitrary?

jobrun = None

self._eval_cache.pop(key=eval_id, default=None)
self._deletion_cache.__setitem__(key=eval_id, value="")
Copy link
Member

Choose a reason for hiding this comment

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

curious about this: would this cache be set incorrectly if the _delete_job_and_model comes across ServiceError while deleting resources? In that case, will the list API will show status as DELETED for 10 mins (TTL duration) and and then get updated with the correct status?

job_run_status = jobrun.lifecycle_state

if jobrun is None:
if model.identifier in self._deletion_cache.keys():
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't this show DELETED status when list API is called even if the job run is still in the process of being deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @VipulMascarenhas , jobrun status is tracked by rqs in LIST. So if job run is still active (i.e. jobrun status != deleted), the jobrun will not be None, and the real status will be used. For jobrun has been deleted by DELETE Button in AQUA, RQS won't be able to track that jobrun, and the jobrun status should show as DELETED in this case.

Copy link
Member

@VipulMascarenhas VipulMascarenhas left a comment

Choose a reason for hiding this comment

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

lgtm 👍

@mingkang111 mingkang111 merged commit 2031b02 into feature/aquav1.0.2 May 6, 2024
2 checks passed
@mingkang111 mingkang111 deleted the ODSC-56003/fix_eval_delete branch May 6, 2024 19:24
mayoor pushed a commit that referenced this pull request May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AQUA OCA Verified All contributors have signed the Oracle Contributor Agreement. ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants