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.

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

feat(localpv-device): allow local pv device on select devices #1648

Merged
merged 4 commits into from
Apr 8, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
47 changes: 47 additions & 0 deletions cmd/provisioner-localpv/app/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,45 @@ const (
//KeyPVStorageType defines if the PV should be backed
// a hostpath ( sub directory or a storage device)
KeyPVStorageType = "StorageType"

//KeyPVBasePath defines base directory for hostpath volumes
// can be configured via the StorageClass annotations.
KeyPVBasePath = "BasePath"

//KeyPVFSType defines filesystem type to be used with devices
// and can be configured via the StorageClass annotations.
KeyPVFSType = "FSType"

//KeyBDPoolName defines the value for the Block Device Pool
//label selector configured via the StorageClass annotations.
//User can group block devices across nodes to form a block
//device pool, by setting the label on block devices as:
// openebs.io/block-device-pool=<pool-name>
//
//The <pool-name> used above can be passsed to the
//Local PV device provisioner via the StorageClass
//CAS annotations.
//
//Example: Local PV device StorageClass for picking devices
//labeled as: openebs.io/block-device-pool=pool-x
//will be as follows
//
// kind: StorageClass
// metadata:
// name: openebs-device-pool-x
// annotations:
// openebs.io/cas-type: local
// cas.openebs.io/config: |
// - name: StorageType
// value: "device"
// - name: BlockDevicePoolName
// value: "pool-x"
// provisioner: openebs.io/local
// volumeBindingMode: WaitForFirstConsumer
// reclaimPolicy: Delete
//
KeyBDPoolName = "BlockDevicePoolName"
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be called as 'LocalPVPoolName'?
Not sure how BlockDevicePoolName makes sense here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name is being derived from label openebs.io/block-device-pool, is set on the block devices.

Just thinking out loud, the pool concept can be implemented using this label selector. We need a fixed label key that is shared by NDM and all its consumers. "Pool" may be a over-loaded term.

How about using "openebs.io/block-device-tag"? And make the corresponding naming changes to the above Key, and other method names and variables.

Copy link

@AmitKumarDas AmitKumarDas Apr 6, 2020

Choose a reason for hiding this comment

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

Is this understood by NDM? If yes then label key should have some indication.
I could think of below label key

blockdevice.ndm.openebs.io/tag

Choose a reason for hiding this comment

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

If we want to go with pool then blockdevice.ndm.openebs.io/pool-name

Choose a reason for hiding this comment

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

If this label is applicable to BlockDevice & BlockDeviceClaim then NDM is not required in the label key. Sorry for above confusions.

So we can have openebs.io/block-device-tag. The one you had suggested initially.

Copy link
Contributor

Choose a reason for hiding this comment

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

with this stmt, We need a fixed label key that is shared by NDM and all its consumers are we saying - every NDM consumer should be fixed to this label? We don't need this as there is 'Selector' in spec of BD. If it was part of some labels of BD, it might be the case.
By fixing it to fixed string, 'Selector' in 'spec' of BD can be removed and moved to label of 'BD'.
If this is just for local pv, this key has to get into StorageClass, and in the 'Selector' of BD.
Is my understanding correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will be understood by NDM and Local PV (and other storage engines possibly in future):

  • NDM will have special handling for this label. Like if BD has label, provide it to only those BDCs coming with the label.
  • Local PV will use it by reading it from Storage Class and passing it to BDC
  • CSPC operator might label the block devices with a tag, before claiming them.

Copy link
Contributor

Choose a reason for hiding this comment

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

is this the same tag for every consumer of NDM?

Copy link
Contributor

Choose a reason for hiding this comment

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

can we get example of BD for above example?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant - example mentioned in PR


//KeyPVRelativePath defines the alternate folder name under the BasePath
// By default, the pv name will be used as the folder name.
// KeyPVBasePath can be useful for providing the same underlying folder
Expand Down Expand Up @@ -139,6 +172,20 @@ func (c *VolumeConfig) GetFSType() string {
return fsType
}

//GetBDPoolName returns the block device pool label
//value configured in StorageClass.
//
//Default is "", no device pool will be set and all
//the available block devices can be used for creating
//Local PV.
func (c *VolumeConfig) GetBDPoolName() string {
bdPoolName := c.getValue(KeyBDPoolName)
if len(strings.TrimSpace(bdPoolName)) == 0 {
return ""
}
return bdPoolName
}

//GetPath returns a valid PV path based on the configuration
// or an error. The Path is constructed using the following rules:
// If AbsolutePath is specified return it. (Future)
Expand Down
16 changes: 13 additions & 3 deletions cmd/provisioner-localpv/app/helper_blockdevice.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ type HelperBlockDeviceOptions struct {
bdcName string
// volumeMode of PVC
volumeMode corev1.PersistentVolumeMode

//bdPoolName is the value passed for
// BlockDevicePoolName via StorageClass config
bdPoolName string
}

// validate checks that the required fields to create BDC
Expand Down Expand Up @@ -120,14 +124,20 @@ func (p *Provisioner) createBlockDeviceClaim(blkDevOpts *HelperBlockDeviceOption
return nil
}

bdcObj, err := blockdeviceclaim.NewBuilder().
bdcObjBuilder := blockdeviceclaim.NewBuilder().
WithNamespace(p.namespace).
WithName(bdcName).
WithHostName(blkDevOpts.nodeHostname).
WithCapacity(blkDevOpts.capacity).
WithFinalizer(LocalPVFinalizer).
WithBlockVolumeMode(blkDevOpts.volumeMode).
Build()
WithBlockVolumeMode(blkDevOpts.volumeMode)

// If bdPoolName is configure, set it on the BDC
if len(blkDevOpts.bdPoolName) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make poolName as compulsory? This will avoid for any race between app/pvc deployment and labeling of BDs.
Even though this doesn't happen usually, this can happen with south bound provisioner.

bdcObjBuilder.WithBlockDevicePoolName(blkDevOpts.bdPoolName)
}

