Spectrumscale-epic1 fix for UB-1487,UB-1488,UB-1495 #243

deeghuge · 2018-08-29T07:18:34Z

UB-1487 : Using ManagementIP and Port instead of accepting URL for endpoint
UB-1488 : Using SPECTRUMSCALE_ prefix instead of SSC_
UB-1495 : Improve the check in backend initilization. Added check for scbe backend. Check for Spectrum Scale will be added later.
Removed MMCLI and MMSSH backend initialization as they are no longer valid
Removed sshconfig from resource.go because it is no longer required
Minor updates in test to use ManagementIP and Port instead of Endpoint

This change is

UB-1487 : Using ManagementIP and Port instead of accepting URL for endpoint UB-1488 : Using SPECTRUMSCALE_ prefix instead of SSC_ UB-1495 : Improve the check in backend initilization. Added check for scbe backend. Check for Spectrum Scale will be added later. Removed MMCLI and MMSSH backend initialization as they are no longer valid Removed sshconfig from resource.go because it is no longer required

coveralls · 2018-08-29T07:22:06Z

Coverage decreased (-2.1%) to 49.869% when pulling 45beb36 on spectrumscale_epic1 into a91d998 on dev.

shay-berman

Deepak please review the comments.
Also please make sure you are using the new logger and also have unit tests for new changes.

In addition, you have some scale backend changes. but for epic1 we should not do anything about scale backend changes - so please try to avoid scale backend changes until epic1 is over.

Thanks

Reviewable status: 0 of 8 files reviewed, 8 unresolved discussions (waiting on @deeghuge, @shay-berman, and @olgashtivelman)

local/clients.go, line 36 at r1 (raw file):

	  } else {
		  clients[resources.SCBE] = ScbeClient
    }

what happens if its empty? It should raise error as we discussed in the design.
please make sure you have unit test for this change (and every other change in the code)
And what about Scale? why you are not adding it here as well?

