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

Fix for GCU function modification for HEAD PR #2993

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

Conversation

sabakram
Copy link
Contributor

What I did

This PR is created to resolve submodule HEAD PR KVM t0 Elastic failure issues. KVM Elastic test cases are failing due to missing mode attribute in configuration. When a rollback checks an exisitng patch it showns error:
"All keys are not parsed in PORT".
"dict_keys(['Ethernet4'])"
"exceptionList: [" 'mode' "]

This PR Updated GCU function so that it can be compatible with HEAD PR and passed. This proposed change is a temporary solution to pass failing testcases and it will be reverted back once in hand issue is resolved and sub-modules are added in HEAD.

How I did it

Modified generic_updater.py. Added a logic in def replace function such that it will not fail in rollback. In this way it will pass KVM Elastic (GCU_Dhcp_relay.py test)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 21, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@sabakram sabakram changed the title Mode removal from config replacer to fix HEAD PR issue Fix for GCU function modification for HEAD PR Sep 21, 2023
@sabakram sabakram marked this pull request as ready for review September 21, 2023 15:34
@ridahanif96
Copy link

@qiluo-msft , can you please help review and merge this PR.

@ridahanif96
Copy link

@gechiang can you please help review this PR. Thanks!

@gechiang
Copy link
Contributor

@yejianquan , I think you were helping @ridahanif96 on this submodule head PR failure. Please check if this is what would "fix" the issue?
Also tagged @qiluo-msft to help review and what this PR is trying to do is acceptable...

@ridahanif96
Copy link

@yejianquan , I think you were helping @ridahanif96 on this submodule head PR failure. Please check if this is what would "fix" the issue?
Also tagged @qiluo-msft to help review and what this PR is trying to do is acceptable...

@gechiang thanks!

@@ -122,6 +125,14 @@ def replace(self, target_config):

self.logger.log_notice("Config replacement completed.")

@staticmethod
def switchport_mode_remove(config):

Choose a reason for hiding this comment

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

Please add some comments for the background reason for removing the config['PORT'][port]['mode']

Choose a reason for hiding this comment

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

Added as per suggestion, thanks

if 'PORT' in config:
for port, port_data in config['PORT'].items():
if 'mode' in port_data:
del config['PORT'][port]['mode']

Choose a reason for hiding this comment

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

Please extract constants for string 'mode' and 'PORT'

Choose a reason for hiding this comment

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

updated as suggested,thanks

@yejianquan
Copy link

yejianquan commented Sep 27, 2023

@yejianquan , I think you were helping @ridahanif96 on this submodule head PR failure. Please check if this is what would "fix" the issue? Also tagged @qiluo-msft to help review and what this PR is trying to do is acceptable...

@gechiang , In sonic-mgmt repo, Rida had made the compatible change, but I'm still not sure whether this PR could resolve the advancing issue totally since there's implicitly data format things...

I left some comments on the code style side, still need @qiluo-msft to help review for make sure whether 'what this PR is trying to do is acceptable'.

@ridahanif96
Copy link

@yejianquan , I think you were helping @ridahanif96 on this submodule head PR failure. Please check if this is what would "fix" the issue? Also tagged @qiluo-msft to help review and what this PR is trying to do is acceptable...

@gechiang , In sonic-mgmt repo, Rida had made the compatible change, but I'm still not sure whether this PR could resolve the advancing issue totally since there's implicitly data format things...

I left some comments on the code style side, still need @qiluo-msft to help review for make sure whether 'what this PR is trying to do is acceptable'.

@yejianquan all of your suggestions are incorportated,thanks. @qiluo-msft can you please help in review this PR, so we can resolve HEAD PR issue, thanks.

@ridahanif96
Copy link

@qiluo-msft , can you please help review this PR? Its a time sensitive issue for HEAD PR.

@qiluo-msft
Copy link
Contributor

The root issue is PORT/mode is not yang modelled. I see open PR sonic-net/sonic-buildimage#13580.

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.

Block this PR until yang model is avaiable.

@qiluo-msft qiluo-msft requested a review from yxieca October 6, 2023 00:40
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.

None yet

5 participants