Skip to content

Conversation

@denis-tingaikin
Copy link
Member

Description

Issue link

Fixes networkservicemesh/integration-k8s-kind#845

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 functionality
  • Documentation
  • Refactoring
  • CI

@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@7ec603e). Learn more about missing BASE report.

Files with missing lines Patch % Lines
pkg/networkservice/common/begin/server.go 33.33% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1485   +/-   ##
=======================================
  Coverage        ?   70.38%           
=======================================
  Files           ?      248           
  Lines           ?    11197           
  Branches        ?        0           
=======================================
  Hits            ?     7881           
  Misses          ?     2815           
  Partials        ?      501           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

if closeErr != nil {
log.FromContext(ctx).Errorf("Can't close old connection: %v", closeErr)
} else {
request.GetConnection().State = networkservice.State_UP
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if we should set State_UP after closing.
Maybe State_DOWN?

It may also be worth moving eventFactoryServer.state into else block. In this case we can left eventFactoryServer.state == established condition:

if eventFactoryServer.state == established && 
                request.GetConnection().GetState() == networkservice.State_RESELECT_REQUESTED &&
		eventFactoryServer.request != nil && eventFactoryServer.request.Connection != nil {
		log.FromContext(ctx).Info("Closing connection due to RESELECT_REQUESTED state")
		_, closeErr := next.Server(withEventFactoryCtx).Close(withEventFactoryCtx, eventFactoryServer.request.Connection)
		if closeErr != nil {
			log.FromContext(ctx).Errorf("Can't close old connection: %v", closeErr)
		} else {
			request.GetConnection().State = networkservice.State_DOWN
	                eventFactoryServer.state = closed
		}
	}

Copy link
Member Author

@denis-tingaikin denis-tingaikin Jul 10, 2023

Choose a reason for hiding this comment

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

  1. we may lost eventFactoryServer.state == established
  2. we don't need to request a networkservice with DOWN state if we closed previous connection

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