local/spectrumscale/spectrumscale.go, line 633 at r1 (raw file):

	}

	if s.config.RestConfig.ManagementIP != "" {

make sure you fix Unit tests if any

local/spectrumscale/connectors/rest.go, line 51 at r1 (raw file):

func NewSpectrumRestWithClient(logger *log.Logger, restConfig resources.RestConfig, client *http.Client) (SpectrumScaleConnector, error) {
	endpoint := fmt.Sprintf("https://%s:%d/", restConfig.ManagementIP, restConfig.Port)

instead of duplicate code, you should do newSpectrumRestWithClient (with lower new) and it should do the logic, and then just call it in NewSpectrumRest and NewSpectrumRestWithClient.
so you will not have to duplicate the line.

local/spectrumscale/connectors/rest_v2.go, line 126 at r1 (raw file):

func NewSpectrumRestV2(logger *log.Logger, restConfig resources.RestConfig) (SpectrumScaleConnector, error) {

	endpoint := fmt.Sprintf("https://%s:%d/", restConfig.ManagementIP, restConfig.Port)

the same as mentioned above.

BTW what is this v2? do you please to support both v1 and v2?

utils/utils.go, line 229 at r1 (raw file):

	sscConfig := resources.SpectrumScaleConfig{}
	restConfig := resources.RestConfig{}
	restConfig.User = os.Getenv("SPECTRUMSCALE_REST_USER")

please keep the prefix as const, because who knows maybe we will change it.

Note: later on we should move this specific load part into the backend code and not in util which should stay generic code.

utils/utils.go, line 235 at r1 (raw file):

	spectrumscalePort, err := strconv.ParseInt(os.Getenv("SPECTRUMSCALE_MANAGEMENT_PORT"), 0, 32)
	if err != nil {
		restConfig.Port = resources.SpectrumscaleDefaultPort

what is this? but if it given a wrong port, then its better to raise ERROR and not take a default.
take default only if its empty.

OK?

also add unit testing.

utils/utils.go, line 240 at r1 (raw file):

	}
	sscConfig.RestConfig = restConfig
	sscConfig.DefaultFilesystemName = os.Getenv("SPECTRUMSCALE_DEFAULT_FILESYSTEM_NAME")

as mentioned use prefix as const

utils/utils.go, line 241 at r1 (raw file):

	sscConfig.RestConfig = restConfig
	sscConfig.DefaultFilesystemName = os.Getenv("SPECTRUMSCALE_DEFAULT_FILESYSTEM_NAME")
	sscConfig.NfsServerAddr = os.Getenv("SPECTRUMSCALE_NFS_SERVER_ADDRESS")

why NFS? don't you remove the NFS, or its on different PR? anyway if NFS not relevant then don't touch it please.
its confusing the PR review

shay-berman

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @deeghuge and @olgashtivelman)

local/spectrumscale/connectors/connectors.go, line 61 at r1 (raw file):

		return NewSpectrumRestV2(logger, config.RestConfig)
	}
	if config.SshConfig.User != "" && config.SshConfig.Host != "" {

please lets focus on the epic1 if posible.

deeghuge

Reviewable status: 0 of 8 files reviewed, 9 unresolved discussions (waiting on @deeghuge, @shay-berman, and @olgashtivelman)

local/clients.go, line 36 at r1 (raw file):

Previously, shay-berman wrote…

what happens if its empty? It should raise error as we discussed in the design.

please make sure you have unit test for this change (and every other change in the code)

And what about Scale? why you are not adding it here as well?

If empty, there is check for len(clients) ==0 return error
Will add testcases in the existing testcases for these changes
Scale backend need multiple fixes like logger changes hence check for scale will be added along with changes for epic2

local/spectrumscale/spectrumscale.go, line 633 at r1 (raw file):

Previously, shay-berman wrote…

make sure you fix Unit tests if any

Ok Will do

local/spectrumscale/connectors/connectors.go, line 61 at r1 (raw file):

Previously, shay-berman wrote…

please lets focus on the epic1 if posible.

Yes, I will move these changes as part of epic2.

local/spectrumscale/connectors/rest.go, line 51 at r1 (raw file):

Previously, shay-berman wrote…

instead of duplicate code, you should do newSpectrumRestWithClient (with lower new) and it should do the logic, and then just call it in NewSpectrumRest and NewSpectrumRestWithClient.
so you will not have to duplicate the line.

We are removing rest v1 code so this file will be deleted as part of epic2 change.

local/spectrumscale/connectors/rest_v2.go, line 126 at r1 (raw file):

Previously, shay-berman wrote…

the same as mentioned above.

BTW what is this v2? do you please to support both v1 and v2?

No, We will support only v2.

utils/utils.go, line 229 at r1 (raw file):

Previously, shay-berman wrote…

please keep the prefix as const, because who knows maybe we will change it.

Note: later on we should move this specific load part into the backend code and not in util which should stay generic code.

Will do

utils/utils.go, line 235 at r1 (raw file):

Previously, shay-berman wrote…

what is this? but if it given a wrong port, then its better to raise ERROR and not take a default.
take default only if its empty.

OK?

also add unit testing.

I referred the SCBE part of code for handling port and kept it similar.

utils/utils.go, line 240 at r1 (raw file):

Previously, shay-berman wrote…

as mentioned use prefix as const

OK Will do.

utils/utils.go, line 241 at r1 (raw file):

Previously, shay-berman wrote…

why NFS? don't you remove the NFS, or its on different PR? anyway if NFS not relevant then don't touch it please.
its confusing the PR review

Ok, Will not change the NFS part

- Reverted changes of NFS, Connector, SSH as they should not be part of epic1 changes. - Added variable for SpectrumScaleParamPrefix

shay-berman

Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @shay-berman, @deeghuge, and @olgashtivelman)

local/clients.go, line 36 at r1 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

If empty, there is check for len(clients) ==0 return error

Will add testcases in the existing testcases for these changes

Scale backend need multiple fixes like logger changes hence check for scale will be added along with changes for epic2

you right, none issue
ok so please make this comment as OK after adding the test.
I am ok with this approach. so its none issue

shay-berman · 2018-09-05T11:29:42Z

@deeghuge any news about this PR? the comments still open in the code review.

deeghuge

I am working on adding new testcases. I have addressed all other comments.

Reviewable status: 0 of 8 files reviewed, 4 unresolved discussions (waiting on @deeghuge, @shay-berman, and @olgashtivelman)

deeghuge

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @deeghuge, @shay-berman, and @olgashtivelman)

