Skip to content

Conversation

@tamilmani1989
Copy link
Member

@tamilmani1989 tamilmani1989 commented Apr 10, 2023

Reason for Change:

This PR adds constant mac (aa:aa:aa:aa:aa:aa) for host veth interface. This prevents kernel to allocate unique mac for each host veth interface. Transparent mode already has this change and doing the same for transparent vlan mode here. Also from ubuntu22, we saw mac address getting changed after veth creation and arp entry set to wrong mac due to this. This prevents that if transparent-vlan mode used in ubuntu22 or later.

  • Validate netwrok namespace creation and if its same as VM namespace, delete and recreate it
  • Check for interface exists after creating vlan interface
  • make sure calls are idempotent. even if it fails, retry should succeed
  • delete route before adding new route for same ip (only for transparent vlan)

Issue Fixed:

Requirements:

Notes:

@tamilmani1989 tamilmani1989 added cni Related to CNI. multitenancy labels Apr 10, 2023
@tamilmani1989 tamilmani1989 requested a review from vipul-21 April 10, 2023 18:50
}

// Create veth pair
if err = client.netUtilsClient.CreateEndpoint(client.vnetVethName, client.containerVethName, mac); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we update the test cases as we are now assigning a constant mac ?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep good catch. let me fix it

Copy link
Member Author

Choose a reason for hiding this comment

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

can you point me to test case which need to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

UT abstract creation of host veth interface..so need to update UT for this


/**
/*
*
Copy link
Contributor

Choose a reason for hiding this comment

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

same

@tamilmani1989 tamilmani1989 force-pushed the tamanoha/transvlan_macaddr branch from 6738b8a to c1013dc Compare April 14, 2023 23:31
@tamilmani1989 tamilmani1989 force-pushed the tamanoha/transvlan_macaddr branch from c1013dc to f39c515 Compare April 16, 2023 03:31
Copy link
Contributor

@vipul-21 vipul-21 left a comment

Choose a reason for hiding this comment

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

Can we update the description with the other changes in the PR as well? like using containerId now to fetch the endpoint to delete, updating how namespaces are being created etc


ifIndex = devIf.Index
} else {
} else if interfaceName != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this failing in a case where interfaceName is empty ?
Then should we add another else and log the error or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

it would not fail but skip deleting the route if interface not provided but the requirement is to delete route even if interface name not provided

return errors.Wrap(err, "failed to remove routes")
}

routesLeft, err := getNumRoutesLeft()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we removing the getNumRoutesLeft ?

And if we need to remove the func, we can remove from the fucn parameters as well then

Copy link
Member Author

Choose a reason for hiding this comment

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

not required.. but i wanted to reevaluate this in future. so keeping it

vipul-21
vipul-21 previously approved these changes Apr 17, 2023
Copy link
Contributor

@vipul-21 vipul-21 left a comment

Choose a reason for hiding this comment

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

Let's update the description of PR as well with the new changes added in the PR.

@tamilmani1989 tamilmani1989 enabled auto-merge (squash) April 17, 2023 18:46
@tamilmani1989 tamilmani1989 disabled auto-merge April 17, 2023 18:46
@tamilmani1989
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tamilmani1989 tamilmani1989 merged commit a82b312 into master Apr 17, 2023
@tamilmani1989 tamilmani1989 deleted the tamanoha/transvlan_macaddr branch April 17, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cni Related to CNI. multitenancy

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants