Spectrumscale epic2 - Githubissues

deeghuge commented 5 years ago

Removed ssh and mmcli connector support from spectrum scale
Removed NFS and LightWeight backend support from spectrum scale
Changes for using postgresql for databases instead of sqlite.
Changes in Rest v2 - Using the different mechanism to check filesystem is mounted or not
Added support for accepting ssl ca-certificate for Communication between ubiquity server and Spectrum scale rest api server.
Use of new logger, fixes for UT and removed scale specific error.go instead using errors from resource.
Removed spectrum scale specific activation check while activating Provisioner and flex.

This change is

coveralls commented 5 years ago

Coverage increased (+6.6%) to 56.482% when pulling 3c44c9bbeb3e717293fd747648e8639d271c6cb2 on spectrumscale_epic2 into 422904c4c777d359341f22687ab6e3c9b459c470 on dev.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 386 at r4 (raw file):

Previously, shay-berman wrote…

why u need to run it agai

If volume is not DB volume we need to create it OR In case dbVolume is not present we want to create it. Just above this check we verify if it is Dbvolume and checks for its existence.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 383 at r4 (raw file):

Previously, shay-berman wrote…

what about error handling? i think u have a bug here

Here we mainly checks for existence of DbVolume and based on return value we create it.

shay-berman commented 5 years ago

Hi @deeghuge , I finished to review it. please go though and apply my and Olga comments. Then let us know again for another review before merging.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 556 at r4 (raw file):

Previously, shay-berman wrote…

r u using the right logger? just double check

yes we are using correct logger

yadaven commented 5 years ago

local/clients.go, line 42 at r4 (raw file):

Previously, shay-berman wrote…

i think we should improve it. we hide the error in the log message. lets show the error instead of hiding it. (fix it for both scbe and scale) In addition why you are not using here the new logger? with Debug? lets consider to just use it in standard way of loggign.

If initialisation of any backend fails then we return error, for this we have introduced new error i.e. BackendInitializationError. Also made changes to use new Logger function.

yadaven commented 5 years ago

local/clients.go, line 49 at r4 (raw file):

Previously, shay-berman wrote…

fix text please and use Error() of the new logger. text should be "check backend configuration - Scale ManagementIP or SpectrumConnect managmentIP is mandator in the ubiqutiy-configmap "

made changes to use logger.ErrorRet with the proposed error text.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 72 at r4 (raw file):

Previously, olgashtivelman wrote…

maybe you want to use logger.Error/Warning here? (and for the other errors as well) so that it will be recorded in the logs even if its not in debug mode?

using logger.ErrorRet to return error.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 230 at r4 (raw file):

Previously, shay-berman wrote…

remove this line and the second instead do return logger.ErrorRet(err, "short description") you can see other places.

made the proposed changes

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 238 at r4 (raw file):

Previously, shay-berman wrote…

the same and i see many other places - so please apply to all of them

took care of this in almost all places.

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 267 at r4 (raw file):

Previously, shay-berman wrote…

the attach is less important here consider even to delete it because from what I know this function will never called right? if so then i don't care about its code. please clarify

made Attach/Detach to return success. It is needed since we use StorageClient Interface. Removed Attach/Detach UT

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 309 at r4 (raw file):

Previously, shay-berman wrote…

same as the attach note if u don't use it then its better to avoid adding it here. because u need to add UT for it. and also if have some idempotent aspects here that you are not handling so please decide - is this code for v2 relevant? if not then drop it or put in a comment

Made Detach to always return success. Removed UT

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 353 at r4 (raw file):

Previously, shay-berman wrote…

not debug

Changed to use logger.ErrorRet

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 618 at r4 (raw file):

Previously, shay-berman wrote…

you can use the logger function so it will go also to the log and then return the error. mentioned it in the past

Done

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 628 at r4 (raw file):

Previously, shay-berman wrote…

r u sure about this 777? please make sure you align with security aspects, maybe it should take premission from user define. please double checkit.,

Changes it to 750

shay-berman commented 5 years ago

Hi @deeghuge , I finished the second review on this PR. Please try to resolve all the comments and let me know when its ready. Thanks

yadaven commented 5 years ago

local/clients.go, line 42 at r4 (raw file):

Previously, shay-berman wrote…

but on this line you hide the real error "err" from the backendinitilizationError. So you can add the err into this BackendInitializationError so it will be visible and not hiden Does it make sense?

I agree. Made changes such that BackendInitializationError also have error field and we print this error.

yadaven commented 5 years ago

local/spectrumscale/datamodel_wrapper.go, line 1 at r4 (raw file):

Previously, shay-berman wrote…

waiting for your reply.

made it 2018

yadaven commented 5 years ago

local/spectrumscale/datamodel_wrapper_test.go, line 2 at r4 (raw file):

Previously, shay-berman wrote…

waiting for your reply.

changed it to 2018

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 386 at r4 (raw file):

Previously, shay-berman wrote…

> ``` > s.dataModel.IsDbVolume(name) > ``` yes but why to run "s.dataModel.IsDbVolume(name)" twice in the same function? you can run it once and reuse it 2 lines later on. make sense?

agree. Took s.dataModel.IsDbVolume output in a variable and used it in both places

yadaven commented 5 years ago

local/spectrumscale/spectrumscale.go, line 522 at r4 (raw file):

Previously, shay-berman wrote…

please reply

That's right, it is static info and we update quota only during volume creation and is not changed later on.

yadaven commented 5 years ago

local/spectrumscale/connectors/connectors.go, line 53 at r5 (raw file):

Previously, shay-berman wrote…

don't u want to use the standard logger?

We are using standard logger here. Also added logger.Trace for this function.

yadaven commented 5 years ago

local/spectrumscale/connectors/resources.go, line 312 at r5 (raw file):

Previously, shay-berman wrote…

is this Scale specific? if so maybe add Scale as prefix.

These structures are defined in ubiquity/local/spectrumscale/connectors/resources.go file which is specific to SpectrumScale. Since this is specific to Scale I think no need to add Scale Prefix. Kindly let us know if you agree with this.

yadaven commented 5 years ago

resources/resources.go, line 89 at r5 (raw file):

Previously, shay-berman wrote…

BTW you can just reuse the scaleprefix above here in the text instead of duplicate the prefix.

Done

yadaven commented 5 years ago

local/clients_test.go, line 37 at r4 (raw file):

Previously, shay-berman wrote…

Any response here?

Added UT for all cases.

shay-berman commented 5 years ago

You have LGTM from CI testing.

@deeghuge you have green light from CI staging for merging this PR to dev. Remember to merge it with squash commits and also remember also to update the ubiqutiy-k8s glide.yml in https://github.com/IBM/ubiquity-k8s/pull/216 with the latest ubiquity commit (after you merge this ubiqutiy PR first) and then merge it as well to dev.

IBM / ubiquity

Spectrumscale epic2 #249