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

sonic_sku_create.py util for generating a new sku #547

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

Conversation

sschlafman
Copy link

- What I did
Created a utility for generating a new SKU based on a base sku (-b) and a sku definition file (-f)
- How I did it
Copied the base sku folder and modified port_config.ini based on the sku definition
- How to verify it
Tested on Mellanox platform.
- Previous command output (if the output of a command-line utility has changed)

- New command output (if the output of a command-line utility has changed)

-->

Utility for creating a new SKU on existing image. SKU definition is provided in an xml file
Added base (-b) flag to support base sku instead of extracting it from xml.
In addition updated with support to the new xml format dfinition
Swapped Speed and Alias columns.
In addition added support for L2 mode
@stcheng
Copy link
Contributor

stcheng commented Jun 4, 2019

retest this please

parser.add_argument('-b', '--base', action='store', help='SKU base definition ', default=None)
parser.add_argument('-r', '--remove', action='store_true', help='Remove SKU folder')
parser.add_argument('-c', '--cmd', action='store', choices=['new_sku_only', 'l2_mode_only', 'new_sku_l2'], help='Choose action to preform (Generate a new SKU, Configure L2 mode, Both', default="new_sku_only")
parser.add_argument('-l2_sku_name', '--l2_sku_name', action='store', help='SKU name to be used in the L2 configuration mode', default=None)
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 5, 2019

Choose a reason for hiding this comment

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

l2_sku_name [](start = 23, length = 11)

In concept, SKU is independent with L2 mode. So there is no l2_sku_name.

We already have feature to config any SKU into L2 mode https://github.com/Azure/SONiC/wiki/L2-Switch-mode#3-generate-a-configuration-for-l2-switch-mode. If your feature is the same, let's keep original method, and remove feature duplication here. #Closed

Copy link
Author

Choose a reason for hiding this comment

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

The l2_mode option in the script is just an implementation of the process described ( [https://github.com/Azure/SONiC/wiki/L2-Switch-mode#3-generate-a-configuration-for-l2-switch-mode.]).
Instead of manually doing it step by step, the script is doing everything.
Why do you want to remove it?

Copy link
Contributor

@qiluo-msft qiluo-msft left a comment

Choose a reason for hiding this comment

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

As comments

@@ -0,0 +1,329 @@
#! /usr/bin/python
"""
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 5, 2019

Choose a reason for hiding this comment

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

Move this script into /script directory?
Add it into setup.py?
Add a unit test?

Copy link
Author

Choose a reason for hiding this comment

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

Moved into scripts directory and added it to setup.py

Fixed PR comments and added few improvments
Moved script to scripts folder
Removed SKU def file from commit
added sonic_sku_create to scripts
try:
platform = subprocess.check_output("sonic-cfggen -H -v DEVICE_METADATA.localhost.platform",shell=True) #self.metadata['platform']
self.platform = platform.rstrip()
except KeyError:
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 22, 2019

Choose a reason for hiding this comment

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

except KeyError [](start = 2, length = 15)

If this is critical, we should exit. #Closed

Fixed L2 mode configuration in case no l2_sku_name is provided
Fixed PR comments an Error handling
-m (minigraph) and -f (file) are mutually exclusive options to create a new SKU
"""
usage: sonic_sku_create.py [-h] [-v] [-f FILE | -m MINIGRAPH] [-b BASE] [-r]
[-c {new_sku_only,l2_mode_only,new_sku_l2}]
[-s SKU] [-p] [-vv]
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 30, 2019

Choose a reason for hiding this comment

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

s [](start = 29, length = 1)

Not your fault. Let's make it -k to be consistent with https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic-cfggen #Closed

self.platform = platform.rstrip()
except KeyError:
print ("Couldn't find platform info in CONFIG_DB DEVICE_METADATA", file=sys.stderr)
exit()
Copy link
Contributor

@qiluo-msft qiluo-msft Jul 30, 2019

Choose a reason for hiding this comment

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

exit [](start = 3, length = 4)

In exception cases, exit with non-zero error codes. Multiple times in this script. #Closed

changed -s flag to -k
fixed exit code to non-zero
added a check to ensure user can't remove the base/default SKU
Removed the usage of "Index" information from xml sku definition file and minigraph as this is not a reilable field.
@liat-grozovik
Copy link
Collaborator

retest this please

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request introduces 6 alerts when merging 503585a into d03d151 - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Unused import
  • 1 for Except block handles 'BaseException'
  • 1 for Redundant assignment

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request introduces 6 alerts when merging a500e7e into b37135c - view on LGTM.com

new alerts:

  • 2 for Unused local variable
  • 2 for Unused import
  • 1 for Except block handles 'BaseException'
  • 1 for Redundant assignment

@lgtm-com
Copy link

lgtm-com bot commented May 28, 2020

This pull request introduces 1 alert when merging 4a261df into b37135c - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'

@qiluo-msft
Copy link
Contributor

Please fix remaining LGTM error, and add unit test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants