Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

modules/nilrt_ip.py: Add EtherCAT Support #48617

Merged

Conversation

Projects
None yet
4 participants
@alexvasiu
Copy link
Contributor

commented Jul 17, 2018

  • modules/nilrt_ip.py: Add EtherCAT Support

The possibilities for adapter mode are: TCP/IP, EhterCAT and Disabled.

Signed-off-by: Alexandru Vasiu alexandru.vasiu@ni.com
Signed-off-by: Ovidiu-Adrian Vancea ovidiu.vancea@ni.com

  • modules/nilrt_ip.py: Fix configparser compatibility

read_file is available only since python 3.2 so readfp should
be used instead

Signed-off-by: Alexandru Vasiu alexandru.vasiu@ni.com

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from 1cd1b8b to 14dcec7 Jul 17, 2018

modules/nilrt_ip.py: Add EtherCAT Support
The possibilities for adapter mode are: TCP/IP, EhterCAT and
Disabled.

Signed-off-by: Alexandru Vasiu <alexandru.vasiu@ni.com>
Signed-off-by: Ovidiu-Adrian Vancea <ovidiu.vancea@ni.com>

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from 14dcec7 to 3e502c1 Jul 17, 2018

config_parser = configparser.RawConfigParser(dict_type=CaseInsensitiveDict)
config_parser.read_file(config_file)
mode = '' if not config_parser.has_option(interface, 'mode') else \
_remove_quotes(config_parser.get(interface, 'mode').lower())

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 17, 2018

Contributor

This seems to be an anti-pattern - doing an 'has_option' then a 'get()'. Can we use a default for the get() and do only that?

More than that, the first 5 lines seems to be a pattern that is repeated throughout the code. Can/Should we create a higher-level wrapper over the configparser to hide all this boilerplate code?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

get() throws an exception if the option doesn't exist

We can make a class or a global variable where the config will be read only one time, but the problem is that if we write something with nirtcfg then we should reload config from INI_FILE in configparser, so a method like this one will be ok:

def _get_option_configparser(section, option, default_value='', file=INI_FILE):
        with salt.utils.files.fopen(file, 'r') as config_file:
            config_parser = configparser.RawConfigParser(dict_type=CaseInsensitiveDict)
            config_parser.read_file(config_file)
            if config_parser.has_section(section) and config_parser.has_option(option):
                return config_parser.get(section, option)
            return default_value

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

There should be some option to pass defaults to the get, so you can avoid the has_option/get pair.

We shouldn't need a class, but only a function that can take a list of keys and returns a dict with keys/values

if initial_mode == 'ethercat':
__salt__['system.set_reboot_required_witnessed']()
else:
disable(interface)

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 17, 2018

Contributor

there seem to be a lot of disable(interface) followed by enable(interface). Should we create a 'restart' or something?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

Yep 👍

