fix: Handle async delete in stateless cni #2967

behzad-mir · 2024-08-28T16:45:19Z

Issue Fixed:

This is an attempt to fix the HNS endpoint leak by stateless CNI when the aynch delete is enabled.
CNS needs to delete the associate HNS endpoint ids upon releasing IPs in asynch delete mode.

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

Notes:

tamilmani1989

can we add any UTs for this?

cni/network/network.go

tamilmani1989 · 2024-08-28T19:41:05Z

cni/network/network.go

+		// if stateless CNI fail to get the endpoint from CNS for any reason other than  Endpoint Not found
+		if err != nil && !strings.Contains(err.Error(), "endpoint state could not be found in the statefile") {
+			if strings.Contains(err.Error(), "No connection could be made") {
+				addErr := fsnotify.AddFile("stateless_"+args.ContainerID, args.ContainerID, watcherPath)


why adding stateless prefix? why can't we follow same pattern for release ip

There's no way for me to specify that this is a stateless case other than writing to the file. CNS only delete HNS in statelss case and there is no way for CNS to determine if the CNI is stateless or statefull

why not always try to delete the HNS state?

Because for stateful CNI we don't have the HNS ID in the state so we have to find the HNS via IP address first and then issue a delete. That's what we decided to do. We have to also make sure that the manage Endpoint state is indeed set to true. If in the future CIlium for Windows introduced then we need to revisit this again.

THe check has moved to a function named isStalessCNIWindows()

cni/network/network.go

cns/fsnotify/fsnotify.go

cns/hnsclient/hnsclient_linux.go

cni/network/network.go

cns/client/client.go

cns/fsnotify/fsnotify.go

cns/hnsclient/hnsclient_windows.go

.pipelines/pipeline.yaml

cni/network/network.go

cns/fsnotify/fsnotify.go

cns/hnsclient/hnsclient_windows.go

cns/fsnotify/fsnotify.go

cns/types/codes.go

Co-authored-by: tamilmani1989 <[email protected]> Signed-off-by: Behzad Mirkhanzadeh <[email protected]>

cns/hnsclient/hnsclient_windows.go

rbtr · 2024-09-19T00:06:25Z

network/errors.go

+	errSubnetV6NotFound        = errors.New("Couldn't find ipv6 subnet in network info")                // nolint
+	errV6SnatRuleNotSet        = errors.New("ipv6 snat rule not set. Might be VM ipv6 address missing") // nolint
+	ErrEndpointStateNotFound   = errors.New("endpoint state could not be found in the statefile")
+	ErrConnectionFailure       = errors.New("couldn't connect to CNS daemonset")


probably not daemonset?

Suggested change

ErrConnectionFailure = errors.New("couldn't connect to CNS daemonset")

ErrConnectionFailure = errors.New("couldn't connect to CNS")

rbtr · 2024-09-19T00:07:38Z

cns/service/main.go

 		cnsclient, err := cnsclient.New("", cnsReqTimeout) //nolint
 		if err != nil {
 			z.Error("failed to create cnsclient", zap.Error(err))
 		}
 		go func() {
 			_ = retry.Do(func() error {
 				z.Info("starting fsnotify watcher to process missed Pod deletes")
-				w, err := fsnotify.New(cnsclient, cnsconfig.AsyncPodDeletePath, z)
+				logger.Printf("starting fsnotify watcher to process missed Pod deletes")


i don't think we need to dupe logs like this

rbtr · 2024-09-19T00:11:02Z

cns/fsnotify/fsnotify.go

 }

 type watcher struct {
-	cli  releaseIPsClient
-	path string
-	log  *zap.Logger


why did you change this from injecting a zap logger to the old bad global logger package?

Zap logger was not logging anything at all in CNS log file. I don't know if it works for Linuxor not but in case of Windows I was only able to see the logs in CNS log file after I used the old logger. Seems like we cannot use the two logger to write on the same file.

rbtr · 2024-09-19T00:13:19Z

cns/fsnotify/fsnotify.go

 	pendingDelete map[string]struct{}
 	lock          sync.Mutex
+	cnsconfig     *configuration.CNSConfig


don't pass the cns config in here, this package does not need to know all of that 🙁

Correct. I'll only pass the IntiFromCNI and ManagedEndpointState.

rbtr · 2024-09-19T00:22:05Z

cns/fsnotify/fsnotify.go

-
-		w.log.Info("releasing IP for missed delete", zap.String("podInterfaceID", podInterfaceID), zap.String("containerID", containerID))
+		// in case of stateless CNI for Windows, CNS needs to remove HNS endpoitns first
+		if isStalessCNIWindows(w.cnsconfig) {


The asyncdelete watcher should not need to know about the CNI mode. You need to compose it in such a way that it does the right thing with no additional knowledge instead of teaching it about all this extra CNS config and adding branches and complication.
One way you might accomplish this is by decorating the ReleaseIPs implementation that gets passed in during initialization. In main when we bulid this object, you already have all the context you need about if this is stateless CNI. There, wrap ReleaseIPs with this extra functionality and the call the underlying impl after you deleteEndpoint, like middleware.
I think you could implement that in a way that there are actually no changes to the fsnotify package at all, and only a small anonymous wrapper func added to this initialization in main.

That was my first thought implementing this then I got some issues with the pattern of going through CNS client and Rest APIs internally from CNS. The async delete is not a direct request and I don't think we should go through Rest APIs for its related operations. For the ReleaseIP and GetEndpointState, I think we don't have any other choice other than CNS client and APIs since they are written that way. However, for HNS we still can avoid this.
I had some concerns about having let's say 250 async delete ops and as a result it will create 250*3 rest APIs for (releaseIP + GetEndpointState + DeleteHNSEndpoint).
I'm more inclined to a solution to avoid going through cnsClient for this. Not sure if we can achieve it without tying up asynchdelte watched to extra packages.

@tamilmani1989 Let me know what you think.

cns/hnsclient/hnsclient_linux.go

tamilmani1989 · 2024-09-19T20:26:13Z

cns/client/client.go

@@ -1023,35 +1024,63 @@ func (c *Client) GetHomeAz(ctx context.Context) (*cns.GetHomeAzResponse, error)
 	return &getHomeAzResponse, nil
 }

+// GetEndpoint calls the EndpointHandlerAPI in CNS to retrieve the state of a given EndpointID
+func (c *Client) GetHNSEndpointID(ctx context.Context, endpointID string) ([]string, [][]net.IPNet, error) {


can we add a brief comment on what this function returns

funciton name is GetHNSEndpointID and it returns IPs too. lets consider renaming it and should we return as [] struct

or a map[string][]net.IPNet

Ended up removing the function since we can cover the previous version (with GetEndpoint+deleteEndpoint) in EndpointManger package without import cycle

… to HNS

behzad-mir requested review from a team as code owners August 28, 2024 16:45

behzad-mir requested review from vipul-21 and neaggarwMS August 28, 2024 16:45

behzad-mir force-pushed the statelesCNI-asynchdelete branch 3 times, most recently from c75da1b to ccaa383 Compare August 28, 2024 19:23

tamilmani1989 reviewed Aug 28, 2024

View reviewed changes

rbtr reviewed Aug 28, 2024

View reviewed changes

cns/hnsclient/hnsclient_linux.go Outdated Show resolved Hide resolved

behzad-mir force-pushed the statelesCNI-asynchdelete branch 6 times, most recently from c456ffa to 5aed0db Compare September 12, 2024 00:05

tamilmani1989 reviewed Sep 12, 2024

View reviewed changes

tamilmani1989 changed the title ~~fix: Stateles cni asynchdelete~~ fix: Handle async delete in stateless cni Sep 12, 2024

tamilmani1989 added cns Related to CNS. cni Related to CNI. ci Infra or tooling. labels Sep 12, 2024

tamilmani1989 reviewed Sep 16, 2024

View reviewed changes

behzad-mir requested review from tamilmani1989 and rbtr September 17, 2024 01:40

behzad-mir force-pushed the statelesCNI-asynchdelete branch 5 times, most recently from 36ac33f to 2d55822 Compare September 17, 2024 22:56

tamilmani1989 requested changes Sep 18, 2024

View reviewed changes

cns/fsnotify/fsnotify.go Outdated Show resolved Hide resolved

cns/fsnotify/fsnotify.go Outdated Show resolved Hide resolved

cns/types/codes.go Outdated Show resolved Hide resolved

behzad-mir requested a review from tamilmani1989 September 18, 2024 01:31

behzad-mir force-pushed the statelesCNI-asynchdelete branch from 14dc742 to e2dfe7d Compare September 18, 2024 16:35

behzad-mir and others added 11 commits September 18, 2024 10:01

feat: adding stateless CNI pipeline test

27c5f98

feat: making change for stateless CNI pipeline

1589676

feat: addressing the comments

8ad3f47

fix: fixing stateles cni yaml

c9e138c

fix: stateless CNI delete fix

492ee3e

fix: addressing the comments

09ab395

fix: addressing the comments and fix linter issues

105d110

Update cns/fsnotify/fsnotify.go

f3903ee

Co-authored-by: tamilmani1989 <[email protected]> Signed-off-by: Behzad Mirkhanzadeh <[email protected]>

Update cni/network/network.go

a04c658

Co-authored-by: tamilmani1989 <[email protected]> Signed-off-by: Behzad Mirkhanzadeh <[email protected]>

Update cni/network/network.go

8215c5b

Co-authored-by: tamilmani1989 <[email protected]> Signed-off-by: Behzad Mirkhanzadeh <[email protected]>

fix: addressing the comments

2aa6dfc

behzad-mir force-pushed the statelesCNI-asynchdelete branch 2 times, most recently from 2df2c99 to 11ea4e1 Compare September 18, 2024 19:24

tamilmani1989 reviewed Sep 18, 2024

View reviewed changes

cns/hnsclient/hnsclient_windows.go Outdated Show resolved Hide resolved

fix: fix the error code.

0ab2e16

behzad-mir force-pushed the statelesCNI-asynchdelete branch from 11ea4e1 to 0ab2e16 Compare September 18, 2024 20:04

behzad-mir requested a review from tamilmani1989 September 18, 2024 21:27

rbtr requested changes Sep 19, 2024

View reviewed changes

Fix: decoupling hnsclient form CNS watcher

b287b3b

tamilmani1989 reviewed Sep 19, 2024

View reviewed changes

behzad-mir requested review from tamilmani1989 and rbtr September 19, 2024 23:21

behzad-mir force-pushed the statelesCNI-asynchdelete branch from 36e5db7 to 03434f9 Compare September 19, 2024 23:33

fix: adding endpointmanager package to resolve platfrom specific call…

d66252e

… to HNS

behzad-mir force-pushed the statelesCNI-asynchdelete branch from 03434f9 to d66252e Compare September 19, 2024 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Handle async delete in stateless cni #2967

fix: Handle async delete in stateless cni #2967

behzad-mir commented Aug 28, 2024

tamilmani1989 left a comment

tamilmani1989 Aug 28, 2024

behzad-mir Aug 28, 2024

rbtr Aug 28, 2024

behzad-mir Aug 29, 2024

behzad-mir Sep 18, 2024

rbtr Sep 19, 2024

rbtr Sep 19, 2024

rbtr Sep 19, 2024

behzad-mir Sep 19, 2024

rbtr Sep 19, 2024

behzad-mir Sep 19, 2024

rbtr Sep 19, 2024

behzad-mir Sep 19, 2024 •

edited

Loading

behzad-mir Sep 19, 2024

tamilmani1989 Sep 19, 2024

tamilmani1989 Sep 19, 2024

rbtr Sep 19, 2024

behzad-mir Sep 19, 2024

	ErrConnectionFailure = errors.New("couldn't connect to CNS daemonset")
	ErrConnectionFailure = errors.New("couldn't connect to CNS")

fix: Handle async delete in stateless cni #2967

Are you sure you want to change the base?

fix: Handle async delete in stateless cni #2967

Conversation

behzad-mir commented Aug 28, 2024

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

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

behzad-mir Sep 19, 2024 • edited Loading

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

behzad-mir Sep 19, 2024 •

edited

Loading