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

hypervisor command to clean the files of a vm #26

Closed
flaviojs opened this issue Oct 6, 2013 · 9 comments
Closed

hypervisor command to clean the files of a vm #26

flaviojs opened this issue Oct 6, 2013 · 9 comments
Milestone

Comments

@flaviojs
Copy link
Contributor

flaviojs commented Oct 6, 2013

After making this post I realized that all the old files still remain after you rename a vm.
These files might interfere with a new vm that has the original name and the same platform, so there should be a way to clean them.

Hypervisor command:

vm clean_delete <instance_name>

Deletes the router instance and all the <platform>_<instance_name>_*/<platform_name>_i<instance_id>_* files in the working directory.

Prototype:

vm clean <instance_name>

Deletes all <platform>_<instance_name>_* files in the working directory.
The vm must exist and be stopped.

My concerns:

1. It can delete unrelated files with the same prefix - is this ok?

2. I considered not requiring the vm to exist by providing an optional argument [<platform>], but this raises security concerns since it can build any path for files that have at least one underscore. (ex: platform /tmp/ and instance name /../file produces the pattern /tmp/_/../file_*)
This can be minimized by traversing the working dir and comparing the filename prefixes, but it can still target any file that has two underscores.

What do you think @grossmj?

@flaviojs
Copy link
Contributor Author

flaviojs commented Oct 6, 2013

Another possibility:

extend the hypervisor command vm delete <instance_name> with an optional extra argument to indicate it should delete the files.

Yet another possibility:

add a setting to indicate the vm instance should delete the files when deleted


Hmm... they would require heavier internal modifications. But it could probably be adapted to make the prototype on the first post only clean it's own files...
So in the end the main question is: what sort of hypervisor command should be added?

@grossmj
Copy link
Member

grossmj commented Oct 6, 2013

Regarding your concerns, I think there is little chance to delete unrelated files, so this would be ok but best to delete files created ONLY by Dynamips. For your second concern, I wouldn't implement it as this could create other unexpected security issues but also because I think this is not gonna be useful.

vm delete <instance_name> with an optional option to clean it's own files appears to be the right solution. A setting when creating an instance would work too. Whatever is the easier to implement I would say. Something to think about: what happen if the vm crash?

I wonder if I would add vm clean <instance_name> too. This could be useful if for instance the user doesn't want to keep the vm files but doesn't want to delete the vm either. Can you think of a situation where this would be needed?

Finally, make sure files are moved to the new vm name when renaming a vm or all vm files created should include the vm id instead of its name, e.g. (platform_name)_(vm_id)_(nvram). Thus, no need to rename the files.

@flaviojs
Copy link
Contributor Author

flaviojs commented Oct 6, 2013

We can have the vm id in the names with the command line option -N 1. It will produce files in the format <platform_name>_i<instance_id>_<name>.


The easiest way to implement is to delete all <platform>_<instance_name>_* or <platform>_i<instance_id>_* files when the vm is deleted.

I investigated a bit more and the log and lock files remain open when the vm is stopped. Which means the current vm rename is bugged, since it keeps using files with the old name.

The other files are closed (and the filename forgotten) when the vm is stopped. Deleting them at that time is undesirable, since we would lose all changes just by rebooting with a stop+start... 😔

This means that doing vm clean while the instance exists wouldn't delete all the files, so it's better to have an optional argument in vm delete or create a new vm command like vm delete_clean <instance_name> or vm clean_delete <instance_name>.

...
I'm starting to think that having vm rename is a mistake, and it might be better to have vm create_copy <name> <instance_id> <other_name>... 💭


Regarding crashes:

  • if dynamips crashes - the files should remain there, since they might hold a clue as to the cause of the crash
  • if IOS crashes - this is normal behavior from the hardware perspective, the only difference is that the vm stopped by itself

In short, the current behavior is appropriate.

@grossmj
Copy link
Member

grossmj commented Oct 7, 2013

Actually the vm id in the names might be a problem if you have several hypervisors writing in the same working directory. How do you ensure uniqueness? Maybe by having an hypervisor client assigning an ID for each vm and be responsible to ensure they are unique? or is this going too far? ;)

So we agree you should have the option to delete vm files when deleting a vm, either using <platform>_<instance_name>_* or <platform>_i<instance_id>_*. Adding an optional argument to vm delete seems the best way to me.

Now the problem is for the rename command and the only solution I see is that we keep using the same files after we rename a vm. The files should therefore not contain the instance name which bring us back to the idea of assigning unique ids and use these ids to name the vm files... What do you think?

I don't think the rename is a bad idea, of course this brings problems but this is gonna be way cleaner to rename than deleting and recreating a vm from scratch.

@flaviojs
Copy link
Contributor Author

flaviojs commented Oct 7, 2013

Regarding the vm id uniqueness...
The instance_id is chosen by GNS3/Dynagen, not dynamips.
The lock file is there to prevent collisions.VM creation will fail if there is a lock file already being used with the same <platform>_<vm_name> or <platform>_i<instance_id> pattern.

Running in normal hypervisor mode or with the -N 0 option uses the vm name for the file names.
It's impossible to have two dynamips working in the same dir if they have routers with the same platform and name.

Running with the -N 1 option, instance_id is used in the file names.
Running in standalone mode (without hypervisor) enables it by default, and also generates NIO names with it.
You can just check which lock files exist before choosing the instance_id.


Regarding cleaning files...
I'm gonna opt for vm clean_delete <instance_name> since it has the same arguments as vm delete and it's existence can be tested easily with the info from hypervisor cmd_list vm.


Regarding vm rename...
Since the other files are left as is, I'm gonna make it create new lock/log files when the vm name is part of the file name, and leave the old ones as is.

@grossmj
Copy link
Member

grossmj commented Oct 8, 2013

You are right the vm id is chosen by Dynagen and sent along with thevm create command to Dynamips. So no problems on the uniqueness side :)


I didn't know about the -N argument, why not make it the default when running in hypervisor mode? Also, I will probably check for lock files in the new design, thanks for the tip :)


Regarding cleaning files, I agree to create vm clean_delete <instance_name>.


For vm rename, if I understand you won't need to create new lock/log files if the -N1 parameter is the default, right? This seems to be the simplest solution or am I missing something?

@flaviojs
Copy link
Contributor Author

flaviojs commented Oct 8, 2013

I'm still getting familiar with dynamips, so for now I'll keep -N behavior and focus on incremental changes/fixes. =~~

@grossmj
Copy link
Member

grossmj commented Feb 6, 2014

vm rename works well and regarding the file names, I am using the -N1 by default in GNS3 so I don't really need anything else.

@grossmj grossmj closed this as completed Feb 6, 2014
flaviojs added a commit that referenced this issue Feb 7, 2014
@flaviojs
Copy link
Contributor Author

flaviojs commented Feb 7, 2014

Only noticed you comment now... well, it's already done so I'll leave it there. =~~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants