Documentation #5
Conversation
libiocage/lib/Jail.py
Outdated
| @@ -18,6 +18,49 @@ | |||
|
|
|||
|
|
|||
| class Jail: | |||
| """ | |||
| Iocage unit orchestrates a jail's configuration and manages state | |||
There was a problem hiding this comment.
Perhaps say iocage Jail unit? Remember iocage is always lowercase, it's our name. That goes for all over.
libiocage/lib/Jail.py
Outdated
| """ | ||
| Iocage unit orchestrates a jail's configuration and manages state | ||
|
|
||
| Jails are represented as zfs dataset `zpool/iocage/jails/<NAME>` |
libiocage/lib/Jail.py
Outdated
| Directory Structure: | ||
|
|
||
| zpool/iocage/jails/<NAME>: The jail's dataset containing it's | ||
| configuration and root directory. Iocage legacy used to store |
There was a problem hiding this comment.
lowercase iocage, we can drop and root directory. It's not a directory, but a dataset. And isn't terribly important in this description. I think we should drop the JSON being preferred, and make the language stronger. It's the only supported mechanism. There are a lot of new things and properties, and this will help ease the migrations to it if we only support that mechanism.
libiocage/lib/Jail.py
Outdated
| a jails configuration as ZFS properties on this dataset. Even | ||
| though the modern JSON config mechanism is preferred. | ||
|
|
||
| zpool/iocage/jails/<NAME>/root: This directory is the dataset |
There was a problem hiding this comment.
This can just be said as This dataset is used as the jail's root when starting the jail. Perhaps instead of origin we say a clone's root or release's root dataset are normally the sources
libiocage/lib/Jail.py
Outdated
| the jail to be a JSON-style jail and ignores other configuration | ||
| mechanisms | ||
|
|
||
| zpool/iocage/jails/<NAME>/config: Another supported configuration |
There was a problem hiding this comment.
Drop the word supported, it isn't supported, but is able to be read. We need to offer a cutoff, we don't want to juggle supporting 3 different configuration methods.
libiocage/lib/Jail.py
Outdated
| NullFS Basejail: The fastest method to spawn a basejail by mounting | ||
| read-only directories from the release's root dataset by creating | ||
| a snapshot of the release on each boot of the jail. When a release | ||
| was updated, the jail is updated as well on the next reboot. This |
There was a problem hiding this comment.
When a release is updated not was. We should clarify that the jails userland will be updated, but not the configuration in /etc. The user would have to check that.
| @@ -63,25 +63,28 @@ def read(self): | |||
| libiocage.lib.JailConfigJSON.JailConfigJSON.read(self) | |||
| self["legacy"] = False | |||
| self.logger.log("Configuration loaded from JSON", level="verbose") | |||
| return | |||
| return "json" | |||
There was a problem hiding this comment.
is this one of those instances where we should be returning a tuple value rather than a plain string?
There was a problem hiding this comment.
We should introduce tuples in another PR. Good idea.
libiocage/lib/Jail.py
Outdated
| release_name (string): | ||
| The jail is created from the release matching the name provided | ||
|
|
||
| auto_download (bool): (default=False) |
There was a problem hiding this comment.
auto_download seems to be unused.
There was a problem hiding this comment.
Good catch, haha! This can be implemented on CLI side, so no need to expose an interface to automatically fetch the release here. Thanks!
| if not self.running: | ||
| raise libiocage.lib.errors.JailNotRunning( | ||
| jail=self, | ||
| logger=self.logger | ||
| ) | ||
|
|
||
| def update_jail_state(self): | ||
| """ | ||
| Invoke update of the jail state from jls output | ||
| """ | ||
| try: | ||
| stdout = subprocess.check_output([ | ||
| "/usr/sbin/jls", |
There was a problem hiding this comment.
consiser using --libxo=json here, for easier (no) parsing.
There was a problem hiding this comment.
💛! But this will go into a separate PR.
| def name(self): | ||
| """ | ||
| The name (formerly UUID) of the Jail | ||
| """ | ||
| return self.config["id"] |
There was a problem hiding this comment.
how does the setter work here?
(i.e.: how do we add validation for this?)
There was a problem hiding this comment.
This is write-only here. Just an abstraction of a jails given identifier. I'm sure we can improve validation in JailConfig.py
| @@ -601,7 +797,14 @@ def __getattr__(self, key): | |||
|
|
|||
| raise AttributeError(f"Jail property {key} not found") | |||
|
|
|||
| def getattr_str(self, key): | |||
| def getstring(self, key): | |||
There was a problem hiding this comment.
why is this a public function rather than a __str__ on the properties?
or would that be too convoluted?
There was a problem hiding this comment.
That would require to wrap all types returned to return a - in case the original value is None.
There was a problem hiding this comment.
i wonder if that's an improvement in any regard, esp for downstream users who consume our output automatically.
There was a problem hiding this comment.
No it's not. When you don't intent to print the outputs, you don't want to get the strings. Some objects, like for example the JailConfigAddresses, are a custom data structure you want to have access to.
| @@ -72,7 +72,7 @@ def exec(command, logger=None, ignore_error=False): | |||
| if child.returncode > 0: | |||
|
|
|||
| if logger: | |||
| log_level = "spam" if ignore_error else "warning" | |||
| log_level = "spam" if ignore_error else "warn" | |||
There was a problem hiding this comment.
starting to think our logger levels should also be tuples.
This pull-request summarizes the documentation of public and private API classes/methods.