Skip to content
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

[deployments-k8s#2003] Add discover close for expired NSE #1013

Conversation

Bolodya1997
Copy link

@Bolodya1997 Bolodya1997 commented Jul 12, 2021

Description

Adds Close propagation for discover for not existing (probably expired) NSE case.

Issue link

networkservicemesh/deployments-k8s#2003

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@Bolodya1997 Bolodya1997 self-assigned this Jul 12, 2021
}

return next.Server(ctx).Close(clienturlctx.WithClientURL(ctx, u), conn)
Copy link
Member

Choose a reason for hiding this comment

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

Am I get it correctly that in case if endpoint is unreachable we will call next with expired ctx?

Copy link
Author

Choose a reason for hiding this comment

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

Missed this, thank you.

Copy link
Author

Choose a reason for hiding this comment

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

We have ctx.Deadline() / 10 timeout for discoverNetworkServiceEndpoint, so it wouldn't be expired.

@Bolodya1997 Bolodya1997 marked this pull request as draft July 12, 2021 05:01
@Bolodya1997 Bolodya1997 force-pushed the deployments-k8s#2003/discover-close branch from 7835ba9 to 8abe6be Compare July 12, 2021 05:28
@Bolodya1997 Bolodya1997 marked this pull request as ready for review July 12, 2021 05:29
@Bolodya1997 Bolodya1997 marked this pull request as draft July 12, 2021 08:30
@Bolodya1997 Bolodya1997 force-pushed the deployments-k8s#2003/discover-close branch from 7d72de1 to c0176e8 Compare July 12, 2021 10:00
@Bolodya1997 Bolodya1997 marked this pull request as ready for review July 12, 2021 10:05
@Bolodya1997 Bolodya1997 changed the title [deployments-k8s#2003] Add discover close for expired NSE [deployments-k8s#2003] Don't set client URL in discover/interpose Close Jul 12, 2021
@@ -1,4 +1,4 @@
// Copyright (c) 2020 Cisco Systems, Inc.
// Copyright (c) 2020-2021 Cisco Systems, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright (c) 2020-2021 Cisco Systems, Inc.
// Copyright (c) 2020 Cisco Systems, Inc.
//
// Copyright (c) 2021 Doc.ai and/or its affiliates.

Copy link
Author

Choose a reason for hiding this comment

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

Yep, fixed.

touchServer.touched = false

_, err = client.Close(context.TODO(), conn)
_, err = client.Request(context.TODO(), request)
Copy link
Member

Choose a reason for hiding this comment

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

Can we change TODO context?

Copy link
Author

Choose a reason for hiding this comment

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

Removed commit modifying interpose, guess it is no more part of the PR.

Comment on lines 156 to 158
var crossCTX context.Context
if conn.GetId() == connInfo.clientConnID {
crossCTX = clienturlctx.WithClientURL(ctx, connInfo.interposeNSEURL)
Copy link
Member

Choose a reason for hiding this comment

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

Why we don't need more to pass clienturl to interposeNSE?

Copy link
Author

Choose a reason for hiding this comment

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

Removed commit modifying interpose, guess it is no more part of the PR.

}
return next.Server(ctx).Close(clienturlctx.WithClientURL(ctx, u), conn)

// Connect server already has a client URL for the conn.Id, so we don't need to set it here.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that discover server should know about connect server.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this commit.

@denis-tingaikin denis-tingaikin self-requested a review July 12, 2021 10:52
Copy link
Member

@denis-tingaikin denis-tingaikin left a comment

Choose a reason for hiding this comment

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

Added a few comments

@Bolodya1997 Bolodya1997 force-pushed the deployments-k8s#2003/discover-close branch from f5c8bc7 to d42b11c Compare July 12, 2021 11:12
@Bolodya1997 Bolodya1997 changed the title [deployments-k8s#2003] Don't set client URL in discover/interpose Close [deployments-k8s#2003] Add discover close for expired NSE Jul 12, 2021
Signed-off-by: Vladimir Popov <[email protected]>
@Bolodya1997 Bolodya1997 force-pushed the deployments-k8s#2003/discover-close branch from d42b11c to 34f33d7 Compare July 12, 2021 11:18
@denis-tingaikin denis-tingaikin merged commit 7750810 into networkservicemesh:main Jul 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants