Check if jail is running before destroying it and if, don't attempt t…

[Corvan]

I stumbled over the issue, that I tried to destroy a running Jail and did not realize that the error is due to it still running, so this Pull Request is a suggestion to deal with that.

• Explain the feature
• Read CONTRIBUTING.md

[skarekrow]

Hi thanks for the PR!

I have a few issues with it, but before I review it properly, what issue did you stumble over? iocage should have stopped the jail and then removed it.

root@freenas[~]# /root/iocage/iocage start foobar
* Starting foobar
  + Started OK
  + Using devfs_ruleset: 5
  + Starting services OK
  + Executing poststart OK
root@freenas[~]# /root/iocage/iocage destroy foobar

This will destroy jail foobar

Are you sure? [y/N]: y
Stopping foobar
Destroying foobar

[Corvan]

I tried to destroy a jail which had (nullfs) mounts. I have to admit that we are not on 1.0 yet. The destruction failed with the message that zfs do datasets could not be destroyed l, but not that the jail was still running, so I thought I'd suggest the output of a hint that it is still running.
If you have issues with the code or with the approach, feel free to do with the PR as you like, I won't be disappointed. But maybe it might be good to tell the person trying to destroy it, that it is actually still running, before destroying it.

[skarekrow]

@pmhausen You haven't used 1.0 yet? :O Well 1.0/1.1 prompt that they are stopping the jail (including those with nullfs mounts) and also destroys as shown in my example.

Would you prefer it doesn't destroy the jail and instead tells you the jail is running and exits? We could perhaps change the behavior that happens in my example to only occur with the force flag.

[pmhausen]

Hi Brandon,

Am 29.01.2019 um 22:57 schrieb Brandon Schneider ***@***.***>: @pmhausen You haven't used 1.0 yet? :O Well 1.0/1.1 prompt that they are stopping the jail (including those with nullfs mounts) and also destroys as shown in my example. Would you prefer it doesn't destroy the jail and instead tells you the jail is running and exits? We could perhaps change the behavior that happens in my example to only occur with the force flag.

wrong person - I don’t have a clue what you are talking about. Please contact Lars, who sent the pull request. Kind regards Patrick — punkt.de GmbH Internet - Dienstleistungen - Beratung Kaiserallee 13a Tel.: 0721 9109-0 Fax: -100 76133 Karlsruhe info@punkt.de http://punkt.de AG Mannheim 108285 Gf: Juergen Egeling

[Corvan]

@skarekrow We are in the transition phase of going to 1.0/1.1 but in this case it was not 1.0 yet. I for myself would prefer being notified that the jail is still running and not have the jail destroyed. But if you have another opinion on that, I am not against the way it is now. Then I'd like to suggest "just" to get an additional warning.

[skarekrow]

This will have some performance implications, I think we should handle this in iocage_lib/iocage.py instead to avoid those pitfalls

[skarekrow]

@pmhausen Sorry I saw punkt.de and I assumed you were also involved with this :P

@Corvan I think that's a good idea, I left a request changes so that we can tackle that in the lib instead. I think an exception that the jail is running unless force is supplied is reasonable default behavior.

Thanks for the PR!

[Corvan]

Looks good to me

[Corvan]

@skarekrow thanks for considering my idea

[skarekrow]

@Corvan No problem, I'm fine if you want to tackle that still in this PR! After all, it was your idea :)

[Corvan]

What can I do to finish it off?

[skarekrow]

Well the changes I suggested should be addressed first, and then you can check for the force flag in the lib and refuse to destroy if that's the case. Instantiating the iocage lib for each of those could be slightly costly, so I think we should move this into the lib in general, help get more logic out of the cli

[Corvan]

OK, I'll try. Maybe I might have some more questions, so be prepared ;-)

[skarekrow]

Sounds good!

On Sat, Feb 2, 2019 at 6:13 AM Lars Liedtke ***@***.***> wrote: OK, I'll try. Maybe I might have some more questions, so be prepared ;-) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#831 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABg_NyGAZkPr4rQw74TeLQ4gkcC-Hgg_ks5vJYECgaJpZM4aVcZk> .

-- -Brandon

[Corvan]

So, After some hassle setting up a development environment in a VM, I moved the stuff into the lib (see https://github.com/Corvan/iocage/tree/features/destroy), But I have got a problem with the release, because destroying a release with --recursive is not using the destroy_jail-method. I could not find a way to check in the destroy_release-method to check if a jail is still running. Am I right here, or did I miss something?

[skarekrow]

@Corvan Correct, in this case we should check if there are any dependents in destroy_release and stop them there or just call destroy_jail for each and then finally end by calling destroy_release.

[Corvan]

If it would be feasible to call destroy_jail from destroy_release if there are dependent jails, I would go this way. I just was not sure if there is a reason why destroy_jail is not called.

[skarekrow]

@Corvan That would be fine too, my memory says I didn't do that to avoid unnecessary calls/exceptions since most of the time RELEASEs are just being destroyed bare. If a user forces a release destruction, it makes sense that jails are forcefully torn down instead of stopping them in my book.

But since we're adding the rest of what you suggested, it makes sense to have it do the right thing there as well.

[Corvan]

I'll look into it and will try to do it in a nice way, but this might take some days until my head is free enough again to work on this

[skarekrow]

No rush :) Appreciate the PR

[pmhausen]

Hey guys,

minor remark: similarly to this, iocage should IMHO not silently start a jail that is offline when I issue iocage console foo ...
Suggestion: error message "jail is down, use -f to force-start" or similar.

Kind regards,
Patrick

[skarekrow]

@pmhausen Can you open a ticket for that and exec. Perhaps it shouldn't. My line of thinking was if you're asking iocage to console into a jail, it must start. Otherwise, not much a console ;)

[Corvan]

So, now I got to work on this. I am aware that this way might not be the most performant but it was the least intrusive. So I'd be interested, what you think about it.
I tried to run the tests locally but failed to get pytest running in my FreeBSD-Box, but this might be due to me not being familiar with pytest; I use unittest normally.
So when you approve i will look into writing specific tests for this case

[skarekrow]

Thanks!

[Corvan]

Thank you!