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.

Sign up for GitHub

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

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

Merged
merged 1 commit into from
Aug 2, 2017
Merged

ScaleIO Volume Plugin - Volume attribute fixes and updates #48999

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions pkg/volume/scaleio/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ go_test(
"//pkg/volume:go_default_library",
"//pkg/volume/testing:go_default_library",
"//vendor/github.com/codedellemc/goscaleio/types/v1:go_default_library",
"//vendor/github.com/golang/glog:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
Expand Down
9 changes: 5 additions & 4 deletions pkg/volume/scaleio/sio_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ type sioInterface interface {
FindVolume(name string) (*siotypes.Volume, error)
Volume(sioVolumeID) (*siotypes.Volume, error)
CreateVolume(name string, sizeGB int64) (*siotypes.Volume, error)
AttachVolume(sioVolumeID) error
AttachVolume(sioVolumeID, bool) error
DetachVolume(sioVolumeID) error
DeleteVolume(sioVolumeID) error
IID() (string, error)
Expand Down Expand Up @@ -217,8 +217,9 @@ func (c *sioClient) CreateVolume(name string, sizeGB int64) (*siotypes.Volume, e
return c.Volume(sioVolumeID(createResponse.ID))
}

// AttachVolume maps the scaleio volume to an sdc node.
func (c *sioClient) AttachVolume(id sioVolumeID) error {
// AttachVolume maps the scaleio volume to an sdc node. If the multipleMappings flag
// is true, ScaleIO will allow other SDC to map to that volume.
func (c *sioClient) AttachVolume(id sioVolumeID, multipleMappings bool) error {
if err := c.init(); err != nil {
glog.Error(log("failed to init'd client in attach volume: %v", err))
return err
Expand All @@ -232,7 +233,7 @@ func (c *sioClient) AttachVolume(id sioVolumeID) error {

params := &siotypes.MapVolumeSdcParam{
SdcID: iid,
AllowMultipleMappings: "false",
AllowMultipleMappings: strconv.FormatBool(multipleMappings),
AllSdcs: "",
}
volClient := sio.NewVolume(c.client)
Expand Down
9 changes: 5 additions & 4 deletions pkg/volume/scaleio/sio_mgr.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (

type storageInterface interface {
CreateVolume(string, int64) (*siotypes.Volume, error)
AttachVolume(string) (string, error)
AttachVolume(string, bool) (string, error)
IsAttached(string) (bool, error)
DetachVolume(string) error
DeleteVolume(string) error
Expand Down Expand Up @@ -103,8 +103,9 @@ func (m *sioMgr) CreateVolume(volName string, sizeGB int64) (*siotypes.Volume, e
return vol, nil
}

// AttachVolume maps a ScaleIO volume to the running node
func (m *sioMgr) AttachVolume(volName string) (string, error) {
// AttachVolume maps a ScaleIO volume to the running node. If flag multiMaps,
// ScaleIO will allow other SDC to map to volume.
func (m *sioMgr) AttachVolume(volName string, multipleMappings bool) (string, error) {
client, err := m.getClient()
if err != nil {
glog.Error(log("attach volume failed: %v", err))
Expand Down Expand Up @@ -139,7 +140,7 @@ func (m *sioMgr) AttachVolume(volName string) (string, error) {
}

// attach volume, get deviceName
if err := client.AttachVolume(sioVolumeID(vol.ID)); err != nil {
if err := client.AttachVolume(sioVolumeID(vol.ID), multipleMappings); err != nil {
glog.Error(log("attachment for volume %s failed :%v", volName, err))
return "", err
}
Expand Down
23 changes: 13 additions & 10 deletions pkg/volume/scaleio/sio_mgr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func TestMgrCreateVolume(t *testing.T) {
func TestMgrAttachVolume(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
device, err := mgr.AttachVolume("test-vol-0001")
device, err := mgr.AttachVolume("test-vol-0001", false)
if err != nil {
t.Fatal(err)
}
Expand All @@ -111,8 +111,8 @@ func TestMgrAttachVolume(t *testing.T) {
func TestMgrAttachVolume_AlreadyAttached(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
mgr.AttachVolume("test-vol-0001")
dev, err := mgr.AttachVolume("test-vol-0001")
mgr.AttachVolume("test-vol-0001", false)
dev, err := mgr.AttachVolume("test-vol-0001", false)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
Expand All @@ -124,7 +124,8 @@ func TestMgrAttachVolume_AlreadyAttached(t *testing.T) {
func TestMgrAttachVolume_VolumeNotFoundError(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
_, err := mgr.AttachVolume("test-vol-0002")
_, err := mgr.AttachVolume("test-vol-0002", false)

if err == nil {
t.Error("attachVolume should fail with volume not found error")
}
Expand All @@ -137,7 +138,7 @@ func TestMgrAttachVolume_WaitForAttachError(t *testing.T) {
c := mgr.client.(*fakeSio)
close(c.waitAttachCtrl)
}()
_, err := mgr.AttachVolume("test-vol-0001")
_, err := mgr.AttachVolume("test-vol-0001", false)
if err == nil {
t.Error("attachVolume should fail with attach timeout error")
}
Expand All @@ -146,7 +147,7 @@ func TestMgrAttachVolume_WaitForAttachError(t *testing.T) {
func TestMgrDetachVolume(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
mgr.AttachVolume("test-vol-0001")
mgr.AttachVolume("test-vol-0001", false)
if err := mgr.DetachVolume("test-vol-0001"); err != nil {
t.Fatal(err)
}
Expand All @@ -162,7 +163,7 @@ func TestMgrDetachVolume(t *testing.T) {
func TestMgrDetachVolume_VolumeNotFound(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
mgr.AttachVolume("test-vol-0001")
mgr.AttachVolume("test-vol-0001", false)
err := mgr.DetachVolume("test-vol-0002")
if err == nil {
t.Fatal("expected a volume not found failure")
Expand All @@ -181,7 +182,7 @@ func TestMgrDetachVolume_VolumeNotAttached(t *testing.T) {
func TestMgrDetachVolume_VolumeAlreadyDetached(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
mgr.AttachVolume("test-vol-0001")
mgr.AttachVolume("test-vol-0001", false)
mgr.DetachVolume("test-vol-0001")
err := mgr.DetachVolume("test-vol-0001")
if err != nil {
Expand All @@ -192,7 +193,7 @@ func TestMgrDetachVolume_VolumeAlreadyDetached(t *testing.T) {
func TestMgrDetachVolume_WaitForDetachError(t *testing.T) {
mgr := newTestMgr(t)
mgr.CreateVolume("test-vol-0001", 8*1024*1024)
mgr.AttachVolume("test-vol-0001")
mgr.AttachVolume("test-vol-0001", false)
err := mgr.DetachVolume("test-vol-0001")
if err != nil {
t.Error("detachVolume failed")
Expand Down Expand Up @@ -227,6 +228,7 @@ type fakeSio struct {
waitAttachCtrl chan struct{}
waitDetachCtrl chan struct{}
devs map[string]string
isMultiMap bool
}

func newFakeSio() *fakeSio {
Expand Down Expand Up @@ -261,7 +263,8 @@ func (f *fakeSio) CreateVolume(volName string, sizeGB int64) (*siotypes.Volume,
return f.volume, nil
}

func (f *fakeSio) AttachVolume(id sioVolumeID) error {
func (f *fakeSio) AttachVolume(id sioVolumeID, multiMaps bool) error {
f.isMultiMap = multiMaps
_, err := f.Volume(id)
if err != nil {
return err
Expand Down
1 change: 1 addition & 0 deletions pkg/volume/scaleio/sio_plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ var _ volume.PersistentVolumePlugin = &sioPlugin{}
func (p *sioPlugin) GetAccessModes() []api.PersistentVolumeAccessMode {
return []api.PersistentVolumeAccessMode{
api.ReadWriteOnce,
api.ReadOnlyMany,
}
}

Expand Down
20 changes: 13 additions & 7 deletions pkg/volume/scaleio/sio_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,13 @@ var (
nsSep = "%"
sdcRootPath = "/opt/emc/scaleio/sdc/bin"

secretNotFoundErr = errors.New("secret not found")
configMapNotFoundErr = errors.New("configMap not found")
gatewayNotProvidedErr = errors.New("gateway not provided")
secretRefNotProvidedErr = errors.New("secret ref not provided")
systemNotProvidedErr = errors.New("secret not provided")
secretNotFoundErr = errors.New("secret not found")
configMapNotFoundErr = errors.New("configMap not found")
gatewayNotProvidedErr = errors.New("ScaleIO gateway not provided")
secretRefNotProvidedErr = errors.New("secret ref not provided")
systemNotProvidedErr = errors.New("ScaleIO system not provided")
storagePoolNotProvidedErr = errors.New("ScaleIO storage pool not provided")
protectionDomainNotProvidedErr = errors.New("ScaleIO protection domain not provided")
)

// mapScaleIOVolumeSource maps attributes from a ScaleIOVolumeSource to config
Expand Down Expand Up @@ -107,6 +109,12 @@ func validateConfigs(config map[string]string) error {
if config[confKey.system] == "" {
return systemNotProvidedErr
}
if config[confKey.storagePool] == "" {
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this break backward compatibility? A config file without storagePool and protectionDomain was accepted before and now it is not. This needs at least a release note.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsafrane You are correct, existing/deployed config are ok, but newer configs will require these values to be provided in subsequent deployments. I will add that to the release notes and the documentations will also be updated (separate PR).

Copy link
Member Author

Choose a reason for hiding this comment

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

@jsafrane another clarification on that point. The code was substituting "default" for those values when they were not provided. However, production deployments are most likely not using that value anyway. Backward compat is ok. Will still add it to release note.

return storagePoolNotProvidedErr
}
if config[confKey.protectionDomain] == "" {
return protectionDomainNotProvidedErr
}

return nil
}
Expand All @@ -119,8 +127,6 @@ func applyConfigDefaults(config map[string]string) {
b = false
}
config[confKey.sslEnabled] = strconv.FormatBool(b)
config[confKey.protectionDomain] = defaultString(config[confKey.protectionDomain], "default")
config[confKey.storagePool] = defaultString(config[confKey.storagePool], "default")
config[confKey.storageMode] = defaultString(config[confKey.storageMode], "ThinProvisioned")
config[confKey.fsType] = defaultString(config[confKey.fsType], "xfs")
b, err = strconv.ParseBool(config[confKey.readOnly])
Expand Down
4 changes: 2 additions & 2 deletions pkg/volume/scaleio/sio_util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ func TestUtilApplyConfigDefaults(t *testing.T) {
if data[confKey.system] != "sio" {
t.Error("Unexpected system value")
}
if data[confKey.protectionDomain] != "default" {
if data[confKey.protectionDomain] != "" {
t.Error("Unexpected protection domain value")
}
if data[confKey.storagePool] != "default" {
if data[confKey.storagePool] != "" {
t.Error("Unexpected storage pool value")
}
if data[confKey.volumeName] != "sio-vol" {
Expand Down
Loading