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

Separate decoding/updating in update_eeprom_db #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

zzhiyuan
Copy link
Contributor

@zzhiyuan zzhiyuan commented Jul 8, 2019

I think it would be best for the syseeprom daemon to publish the
information to the database. However, for now I want to separate the
decoding and the publishing, so that platform plugins can simply
implement get_eeprom_dict and not worry about publishing. Please let me
know why or why not the publishing is not handled in the daemon instead.

Also created symlink for sonic_eeprom that points to the exact same
files in sonic_platform_base/sonic_eeprom.

@jleveque
Copy link
Contributor

jleveque commented Jul 9, 2019

@kevinwangsk / @keboliu: Can you please help review?

Copy link
Collaborator

@keboliu keboliu left a comment

Choose a reason for hiding this comment

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

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

  2. Thanks for creating the symbol link, I have planned to do it but not yet.

  3. would like to see a whole solution including a PR to the daemon parts.

@zzhiyuan
Copy link
Contributor Author

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

Does this change break platform initialization? It will break "decode-syseeprom --init" because that will call update_eeprom_db, but in sonic_device_util it does not call it with --init. Additionally, might want to consider removing that option with the existence of syseepromd. I've put in a default dict return for eeprom_base.get_eeprom_dict so "decode-syseeprom --init" will not fail, let me know if you'd like this behavior changed.

  1. Thanks for creating the symbol link, I have planned to do it but not yet.
  2. would like to see a whole solution including a PR to the daemon parts.

I think for the daemon, all that will need to change is call get_eeprom_dict from the plugin, and update the database with the result. I think this is a fundamentally better implementation than having the platform plugins interact with the database. Then after that is implemented, update_eeprom_db can be removed from platform plugin. I'm not sure if I want to take on all of that yet, but this change will allow platform vendors to implement "get_eeprom_dict" without worrying about the database.

@keboliu
Copy link
Collaborator

keboliu commented Jul 11, 2019

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

Does this change break platform initialization? It will break "decode-syseeprom --init" because that will call update_eeprom_db, but in sonic_device_util it does not call it with --init. Additionally, might want to consider removing that option with the existence of syseepromd. I've put in a default dict return for eeprom_base.get_eeprom_dict so "decode-syseeprom --init" will not fail, let me know if you'd like this behavior changed.

I am ok as long as "decode-syseeprom -m" not broken and "--init" option be safely removed.

  1. Thanks for creating the symbol link, I have planned to do it but not yet.
  2. would like to see a whole solution including a PR to the daemon parts.

I think for the daemon, all that will need to change is call get_eeprom_dict from the plugin, and update the database with the result. I think this is a fundamentally better implementation than having the platform plugins interact with the database. Then after that is implemented, update_eeprom_db can be removed from platform plugin. I'm not sure if I want to take on all of that yet, but this change will allow platform vendors to implement "get_eeprom_dict" without worrying about the database.

IMO if only submit this PR, the daemon will be broken until someone offers a fix, this is what we don't want to see.

I think it would be best for the syseeprom daemon to publish the
information to the database. However, for now I want to separate the
decoding and the publishing, so that platform plugins can simply
implement get_eeprom_dict and not worry about publishing. Please let me
know why or why not the publishing is not handled in the daemon instead.

Also created symlink for sonic_eeprom that points to the exact same
files in sonic_platform_base/sonic_eeprom.
@zzhiyuan
Copy link
Contributor Author

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

Does this change break platform initialization? It will break "decode-syseeprom --init" because that will call update_eeprom_db, but in sonic_device_util it does not call it with --init. Additionally, might want to consider removing that option with the existence of syseepromd. I've put in a default dict return for eeprom_base.get_eeprom_dict so "decode-syseeprom --init" will not fail, let me know if you'd like this behavior changed.

I am ok as long as "decode-syseeprom -m" not broken and "--init" option be safely removed.

  1. Thanks for creating the symbol link, I have planned to do it but not yet.
  2. would like to see a whole solution including a PR to the daemon parts.

I think for the daemon, all that will need to change is call get_eeprom_dict from the plugin, and update the database with the result. I think this is a fundamentally better implementation than having the platform plugins interact with the database. Then after that is implemented, update_eeprom_db can be removed from platform plugin. I'm not sure if I want to take on all of that yet, but this change will allow platform vendors to implement "get_eeprom_dict" without worrying about the database.