bdcObj, err := bdcObjBuilder.Build()

if err != nil {
//TODO : Need to relook at this error
Expand Down
1 change: 1 addition & 0 deletions cmd/provisioner-localpv/app/provisioner_blockdevice.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ func (p *Provisioner) ProvisionBlockDevice(opts pvController.VolumeOptions, volu
name: name,
capacity: capacity.String(),
volumeMode: *opts.PVC.Spec.VolumeMode,
bdPoolName: volumeConfig.GetBDPoolName(),
}

path, blkPath, err := p.getBlockDevicePath(blkDevOpts)
Expand Down
30 changes: 30 additions & 0 deletions pkg/blockdeviceclaim/v1alpha1/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,13 @@ const (

// StoragePoolKindCSPC holds the value of CStorPoolCluster
StoragePoolKindCSPC = "CStorPoolCluster"

// APIVersion holds the value of OpenEBS version
APIVersion = "openebs.io/v1alpha1"

// bdPoolKey defines the label selector key
// used for grouping block devices on a given node
bdPoolKey = "openebs.io/block-device-pool"
)

// Builder is the builder object for BlockDeviceClaim
Expand Down Expand Up @@ -323,3 +328,28 @@ func (b *Builder) WithBlockVolumeMode(mode corev1.PersistentVolumeMode) *Builder

return b
}

// WithBlockDevicePoolName appends (or creates) the BDC Label Selector
// by setting the provided value to the fixed key
// openebs.io/block-device-pool
// This will enable the NDM to pick only devices that
// match the node (hostname) and block device pool label.
func (b *Builder) WithBlockDevicePoolName(bdPoolName string) *Builder {
kmova marked this conversation as resolved.
Show resolved Hide resolved
if len(bdPoolName) == 0 {
b.errs = append(
b.errs,
errors.New("failed to build BDC object: missing block device pool name"),
)
return b
}

if b.BDC.Object.Spec.Selector == nil {
kmova marked this conversation as resolved.
Show resolved Hide resolved
b.BDC.Object.Spec.Selector = &metav1.LabelSelector{}
}
if b.BDC.Object.Spec.Selector.MatchLabels == nil {
b.BDC.Object.Spec.Selector.MatchLabels = map[string]string{}
}

b.BDC.Object.Spec.Selector.MatchLabels[bdPoolKey] = bdPoolName
return b
}
63 changes: 62 additions & 1 deletion pkg/blockdeviceclaim/v1alpha1/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,16 +206,46 @@ func TestBuildWithCapacity(t *testing.T) {
}
}

func TestBuilderWithBlockDevicePoolName(t *testing.T) {
tests := map[string]struct {
name string
expectErr bool
}{
"Test Builder with pool name": {
name: "test",
expectErr: false,
},
"Test Builder without pool name": {
name: "",
expectErr: true,
},
}
for name, mock := range tests {
name, mock := name, mock
t.Run(name, func(t *testing.T) {
b := NewBuilder().WithBlockDevicePoolName(mock.name)
if mock.expectErr && len(b.errs) == 0 {
t.Fatalf("Test %q failed: expected error not to be nil", name)
}
if !mock.expectErr && len(b.errs) > 0 {
t.Fatalf("Test %q failed: expected error to be nil", name)
}
})
}
}

func TestBuild(t *testing.T) {
tests := map[string]struct {
name string
capacity string
poolName string
expectedBDC *apis.BlockDeviceClaim
expectedErr bool
}{
"BDC with correct details": {
name: "BDC1",
capacity: "10Ti",
poolName: "",
expectedBDC: &apis.BlockDeviceClaim{
ObjectMeta: metav1.ObjectMeta{Name: "BDC1"},
Spec: apis.DeviceClaimSpec{
Expand All @@ -228,17 +258,48 @@ func TestBuild(t *testing.T) {
},
expectedErr: false,
},
"BDC with correct details, including device pool": {
name: "BDC1",
capacity: "10Ti",
poolName: "test",
expectedBDC: &apis.BlockDeviceClaim{
ObjectMeta: metav1.ObjectMeta{Name: "BDC1"},
Spec: apis.DeviceClaimSpec{
Resources: apis.DeviceClaimResources{
Requests: corev1.ResourceList{
corev1.ResourceName(ndm.ResourceStorage): fakeCapacity("10Ti"),
},
},
Selector: &metav1.LabelSelector{
MatchLabels: map[string]string{
bdPoolKey: "test",
},
},
},
},
expectedErr: false,
},
"BDC with error": {
name: "",
capacity: "500Gi",
poolName: "test",
expectedBDC: nil,
expectedErr: true,
},
}
for name, mock := range tests {
name, mock := name, mock
t.Run(name, func(t *testing.T) {
bdcObj, err := NewBuilder().WithName(mock.name).WithCapacity(mock.capacity).Build()
bdcObjBuilder := NewBuilder().
WithName(mock.name).
WithCapacity(mock.capacity)

if len(mock.poolName) > 0 {
bdcObjBuilder.WithBlockDevicePoolName(mock.poolName)
}

bdcObj, err := bdcObjBuilder.Build()

if mock.expectedErr && err == nil {
t.Fatalf("Test %q failed: expected error not to be nil", name)
}
Expand Down