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

[Nokia][sonic-platform] Update Nokia sonic-platform submodule #15239

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

snider-nokia
Copy link
Contributor

@snider-nokia snider-nokia commented May 26, 2023

This fixes https://github.com/Nokia-ION/ndk/issues/16.

Why I did it

  1. To support dynamic swapping of module types/speeds (400G/100G/40G)
  2. To optimize CMIS ZR optics operation

How I did it

Reinitialize xcvr_api at module removal/insertion time, and also optimize cache for ZR optics.

How to verify it

  1. Verify that different (supported) module types can be dynamically swapped (removed/inserted) and that each is properly provisioned by Xcvrd and has its EEPROM information accurately reported in Redis DB (using "show transceiver eeprom") as well as "sfputil show eeprom" direct access.

  2. Also verify that Xcvrd initialization and operation with 400G CMIS ZR optics is both efficient and functional.
    ** edit 6/14/23: pushed enhanced caching (full memory map) support and elimination of base class APIs override.

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

Description for the changelog

Cache tuning for ZR optics and add support for dynamic module type/speed swapping.

@snider-nokia snider-nokia requested a review from lguohan as a code owner May 26, 2023 18:39
@snider-nokia
Copy link
Contributor Author

@prgeor and/or @mihirpat1, can you review? Thanks.

This PR fixes both issues described at https://github.com/Nokia-ION/ndk/issues/16

@prgeor
Copy link
Contributor

prgeor commented Jun 8, 2023

@snider-nokia looks like the Cache design in your platform seems not right as the platform sfp.py needs to override the base class APIs (and future APIs) to work with Cache. Why doesn't the cache implementation work seamlessly for any API?

@snider-nokia
Copy link
Contributor Author

@snider-nokia looks like the Cache design in your platform seems not right as the platform sfp.py needs to override the base class APIs (and future APIs) to work with Cache. Why doesn't the cache implementation work seamlessly for any API?

This model was used originally to more closely monitor/examine (and tune for) what the underlying optoe bulk driver methods were/are doing. If you'd prefer, the overriding of these methods can be removed.

The cache design should (and does) indeed work properly/seamlessly for any API at present (for any page that is currently cacheable).

I can also add the balance of the specification mapped page space even prior to those pages being referenced by Xcvrd...

@prgeor
Copy link
Contributor

prgeor commented Jun 14, 2023

@snider-nokia looks like the Cache design in your platform seems not right as the platform sfp.py needs to override the base class APIs (and future APIs) to work with Cache. Why doesn't the cache implementation work seamlessly for any API?

This model was used originally to more closely monitor/examine (and tune for) what the underlying optoe bulk driver methods were/are doing. If you'd prefer, the overriding of these methods can be removed.

The cache design should (and does) indeed work properly/seamlessly for any API at present (for any page that is currently cacheable).

I can also add the balance of the specification mapped page space even prior to those pages being referenced by Xcvrd...

@snider-nokia can we avoid overriding the base class APIs:-

get_transceiver_info()
get_transceiver_info()
get_transceiver_threshold_info()
get_transceiver_pm()

@snider-nokia
Copy link
Contributor Author

snider-nokia commented Jun 14, 2023

All set, Prince. Tested and ready for merge.

**Note: These new changes require NDK version 22.9.10. Both this PR and associated NDK PR should be merged to 202205 at the same time. Same paradigm should be applied for merge to 202211 (both at the same time).
Associated NDK (version 22.9.10) PR is here: Azure/sonic-buildimage-msft#44

@gechiang
Copy link
Collaborator

**Note: These new changes require NDK version 22.9.10. Both this PR and associated NDK PR should be merged to 202205 at the same time. Same paradigm should be applied for merge to 202211 (both at the same time).
Associated NDK (version 22.9.10) PR is here

