Code review comment for lp:~raharper/curtin/trunk.passthrough-netconfig

On Wed, May 31, 2017 at 4:11 PM, Scott Moser <email address hidden> wrote:

> well, first few comments
> i didn't get very far. sorry.
>
>
> Diff comments:
>
> > === modified file 'curtin/__init__.py'
> > --- curtin/__init__.py 2017-03-21 16:41:33 +0000
> > +++ curtin/__init__.py 2017-05-31 00:24:53 +0000
> > @@ -37,6 +37,8 @@
> > 'SUBCOMMAND_SYSTEM_UPGRADE',
> > # supports new format of apt configuration
> > 'APT_CONFIG_V1',
> > + # supports passing network config to target
> > + 'NETWORK_PASSTHROUGH',
>
> Why would someone need to know this ?
> i think it is related to our plan on implementing images that have builtin
> hooks but dont want to use them.
> that is worth an explanation in the commit message.
>

It's worth comment and *documentation*; will add. I think we might also
want to allow caller to
control whether we render eni or passthrough; ie, MaaS may want to *force*
an eni rendering
even if passthrough is possible.

>
> > # has version module
> > 'HAS_VERSION_MODULE',
> > ]
> >
> > === modified file 'curtin/block/__init__.py'
> > --- curtin/block/__init__.py 2017-05-19 18:52:21 +0000
> > +++ curtin/block/__init__.py 2017-05-31 00:24:53 +0000
> > @@ -978,4 +978,71 @@
> > else:
> > raise ValueError("wipe mode %s not supported" % mode)
> >
> > +
> > +def storage_config_required_packages(storage_config, mapping):
> > + """Read storage configuration dictionary and determine
> > + which packages are required for the supplied configuration
> > + to function. Return a list of packaged to install.
> > + """
> > +
> > + if not storage_config or not isinstance(storage_config, dict):
> > + raise ValueError('Invalid storage configuration. '
> > + 'Must be a dict:\n %s' % storage_config)
> > +
> > + if not mapping or not isinstance(mapping, dict):
> > + raise ValueError('Invalid storage mapping. Must be a dict')
> > +
> > + if 'storage' in storage_config:
> > + storage_config = storage_config.get('storage')
> > +
> > + needed_packages = []
> > +
> > + # get reqs by device operation type
> > + dev_configs = set(operation['type']
> > + for operation in storage_config['config'])
> > +
> > + for dev_type in dev_configs:
> > + if dev_type in mapping:
> > + needed_packages.extend(mapping[dev_type])
> > +
> > + # for any format operations, check the fstype and
> > + # determine if we need any mkfs tools as well.
> > + format_configs = set([operation['fstype']
> > + for operation in storage_config['config']
> > + if operation['type'] == 'format'])
> > + for format_type in format_configs:
> > + if format_type in mapping:
> > + needed_packages.extend(mapping[format_type])
> > +
> > + return needed_packages
> > +
> > +
> > +def detect_required_packages_mapping():
> > + """Return a dictionary providing a versioned configuration which
> maps
> > + storage configuration elements to the packages which are required
> > + for functionality.
> > +
> > + The mapping key is either a config type value, or an fstype
> value.
>
> how does this relate to curtin/deps/__init__.py
> i think we'd like to have just one list.
>

I'll check there as well; there should be just one.

>
> > +
> > + """
> > + version = 1
> > + mapping = {
> > + version: {
> > + 'handler': storage_config_required_packages,
> > + 'mapping': {
> > + 'bcache': ['bcache-tools'],
> > + 'btrfs': ['btrfs-tools'],
> > + 'ext2': ['e2fsprogs'],
> > + 'ext3': ['e2fsprogs'],
> > + 'ext4': ['e2fsprogs'],
> > + 'lvm_partition': ['lvm2'],
> > + 'lvm_volgroup': ['lvm2'],
> > + 'raid': ['mdadm'],
> > + 'xfs': ['xfsprogs']
> > + },
> > + },
> > + }
> > + return mapping
> > +
> > +
> > # vi: ts=4 expandtab syntax=python
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.
> passthrough-netconfig/+merge/324801
> You are the owner of lp:~raharper/curtin/trunk.passthrough-netconfig.
>