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

support extension restart and manual deletion of pod #335

Merged
merged 5 commits into from
Feb 16, 2024

Conversation

feloy
Copy link
Contributor

@feloy feloy commented Feb 15, 2024

What does this PR do?

  • application being deployed and extension restarted: because AI Studio manages the restart of the app container, the pod is not ok, it needs to be deleted by user => This deletion is correctly supported and the Play button is enabled again.

  • application deployed and correctly running, and extension restarted => the status "Pod running" is displayed in the Environments page and the pod name is displayed in the "Application Details" block.

  • application being deployed or correctly running, and the pod is deleted manually => tasks cleared and replaced by a single 'Pod deleted manually', visible in "Application Details" (not in Environments, as the line has been deleted)

  • replaced delete with stop button

  • matching pod is deleted before to start a new one

  • handle pod stopped manually

  • handle machine stopped manually

Screenshot / video of UI

What issues does this PR fix or reference?

Fixes #328

How to test this PR?

@feloy feloy requested a review from a team as a code owner February 15, 2024 08:13
@feloy feloy force-pushed the feat-328/corner-cases-app-status-main branch from f86dacb to 82cb3c0 Compare February 15, 2024 08:15
@feloy feloy marked this pull request as draft February 15, 2024 08:15
@feloy feloy marked this pull request as ready for review February 15, 2024 08:39
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

I'm still a bit puzzled about the buttons and their behaviors TBH (i would expect the same behaviors we have for podman machines - the stop and restart button to be enabled when the env is running, or the delete when it's stopped and so on.. ).

@mairin It would be great to know how this should work bc otherwise it's hard to make an objective review and it's just personal taste

My thoughts for the moment as i don't know how it should effectively work:

  1. i have a recipe pod created yesterday (it is stopped now), i open my ai-studio and i see this. I would expect to being able to click the start button and have the restart disabled.

image

  1. I click on restart. Everything is built again correctly but eventually i see. Shouldn't the different states/run application summary be hidden?

image

  1. I stop the pod manually and i see. I would expect to see the name of the pod without the old states/summary

image

  1. i delete the pod and i see. Shouldn't we show again the Run application button?

image

Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

When moving the mouse over a disabled action the cursor remain the same
cursor_same

@feloy
Copy link
Contributor Author

feloy commented Feb 15, 2024

I'm still a bit puzzled about the buttons and their behaviors TBH (i would expect the same behaviors we have for podman machines - the stop and restart button to be enabled when the env is running, or the delete when it's stopped and so on.. ).

@mairin It would be great to know how this should work bc otherwise it's hard to make an objective review and it's just personal taste

My thoughts for the moment as i don't know how it should effectively work:

  1. i have a recipe pod created yesterday (it is stopped now), i open my ai-studio and i see this. I would expect to being able to click the start button and have the restart disabled.

image

For now, because the extension is managing the state of the containers in the pod (it restarts the app container when the model is running), it is not interesting to just restart a stopped pod, because it will never restart correctly. The only way for now is to delete and recreate the pod (with the Restart button)

Bu I agree that when the pod is stopped, the display should reflect it better

  1. I click on restart. Everything is built again correctly but eventually i see. Shouldn't the different states/run application summary be hidden?

image

  1. I stop the pod manually and i see. I would expect to see the name of the pod without the old states/summary

image

Right, I need to work on the Stop status of a pod

  1. i delete the pod and i see. Shouldn't we show again the Run application button?

image

That's an open question we started discuss on #289 (comment)

@feloy feloy requested a review from lstocchi February 15, 2024 17:27
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM
You should also update the message when stopping the env. It says Delete the environment "${recipe.name}"?. It should say Stop ....

@feloy
Copy link
Contributor Author

feloy commented Feb 16, 2024

LGTM You should also update the message when stopping the env. It says Delete the environment "${recipe.name}"?. It should say Stop ....

Good catch, thanks

Copy link
Contributor

@axel7083 axel7083 left a comment

Choose a reason for hiding this comment

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

LGTM 👏 !!

tested and works fine when

  • still starting and deleting from cli
  • still starting and deleting from podman

@feloy feloy merged commit 193b3b0 into containers:main Feb 16, 2024
3 checks passed
mhdawson pushed a commit to mhdawson/podman-desktop-extension-ai-lab that referenced this pull request Nov 22, 2024
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.

support deleting pod manually and restarting extension for application status
3 participants