@snider-nokia , Can you clarify if this dependency can cause existing functionality issue?
Please note that the NDK version 22.9.10 is only in the MSFT repo and not in public master nor in public 202205. Therefore the concern is if there is a dependency, would function be broken if one is to pick up this PR in any public branch without the NDK 22.9.10??

@snider-nokia
Copy link
Contributor Author

snider-nokia commented Jun 23, 2023

**Note: These new changes require NDK version 22.9.10. Both this PR and associated NDK PR should be merged to 202205 at the same time. Same paradigm should be applied for merge to 202211 (both at the same time).
Associated NDK (version 22.9.10) PR is here

@snider-nokia , Can you clarify if this dependency can cause existing functionality issue? Please note that the NDK version 22.9.10 is only in the MSFT repo and not in public master nor in public 202205. Therefore the concern is if there is a dependency, would function be broken if one is to pick up this PR in any public branch without the NDK 22.9.10??

Yes @gechiang, you cannot use one without the other. The PMON changes contained in this PR must be coupled with NDK version 22.9.10 (PR#44) wherever (to whatever branch) either of the referenced PRs are merged. You will indeed see a functionality issue if both PRs are not merged/built together.

@gechiang
Copy link
Collaborator

**Note: These new changes require NDK version 22.9.10. Both this PR and associated NDK PR should be merged to 202205 at the same time. Same paradigm should be applied for merge to 202211 (both at the same time).
Associated NDK (version 22.9.10) PR is here

@snider-nokia , Can you clarify if this dependency can cause existing functionality issue? Please note that the NDK version 22.9.10 is only in the MSFT repo and not in public master nor in public 202205. Therefore the concern is if there is a dependency, would function be broken if one is to pick up this PR in any public branch without the NDK 22.9.10??

Yes @gechiang, you cannot use one without the other. The PMON changes contained in this PR must be coupled with NDK version 22.9.10 (PR#44) wherever (to whatever branch) either of the referenced PRs are merged. You will indeed see a functionality issue if both PRs are not merged/built together.

@snider-nokia , Since we cannot have these separated... I would suggest that we first move this PR to the sonic-buildimage-msft and not attempt to merge into public branches... We should then think about how to make these "dependent PRs" available in a way that they can be both applied in the public community branches...

@mlok-nokia
Copy link
Contributor

@gechiang Per design agreement between MSFT and Nokia, public Master doesn't include NDK module. It means the master image is not applicable to be running on Nokia IXR7250E platform. This PR is Nokia specific sonic-platform submodule. It won't impact another vendor. It is safe to merge to the master branch, then cherry-pick to the 202205. This is the method which both MSFT and Nokia agreed with and has been used since the beginning. Thanks.

@prgeor
Copy link
Contributor

prgeor commented Jun 29, 2023

@lguohan @yxieca please help merge

@yxieca yxieca merged commit aa46167 into sonic-net:master Jun 29, 2023
@yxieca
Copy link
Contributor

yxieca commented Jun 29, 2023

@mlok-nokia can you confirm that this change has been tested on 202205 branch?

@gechiang
Copy link
Collaborator

gechiang commented Jul 3, 2023

@gechiang Per design agreement between MSFT and Nokia, public Master doesn't include NDK module. It means the master image is not applicable to be running on Nokia IXR7250E platform. This PR is Nokia specific sonic-platform submodule. It won't impact another vendor. It is safe to merge to the master branch, then cherry-pick to the 202205. This is the method which both MSFT and Nokia agreed with and has been used since the beginning. Thanks.

Got it! Thanks for your clarification. Since this PR is only applicable to IXR7250E platform, it will only impact this platform at which whoever needs to run on this platform should be building from the msft repo which has the NDK module.

@gechiang
Copy link
Collaborator

gechiang commented Jul 4, 2023

MSFT ADO: 24442945

@mssonicbld
Copy link
Collaborator

@snider-nokia PR conflicts with 202211 branch

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Jul 4, 2023
…net#15239)

Why I did it
To support dynamic swapping of module types/speeds (400G/100G/40G)
To optimize CMIS ZR optics operation
How I did it
Reinitialize xcvr_api at module removal/insertion time, and also optimize cache for ZR optics.

How to verify it
Verify that different (supported) module types can be dynamically swapped (removed/inserted) and that each is properly provisioned by Xcvrd and has its EEPROM information accurately reported in Redis DB (using "show transceiver eeprom") as well as "sfputil show eeprom" direct access.

Also verify that Xcvrd initialization and operation with 400G CMIS ZR optics is both efficient and functional.
** edit 6/14/23: pushed enhanced caching (full memory map) support and elimination of base class APIs override.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202205: #15710

mssonicbld pushed a commit to mssonicbld/sonic-buildimage that referenced this pull request Jul 17, 2023
…net#15239)

Why I did it
To support dynamic swapping of module types/speeds (400G/100G/40G)
To optimize CMIS ZR optics operation
How I did it
Reinitialize xcvr_api at module removal/insertion time, and also optimize cache for ZR optics.

How to verify it
Verify that different (supported) module types can be dynamically swapped (removed/inserted) and that each is properly provisioned by Xcvrd and has its EEPROM information accurately reported in Redis DB (using "show transceiver eeprom") as well as "sfputil show eeprom" direct access.

Also verify that Xcvrd initialization and operation with 400G CMIS ZR optics is both efficient and functional.
** edit 6/14/23: pushed enhanced caching (full memory map) support and elimination of base class APIs override.
@mssonicbld
Copy link
Collaborator

Cherry-pick PR to 202305: #15873

@gechiang
Copy link
Collaborator

gechiang commented Sep 1, 2023

@snider-nokia , can you address the cherry-pick conflict for this PR in the 202211 branch?
Please raise a clone of this PR with conflict resolved directly under the 202211 branch.
Thanks!

@snider-nokia
Copy link
Contributor Author

snider-nokia commented Sep 1, 2023

@snider-nokia , can you address the cherry-pick conflict for this PR in the 202211 branch? Please raise a clone of this PR with conflict resolved directly under the 202211 branch. Thanks!

Sorry @gechiang, it looks like I checked the 202211 inclusion box inadvertently. We have not provided supported for that branch since it was forked. I have checked internally and verified this answer, so I have now unchecked the 202211 inclusion box. Thanks.

@gechiang
Copy link
Collaborator

gechiang commented Sep 1, 2023

@snider-nokia , Thanks for responding, I will go ahead remove the "Request for 202211 branch" label for this PR.
Does this means any Nokia Chassis support PRs will only be supported in 202205 and 202305 going forward and will not be needed in 202211? In general, whenever we backport a fix, we request it be backported to all the previous "higher" releases. In this case if backport to 202205 was needed, therefore we automatically request to back port to 202211 and 202305.

@snider-nokia
Copy link
Contributor Author

I will raise this issue for internal discussion here, @gechiang. I don't think we have a current plan to support every branch that would qualify for backport (based on your description), but will verify at this end.

sonic-otn pushed a commit to sonic-otn/sonic-buildimage that referenced this pull request Sep 20, 2023
…net#15239)

Why I did it
To support dynamic swapping of module types/speeds (400G/100G/40G)
To optimize CMIS ZR optics operation
How I did it
Reinitialize xcvr_api at module removal/insertion time, and also optimize cache for ZR optics.

How to verify it
Verify that different (supported) module types can be dynamically swapped (removed/inserted) and that each is properly provisioned by Xcvrd and has its EEPROM information accurately reported in Redis DB (using "show transceiver eeprom") as well as "sfputil show eeprom" direct access.

Also verify that Xcvrd initialization and operation with 400G CMIS ZR optics is both efficient and functional.
** edit 6/14/23: pushed enhanced caching (full memory map) support and elimination of base class APIs override.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

9 participants