Fix/ub 945 ubiquity for ds8k p1

[hfeish]

Changes:
1. DB name for DS8k: uibm-ubiquity-db, DB name for SVC/XIV: u_{instance}_ibm- ubiquity-db
2. Normal PV name: pv-name label in mandatory

---

This change is 

[coveralls]

Coverage increased (+0.1%) to 54.927% when pulling 5dca665 on fix/UB-945_Ubiquity_For_DS8k_P1 into c94b05f on dev.

[shay-berman]

I think you should keep instance name in the DB volume name also for DS8k. does make sense to remove this feature for DS8k.

[shay-berman]

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

local/scbe/scbe.go, line 45 at r1 (raw file):

	OptionNameForVolumeSize  = "size"
	volumeNamePrefix         = "u_"
	volumeNamePrefix_DS8k    = "u"

I think we should keep u__ so only the vol DB you should shrink it from ibm_ubiquity_db to ibmdb
so you don't change any behavior just the vol name.

---

local/scbe/scbe.go, line 53 at r1 (raw file):

	GetVolumeConfigExtraParams = 2 // number of extra params added to the VolumeConfig beyond the scbe volume struct
	VolumeNameLengthInvalidMessage = "Volume names are limited to 16 characters"

looks like this error message is from SCBE. so please put this error str inside scbe_rest_client.go (its not related to local/scbe.go file) and just use it from there.
I would even add function in scbe_rest_client.go that check if error contains this type of error, and just call this function in the scbe.go.
seperation of concerns, scbe.go should focus on ubiquity integration to local backend. scbe_rest_client.go focus on scbe communications.

---

local/scbe/scbe.go, line 275 at r1 (raw file):

	volInfo, err = scbeRestClient.CreateVolume(volNameToCreate, profile, size)
	if err != nil {
		if strings.Contains(err.Error(), VolumeNameLengthInvalidMessage) {

please put all this scope in a function and call it ds8k_short_volume_name_handling()

---

local/scbe/scbe.go, line 278 at r1 (raw file):

			if database.IsDatabaseVolume(volNameToCreate) {
				//Only if db volume longer than 16, will recompose the db volume name and recreate, for other volume, will return err
				volNameToCreate = fmt.Sprintf(ComposeVolumeName_DS8k, database.VolumeNameSuffix)

just use the same ComposeVolumeName but instead of database.VolumeNameSuffix, just database.VolumeNameSuffixForDS8K
So only the volume suffix change for ds8k but the pattern of u__ is still in place.

of cause you should update the IsDatabaseVolume to check also the ds8k suffix.

I think its better because it will enable you to use the instance to separate between different instances of ubiquities.

---

local/scbe/scbe.go, line 281 at r1 (raw file):

				volInfo, err = scbeRestClient.CreateVolume(volNameToCreate, profile, size)
				if err != nil {
					return s.logger.ErrorRet(err, "scbeRestClient.CreateDBVolume failed")

improve the message : "scbeRestClient.CreateDBVolume failed also with short name for DS8K"

---

local/scbe/scbe.go, line 286 at r1 (raw file):

		}
		return s.logger.ErrorRet(err, "scbeRestClient.CreateVolume failed")
	}

Fei, please add Unit testing to cover this new functionality in scbe_test.go.
Make sure the coverage will not go done due to this fix.

---

local/scbe/simple_rest_client.go, line 172 at r1 (raw file):

    if response.StatusCode != exitStatus {
        return s.logger.ErrorRet(errors.New("bad status code " + response.Status), "failed", logs.Args{{actionName, url},
            {"data", string(data[:])}})

what is the motivation?

---

Comments from Reviewable

[alonm]

Review status: 0 of 2 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

local/scbe/scbe.go, line 45 at r1 (raw file):

Previously, shay-berman wrote…

I think we should keep u__ so only the vol DB you should shrink it from ibm_ubiquity_db to ibmdb
so you don't change any behavior just the vol name.

An alternative solution. volumeNamePrefix should be "u". ComposeVolumeName would be volumeNamePrefix + "%s%s" and for ds8k it would be just "%s".

Note that we lose some functionality from removing the instance-id.

---

Comments from Reviewable

[hfeish]

@Shay
I agree with alonm to use u%s for ds8k, because I have some concerns if we use u_{instance}_ibmdb for DS8k

1. For SVC/XIV, we use u_instance_ibm-ubiquity-db, for DS8k we use u_instance_ibmdb, do we have upgrade issue to change the db name to ibmdb? Also we hardcode "ibm-ubiquity-db" in install/uninstall script, seems we also need to change it, right?
2. If we change name in the ubiquity server side, we need return this name to Provisioner side, so that provisioner will get the new db name, and get it successfully, so we also need add new api between ubiquity server and provisioner to return this new volume name, this was implemented in proposal 2, but didn't test, so maybe some issue here
3. In the email from Samuel, DS8k has a PCR for changing the volume name length in Version 8.3, 3Q2018, so if we use uibm-ubiquity-db, it is easy for us to change back to u_instance_ibm-ubiquity-db for ds8k then
4. Do we need to complete Ds8k dev and test before our vacation this Friday? if yes, we have little time left.

For local/scbe/simple_rest_client.go, line 172 at r1 (raw file):
if response.StatusCode != exitStatus {
return s.logger.ErrorRet(errors.New("bad status code " + response.Status), "failed", logs.Args{{actionName, url},
{"data", string(data[:])}})

SCBE add the error message which contains "Volume names are limited to 16 characters" in the body, so we need to add body in the return

BTW,I cannot access Reviewable for network issue.

[shay-berman]

As mentioned, I prefer not to accept this PR because the right way to support DS8k is just to make the ibm-ubiquity-db name as a configurable value, Instead of doing hooks to handle ds8k short name in the code.

So Please open a new PR with changes needed (mainly in the ubiquity-k8s repo) to make it configurable. Then ds8k customer will just configure a shorter ubiquity db name shorter then the default(ibm-ubiquity-db). No code change in the ubiquity logic, accept then getting the db vol name as environment variable.

[hfeish]

Will close this PR:
Create new PRs with ibm-ubiquity-db configurable:
IBM/ubiquity-k8s#167
#186