UB-1503: epic1 : Fix ubiqutiy flex Mount API to fetch the right Scale PV mountpoint

[yadaven]

This PR is needed for getting mounpoints based on respective backends during mount operation.

1. For Scale backend Attach/Detach/IsAttached will return 'Not supported'. For idempotent aspects see below:

   • in Detach if PV not found in DB it return success
   • in IsAttached if PV not found it return false.
   • Attach if PV not found in DB or for other error, it return error.
   • Note: if PV has backend that is not SCBE nor Scale the APIs should return error PvBackendNotSupportedError.

2. Fixing the Mount Flex API to support scale as well:
   For SpectrumScale we get MountPoint from volumeConfig[‘mountpoint’]. For spectrum-scale backend, mountpoint is ways fetched from Scale backend during GetVolumeConfig. For SCBE we use “Wwn” from mountRequestOptions.

---

This change is 

[shay-berman]

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

---

controller/controller.go, line 530 at r2 (raw file):

	if (volumeBackend == resources.SpectrumScale) {
		volumeMountPoint, ok = volumeConfig["mountpoint"].(string)

note - you still don't sure if mountpoint is needed in the DB.
So i am not sure you can complete this until you know if you will have mountpoint in the DB for scale backend.

---

controller/controller_test.go, line 505 at r2 (raw file):

		It("should fail to Mount when there are errors from checking other slinks (idempotent) (doMount)", func() {
			err :=  fmt.Errorf("an Error has occured")
			fakeExec.GetGlobFilesReturns(nil, err)

I am expecting to see more UT to cover the new things you added to controller.go

because it looks like you have lack of UT here.

---

controller/errors.go, line 137 at r2 (raw file):

}

const MissingMountPointVolumeErrorStr = "MountPoint Missing in Volume Config."

since its scale error, then please add Scale as prefix to the const\type of this error.
So at least it will be clear that its Scale thing

[yadaven]

---

controller/controller.go, line 530 at r2 (raw file):

Previously, shay-berman wrote…

note - you still don't sure if mountpoint is needed in the DB.
So i am not sure you can complete this until you know if you will have mountpoint in the DB for scale backend.

When we call GetVolumeConfig we get mount point SpectrumScale backend and not from Db. You can refer to getVolumeMountPoint function present in local/spectrumscale/spectrumscale.go.

[yadaven]

---

controller/controller_test.go, line 505 at r2 (raw file):

Previously, shay-berman wrote…

I am expecting to see more UT to cover the new things you added to controller.go

because it looks like you have lack of UT here.

Yes will add more UT. Kindly let me know If we can finalise above code changes so that I can start on writing UT.

[yadaven]

---

controller/errors.go, line 137 at r2 (raw file):

Previously, shay-berman wrote…

since its scale error, then please add Scale as prefix to the const\type of this error.
So at least it will be clear that its Scale thing

ok will make this change

[yadaven]

---

controller/controller_test.go, line 505 at r2 (raw file):

Previously, yadaven (Yadavendra Yadav) wrote…

Yes will add more UT. Kindly let me know If we can finalise above code changes so that I can start on writing UT.

Added UT in commit b5ef6fe

[yadaven]

---

controller/errors.go, line 137 at r2 (raw file):

Previously, yadaven (Yadavendra Yadav) wrote…

ok will make this change

Modified const/type for Missing mountpoint error to contain SpectrumScale. Changes have been added as part of commit b5ef6fe

[shay-berman]

First, please update the PR description with more detail about this PR content. So everyone will be able to understand what this PR offers.

Reviewable status: 0 of 3 files reviewed, all discussions resolved (waiting on @olgashtivelman)

---

controller/controller.go, line 530 at r2 (raw file):

Previously, yadaven (Yadavendra Yadav) wrote…

When we call GetVolumeConfig we get mount point SpectrumScale backend and not from Db. You can refer to getVolumeMountPoint function present in local/spectrumscale/spectrumscale.go.

ok good, so just clarify - you removed the mountpoint from the DB and instead everytime getVolumeConfig called it automatically get the mountpoint from the Scale system? (BTW i didn't see any PR that propose this fix, so please clarify it)

in addition, please make sure that you don't need to make it as 2 commands - volumemountpoint, ok = volumeConfig["mountpoint"]..... and then to make it a string. just make sure its ok. and add UT that check case that mountpoint key not exist.

[yadaven]

---

controller/controller.go, line 530 at r2 (raw file):

Previously, shay-berman wrote…

ok good, so just clarify - you removed the mountpoint from the DB and instead everytime getVolumeConfig called it automatically get the mountpoint from the Scale system? (BTW i didn't see any PR that propose this fix, so please clarify it)

in addition, please make sure that you don't need to make it as 2 commands - volumemountpoint, ok = volumeConfig["mountpoint"]..... and then to make it a string. just make sure its ok. and add UT that check case that mountpoint key not exist.

While getting VolumeConfig we always got mountpoint from Scale system. This code is already present and thus no PR for that ( we can refer getVolumeMountPoint in local/spectrumscale/spectrumscale.go.) .

Currently volumeConfig is map[string]interface{}. Since value for key is interface{}. we need to typecast it to string.

[shay-berman]

Reviewable status: 0 of 3 files reviewed, 5 unresolved discussions (waiting on @yadaven and @olgashtivelman)

---

controller/controller.go, line 714 at r5 (raw file):

		lnPath = k8sPVDirectory
	} else {
		lnPath = k8sPVDirectory

So why do you need the if\else here?
just drop it and lnPath=k8sPVDirctory

though? do i miss something here?

---

controller/controller.go, line 715 at r5 (raw file):

	} else {
		lnPath = k8sPVDirectory
		// Keeping this separate condition for scale-nfs support in future.

please put TODO prefix to this comment. but anyway you don't need if\else. in the future if you need it add it. but not now

---

controller/controller.go, line 963 at r5 (raw file):

        getVolumeRequest := resources.GetVolumeRequest{Name: volName}
        volume, err := c.Client.GetVolume(getVolumeRequest)

are you sure that we don't call to GetVolume already in the flow? if so it means we do so twice. please doublecheck to reduce the calls to ubiqutiy server.

maybe you can avoid hostattach in higher level and not here.

---

controller/controller.go, line 996 at r5 (raw file):

	// Only in k8s 1.5 this os.Hostname will happened,
	// because in k8s 1.5 the flex CLI doesn't get the host to attach with. TODO consider to refactor to remove support for 1.5
	// Spectrum Scale uses this method for 2.0 release.

why do you need this comment? what does it mean? is it being used for scale? if so how. try to be more specific in your comment here.

---

controller/controller_test.go, line 227 at r5 (raw file):

		It("should fail to prepareUbiquityMountRequest if GetVolumeConfig does not contain valid backend", func() {

please make sure you cover byunit testing all the new if\else you added

[yadaven]

---

controller/controller_test.go, line 227 at r5 (raw file):

Previously, shay-berman wrote…

please make sure you cover byunit testing all the new if\else you added

Yes I have added UT for below cases i.e

1. If backend is spectrum scale and mountpoint is not present in volumeConfig we return error
2. if backend is invalid we return proper error.
3. For SCBE if volumeConfig does not contain Wwn that test case was already there.

[deeghuge]

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

---

controller/controller.go, line 714 at r5 (raw file):

Previously, shay-berman wrote…

So why do you need the if\else here?
just drop it and lnPath=k8sPVDirctory

though? do i miss something here?

I had kept that way because I wanted to keep condition to for scbe backend as is. And in future when we add nfs functionality only scale condition need to be added. But I agree with you comment. Also code doesnt look good this way. Hence as you suggested I will remove the redundancy and unnecessory check. In future whenever we add nfs support we will add required condition's again.

---

controller/controller.go, line 715 at r5 (raw file):

Previously, shay-berman wrote…

please put TODO prefix to this comment. but anyway you don't need if\else. in the future if you need it add it. but not now

Agree

---

controller/controller.go, line 963 at r5 (raw file):

Previously, shay-berman wrote…

are you sure that we don't call to GetVolume already in the flow? if so it means we do so twice. please doublecheck to reduce the calls to ubiqutiy server.

maybe you can avoid hostattach in higher level and not here.

Yes, This is the only call for getVolume in Detach flow.

---

controller/controller.go, line 996 at r5 (raw file):

Previously, shay-berman wrote…

why do you need this comment? what does it mean? is it being used for scale? if so how. try to be more specific in your comment here.

Agree. Added more details

•   // Spectrum Scale uses this method to get the hostname of current node for detach and isHostAttached functionality as Spectrum Scale volume is not attached to any specific node.
  

•   // TODO: Remove this dependancy while optimizing the Spectrum Scale detach functionality.
  

[olgashtivelman]

Reviewed 2 of 3 files at r4, 1 of 1 files at r5.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @shay-berman)

[deeghuge]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @shay-berman and @yadaven)

---

controller/controller.go, line 963 at r5 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

Yes, This is the only call for getVolume in Detach flow.

The current code fix both IsAttached and Detach functionality.
Other option is
in Detach function itself before calling doDetach() get the backend by calling GetVolume() and for spectrumscale backend return success.

Above will only fix the Detach for spectrumscale.
To Fix the IsAttached functionality - In IsAttached() before calling doIsAttached(), get the backend by calling GetVolume() and for spectrumscale backend return success.

Please suggest which option to go for?

[shay-berman]

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

---

controller/controller.go, line 963 at r5 (raw file):

twice
as mentioned in the call
please rebase from dev - because u r working on old code. so first rebase, then make sure your solution is ok.

---

controller/controller.go, line 712 at r6 (raw file):

	lnPath = k8sPVDirectory
	// TODO: Keeping this function for scale-nfs support in future.
	return lnPath

so this function do nothing.
if u want for future use to keep it its ok. but then please just do one line : return k8sPVDirectory

[deeghuge]

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @olgashtivelman, @shay-berman, and @yadaven)

---

controller/controller.go, line 264 at r16 (raw file):

Previously, olgashtivelman wrote…

its better to use "successFlexVolumeResponse" or "failureFlexVolumeResponse" for success/failure responses here and in the other places where there are no special parameters (since it also logs and we do not have to depend on the "success" string)

also it should be "Retuning success since volume does not exist" (not - exists :) )

and maybe we should also add a func for NotSupportedFlexVolumeResponse that will do the logging as well. just a thought.

Agree, Added notSupportedFlexVolumeResponse as well as used successFlexVolumeResponse wherever required.

[olgashtivelman]

Reviewable status: 3 of 5 files reviewed, 5 unresolved discussions (waiting on @olgashtivelman and @shay-berman)

[shay-berman]

Hi Deepack, I just reviewed your PR and you need to fix some aspects. Please review my comments.

Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions (waiting on @olgashtivelman and @yadaven)

---

glide.yaml, line 113 at r17 (raw file):

  - lib
- package: github.com/IBM/ubiquity
  version: 19891dbfbc526a5f56e87bcccb6342f638dd8080

just make sure its the latest ubiqutiy commit in dev.
I think its now 422904c4c777d359341f22687ab6e3c9b459c470

---

cmd/provisioner/main/main.go, line 46 at r17 (raw file):

		if we ever move to a newer code version this can be removed.
	*/  
	flag.CommandLine.Parse([]string{})

why this code is in this PR?
isn't it a code that already exist in dev? (please make sure you will not have collision)

---

controller/controller.go, line 712 at r6 (raw file):

Previously, deeghuge (Deepak Ghuge) wrote…

Done

second though, just delete this function. its do nothing. instead just use the k8sPVDirectory.
In next version if you want to support NFS then do this , but not now.

---

controller/controller.go, line 137 at r17 (raw file):

	if err != nil {
		errormsg := fmt.Sprintf("Failed to get Volume details [%s]", attachRequest.Name)

please Add comment

TODO, later we should consider to get the volume backend by using the attachRequest.opts instead of calling to ubiqutiy server.

---

controller/controller.go, line 143 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
            	response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

Please clarify the text :
"Flex Attach API is not supported for Spectrum-Scale backend, because the backend assume the volume already attached to the node"
Please make sure my english is correct.

---

controller/controller.go, line 208 at r17 (raw file):

	getVolumeRequest := resources.GetVolumeRequest{Name: volName}
	volume, err :=  c.Client.GetVolume(getVolumeRequest)

You forgot to handle idempotancy here as mentioned in the requirement: "add GetVolume if scale then not support (if PV not exist then just return error)"

So you need to check if getvolume return error because PV not exist in ubiqutiy-DB, and if so(PV not in ubiqutiy-db) then you should continue OK with idempotent warning (but if getvolume failed due to other error then you should fail with error). Exactly the same as -> https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

Also please add here a comment with # TODO, later we should consider to get the volume backend by using the IsAttachRequest.opts instead of calling to ubiqutiy server.

---

controller/controller.go, line 218 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
        	response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

please fix the text, with the same spirit as mentioned above

---

controller/controller.go, line 257 at r17 (raw file):

		if err != nil {
            		response = c.successFlexVolumeResponse("Returning success since volume does not exist")

wait you have here the same bug as mentioned above.
you need to check if the getvolume error is specifically mentioned that PV not exist, and only then you should return success with warnning idempotancy message.
please fix, it should be the same as https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

---

controller/controller.go, line 262 at r17 (raw file):

	    	if (volume.Backend == resources.SpectrumScale) {
            		response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")

please fix text as mentioned above.

---

controller/controller.go, line 263 at r17 (raw file):

	    	if (volume.Backend == resources.SpectrumScale) {
            		response = c.notSupportedFlexVolumeResponse("Not supported for spectrum-scale backend")
			return response

I think you have indentation issue. you forgot few spaces for the return. (try to use gofmt in your IDE)

---

controller/controller.go, line 449 at r17 (raw file):

		return fmt.Sprintf(resources.PathToMountUbiquityBlockDevices, volumeConfig["Wwn"].(string)), nil
	} else if volumeBackend == resources.SpectrumScale {
                return volumeConfig["mountpoint"].(string), nil

Add comment:
#TODO, scale should remove the mountpoint.

---

controller/controller.go, line 544 at r17 (raw file):

	if (volume.Backend == resources.SpectrumScale) {
		return c.successFlexVolumeResponse("")
	}

I prefer more explicit here. So it will be clear that its different for scale vs scbe and also check if its other backend (if so then fail with not supported backend.)
something like this:

if (volume.Backend == resources.SpectrumScale) {
            # Scale has no detach process after unmount
	return c.successFlexVolumeResponse("")
    } else if (volume.Backend == resources.SCBE) {
    // Do legacy detach (means trigger detach as part of the umount from the k8s node)
	    if err := c.doLegacyDetach(unmountRequest); err != nil {
  	return c.failureFlexVolumeResponse(err, "")
    } 

 
return c.successFlexVolumeResponse("")
} else {

print some error like backend not supported (I think there is already error like that you can leverage -> see PvBackendNotSupportedError

)
}

In addition, I would expect to see UT for this scale if\else

---

controller/controller.go, line 589 at r17 (raw file):

}

func (c *Controller) getMountpointForVolume(mountRequest k8sresources.FlexVolumeMountRequest, volumeConfig map[string]interface{}, volumeBackend string) (string, error) {

please make sure u have UT for this

---

controller/controller.go, line 977 at r17 (raw file):

		// we can go back to using regulat getHostAttached and remove getHostAttachUsingConfig.
		getVolumeConfigRequest := resources.GetVolumeConfigRequest{Name: detachRequest.Name, Context: detachRequest.Context}
		volumeConfig, err := c.Client.GetVolumeConfig(getVolumeConfigRequest)

what is this code?
how its related to PR detail? it looks like olga code that is already in DEV.
please check it out

---

controller/controller.go, line 989 at r17 (raw file):

		if host == "" {
			// this means that the host is not attached to anything so no reason to call detach
			c.logger.Warning(fmt.Sprintf("Idempotent issue encoutered - vol is not attached to any host. so no detach action is called"), logs.Args{{"vol wwn", volumeConfig["Wwn"]}})

also this one?

---

controller/controller.go, line 1022 at r17 (raw file):

}

func (c *Controller) getHostAttachUsingConfig(volumeConfig map[string]interface{}) (string, error){

is this related to scale changes?

---

controller/controller_test.go, line 1031 at r17 (raw file):

		})

        It("IsAttached should return success with Attached as false when GetVolume Fails", func() {

need to fix it

it should succeed only if getvolume return error that the PV not in ubiqutiy DB, else it should fail. as mentioned above. please fix UT according.

---

controller/controller_test.go, line 1068 at r17 (raw file):

		})

        It("Pass when Detach fails to get volume details", func() {

only if PV not exist in DB error, else it should fail.

[yadaven]

---

controller/controller.go, line 589 at r17 (raw file):

Previously, shay-berman wrote…

please make sure u have UT for this

Yes we have added UT for this

[shay-berman]

Reviewable status: 3 of 5 files reviewed, 19 unresolved discussions (waiting on @olgashtivelman, @yadaven, and @shay-berman)

---

controller/controller.go, line 222 at r17 (raw file):

	}

	isAttached, err := c.doIsAttached(isAttachedRequest)

BTW
please check that the detach\attach or mount\unmount doesn't use this doIsAttached - because if so then maybe you should skip calling it if its scale backend.
please check and let me know if it could lead you to an issue.

thanks

---

controller/controller.go, line 1022 at r17 (raw file):

Previously, shay-berman wrote…

is this related to scale changes?

maybe its reviewable issue. it look like olga code and i can see it here as added into the PR. So i think its ok.

[yadaven]

---

controller/controller.go, line 222 at r17 (raw file):

Previously, shay-berman wrote…

BTW
please check that the detach\attach or mount\unmount doesn't use this doIsAttached - because if so then maybe you should skip calling it if its scale backend.
please check and let me know if it could lead you to an issue.

thanks

doIsAttached is called from below functions

• doDetach

• IsAttached

For IsAttached we return “Not Supported”

doDetach is called from Detach & doLegacyDetach.

We return “Not Supported” for Detach and we do not call doLegacyDetach during unmount.

[yadaven]

---

glide.yaml, line 113 at r17 (raw file):

Previously, shay-berman wrote…

just make sure its the latest ubiqutiy commit in dev.
I think its now 422904c4c777d359341f22687ab6e3c9b459c470

Sure we will make sure to use latest ubiquity commit.

[yadaven]

---

cmd/provisioner/main/main.go, line 46 at r17 (raw file):

Previously, shay-berman wrote…

why this code is in this PR?
isn't it a code that already exist in dev? (please make sure you will not have collision)

This is due to rebase to dev

[yadaven]

---

controller/controller.go, line 208 at r17 (raw file):

Previously, shay-berman wrote…

You forgot to handle idempotancy here as mentioned in the requirement: "add GetVolume if scale then not support (if PV not exist then just return error)"

So you need to check if getvolume return error because PV not exist in ubiqutiy-DB, and if so(PV not in ubiqutiy-db) then you should continue OK with idempotent warning (but if getvolume failed due to other error then you should fail with error). Exactly the same as -> https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

Also please add here a comment with # TODO, later we should consider to get the volume backend by using the IsAttachRequest.opts instead of calling to ubiqutiy server.

This has been taken care in latest commit 30810a7

[yadaven]

---

controller/controller.go, line 257 at r17 (raw file):

Previously, shay-berman wrote…

wait you have here the same bug as mentioned above.
you need to check if the getvolume error is specifically mentioned that PV not exist, and only then you should return success with warnning idempotancy message.
please fix, it should be the same as https://github.com/IBM/ubiquity-k8s/blob/master/controller/controller.go#L457

This has been taken care in latest commit 30810a7

[yadaven]

---

controller/controller.go, line 544 at r17 (raw file):

Previously, shay-berman wrote…

I prefer more explicit here. So it will be clear that its different for scale vs scbe and also check if its other backend (if so then fail with not supported backend.)
something like this:

if (volume.Backend == resources.SpectrumScale) {
            # Scale has no detach process after unmount
	return c.successFlexVolumeResponse("")
    } else if (volume.Backend == resources.SCBE) {
    // Do legacy detach (means trigger detach as part of the umount from the k8s node)
	    if err := c.doLegacyDetach(unmountRequest); err != nil {
  	return c.failureFlexVolumeResponse(err, "")
    } 

 
return c.successFlexVolumeResponse("")
} else {

print some error like backend not supported (I think there is already error like that you can leverage -> see PvBackendNotSupportedError

)
}

In addition, I would expect to see UT for this scale if\else

This has been taken care in 30810a7. We have not added UT for this because if backend is not valid getMounterForBackend will fail and control will not reach till this step.

[yadaven]

---

controller/controller.go, line 977 at r17 (raw file):

Previously, shay-berman wrote…

what is this code?
how its related to PR detail? it looks like olga code that is already in DEV.
please check it out

This is due to rebase to dev

[yadaven]

---

controller/controller.go, line 989 at r17 (raw file):

Previously, shay-berman wrote…

also this one?

this is die to rebase to dev

[yadaven]

---

controller/controller.go, line 1022 at r17 (raw file):

Previously, shay-berman wrote…

maybe its reviewable issue. it look like olga code and i can see it here as added into the PR. So i think its ok.

This is due to rebase to dev

[yadaven]

---

controller/controller_test.go, line 1031 at r17 (raw file):

Previously, shay-berman wrote…

need to fix it

it should succeed only if getvolume return error that the PV not in ubiqutiy DB, else it should fail. as mentioned above. please fix UT according.

We have fixed this UT in latest commit 30810a7

[yadaven]

---

controller/controller_test.go, line 1068 at r17 (raw file):

Previously, shay-berman wrote…

only if PV not exist in DB error, else it should fail.

We have fixed this UT in latest commit 30810a7

[deeghuge]

Reviewable status: 1 of 5 files reviewed, 17 unresolved discussions (waiting on @olgashtivelman, @yadaven, and @shay-berman)

---

controller/controller.go, line 712 at r6 (raw file):

Previously, shay-berman wrote…

second though, just delete this function. its do nothing. instead just use the k8sPVDirectory.
In next version if you want to support NFS then do this , but not now.

Done

---

controller/controller.go, line 137 at r17 (raw file):

Previously, shay-berman wrote…

please Add comment

TODO, later we should consider to get the volume backend by using the attachRequest.opts instead of calling to ubiqutiy server.

Done

---

controller/controller.go, line 143 at r17 (raw file):

Previously, shay-berman wrote…

Please clarify the text :
"Flex Attach API is not supported for Spectrum-Scale backend, because the backend assume the volume already attached to the node"
Please make sure my english is correct.

done

---

controller/controller.go, line 218 at r17 (raw file):

Previously, shay-berman wrote…

please fix the text, with the same spirit as mentioned above

Done

---

controller/controller.go, line 262 at r17 (raw file):

Previously, shay-berman wrote…

please fix text as mentioned above.

Done

---

controller/controller.go, line 263 at r17 (raw file):

Previously, shay-berman wrote…

I think you have indentation issue. you forgot few spaces for the return. (try to use gofmt in your IDE)

Done

---

controller/controller.go, line 449 at r17 (raw file):

Previously, shay-berman wrote…

Add comment:
#TODO, scale should remove the mountpoint.

Done

[shay-berman]

Well done
I will trigger staging now. after staging is green then you will be able to merge to dev.

Reviewed 1 of 1 files at r9, 2 of 4 files at r18, 2 of 2 files at r19.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @olgashtivelman)

[shay-berman]

CI sanity and extended pass OK.
@deeghuge go ahead and merge it to dev (in "squash and merge" mode)