local/clients.go, line 36 at r1 (raw file):

Previously, shay-berman wrote…

you right, none issue

ok so please make this comment as OK after adding the test.

I am ok with this approach. so its none issue

Currently there are no existing testcases. I have added new testcase for clients.go. More testcases will be added along with spectrumscale epic2 changes.

local/spectrumscale/spectrumscale.go, line 633 at r1 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

Ok Will do

No fix required. New testcases will be added as part of epic 2 changes

[root@node2 spectrumscale]# ginkgo -cover
Running Suite: Spectrum Test Suite

Random Seed: 1536254818
Will run 56 of 56 specs
....
Ran 56 of 56 Specs in 0.007 seconds
SUCCESS! -- 56 Passed | 0 Failed | 0 Pending | 0 Skipped

utils/utils.go, line 229 at r1 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

Will do

Done

utils/utils.go, line 240 at r1 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

OK Will do.

Done

deeghuge

I have addressed all of the review comments.

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @shay-berman and @olgashtivelman)

shay-berman

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @deeghuge and @olgashtivelman)

local/clients.go, line 40 at r3 (raw file):

	if len(clients) == 0 {
		logger.Println("No client can be initialized....please check config file")

text fix. please remove thew "....." thing and just put one dot.
also instead of check "config file" its better to state something else like: check ubiquity-configmap parameters.

local/clients_test.go, line 26 at r3 (raw file):

	Context(".GetLocalClients", func() {
	It("should fail when ManagementIP is empty for SCBE backend", func() {

please also add test for scale has no IP + scbe has no IP.
and another test, that you have only scale IP (without SCBE IP) and it should initialize ok.
so it will cover your change in this PR

local/spectrumscale/spectrumscale.go, line 633 at r3 (raw file):

	}

	if s.config.RestConfig.ManagementIP != "" {

but what if ManagmentIP == "" ?

local/spectrumscale/connectors/connectors.go, line 60 at r3 (raw file):

		logger.Printf("Initializing SpectrumScale REST connector\n")
		return NewSpectrumRestV2(logger, config.RestConfig)
	}

i think you have here a bug.
what if ManagmentIP == "" then what?

deeghuge

Reviewable status: 0 of 9 files reviewed, 4 unresolved discussions (waiting on @deeghuge, @shay-berman, and @olgashtivelman)

local/clients.go, line 40 at r3 (raw file):

Previously, shay-berman wrote…

text fix. please remove thew "....." thing and just put one dot.
also instead of check "config file" its better to state something else like: check ubiquity-configmap parameters.

Done.

local/clients_test.go, line 26 at r3 (raw file):

Previously, shay-berman wrote…

please also add test for scale has no IP + scbe has no IP.
and another test, that you have only scale IP (without SCBE IP) and it should initialize ok.
so it will cover your change in this PR

In Current code only scbe is being initialized. Scale is not initialized because it need more changes like changing logger, db etc. And to keep the PR minimal, scale initilization will be added as part of epic2.
Along with that change we will add testcases which you mentioned.
Do you agree ?

local/spectrumscale/spectrumscale.go, line 633 at r3 (raw file):

Previously, shay-berman wrote…

but what if ManagmentIP == "" ?

This is spectrum scale specific change. We had decided that we will keep this PR minimal.
This will be taken care as part of EPIC2 changes.
Do you agree ?

local/spectrumscale/connectors/connectors.go, line 60 at r3 (raw file):

Previously, shay-berman wrote…

i think you have here a bug.
what if ManagmentIP == "" then what?

This is spectrum scale specific change. We had decided that we will keep this PR minimal.
This will be taken care as part of EPIC2 changes.
Do you agree ?

shay-berman

Reviewed 4 of 8 files at r1, 3 of 3 files at r2, 2 of 2 files at r4.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @olgashtivelman)

olgashtivelman

Reviewed 4 of 8 files at r1, 3 of 3 files at r2, 2 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved

deeghuge requested a review from shay-berman August 29, 2018 07:18

deeghuge requested a review from olgashtivelman August 29, 2018 11:16

shay-berman suggested changes Aug 29, 2018

View reviewed changes

deeghuge commented Aug 30, 2018

View reviewed changes

addressed pull request review comments for PR#243

5841a2a

- Reverted changes of NFS, Connector, SSH as they should not be part of epic1 changes. - Added variable for SpectrumScaleParamPrefix

shay-berman suggested changes Aug 30, 2018

View reviewed changes

deeghuge commented Sep 5, 2018

View reviewed changes

SpectrumScale-epic1 Testcase for clients.go changes

6fbeb27

deeghuge commented Sep 6, 2018

View reviewed changes

shay-berman suggested changes Sep 10, 2018

View reviewed changes

PR#243 Fixed the formatting of Error msg

45beb36

deeghuge commented Sep 11, 2018

View reviewed changes

shay-berman approved these changes Sep 20, 2018

View reviewed changes

olgashtivelman approved these changes Sep 20, 2018

View reviewed changes

deeghuge merged commit 422904c into dev Oct 2, 2018

deeghuge deleted the spectrumscale_epic1 branch November 20, 2018 04:36

shay-berman added the v2.0.0 label Nov 21, 2018

shay-berman added this to the v2.0.0 - Spectrum Scale support milestone Nov 21, 2018

shay-berman added Spectrum Scale and removed Spectrum Scale labels Nov 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Spectrumscale-epic1 fix for UB-1487,UB-1488,UB-1495 #243

Spectrumscale-epic1 fix for UB-1487,UB-1488,UB-1495 #243

deeghuge commented Aug 29, 2018 •

edited by midoblgsm

Loading

coveralls commented Aug 29, 2018 •

edited

Loading

shay-berman left a comment

shay-berman left a comment

deeghuge left a comment

shay-berman left a comment

shay-berman commented Sep 5, 2018

deeghuge left a comment

deeghuge left a comment

deeghuge left a comment

shay-berman left a comment

deeghuge left a comment

shay-berman left a comment

olgashtivelman left a comment

Spectrumscale-epic1 fix for UB-1487,UB-1488,UB-1495 #243

Spectrumscale-epic1 fix for UB-1487,UB-1488,UB-1495 #243

Conversation

deeghuge commented Aug 29, 2018 • edited by midoblgsm Loading

coveralls commented Aug 29, 2018 • edited Loading

shay-berman left a comment

Choose a reason for hiding this comment

shay-berman left a comment

Choose a reason for hiding this comment

deeghuge left a comment

Choose a reason for hiding this comment

shay-berman left a comment

Choose a reason for hiding this comment

shay-berman commented Sep 5, 2018

deeghuge left a comment

Choose a reason for hiding this comment

deeghuge left a comment

Choose a reason for hiding this comment

[root@node2 spectrumscale]# ginkgo -cover Running Suite: Spectrum Test Suite

deeghuge left a comment

Choose a reason for hiding this comment

shay-berman left a comment

Choose a reason for hiding this comment

deeghuge left a comment

Choose a reason for hiding this comment

shay-berman left a comment

Choose a reason for hiding this comment

olgashtivelman left a comment

Choose a reason for hiding this comment

deeghuge commented Aug 29, 2018 •

edited by midoblgsm

Loading

coveralls commented Aug 29, 2018 •

edited

Loading

[root@node2 spectrumscale]# ginkgo -cover
Running Suite: Spectrum Test Suite