Skip to content

fix: modify cleanup path to always delete link #3519

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

Merged
merged 2 commits into from
Mar 25, 2025
Merged

Conversation

QxBytes
Copy link
Contributor

@QxBytes QxBytes commented Mar 21, 2025

Reason for Change:

Previously, DeleteEndpointsImpl would return an error if it failed to remove routes, skipping any further clean up logic. This could lead to the vnet veth device not being deleted (The vnet veth device is one end of the container's veth pair, the other resides in either the container if it was successfully moved, or still in the vnet ns if it was not moved). On subsequent adds, an error would pop up mentioning the pair already exists.

Issue Fixed:

The PR ensures both pieces of logic always run, even if there is an error (which is logged). Unit tests have been added to verify the correct code paths are hit and there don't seem to be other methods in the delete flow that have this problem for this scenario.

Requirements:

Notes:
Tested on a multitenancy transparent vlan linux setup.

Sorry, something went wrong.

@Copilot Copilot AI review requested due to automatic review settings March 21, 2025 00:20
@QxBytes QxBytes requested a review from a team as a code owner March 21, 2025 00:20
@QxBytes QxBytes requested a review from jc2543 March 21, 2025 00:20
@QxBytes QxBytes self-assigned this Mar 21, 2025
@QxBytes QxBytes added cni Related to CNI. fix Fixes something. multitenancy labels Mar 21, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR modifies the cleanup path in Azure Container Networking's transparent VLAN endpoint handling so that both route deletion and veth deletion are attempted, even if deleting routes fails. Key changes include:

  • Removing error propagation from DeleteEndpointsImpl so that errors are now only logged.
  • Updating related unit tests to reflect the new behavior.
  • Enhancing the mock netlink package to allow injection of custom DeleteLink behavior.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
network/transparent_vlan_endpointclient_linux_test.go Removed error expectations from tests and added a test verifying cleanup on failure
network/transparent_vlan_endpointclient_linux.go Modified DeleteEndpoints and DeleteEndpointsImpl to log errors without returning them
netlink/mocknetlink.go Added DeleteLinkFn support to allow mock customization
Comments suppressed due to low confidence (2)

network/transparent_vlan_endpointclient_linux.go:650

  • Since DeleteEndpointsImpl no longer returns an error, ensure that all callers are updated to handle this change appropriately and consider updating the function’s comments to clarify that errors are now only logged.
func (client *TransparentVlanEndpointClient) DeleteEndpointsImpl(ep *endpoint, _ func() (int, error)) {

network/transparent_vlan_endpointclient_linux_test.go:733

  • [nitpick] It may be beneficial to add assertions that verify the correct error log messages are emitted when route deletion fails to further improve test coverage for the error paths.
require.Equal(t, 1, errOnDeleteRouteFlag, "error must occur during delete route path")

@QxBytes
Copy link
Contributor Author

QxBytes commented Mar 21, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@QxBytes
Copy link
Contributor Author

QxBytes commented Mar 21, 2025

/azp run Azure Container Networking PR

@QxBytes QxBytes enabled auto-merge March 21, 2025 21:55
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@QxBytes QxBytes added this pull request to the merge queue Mar 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 22, 2025
@QxBytes QxBytes added this pull request to the merge queue Mar 23, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 24, 2025
@QxBytes QxBytes added this pull request to the merge queue Mar 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 24, 2025
@QxBytes QxBytes added this pull request to the merge queue Mar 24, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 24, 2025
@QxBytes QxBytes enabled auto-merge March 24, 2025 20:19
@QxBytes
Copy link
Contributor Author

QxBytes commented Mar 24, 2025

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@QxBytes QxBytes added this pull request to the merge queue Mar 24, 2025
Merged via the queue into master with commit 49a50da Mar 25, 2025
297 of 302 checks passed
@QxBytes QxBytes deleted the alew/fix-delete-on-err branch March 25, 2025 01:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. fix Fixes something. multitenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants