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

Update / clean up snapshots on environments configuration update #3017

Closed
mmorhun opened this issue Nov 8, 2016 · 16 comments
Closed

Update / clean up snapshots on environments configuration update #3017

mmorhun opened this issue Nov 8, 2016 · 16 comments
Assignees
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Milestone

Comments

@mmorhun
Copy link
Contributor

mmorhun commented Nov 8, 2016

We have API method which updates workspace configuration. One of the fields of this API method is array with environments configurations. But in case of batch update we cannot determine for sure which environments should be updated.

For example, we have 2 environments in our workspace. A user makes update request with 2 environments, but exchanges their names. The problem is that we cannot determine whether user just renamed both environments or changed all parameters except names.

This information is needed for updating snaphots. And if we cannot correctly update snapshots we should delete them.

Also we have other API method which can update specified environment. In this case snapshots should be updated and than workspace will be able to start from snapshot.

Use Cases

  1. User changes only the name of an environment
  • Name is updated
  • Snapshots are retained
  1. User changes the configuration of a workspace or environment (not just the name)
  • Popup is presented to the user:

    Title: Updated Configuration Invalidates Snapshots
    Message: Updating the configuration of a workspace or environment will make any associated snapshots impossible to run. Snapshots will be deleted.
    Buttons: Ok (default); Cancel

    • If "Ok"
      • Configuration is updated
      • Snapshots are deleted
    • If "Cancel"
      • Configuration is reverted
      • Snapshots are retained

Is needed for: #2528

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 8, 2016

@TylerJewell @bmicklea is it ok to delete snaphots on batch environments update of a workspace?

@mmorhun mmorhun added kind/bug Outline of a bug - must adhere to the bug report template. status/analyzing An issue has been proposed and it is currently being analyzed for effort and implementation approach team/enterprise labels Nov 8, 2016
@mmorhun mmorhun self-assigned this Nov 8, 2016
@bmicklea
Copy link

bmicklea commented Nov 8, 2016

@mmorhun - I don't really understand. Are you saying that when a user renames a workspace we need to delete previous snapshots and restart the workspace from recipe?

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 8, 2016

@bmicklea no, I am talking about environment configuration updating (In dashboard it calls Runtimes). I'll try to explain what I meant.
In case of update of specific environment (using update environment API method) we can identifier environment which should be updated and can attach old snapshots to new (updated) environment.
But in case of batch update of environments (using update workspace API method) we cannot determine one correspondence between old and new environments configurations. So, we cannot attach old snapshot to new environments respectively (see example in the issue description). More, new configuration can be incompatible with old snapshots (for example, we changed base image for machine). So, we can delete all snapshots on batch update of environments or just deny batch update.
If my explanation still is not clear, please let me know.

@bmicklea
Copy link

bmicklea commented Nov 8, 2016

Okay I think I understand:

  • Single environment rename and update will work.
  • Batch environment rename and update will work if we delete all snapshots
    or we can deny this operation.

For now let's just deny the operation for batch renames and updates.

On Tue, Nov 8, 2016 at 11:14 AM, Mykola Morhun notifications@github.com
wrote:

@bmicklea https://github.com/bmicklea no, I am talking about
environment configuration updating (In dashboard it calls Runtimes). I'll
try to explain what I meant.
In case of update of specific environment (using update environment API
method) we can identifier environment which should be updated and can
attach old snapshots to new (updated) environment.
But in case of batch update of environments (using update workspace API
method) we cannot determine one correspondence between old and new
environments configurations. So, we cannot attach old snapshot to new
environments respectively (see example in the issue description). More, new
configuration can be incompatible with old snapshots (for example, we
changed base image for machine). So, we can delete all snapshots on batch
update of environments or just deny batch update.
If my explanation still is not clear, please let me know.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#3017 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AHSnCnDrHCV5BaC2aa0bgdcL04qeM1s9ks5q8J_kgaJpZM4KsfbC
.

Brad Micklea | Operations | bmicklea@​codenvy.​com | 4​16​.7​07​.07​92

@TylerJewell
Copy link

@bmicklea, @mmorhun - I think I agree with Brad. If you are trying to do a batch update, we should deny the operation and provide a message explaining why we are denying it.

If we deny batch update of environments, then is it the case that users must create new environment in a workspace if they want to do a big modification. I think the answer here will be yes.

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 9, 2016

