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

[Driver] Enhancement for concurrency #1195

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

sushanthakumar
Copy link
Contributor

@sushanthakumar sushanthakumar commented Jan 22, 2020

What this PR does / why we need it:
This PR is for enhancement for concurrency scenarios at driver side

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
This PR fixes attach part of the issue : #1197

Special notes for your reviewer:

Any other PR(s) this PR is dependant on: #1194 (Contains some base function for concurrency handling)

Test Steps:
Create multiple attachments through multiple pod creation simultaneously
Verify the volume and attachments states

Release note:

@@ -419,16 +464,21 @@ func (ds *dockServer) UpdateVolumeGroup(ctx context.Context, opt *pb.UpdateVolum

func (ds *dockServer) DeleteVolumeGroup(ctx context.Context, opt *pb.DeleteVolumeGroupOpts) (*pb.GenericResponse, error) {
// Get the storage drivers and do some initializations.
ds.Driver = drivers.Init(opt.GetDriverName())
defer drivers.Clean(ds.Driver)
driver, err := drivers.Init(opt.GetDriverName())
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why cannot be this part of the code moved to a new func, say init() and called everywhere? It is repetition of code

Copy link
Contributor

Choose a reason for hiding this comment

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

I just followed the original code logic, however, yes it is repeated.
I'd rather leave this improvement to another pr.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok.

@kumarashit
Copy link
Collaborator

The issue of concurrency happens just because there is no proper error handling?
Can there be a case that:
a) Volume is created and attach request is made
b) Port X, is specified but the Port status is not changed from available->unavailable yet
c) Another Volume created and attach request made
d) It finds that Port X is still available, now there may be a race condtion
e) Should we wait for the next attachment request or keep a list of available and unavailable port? May be some network lag may cause attach/detach request to fall into race condition.
Just thinking...

@@ -102,7 +107,7 @@ func Init(resourceType string) VolumeDriver {
d = &spectrumscale.Driver{}
break
case config.HuaweiOceanStorBlockDriverType:
d = &oceanstor.Driver{}
d = &OceanStorDriver
Copy link
Contributor

Choose a reason for hiding this comment

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

Any Specific reason to change this format of calling (drivername.Driver{}). I saw above is declared same as variable.

Copy link
Contributor

Choose a reason for hiding this comment

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

OceanStorDriver is defined as a global variable, so each time we call Init for Huawei OceanStor driver, we can get the same driver object, not create a new one as before.

Copy link
Contributor

Choose a reason for hiding this comment

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

This's also for the concurrency circumstance, because each oceanstor driver maintains a connect to OceanStor storage, if there are big concurrency invokes, the connection number may exceed the limit of OceanStor.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -136,7 +146,7 @@ func Clean(d VolumeDriver) VolumeDriver {
case *spectrumscale.Driver:
break
case *oceanstor.Driver:
break
return // No need to clean anything for oceanstor
Copy link
Contributor

Choose a reason for hiding this comment

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

What happen, if i really mean to unset/clean the driver??

Copy link
Contributor

Choose a reason for hiding this comment

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

oceanstor driver's unset operation just close the connection to back storage, cause we use the same global oceanstor driver for each call now, so there's no need to unset anymore. Instead, we want to keep the connection session active consistently.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -148,10 +158,9 @@ func Clean(d VolumeDriver) VolumeDriver {
default:
break
}

d.Unset()
Copy link
Contributor

Choose a reason for hiding this comment

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

It's better to handle Unset error also. I have seen you have handled the error in Setup()

Copy link
Contributor

Choose a reason for hiding this comment

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

as the old code's implementation, I think the unset error is ignorable, so I didn't change here.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@kumarashit
Copy link
Collaborator

Can you please check CI issues

@kumarashit
Copy link
Collaborator

@sushanthakumar Please check the CI issue

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

Merging #1195 into development will decrease coverage by 0.16%.
The diff coverage is 3.27%.

@@               Coverage Diff               @@
##           development    #1195      +/-   ##
===============================================
- Coverage        34.82%   34.66%   -0.17%     
===============================================
  Files               97       97              
  Lines            17622    17705      +83     
===============================================
  Hits              6137     6137              
- Misses           10614    10688      +74     
- Partials           871      880       +9
Impacted Files Coverage Δ
contrib/drivers/huawei/oceanstor/client.go 0% <0%> (ø) ⬆️
contrib/drivers/huawei/oceanstor/oceanstor.go 0% <0%> (ø) ⬆️
pkg/dock/dock.go 11.8% <0%> (-1.22%) ⬇️
pkg/dock/discovery/discovery.go 36.73% <0%> (+3.21%) ⬆️
contrib/drivers/drivers.go 23.52% <50%> (-1.48%) ⬇️
pkg/controller/fileshare/filesharecontroller.go 25% <0%> (-4.6%) ⬇️
pkg/controller/volume/volumecontroller.go 16.52% <0%> (-3.24%) ⬇️
pkg/api/util/db.go 35.7% <0%> (-1.29%) ⬇️
osdsctl/cli/volume.go 81.81% <0%> (-0.43%) ⬇️
... and 14 more

Copy link
Member

@NajmudheenCT NajmudheenCT left a comment

Choose a reason for hiding this comment

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

Is it possble to get test report from one more driver(say LVM). Since the changes are impacted in core driver code. It would be better to test at least with 2 drivers.

Copy link
Collaborator

@joseph-v joseph-v left a comment

Choose a reason for hiding this comment

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

LGTM

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.

6 participants