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

underlay: do not set link unmanaged by NetworkManager during provider network configuration #2754

Merged
merged 1 commit into from
May 5, 2023

Conversation

zbb88888
Copy link
Collaborator

@zbb88888 zbb88888 commented May 5, 2023

What type of this PR

Examples of user facing changes:

  • Bug fixes

Which issue(s) this PR fixes:

Fixes #(issue-number)

WHAT

🤖 Generated by Copilot at 03b11a1

Improve network configuration logging and error handling. Add log messages to init.go and avoid name swapping conflicts in ovs_linux.go.

🤖 Generated by Copilot at 03b11a1

Sing, O Muse, of the cunning code that swapped the names
Of ovs bridge and provider nic, to link them in the frame
But when the bridge was made before the nic was there
The code, with skill, avoided the redundant swap with care

HOW

🤖 Generated by Copilot at 03b11a1

  • Add log messages to indicate provider nic configuration on OVS bridge (link, link)
  • Avoid unnecessary name swapping between OVS bridge and provider nic (link)

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

  • Priority 1: In the InitOVSBridges() function, there is a missing error check after calling configProviderNic(). If an error occurs in this function, it will be returned to the caller without being handled. This could lead to unexpected behavior or crashes. It is recommended to add an error check and handle any errors that occur.
  • Priority 2: In the ovsInitProviderNetwork() function, there is a missing error check after calling configProviderNic(). If an error occurs in this function, it will be returned to the caller without being handled. This could lead to unexpected behavior or crashes. It is recommended to add an error check and handle any errors that occur.
  • Priority 3: In the configProviderNic() function, there is a redundant check for the prefix "br-" in the if statement. Since the function is only called with bridge names that start with "br-", this check is unnecessary and can be removed to simplify the code.
  • Priority 4: In the configProviderNic() function, there is a missing error check after calling nmSetManaged(). If an error occurs in this function, it will be logged but not handled. It is recommended to add an error check and handle any errors that occur.
  • Priority 5: In the configProviderNic() function, the error message returned by fmt.Errorf() does not provide enough context to diagnose the error. It is recommended to include more information about the specific error that occurred to aid in debugging.

@zbb88888 zbb88888 marked this pull request as ready for review May 5, 2023 04:30
@zbb88888 zbb88888 requested a review from zhangzujian May 5, 2023 04:30
@zbb88888 zbb88888 marked this pull request as draft May 5, 2023 04:36
@zbb88888
Copy link
Collaborator Author

zbb88888 commented May 5, 2023

image

@zbb88888 zbb88888 marked this pull request as ready for review May 5, 2023 04:41
@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2023

  • The removal of the nmSetManaged function call in configProviderNic might cause issues with NetworkManager. It should be investigated if this function is still necessary and if so, it should be added back.
  • There are inconsistent logging messages in InitOVSBridges and ovsInitProviderNetwork. They should be standardized to follow a consistent format.
  • The fmt.Errorf calls in InitOVSBridges and ovsInitProviderNetwork should include more specific error messages to aid in debugging.
  • The exchangeLinkName and macLearningFlag parameters in ovsInitProviderNetwork are not used and should be removed.
  • The configProviderNic function could benefit from additional comments to explain its purpose and functionality.

@zbb88888
Copy link
Collaborator Author

zbb88888 commented May 5, 2023

需要回合到1.11

@zhangzujian zhangzujian added bug Something isn't working need backport labels May 5, 2023
@zhangzujian zhangzujian changed the title nm not managed only in the swap provide nic name with ovs br case underlay: do not set link unmanaged by NetworkManager during provider network configuration May 5, 2023
@zbb88888 zbb88888 merged commit c77f368 into kubeovn:master May 5, 2023
@zbb88888 zbb88888 deleted the nm1 branch May 5, 2023 07:03
zhangzujian added a commit to zhangzujian/kube-ovn that referenced this pull request Jun 15, 2023
zhangzujian added a commit to zhangzujian/kube-ovn that referenced this pull request Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working need backport
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants