[bug] proxy: do not add to queue multi times when scaling CN

[volgariver6]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

issue #16357 https://github.com/matrixorigin/MO-Cloud/issues/3323

What this PR does / why we need it:

refactor the code that do not allow the same tunnel in the queue
before the tunnel finished tranferring.
reset transferred back to false together with wg.Done.

[matrix-meow]

@volgariver6 Thanks for your contributions!

Here are review comments for file pkg/proxy/rebalancer.go:

Pull Request Review:

Title and Body:

The title and body of the pull request clearly indicate that this is a bug fix related to preventing the same tunnel from being added multiple times to the queue when scaling CN. The PR also mentions that it involves code refactoring. The issue number is referenced along with a brief explanation of the changes made. The body provides a clear context for the changes being made.

Changes in pkg/proxy/rebalancer.go:

1. Removed sync.Mutex and inflight map: The sync.Mutex and inflight map have been removed from the rebalancer struct. This indicates a significant change in how tunnels are managed in the rebalancer.

2. Introduced tunnelDeliver interface: A new TunnelDeliver interface has been introduced to deliver tunnels to the queue. This is a positive change as it abstracts the delivery mechanism and can potentially improve code readability and maintainability.

3. Refactored tunnel delivery logic: The logic for adding tunnels to the queue has been refactored. Instead of directly manipulating the inflight map and checking for duplicates, the Deliver method of tunnelDeliver is now used to add tunnels to the queue. This simplifies the logic and makes it more straightforward.

4. Changes in rebalanceByHash function: The rebalanceByHash function no longer locks and unlocks the mu mutex. This change is appropriate since the mutex is no longer used for managing inflight tunnels. The logic for adding tunnels to the queue has been updated to use the Deliver method of tunnelDeliver.

5. Changes in handleTransfer function: The code that removed tunnels from the inflight map after transfer has been removed. This is consistent with the removal of the inflight map and simplifies the logic in the handleTransfer function.

Suggestions for Improvement:

1. Optimize Tunnel Delivery: Consider further optimizing the Deliver method in the tunnelDeliver interface to handle edge cases efficiently and ensure robust error handling.

2. Unit Testing: Ensure comprehensive unit tests are in place to cover the refactored code and verify that the changes function as expected, especially in scenarios involving concurrent access to the queue.

3. Documentation: Update relevant documentation or comments to reflect the changes made, especially regarding the new tunnelDeliver interface and its usage.

4. Code Review: Conduct a thorough code review to ensure that all edge cases, error handling, and potential race conditions are appropriately addressed in the refactored code.

5. Security Considerations: While the changes seem focused on functionality and code structure, it's essential to consider any security implications of the refactored code, especially in a concurrent environment.

By addressing these suggestions, the codebase can be further improved in terms of readability, maintainability, and robustness.

Here are review comments for file pkg/proxy/scaling.go:

Pull Request Review:

Title: [bug] proxy: do not add to queue multi times when scaling CN

Body:

• The PR is categorized as a bug fix and code refactoring.
• It references and fixes issue [Bug]: rolling-update CN behind proxy cause prepared stmt lost #16357 on GitHub.
• The purpose of the PR is to refactor the code to prevent adding the same tunnel multiple times to the queue before the tunnel finishes transferring.

Changes in pkg/proxy/scaling.go:

1. Code Refactoring:

   • The queue channel is replaced with tunnelDeliver of type TunnelDeliver to deliver tunnels to the queue.
   • The tunnelDeliver field is added to the scaling struct.

2. Function newScaling:

   • The queue parameter is replaced with tunnelDeliver parameter.
   • The tunnelDeliver field is initialized with the provided TunnelDeliver parameter.

3. Function doScaling:

   • The loop that iterates over tuns and adds tunnels to the queue is refactored.
   • The tun.setTransferType(transferByScaling) and queue handling logic is replaced with s.tunnelDeliver.Deliver(t, transferByScaling).

Feedback and Suggestions:

1. Code Optimization:

   • The refactoring of the code to use tunnelDeliver instead of directly manipulating the queue is a good improvement.
   • Consider renaming tunnelDeliver to something more descriptive like tunnelQueueHandler for clarity.

2. Error Handling:

   • Ensure that error handling is in place for the Deliver method in case the delivery fails.

3. Consistency:

   • Ensure consistent naming conventions are followed throughout the codebase for variables and functions.

4. Documentation:

   • Add comments or update existing comments to reflect the changes made in the code for better code understanding.

5. Testing:

   • Verify that the changes do not introduce any regressions by running relevant tests.

6. Security Concern:

   • Ensure that there are no security vulnerabilities introduced by the changes, especially related to the handling of tunnels or queues.

7. Performance:

   • Consider evaluating the performance impact of the new approach compared to the previous queue handling method.

8. Review Dependency Injection:

   • If TunnelDeliver is a new dependency, ensure it is injected properly to maintain code modularity and testability.

By addressing the above points, the code quality and maintainability can be improved, ensuring a more robust and efficient system.

Here are review comments for file pkg/proxy/tunnel.go:

Pull Request Review:

Title and Body:

The title and body of the pull request indicate that it is addressing a bug related to adding the same tunnel multiple times to a queue when scaling CN. The PR aims to refactor the code to prevent this issue and reset the transferred status back to false when necessary. The issue number #16357 is referenced, providing context for the changes made.

Changes in pkg/proxy/tunnel.go:

1. Redundant Code Removal:

   • The removal of the t.mu.inTransfer check in the canStartTransfer function is appropriate as it was redundant and not needed after the refactoring.
   • The removal of setting t.mu.inTransfer = true simplifies the logic and avoids unnecessary state management.

