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

[ZTP] Improvements to allow in-band zero touch provisioning #39

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rajendra-dendukuri
Copy link
Collaborator

  • Download all plugins before processing each configuration sections. This ensures that configuration section plugins are available at all times. If the plugin of a configuration section needs to be downloaded at a later time when ZTP is in-progress and the configuration section is actually processed, set the field "pre-ztp-plugin-download" : false in the corresponding configuration section data in ztp.json.

  • Additional unit test cases added

@lgtm-com
Copy link

lgtm-com bot commented Sep 26, 2022

This pull request introduces 1 alert when merging c8a8eac into f7dd3c5 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@dgsudharsan
Copy link
Contributor

@liorghub Can you please review?


except Exception as e:
logger.debug('Exception [%s] encountered while verifying plugin for configuration section %s.' % (str(e), sec))
logger.info('Exception encountered while verifying plugin for configuration section %s. Marking it as FAILED.' % sec)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be logged as errors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed changed it to a logger.error.

@@ -191,6 +191,7 @@ def test_ztp_dynamic_url_invalid_arg_type(self, tmpdir):
"source": "/tmp/test_firmware_%s.json"
}
},
"pre-ztp-plugin-download": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since default for pre-ztp-plugin-download is defined as true I believe this need not be updated on all the test cases.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way test cases are written, default setting also needs to be set.

}
}
}"""
expected_result = """0001-test-plugin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add another case with ignore_result=true for section 0002?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a new test case test_ztp_json_invalid_plugins_ignore_result.

# Obtain a copy of the list of configuration sections
section_names = list(self.objztpJson.section_names)
abort = False
logger.debug('Verifying and downloading plugins user by configuration sections: %s' % ', '.join(section_names))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: user -> used

abort = True

except Exception as e:
logger.error('Exception [%s] encountered while verifying plugin for configuration section %s. Marking it as FAILED.' % (str(e), sec))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider change from verifying to downloading.

@@ -38,7 +38,7 @@ case $reason in
if inband_interface_check ${interface} ; then
if [ -n "$old_ip_address" ] && [ -n "$old_subnet_mask" ]; then
prefix=$(IPprefix_by_netmask "${old_subnet_mask}")
config interface ip remove ${interface} ${old_ip_address}${prefix}
/usr/local/bin/config interface ip remove ${interface} ${old_ip_address}${prefix}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of changing each line, you can add /usr/local/bin to PATH by adding the following line at start of file.
export PATH=/usr/local/bin:$PATH

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dhclient hooks are executed in the context of the dhclient process. I suggest we don't change the PATH variable unless it is really required.

 - Download all plugins before processing each configuration sections.
   This ensures that configuration section plugins are available at
   all times. If the plugin of a configuration section needs to be
   downloaded at a later time when ZTP is in-progress and the configuration
   section is actually processed, set the field
   "pre-ztp-plugin-download"  : false in the corresponding configuration
   section data in ztp.json.

 - Additional unit test cases added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants