difftodo
difftodo copied to clipboard
Published
20 hours ago •
jml
jml
Missing todo
Given
diff --git a/flocker/node/agents/blockdevice.py b/flocker/node/agents/blockdevice.py
index 540fcac..a97d000 100644
--- a/flocker/node/agents/blockdevice.py
+++ b/flocker/node/agents/blockdevice.py
@@ -1116,6 +1184,7 @@ class BlockDeviceDeployerLocalState(PClass):
These are the only parts of the state that need to be sent to the
control service.
"""
+ # XXX above untested
return (self.node_state, self.nonmanifest_datasets)
diff-todo gives 0 todos.
The problem is that the code snippet in the diff is fundamentally ambiguous.
Pygments is choosing to interpret the docstring as beginning on the third line, rather than ending. This means it thinks the comment is actually part of a docstring, which in turn means that difftodo never sees it.
The least bad fix is probably to run pygments on the whole file (if available), and then use the diff to figure out which comments to extract.
Hmmm. A work around would be to pass --function-context (-W) to git-diff, which will give the entire class or function that contains any change. Looking at how that is implemented, that would likely only break in cases where you have a class or top-level function definition in a docstring.
diff --git a/flocker/node/agents/blockdevice.py b/flocker/node/agents/blockdevice.py
index 540fcac..b5565f9 100644
--- a/flocker/node/agents/blockdevice.py
+++ b/flocker/node/agents/blockdevice.py
@@ -1078,175 +1078,191 @@ def get_blockdevice_volume(api, blockdevice_id):
@implementer(ILocalState)
class BlockDeviceDeployerLocalState(PClass):
"""
An ``ILocalState`` implementation for the ``BlockDeviceDeployer``.
- :ivar NodeState node_state: The current ``NodeState`` for this node.
+ :ivar unicode hostname: The IP address of the node that has this is the
+ state for.
+ :ivar UUID node_uuid: The UUID of the node that this is the state for.
+ :ivar datasets: The datasets discovered from this node.
:ivar NonManifestDatasets nonmanifest_datasets: The current
``NonManifestDatasets`` that this node is aware of but are not attached
to any node.
:ivar volumes: A ``PVector`` of ``BlockDeviceVolume`` instances for all
volumes in the cluster that this node is aware of.
"""
- node_state = field(type=NodeState, mandatory=True)
- nonmanifest_datasets = field(type=NonManifestDatasets, mandatory=True)
+ hostname = field(type=unicode, mandatory=True)
+ node_uuid = field(type=UUID, mandatory=True)
+ datasets = pmap_field(UUID, DiscoveredDataset)
+ # XXX This should go away in FLOC-3386
volumes = pvector_field(BlockDeviceVolume)
def shared_state_changes(self):
"""
Returns the NodeState and the NonManifestDatasets of the local state.
These are the only parts of the state that need to be sent to the
control service.
"""
- return (self.node_state, self.nonmanifest_datasets)
+ # XXX The structure of the shared state changes reflects the model
+ # currently used by the control service. However, that model doesn't
+ # seem to actually match what any consumer wants.
+ manifestations = {}
+ paths = {}
+ devices = {}
+ nonmanifest_datasets = {}
+ for dataset in self.datasets.values():
+ dataset_id = dataset.dataset_id
+ if dataset.state == DatasetStates.MOUNTED:
+ manifestations[unicode(dataset_id)] = Manifestation(
+ dataset=Dataset(
+ dataset_id=dataset_id,
+ maximum_size=dataset.maximum_size,
+ ),
+ primary=True,
+ )
+ paths[unicode(dataset_id)] = dataset.mount_point
+ elif dataset.state in (
+ DatasetStates.NON_MANIFEST, DatasetStates.ATTACHED,
+ ):
+ nonmanifest_datasets[unicode(dataset_id)] = Dataset(
+ dataset_id=dataset_id,
+ maximum_size=dataset.maximum_size,
+ )
+ if dataset.state in (
+ DatasetStates.MOUNTED, DatasetStates.ATTACHED,
+ ):
+ devices[dataset_id] = dataset.device_path
+
+ return (
+ NodeState(
+ uuid=self.node_uuid,
+ hostname=self.hostname,
+ manifestations=manifestations,
+ paths=paths,
+ devices=devices,
+ applications=None,
+ ),
+ NonManifestDatasets(
+ datasets=nonmanifest_datasets
+ ),
+ )
@implementer(IDeployer)
class BlockDeviceDeployer(PRecord):
"""
An ``IDeployer`` that operates on ``IBlockDeviceAPI`` providers.
:ivar unicode hostname: The IP address of the node that has this deployer.
+ :ivar UUID node_uuid: The UUID of the node that has this deployer.
:ivar IBlockDeviceAPI block_device_api: The block device API that will be
called upon to perform block device operations.
:ivar FilePath mountroot: The directory where block devices will be
mounted.
:ivar _async_block_device_api: An object to override the value of the
``async_block_device_api`` property. Used by tests. Should be
``None`` in real-world use.
"""
hostname = field(type=unicode, mandatory=True)
node_uuid = field(type=UUID, mandatory=True)
block_device_api = field(mandatory=True)
_async_block_device_api = field(mandatory=True, initial=None)
mountroot = field(type=FilePath, initial=FilePath(b"/flocker"))
poll_interval = timedelta(seconds=60.0)
- def _get_system_mounts(self, volumes, compute_instance_id):
- """
- Load information about mounted filesystems related to the given
- volumes.
-
- :param list volumes: The ``BlockDeviceVolumes`` known to exist. They
- may or may not be attached to this host. Only system mounts that
- related to these volumes will be returned.
-
- :param unicode compute_instance_id: This node's identifier.
-
- :return: A ``dict`` mapping mount points (directories represented using
- ``FilePath``) to dataset identifiers (as ``UUID``\ s) representing
- all of the mounts on this system that were discovered and related
- to ``volumes``.
- """
- partitions = psutil.disk_partitions()
- device_to_dataset_id = {
- self.block_device_api.get_device_path(volume.blockdevice_id):
- volume.dataset_id
- for volume
- in volumes
- if volume.attached_to == compute_instance_id
- }
- return {
- FilePath(partition.mountpoint):
- device_to_dataset_id[FilePath(partition.device)]
- for partition
- in partitions
- if FilePath(partition.device) in device_to_dataset_id
- }
-
- def discover_state(self, node_state):
+ def _discover_raw_state(self):
"""
- Find all block devices that are currently associated with this host and
- return a ``NodeState`` containing only ``Manifestation`` instances and
- their mount paths.
+ Find the state of this node that is relevant to determining which
+ datasets are on this node, and return a ``RawState`` containing that
+ information.
"""
# FLOC-1819 Make this asynchronous
api = self.block_device_api
compute_instance_id = api.compute_instance_id()
volumes = api.list_volumes()
- manifestations = {}
- nonmanifest = {}
+ system_mounts = get_system_mounts()
def is_existing_block_device(dataset_id, path):
if isinstance(path, FilePath) and path.isBlockDevice():
return True
INVALID_DEVICE_PATH(
dataset_id=dataset_id, invalid_value=path
).write(_logger)
return False
- # Find the devices for any manifestations on this node. Build up a
- # collection of non-manifest dataset as well. Anything that looks like
- # it could be a manifestation on this node but that has some kind of
- # inconsistent state is left out altogether.
+ # FIXME This should probably just be included in
+ # BlockDeviceVolume for attached volumes.
devices = {}
for volume in volumes:
dataset_id = volume.dataset_id
- u_dataset_id = unicode(dataset_id)
if volume.attached_to == compute_instance_id:
device_path = api.get_device_path(volume.blockdevice_id)
if is_existing_block_device(dataset_id, device_path):
devices[dataset_id] = device_path
- manifestations[u_dataset_id] = _manifestation_from_volume(
- volume
- )
- elif volume.attached_to is None:
- # XXX: Looks like we don't attempt to report the size
- # of non-manifest datasets.
- # Why not? The size is available from the volume.
- # https://clusterhq.atlassian.net/browse/FLOC-1983
- nonmanifest[u_dataset_id] = Dataset(dataset_id=dataset_id)
+ else:
+ # XXX We will detect this as NON_MANIFEST
- system_mounts = self._get_system_mounts(volumes, compute_instance_id)
+ pass
- paths = {}
- for manifestation in manifestations.values():
- dataset_id = manifestation.dataset.dataset_id
- mountpath = self._mountpath_for_manifestation(manifestation)
-
- # If the expected mount point doesn't actually have the device
- # mounted where we expected to find this manifestation, the
- # manifestation doesn't really exist here.
- properly_mounted = system_mounts.get(mountpath) == UUID(dataset_id)
-
- # In the future it would be nice to be able to represent
- # intermediate states (at least internally, if not exposed via the
- # API). This makes certain IStateChange implementations easier
- # (for example, we could know something is attached and has a
- # filesystem, so we can just mount it - instead of doing something
- # about those first two state changes like trying them and handling
- # failure or doing more system inspection to try to see what's up).
- # But ... the future.
-
- if properly_mounted:
- paths[dataset_id] = mountpath
+ return RawState(
+ compute_instance_id=compute_instance_id,
+ volumes=volumes,
+ devices=devices,
+ system_mounts=system_mounts,
+ )
+
+ def discover_state(self, node_state):
+ """
+ Find all datasets that are currently associated with this host and
+ return a ``BlockDeviceDeployerLocalState`` containing all the datasets
+ that are not manifest or are located on this node.
+ """
+ raw_state = self._discover_raw_state()
+
+ datasets = {}
+ for volume in raw_state.volumes:
+ dataset_id = volume.dataset_id
+ if dataset_id in raw_state.devices:
+ device_path = raw_state.devices[dataset_id]
+ mount_point = self._mountpath_for_dataset_id(
+ unicode(dataset_id)
+ )
+ if (
+ device_path in raw_state.system_mounts and
+ raw_state.system_mounts[device_path] == mount_point
+ ):
+ datasets[dataset_id] = DiscoveredDataset(
+ state=DatasetStates.MOUNTED,
+ dataset_id=dataset_id,
+ maximum_size=volume.size,
+ blockdevice_id=volume.blockdevice_id,
+ device_path=device_path,
+ mount_point=mount_point,
+ )
+ else:
+ datasets[dataset_id] = DiscoveredDataset(
+ state=DatasetStates.ATTACHED,
+ dataset_id=dataset_id,
+ maximum_size=volume.size,
+ blockdevice_id=volume.blockdevice_id,
+ device_path=device_path,
+ )
else:
- del manifestations[dataset_id]
- # FLOC-1806 Populate the Dataset's size information from the
- # volume object.
- # XXX: Here again, it we mark the dataset as
- # `nonmanifest`` unless it's actually mounted but we
- # don't attempt to report the size.
- # Why not? The size is available from the volume.
- # It seems like state reporting bug and separate from
- # (although blocking) FLOC-1806.
- # https://clusterhq.atlassian.net/browse/FLOC-1983
- nonmanifest[dataset_id] = Dataset(dataset_id=dataset_id)
+ if volume.attached_to is None:
+ datasets[dataset_id] = DiscoveredDataset(
+ state=DatasetStates.NON_MANIFEST,
+ dataset_id=dataset_id,
+ maximum_size=volume.size,
+ blockdevice_id=volume.blockdevice_id,
+ )
local_state = BlockDeviceDeployerLocalState(
- node_state=NodeState(
- uuid=self.node_uuid,
- hostname=self.hostname,
- manifestations=manifestations,
- paths=paths,
- devices=devices,
- # Discovering these is ApplicationNodeDeployer's job, we
- # don't know anything about these:
- applications=None,
- ),
- nonmanifest_datasets=NonManifestDatasets(datasets=nonmanifest),
- volumes=volumes,
+ node_uuid=self.node_uuid,
+ hostname=self.hostname,
+ datasets=datasets,
+ # XXX This should go away in FLOC-3386
+ volumes=raw_state.volumes,
)
return succeed(local_state)