2. Logic Improvement:

   • In the kickoff function, the removal of p.transferred = false when certain conditions are met seems problematic. This change could potentially lead to incorrect behavior if p.transferred is relied upon elsewhere in the code. It's important to ensure that the logic for setting p.transferred is correct and consistent.

3. Code Clarity:

   • The addition of p.transferred = false in the handleTransferIntent function improves code clarity by explicitly resetting the transferred status back to false after the transfer operation is completed.

Suggestions for Improvement:

1. Review kickoff Function:

   • Revisit the logic in the kickoff function to ensure that removing p.transferred = false does not introduce any unintended side effects. If p.transferred is crucial for other parts of the codebase, consider revising the logic to maintain its integrity.

2. Consistency in State Management:

   • Ensure that the state management of p.transferred is consistent throughout the codebase. If it is a critical variable for tracking transfer status, make sure its handling is clear and accurate in all relevant functions.

3. Testing:

   • After making these changes, it is essential to conduct thorough testing to validate that the refactored code behaves as expected and does not introduce new bugs or issues.

4. Documentation:

   • Consider updating the code comments or documentation to reflect the changes made, especially regarding the handling of p.transferred, to aid future developers in understanding the codebase.

Overall, the changes in the pull request address the identified bug and refactor the code for better efficiency. However, it is crucial to ensure that the modifications do not introduce new issues and that the logic for managing the p.transferred variable is accurate and consistent.

Here are review comments for file pkg/proxy/tunnel_deliver.go:

Pull Request Review:

Title:

The title of the pull request indicates that it is addressing a bug related to the proxy functionality when scaling CN. This provides a clear indication of the purpose of the changes.

Body:

The body of the pull request specifies that it is a bug fix related to not adding the same tunnel multiple times to the queue when scaling CN. It also references the related issue number for tracking purposes. The explanation provided is concise and to the point.

Changes in pkg/proxy/tunnel_deliver.go:

1. License Information: Adding license information at the beginning of the file is a good practice to ensure compliance and proper attribution.

2. New Interface and Structs:

   • The addition of the TunnelDeliver interface and tunnelDeliver struct helps in organizing the code and defining the required methods for tunnel delivery.

3. New Functionality:

   • The Deliver method now ensures that the same tunnel is not added to the queue multiple times or while it is being transferred. This prevents duplicate entries and maintains the integrity of the transfer process.
   • The Count method provides the number of tunnels currently in the queue, which can be useful for monitoring and management.

4. Code Refactoring:

   • The code has been refactored to handle the tunnel transfer logic more efficiently by introducing helper functions like preDeliver and queueFullAction.
   • The use of logging with zap for informative messages enhances the code's readability and debugging capabilities.

Suggestions for Improvement:

1. Error Handling:

   • Consider adding error handling mechanisms, such as returning errors from functions or logging errors when necessary, to provide better feedback in case of failures.

2. Input Validation:

   • Validate input parameters to functions like Deliver to ensure that unexpected or invalid values are handled appropriately.

3. Concurrency:

   • Since the code involves locking and unlocking the tunnel's mutex, ensure that it is done safely to prevent race conditions or deadlocks.

4. Optimization:

   • Depending on the use case, consider optimizing the queue handling logic for better performance, especially in scenarios with high concurrency.

5. Documentation:

   • Add comments or documentation to explain the purpose of each function, its parameters, and expected behavior for future reference.

6. Testing:

   • Write unit tests to cover the new functionality and edge cases to ensure the reliability of the code changes.

7. Security:

   • Ensure that sensitive information or operations are handled securely, especially when dealing with tunnel transfers or queue management.

By addressing these suggestions, the codebase can be enhanced in terms of functionality, performance, and maintainability.

Here are review comments for file pkg/proxy/tunnel_deliver_test.go:

Pull Request Review:

Title:

The title of the pull request indicates that it is addressing a bug related to the proxy functionality when scaling CN.

Body:

1. The PR type is correctly marked as a bug fix.
2. It references the specific issue [Bug]: rolling-update CN behind proxy cause prepared stmt lost #16357 that this PR fixes.
3. The purpose of the PR is to refactor the code to prevent adding the same tunnel multiple times to the queue before it has finished transferring.
4. It mentions resetting the transferred status back to false along with wg.Done.

Changes in pkg/proxy/tunnel_deliver_test.go:

1. License header added, which is good practice for open-source projects.
2. A test function TestTunnelDeliver_Deliver is added to test the Deliver method in the tunnelDeliver struct.
3. The test covers scenarios with nil tunnel, starting a tunnel, delivering a tunnel, and checking the count of tunnels in the queue.
4. The test ensures that the same tunnel is not added multiple times to the queue before it has started transferring.
5. The test covers various cases of delivering tunnels and checking the count in the queue.

Suggestions for Improvement:

1. License Header: Ensure that the license header is consistent across all files in the project to maintain uniformity.
2. Test Coverage: Consider adding more test cases to cover edge cases and potential failure scenarios to increase the robustness of the test suite.
3. Code Refactoring: While the test seems to cover the intended behavior, ensure that the code is clean, concise, and follows best practices.
4. Documentation: Add comments to the test cases to explain the purpose of each test and any specific scenarios being tested.
5. Variable Naming: Ensure variable names are descriptive and follow a consistent naming convention for better readability.
6. Error Handling: Consider adding test cases to cover error scenarios and how the code behaves in such cases.

Security Concerns:

1. Input Validation: Ensure that input parameters to the Deliver method are properly validated to prevent any potential security vulnerabilities like injection attacks.
2. Concurrency: Since the test involves concurrent operations on tunnels, ensure that proper synchronization mechanisms are in place to handle race conditions.

By addressing the suggestions and potential security concerns mentioned above, the code quality and reliability can be improved, ensuring a more robust and secure codebase.