@TylerJewell I am not sure that I fully understand you. Please clarify.
We have two cases:

  1. Update of specified environment.
    This update can be minor (changing of environment name only!) and major (changing of base image / recipe, machines configurations). In case of rename of specified environment we can attach old snaphot to new environment, but in case of major update old shapshot cannot be used safety (for example, if base image changed then old snapshot cannot be used at all. Also, we cannot guarantee (determine) workspace stability in case of changing configurations and machines set). So, should I delete snapshots on major update of environment or deny major update of one specified environment (but denying changing of machine configuration is not good idea, I think). Better when we reject update if snapshot is saved and ask a user to remove snapshot before update configs.
  2. Workspace configuration update. One of the parts of it is environments update.
    In case when workspace has only one default environment and user just update it (not add another one), the situation is similar to update of specified environment.
    Otherwise (most cases) we cannot determine one correspondence between old snapshots and new environments. Also problem with incompatible snapshots still remains. I think, that when a user do update of all environments he understand what he is doing (we can add description in swagger). My suggestion is to delete snapshots on batch update of environments. Otherwise we have no way to update workspace configs with one query. We can deny this operation, but in my opinion it will be a mess, because a user can change only one environment per query. If a user should add new env on changing configuration of existing one, then we'll have trash of environments soon.

@bmicklea
Copy link

bmicklea commented Nov 9, 2016

Reading this through I think that when a user makes a change to a workspace configuration or environment we need to warn them that the change will force a deletion of the snapshot. The popup would be something like:

  • Title: Configuration Changes Affect Snapshots
  • Content: Changing a {workspace/environment} configuration will force a deletion of associated snapshots. Do you still wish to make the change?
  • Buttons: Yes; No (default)

@mmorhun is there a way that the user can find out the snapshot ID in case they want to save it elsewhere for example?

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 9, 2016

is there a way that the user can find out the snapshot ID in case they want to save it elsewhere for example?

No, a user cannot get internal ID of a snapshot. And in case of snapshot deletion we delete record about it in database.
If you want to store snapshot elsewhere just configure docker registry for snapshots. But note, that after snapshot deletion a user will not be able to restore from it.

@bmicklea
Copy link

bmicklea commented Nov 9, 2016

@mmorhun is there really no way to keep the old snapshot when renaming a workspace or environment? That seems like a small change that shouldn't require deleting snapshots...

@tolusha
Copy link
Contributor

tolusha commented Nov 9, 2016

It is a new environment, no need to keep old snapshots.

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 9, 2016

@bmicklea In case of workspace renaming nothing changes.
If we just rename environment using environment update API method we can save snapshots.
But, if a user updates some environment and changes recipe we cannot be sure that old snapshot can be used. In some case workspace will start, in some cases workspace will fail to start. And we cannot determine for sure when it can start or it will fail (yes in some cases we can, but not always). To avoid a mess and unexpected fails it will be better to remove snapshots.
If a user wants to save snapshots and change configs he can create another environment.

@bmicklea
Copy link

bmicklea commented Nov 9, 2016

But can't we make the rule:

  1. If only the name is changed for the workspace / environment then we keep snapshots.
  2. If anything except the name is changed then we need to delete the snapshots and we show the popup.

@mmorhun
Copy link
Contributor Author

mmorhun commented Nov 9, 2016

@bmicklea workspace renaming doesn't touch snapshots.
As about environment: yes we can. Okay.

@bmicklea
Copy link

bmicklea commented Nov 9, 2016

Great, thanks

@TylerJewell
Copy link

Makes sense - I see @mmorhun you had a question for me, but it looks like @bmicklea and @tolusha answered it for you.

@mmorhun mmorhun added status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. and removed status/analyzing An issue has been proposed and it is currently being analyzed for effort and implementation approach labels Nov 10, 2016
@mmorhun mmorhun added status/in-progress This issue has been taken by an engineer and is under active development. and removed status/open-for-dev An issue has had its specification reviewed and confirmed. Waiting for an engineer to take it. labels Nov 22, 2016
@mmorhun mmorhun added status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. and removed status/in-progress This issue has been taken by an engineer and is under active development. labels Nov 29, 2016
@riuvshin riuvshin added this to the 5.0.0-M9 milestone Dec 19, 2016
@TylerJewell TylerJewell modified the milestones: 5.1.0, 5.0.0 Dec 22, 2016
@mmorhun
Copy link
Contributor Author

mmorhun commented Dec 26, 2016

Because of new spec and according to docs is not needed any more :( .

@mmorhun mmorhun closed this as completed Dec 26, 2016
@vkuznyetsov vkuznyetsov removed sprint/current status/code-review This issue has a pull request posted for it and is awaiting code review completion by the community. labels Jan 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Outline of a bug - must adhere to the bug report template.
Projects
None yet
Development

No branches or pull requests

6 participants