IMO if only submit this PR, the daemon will be broken until someone offers a fix, this is what we don't want to see.

With this change, "decode-syseeprom -m" is not broken, and neither is the daemon. The logic for eeprom_tlvinfo has not changed, and eeprom_base now returns a default dict to be put into the database.

@keboliu
Copy link
Collaborator

keboliu commented Jul 16, 2019

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

Does this change break platform initialization? It will break "decode-syseeprom --init" because that will call update_eeprom_db, but in sonic_device_util it does not call it with --init. Additionally, might want to consider removing that option with the existence of syseepromd. I've put in a default dict return for eeprom_base.get_eeprom_dict so "decode-syseeprom --init" will not fail, let me know if you'd like this behavior changed.

I am ok as long as "decode-syseeprom -m" not broken and "--init" option be safely removed.

  1. Thanks for creating the symbol link, I have planned to do it but not yet.
  2. would like to see a whole solution including a PR to the daemon parts.

I think for the daemon, all that will need to change is call get_eeprom_dict from the plugin, and update the database with the result. I think this is a fundamentally better implementation than having the platform plugins interact with the database. Then after that is implemented, update_eeprom_db can be removed from platform plugin. I'm not sure if I want to take on all of that yet, but this change will allow platform vendors to implement "get_eeprom_dict" without worrying about the database.

IMO if only submit this PR, the daemon will be broken until someone offers a fix, this is what we don't want to see.

With this change, "decode-syseeprom -m" is not broken, and neither is the daemon. The logic for eeprom_tlvinfo has not changed, and eeprom_base now returns a default dict to be put into the database.

I can approve this change ONLY if all "decode-syseeprom" command are all OK, including "-s","-m", "--init", if any one of them be broken, we should either have fix here or in the "decode-syseeprom" scripts. I didn't test the "decode-syseeprom" scripts with your change, so would like to get your confirmation here.

@zzhiyuan
Copy link
Contributor Author

I don't oppose to refactor syseeprom daemon and the plugin, but some point needs to highlight:

  1. please be noted that sonic-utility scripts "decode-syseeprom" also rely on this eeprom plugin, and on some platform it very critical, because during the first-time reboot need to use this script to decode the system eeprom and get the switch base mac, please refer to https://github.com/Azure/sonic-utilities/blob/master/scripts/decode-syseeprom and https://github.com/Azure/sonic-buildimage/blob/master/src/sonic-config-engine/sonic_device_util.py#L68. Which we definitely don't want to break.

Does this change break platform initialization? It will break "decode-syseeprom --init" because that will call update_eeprom_db, but in sonic_device_util it does not call it with --init. Additionally, might want to consider removing that option with the existence of syseepromd. I've put in a default dict return for eeprom_base.get_eeprom_dict so "decode-syseeprom --init" will not fail, let me know if you'd like this behavior changed.

I am ok as long as "decode-syseeprom -m" not broken and "--init" option be safely removed.

  1. Thanks for creating the symbol link, I have planned to do it but not yet.
  2. would like to see a whole solution including a PR to the daemon parts.

I think for the daemon, all that will need to change is call get_eeprom_dict from the plugin, and update the database with the result. I think this is a fundamentally better implementation than having the platform plugins interact with the database. Then after that is implemented, update_eeprom_db can be removed from platform plugin. I'm not sure if I want to take on all of that yet, but this change will allow platform vendors to implement "get_eeprom_dict" without worrying about the database.

IMO if only submit this PR, the daemon will be broken until someone offers a fix, this is what we don't want to see.

With this change, "decode-syseeprom -m" is not broken, and neither is the daemon. The logic for eeprom_tlvinfo has not changed, and eeprom_base now returns a default dict to be put into the database.

I can approve this change ONLY if all "decode-syseeprom" command are all OK, including "-s","-m", "--init", if any one of them be broken, we should either have fix here or in the "decode-syseeprom" scripts. I didn't test the "decode-syseeprom" scripts with your change, so would like to get your confirmation here.

It should not break any of decode-syseeprom functions. However I have only tested on Arista platforms, I think it would be worthwhile for you to confirm that this is fine in ONIE platforms, but from looking at the changes there should not be any behavioral differences.

@jleveque jleveque force-pushed the master branch 2 times, most recently from affe6be to c31636e Compare February 10, 2021 22:57
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-platform-common that referenced this pull request Oct 25, 2024
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.

3 participants