[poc] [rfc] item zfs_dataset: tidy up previously changed properties #686
Conversation
CroneKorkN
changed the title
CroneKorkN
force-pushed
the
zfs_tidy_up_properties
branch
2 times, most recently
from
2f41765
to
57a9a28
Compare
CroneKorkN
force-pushed
the
zfs_tidy_up_properties
branch
from
57a9a28
to
a885255
Compare
| parent_directory = '/'.join(self.__item_properties.get('mountpoint', '').split('/')[0:-1]) | ||
| if ( | ||
| item.ITEM_TYPE_NAME == "zfs_dataset" and | ||
| item.attributes.get('mountpoint') == parent_directory or |
There was a problem hiding this comment.
This now won't work if we have datasets which get mounted to /foo and /foo/bar/baz. The old mechanism did take that into account.
There was a problem hiding this comment.
Only depending on the direct parent was a reaction to the comment:
# parent only.
But i see the descriptancy between "largest parent item" and "parent directory".
| ), | ||
| may_fail=True, | ||
| ) | ||
| return f"<ZFSDataset name:{self.name} {' '.join(f'{k}:{v}' for k,v in self.__item_properties.items())}>" |
There was a problem hiding this comment.
__repr__() should not need to run anything on the node. Calling __item_properties() will try to get the properties from the node itself.
| """ | ||
| BUNDLE_ATTRIBUTE_NAME = "zfs_datasets" | ||
| REJECT_UNKNOWN_ATTRIBUTES = False | ||
| ITEM_TYPE_NAME = "zfs_dataset" | ||
| # for defaults which should be different from 'zfs inherit -S' behaviour | ||
| PROPERTY_DEFAULTS = { | ||
| 'mountpoint': 'none', |
There was a problem hiding this comment.
Don't we want to default to None here?
There was a problem hiding this comment.
None would result in the zfs default value, which is mounting a dataset like foo/bar/baz to /foo/bar/baz.
| item.attributes.get('mountpoint') and | ||
| self.attributes['mountpoint'].startswith(item.attributes['mountpoint']) | ||
| # add dependency to parent dataset | ||
| needs.add(item.id) |
There was a problem hiding this comment.
We should also check wether the parent dataset does exist (like we already do for pool items). Otherwise the creation will fail.
|
The details aside, my biggest concern is that i generally only found kind of ugly ways to implement this. |
|
Revisiting this: Is it important to show the changed properties in the If we don't need to have the properties in the cdict, you could do something like this: diff --git a/bundlewrap/items/zfs_dataset.py b/bundlewrap/items/zfs_dataset.py
index e4d8f5be..429d2a60 100644
--- a/bundlewrap/items/zfs_dataset.py
+++ b/bundlewrap/items/zfs_dataset.py
@@ -41,13 +41,6 @@ class ZFSDataset(Item):
)
return status_result.return_code == 0
- def __get_option(self, path, option):
- cmd = "zfs get -Hp -o value {} {}".format(quote(option), quote(path))
- # We always expect this to succeed since we don't call this function
- # if we have already established that the dataset does not exist.
- status_result = self.run(cmd)
- return status_result.stdout.decode('utf-8').strip()
-
def __set_option(self, path, option, value):
if option == 'mounted':
# 'mounted' is a read-only property that can not be altered by
@@ -120,7 +113,9 @@ class ZFSDataset(Item):
return None
sdict = {}
- for option in self.attributes:
- sdict[option] = self.__get_option(self.name, option)
- sdict['mounted'] = self.__get_option(self.name, 'mounted')
+ options = self.run(f'zfs get all {quote(self.name)} -H -p -o property,value,source')
+ for line in options.stdout.decode('UTF-8').splitlines():
+ prop, value, source = line.strip().split('\t', 2)
+ if prop in self.attributes or source == 'local' or prop in ('mounted',):
+ sdict[prop] = value
return sdict |
Zfs_dataset properties can orphan easily, because previously changed values are currently not considered by the item, unless explicitly set. We could address that by looking for changed properties via
zfs get -o local.