'''
if __grains__['lsb_distrib_id'] == 'nilrt':
initial_mode = _get_adaptermode_info(interface)
_set_adapter_mode(interface, 'EtherCAT')

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 17, 2018

Contributor

why you spell 'EtherCAT' here, but do this in other places: if initial_mode == 'ethercat':

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

that is for network settings to understand the mode is EtherCAT but when I parse INI_FILE, I user CaseInsensitiveDict so that will transform EtherCAT in ethercat

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

how about creating a constant and use that everywhere?

I find confusing to see both 'EtherCAT' and 'ethercat' as values.

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

I will make a constant:

NIRTCFG_ETHERCAT = 'EtherCAT'

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from 3e502c1 to b3d626d Jul 18, 2018


# some versions of nirtcfg don't set the dhcpenabled/linklocalenabled variables
# when selecting "DHCP or Link Local" from MAX, so return it by default to avoid
# having the requestmode "None" because none of the conditions above matched.
return 'dhcp_linklocal'


def _get_adapter_mode_info(interface):

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

can this benefit from the newly created function? - _get_config_options

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

Forgot about that function ... I'll change it

},
'hwaddr': interface.hwaddr[:-1]
}
with salt.utils.files.fopen(INI_FILE, 'r') as config_file:

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

Is this used anywhere?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

I will delete that

},
'hwaddr': interface.hwaddr[:-1]
}
with salt.utils.files.fopen(INI_FILE, 'r') as config_file:
config_parser = configparser.RawConfigParser(dict_type=CaseInsensitiveDict)

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

shouldn't this use the _persist_config method? And have that method fixed to use the config parser?

should we rename the _persist_config and _get_config_options to match i.e. (save/load or serialize/deserialize or something that makes sense).

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

Maybe _save_config for _persist_config and _load_config for _get_config_options

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

That makes sense

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from b3d626d to e0f8d6a Jul 18, 2018

'''
return adaptermode for given interface
'''
mode = _load_config(interface, ['mode'])['mode']

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

I didn't put default_value = 'tcpip' because the file may be corrupted and mode should be one of these values:

  • disabled
  • ethercat
  • tcpip (default value)
'''
return adaptermode for given interface
'''
mode = _load_config(interface, ['mode'])['mode']

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

this will raise if 'mode' is not in the file.

Should you go with:
mode = _load_config(interface, ['mode']).get('mode') ?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

If mode is not in file, _load_config will return the default_value for it:

mode = {
    'mode': ''
}

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

Ah, forgot about that.

if out['retcode'] != 0:
msg = 'Couldn\'t disable interface {0}. Error: {1}'.format(interface, out['stderr'])
raise salt.exceptions.CommandExecutionError(msg)
initial_mode = _get_adapter_mode_info(interface)

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

up/down code is almost identical. Can you create a worker to share the logic?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 18, 2018

Author Contributor

A function called _change_state(state) and state can be up or down ?

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 18, 2018

Contributor

Looks good to me!

alexvasiu added some commits Apr 13, 2018

modules/nilrt_ip.py: Add EtherCAT Support
The possibilities for adapter mode are: TCP/IP, EhterCAT and
Disabled.

Signed-off-by: Alexandru Vasiu <alexandru.vasiu@ni.com>
Signed-off-by: Ovidiu-Adrian Vancea <ovidiu.vancea@ni.com>
modules/nilrt_ip.py: Fix configparser compatibility
read_file is available only since python 3.2 so readfp should
be used instead

Signed-off-by: Alexandru Vasiu <alexandru.vasiu@ni.com>

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from e0f8d6a to 73f533e Jul 18, 2018

@rares-pop

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2018

Things look good to me.

@gtmanfred can you take a look on these? They are something we'd need for Fluorine.

@gtmanfred
Copy link
Contributor

left a comment

blind-review

@gtmanfred gtmanfred added the Fluorine label Jul 19, 2018

@rares-pop
Copy link
Contributor

left a comment

There is still a small cleanup to be made. I'm fine doing it in a different review since the functionality is correct as is.

@@ -404,43 +495,81 @@ def get_interfaces_details():
return {'interfaces': list(map(_get_info, _interfaces))}


def up(interface, iface_type=None): # pylint: disable=invalid-name,unused-argument
def _set_adapter_mode(interface, mode):

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 20, 2018

Contributor

I overlooked this before - but this shouldn't be needed. We should use _save_config() directly instead of this

@@ -504,7 +621,7 @@ def disable(interface):
return down(interface)


def _persist_config(section, token, value):
def _save_config(section, token, value):

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 20, 2018

Contributor

I thought this was supposed to use the configparser too. Did we change our mind?

This comment has been minimized.

Copy link
@alexvasiu

alexvasiu Jul 23, 2018

Author Contributor

for reading, we use config parser and for writing nirtcfg (is also used by the system) ... we should test if the file will be corrupted if we are writing at the same time with nirtcfg and config parser

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 23, 2018

Contributor

Okay. It looks good!

if __grains__['lsb_distrib_id'] == 'nilrt':
initial_mode = _get_adapter_mode_info(interface)
_set_adapter_mode(interface, NIRTCFG_ETHERCAT)
if __salt__['cmd.run_all']('{0} --set section={1},token=MasterID,value={2}'.

This comment has been minimized.

Copy link
@rares-pop

rares-pop Jul 20, 2018

Contributor

why not _save_config()?

@rallytime
Copy link
Contributor

left a comment

I just have a couple of small requests.

@@ -258,45 +259,102 @@ def _remove_quotes(value):
return value


def _get_requestmode_info(interface):
def _load_config(section, options, default_value='', file=INI_FILE):

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 23, 2018

Contributor

Can we use a variable name other than file here? It's a reserved word and can cause problems.

@@ -258,45 +259,102 @@ def _remove_quotes(value):
return value


def _get_requestmode_info(interface):
def _load_config(section, options, default_value='', file=INI_FILE):
"""

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 23, 2018

Contributor

I know this is picky, but can you use single quotes for docstrings? That's how we like them in Salt. :)

'''
Enable the specified interface
def _change_state(interface, new_state):
"""

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 23, 2018

Contributor

Single quotes here, too.



def _restart(interface):
"""

This comment has been minimized.

Copy link
@rallytime

rallytime Jul 23, 2018

Contributor

And here. :)

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from 20210cb to 7cd5543 Jul 24, 2018

modules/nilrt_ip.py: Clean-Up
Signed-off-by: Alexandru Vasiu <alexandru.vasiu@ni.com>

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from 7cd5543 to a429ef8 Jul 25, 2018

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

@rares-pop How does this look to you now?

@rares-pop

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

@rallytime - the code looks good and works correctly.

In the near future we'll replace the _save_config to use the ConfigParser as opposed to a binary that's saving the things in the ini.. It's fine to leave it in a separate commit since it's not critical by any means and we have to check that the ConfigParser is 'thread-safe' in terms of not corrupting the ini file when called multiple times from multiple processes.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2018

@alexvasiu There's one lint error that needs to be fixed here: https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48617/10/

modules/nilrt_ip.py: Fix argument for function _load_config
Signed-off-by: Alexandru Vasiu <alexandru.vasiu@ni.com>

@alexvasiu alexvasiu force-pushed the alexvasiu:dev/avasiu/add_ethercat_support branch from e54d761 to 354e664 Jul 27, 2018

@alexvasiu

This comment has been minimized.

Copy link
Contributor Author

commented Jul 27, 2018

@rallytime Fixed

UPDATE: Lint failed because of salt.modules.config

@rallytime rallytime merged commit 73ec7fb into saltstack:develop Jul 27, 2018

3 of 9 checks passed

continuous-integration/jenkins/pr-merge This commit cannot be built
Details
codeclimate 8 issues to fix
Details
jenkins/pr/lint The lint job has failed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has failed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has failed
Details
jenkins/pr/py2-centos-7 running py2-centos-7...
Details
WIP ready for review
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.