Feat: Stateless cni migration #2363

behzad-mir · 2023-11-07T22:19:49Z

This PR contains code changes related to Stateless CNI Migration.
1- There has been code change to use cnireconciler (pointing to statefull CNI binary) when the managedEndpoint and Migration flag on CNS is set.
2- There has been code changes on CNI GetEndpoint function to fetch the HNS Endpoint corresponded to an IP address to cover the cases for the missing HNSEndpointID in statefile.

Reason for Change:

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

rbtr

I'm confused about the state of the world at the point that this would run. This still seems to assume some amount of stateless CNI has happened - in that the existing CNI is expected to be renamed to azure-vnet-stateful, at least.

We need to stop thinking about this as part of the stateless CNI change. This is a CNI-managed state to CNS-managed state migration. It is happening entirely independently of any other feature and should not assume anything about the progress of other features. It shouldn't be part of the stateless CNI changeset (except as a prereq) or branch.

rbtr · 2023-11-09T15:49:08Z

cns/cnireconciler/podinfoprovider.go

+}
+
+func NewStatefullCNIPodInfoProvider() (cns.PodInfoByIPProvider, error) {
+	return newCNIPodInfoProvider(exec.New(), platform.CNIStateFilePath)


why is this the statefile path? shouldn't it be the executable?

OOps, Was a mistake

rbtr · 2023-11-09T15:51:37Z

network/endpoint_linux.go

+
+// getHNSEndpointIdByIP returns an HNS Endpoint IP that matches an specific IPAddress.
+func (nm *networkManager) GetEndpointInfoByIPImpl(ipAddresses []net.IPNet, _ string) (string, error) {
+	return "", errors.New("No HostVethName matches the IPAddress: " + ipAddresses[0].IP.String())


use Errorf with formatter instead of concat and don't sentence case errors

behzad-mir · 2023-11-09T18:54:54Z

I'm confused about the state of the world at the point that this would run. This still seems to assume some amount of stateless CNI has happened - in that the existing CNI is expected to be renamed to azure-vnet-stateful, at least.

We need to stop thinking about this as part of the stateless CNI change. This is a CNI-managed state to CNS-managed state migration. It is happening entirely independently of any other feature and should not assume anything about the progress of other features. It shouldn't be part of the stateless CNI changeset (except as a prereq) or branch.

At the moment that we enter into migration, statless CNI has already been deployed. There are two ways to handles this: 1) We should either duplicate the whole GetEndpointState procedure with store lock initialization from the statefull CNI into the stateless code, or 2) just invoke the statefull CNI binary.
I went with the second option since it will avoid the code duplication and having the whole statefile recovery on stateless CNI will simply spoil the whole point of "stateless" CNI. In my opinion, the migration should call into statefull CNI from design perspective since the statefile is part of that binary.

If you're also advocating for a migration code regardless of having Stateless CNI, I belive we have to add another field to the CNS configmap and based on that condition we can simply call to the azure-vnet.exe binary (which is statefull) to do the migration.

#2102) * Adding getEndpoint and UpdateEndpoint API to CNS with the respective clients in support of stateless CNI. * Updating the unit tests and address the comments. * Addressing the comments. * Addressing the coments regarding CNS support for Stateless CNI * Adddressing the PR comments

feat: stateless cni

