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

Refactor deployments storage on redeploy and undeployment #483

Merged
merged 18 commits into from
Nov 19, 2023

Conversation

ariefrahmansyah
Copy link
Contributor

@ariefrahmansyah ariefrahmansyah commented Nov 2, 2023

What this PR does / why we need it:

The terminated endpoint means the user has triggered Undeployment and there's no deployed endpoint in the cluster. When we redeploy this terminated endpoint, the deployment history should show all previous successful deployment logs as Not Deployed and the new deployment as Pending.

However, when redeploying the terminated endpoint, the deployment history shows the previous successful deployment as the current and deployed one:

Before redeployment of terminated endpoint:

image

Actual view from redeployment of terminated endpoint:

image

Fixes introduced by this PR:

Before redeployment, the current label is given to the latest successful/terminated endpoint:

image

During redeployment:

image

After redeployment succeed:

image

*Note: Please ignore the time displayed as in 7 hours -- this is because I'm running from local and the time zone is not translated correctly.


Behind the scene, we refactor deployments table for given version endpoint for redeployment and unemployment:

  1. Redeployment
    a. new deployment status = running/serving,
    b. old successful deployment status = terminated
  2. Undeployment
    a. old successful deployment status = terminated

Which issue(s) this PR fixes:

  • Fixes redeployment of terminated endpoint
  • Fixes duplicate link icon on Mlflow run

Does this PR introduce a user-facing change?:

NONE

Checklist

  • Added unit test, integration, and/or e2e tests
  • Tested locally
  • Updated documentation
  • Update Swagger spec if the PR introduce API changes
  • Regenerated Golang and Python client if the PR introduce API changes

@ghost
Copy link

ghost commented Nov 2, 2023

👇 Click on the image for a new way to code review

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map legend

@ariefrahmansyah ariefrahmansyah marked this pull request as ready for review November 2, 2023 05:27
@krithika369
Copy link
Collaborator

Hi @ariefrahmansyah, I will have another look at this PR today. In the meantime, I was thinking it may also be good to capture the data migration steps (the SQL script and any ad-hoc action that needs to be taken for deployments since Merlin 0.34.0) in this MR as a .md doc or something like that, so it's easier for everyone to review. Could you help update that too? Thanks.

@ariefrahmansyah
Copy link
Contributor Author

Hi @ariefrahmansyah, I will have another look at this PR today. In the meantime, I was thinking it may also be good to capture the data migration steps (the SQL script and any ad-hoc action that needs to be taken for deployments since Merlin 0.34.0) in this MR as a .md doc or something like that, so it's easier for everyone to review. Could you help update that too? Thanks.

Hi Krithika, I'm still playing on the SQL on my local machine. Will update the MR with the SQL doc. Btw, I'll create the doc with this path: docs/dev-guide/deployments_table_migration.md

Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

@ariefrahmansyah left some comments. As discussed earlier, do capture the data migration script in this PR as well. I think it should be safe to add it as a normal SQL migration file as we don't anticipate any ad-hoc handling for the deployments status update. Thanks!

api/storage/deployment_storage.go Show resolved Hide resolved
api/storage/deployment_storage.go Outdated Show resolved Hide resolved
ui/src/pages/version/HistoryDetails.js Outdated Show resolved Hide resolved
ui/src/pages/version/HistoryDetails.js Show resolved Hide resolved
ui/src/pages/version/HistoryDetails.js Outdated Show resolved Hide resolved
ui/src/pages/version/HistoryDetails.js Outdated Show resolved Hide resolved
ui/src/pages/version/HistoryDetails.js Show resolved Hide resolved
Arief Rahmansyah added 6 commits November 7, 2023 13:42
* Remove console.log
* Make terminated's EuiHealth color to default
* Improve current deployment label logic
* Set pending deployment to terminated too
Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

@ariefrahmansyah Left some more comments. One the whole, LGTM. Will have another look when the open comments are resolved, as some of them may require some thought. Thanks!

Copy link
Collaborator

@krithika369 krithika369 left a comment

Choose a reason for hiding this comment

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

Left one comment. The rest LGTM. Thanks, @ariefrahmansyah !

@ariefrahmansyah ariefrahmansyah merged commit 95a6183 into main Nov 19, 2023
36 checks passed
@ariefrahmansyah ariefrahmansyah deleted the redeploy-update-deployments-table branch November 19, 2023 09:41
@krithika369 krithika369 mentioned this pull request Dec 6, 2023
5 tasks
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.

4 participants