…package. (#2197) * Apllying stateless CNI mode in network package. * Addresing the commetns.

tamilmani1989

Does this change include linux support.? If you intend to create separate PR for it, update the title to reflect its for windows.

tamilmani1989 · 2023-12-01T18:25:51Z

network/endpoint_windows.go

+		for _, ipConfiguration := range hcnEndpoint.IpConfigurations {
+			for _, ipAddress := range ipAddresses {
+				prefixLength, _ := ipAddress.Mask.Size()
+				if ipConfiguration.IpAddress == ipAddress.IP.String() && ipConfiguration.PrefixLength == uint8(prefixLength) {


is matching prefix length required? isn't that be always /32 ip?

tamilmani1989 · 2023-12-01T18:33:02Z

network/endpoint.go

+
+// IsEndpointStateComplete returns true if both HNSEndpointID and HostVethName are missing.
+func (epInfo *EndpointInfo) IsEndpointStateIncomplete() bool {
+	if epInfo.HNSEndpointID == "" && epInfo.IfName == "" {


i guess epInfo.Ifname is not hostvethname. there would be field in epInfo explictly saying hostveth interface name

We do have a HostIfName on ednpoint. DO you want me to add it to endpointInfo struct too?

This would always return false for linux ? As HNSendpointId is empty for linux

rbtr · 2023-12-01T20:58:59Z

cns/service/main.go

@@ -1191,10 +1191,19 @@ func InitializeCRDState(ctx context.Context, httpRestService cns.HTTPService, cn
 		if err != nil {
 			if errors.Is(err, store.ErrKeyNotFound) {
 				logger.Printf("[Azure CNS] No endpoint state found, skipping initializing CNS state")
+				if os.Getenv("StatelessCNIMigration") == "true" {


can this come from the config instead of the env? and to align with the convention there, maybeEnableManagedStateMigration

vipul-21 · 2023-12-04T17:29:32Z

network/endpoint_windows.go

@@ -492,3 +493,28 @@ func (ep *endpoint) getInfoImpl(epInfo *EndpointInfo) {
 func (nm *networkManager) updateEndpointImpl(nw *network, existingEpInfo *EndpointInfo, targetEpInfo *EndpointInfo) (*endpoint, error) {
 	return nil, nil
 }
+
+// GetEndpointInfoByIPImpl returns an endpointInfo with the corrsponding HNS Endpoint ID that matches an specific IP Address.
+func (epInfo *EndpointInfo) GetEndpointInfoByIPImpl(ipAddresses []net.IPNet, networkID string) (*EndpointInfo, error) {


Can we add logs to the functions when it fails/succeeds while fetching Network/Endpoints etc.

vipul-21 · 2023-12-04T17:30:48Z

network/endpoint.go

+
+// IsEndpointStateComplete returns true if both HNSEndpointID and HostVethName are missing.
+func (epInfo *EndpointInfo) IsEndpointStateIncomplete() bool {
+	if epInfo.HNSEndpointID == "" && epInfo.IfName == "" {


This would always return false for linux ? As HNSendpointId is empty for linux

github-actions · 2023-12-27T00:01:12Z

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

github-actions · 2024-01-04T00:01:19Z

Pull request closed due to inactivity.

behzad-mir requested review from a team as code owners November 7, 2023 22:19

behzad-mir requested review from vipul-21 and rbtr November 7, 2023 22:19

behzad-mir added cni Related to CNI. do-not-merge labels Nov 7, 2023

behzad-mir requested a review from tamilmani1989 November 7, 2023 22:20

behzad-mir changed the base branch from master to Dev-Stateless-CNI November 7, 2023 22:22

behzad-mir requested a review from vakalapa November 7, 2023 22:34

behzad-mir force-pushed the SatetlessCNI-Migration branch from 38c9751 to b1812e6 Compare November 7, 2023 23:23

rbtr reviewed Nov 9, 2023

View reviewed changes

behzad-mir and others added 8 commits November 28, 2023 15:37

🌈 feat: adding flags for stateless cni (#2103)

6238fc0

feat: stateless cni

🌈 feat: StatelessCNI: Applying stateless CNI mode changes in network …

15ca91a

…package. (#2197) * Apllying stateless CNI mode in network package. * Addresing the commetns.

feat: create stateless cni binary for swift (#2275)

1ccd253

enabling CNS telemetry

052afba

Master rebase changes

d73eed3

CNI Telemetry enabled on CNS

3009d43

Stateless CNI changes.

20bc807

behzad-mir force-pushed the Dev-Stateless-CNI branch 2 times, most recently from 35227c8 to 54aacbd Compare November 28, 2023 23:45

making change to CNSendpointStorePath

53969b9

behzad-mir force-pushed the Dev-Stateless-CNI branch from 54aacbd to 53969b9 Compare November 28, 2023 23:55

behzad-mir added 4 commits November 28, 2023 17:20

Merge branch 'master' into Dev-Stateless-CNI

7b4ab71

Code changes for Statefull CNI Migration

8ec3f65

Making changes to the CNI state migration code.

82df49e

Make code changes for Stateless CNI migrations.

3d1fa46

behzad-mir force-pushed the SatetlessCNI-Migration branch from b1812e6 to c127777 Compare November 29, 2023 22:42

behzad-mir requested a review from rbtr November 29, 2023 22:43

behzad-mir force-pushed the SatetlessCNI-Migration branch 2 times, most recently from 458ec6d to 671e982 Compare November 29, 2023 23:38

Merge branch 'master' into Dev-Stateless-CNI

189077e

behzad-mir force-pushed the SatetlessCNI-Migration branch from 671e982 to 82df49e Compare November 29, 2023 23:58

Merge branch 'Dev-Stateless-CNI' into SatetlessCNI-Migration

d9b5082

tamilmani1989 requested changes Dec 1, 2023

View reviewed changes

rbtr reviewed Dec 1, 2023

View reviewed changes

vipul-21 reviewed Dec 4, 2023

View reviewed changes

behzad-mir force-pushed the Dev-Stateless-CNI branch 2 times, most recently from ae68d79 to 9344500 Compare December 8, 2023 21:47

Base automatically changed from Dev-Stateless-CNI to master December 12, 2023 00:15

github-actions bot added the stale Stale due to inactivity. label Dec 27, 2023

github-actions bot closed this Jan 4, 2024

github-actions bot deleted the SatetlessCNI-Migration branch January 4, 2024 00:01

behzad-mir restored the SatetlessCNI-Migration branch January 4, 2024 00:03

behzad-mir removed the stale Stale due to inactivity. label Jan 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Stateless cni migration #2363

Feat: Stateless cni migration #2363

behzad-mir commented Nov 7, 2023

rbtr left a comment

rbtr Nov 9, 2023

behzad-mir Nov 9, 2023

rbtr Nov 9, 2023

behzad-mir commented Nov 9, 2023 •

edited

Loading

tamilmani1989 left a comment

tamilmani1989 Dec 1, 2023

tamilmani1989 Dec 1, 2023

behzad-mir Dec 1, 2023

vipul-21 Dec 4, 2023

rbtr Dec 1, 2023

vipul-21 Dec 4, 2023

vipul-21 Dec 4, 2023

github-actions bot commented Dec 27, 2023

github-actions bot commented Jan 4, 2024

Feat: Stateless cni migration #2363

Feat: Stateless cni migration #2363

Conversation

behzad-mir commented Nov 7, 2023

rbtr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

behzad-mir commented Nov 9, 2023 • edited Loading

tamilmani1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 27, 2023

github-actions bot commented Jan 4, 2024

behzad-mir commented Nov 9, 2023 •

edited

Loading