-
Notifications
You must be signed in to change notification settings - Fork 200
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
Conversation
Ref: openebs/openebs#2916 This PR makes use of the Label Selector capability added in the BDC. More details: openebs/openebs#2921 The BDC Label Selector feature makes it possible for administrators to group a set of block devices and use them for creating Local PV (device). To use the Label Selector feature with Local PV (Devices) the administrator is expected to group(or pool) a set of block devices by assigning them the label: openebs.io/block-device-pool=< pool-name >. The `<pool-name>` can be passed to Local PV storage class via cas annotations. If the value is present, then Local PV device provisioner will set the following additional selector on the BDC: `openebs.io/block-device-pool=< pool-name >` Signed-off-by: kmova <kiran.mova@mayadata.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the BDC bdc-pvc-f21fad99-1c5d-4591-b32a-014de39021fb
given in the above example has kubernetes.io/hostname
also in the spec.selector.MatchLabels
?
// volumeBindingMode: WaitForFirstConsumer | ||
// reclaimPolicy: Delete | ||
// | ||
KeyBDPoolName = "BlockDevicePoolName" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Signed-off-by: kmova <kiran.mova@mayadata.io>
WithBlockVolumeMode(blkDevOpts.volumeMode) | ||
|
||
// If bdPoolName is configure, set it on the BDC | ||
if len(blkDevOpts.bdPoolName) > 0 { |
There was a problem hiding this comment.
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.
Signed-off-by: kmova <kiran.mova@mayadata.io>
Signed-off-by: kmova <kiran.mova@mayadata.io>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new builder method WithBlockDeviceTag
may need to be checked in to openebs/api
also.
Ref: openebs-archive/maya#1648 A new label fixed selector (openebs.io/block-device-tag) has been added to BDC to help with narrowing down the block devices that can be claimed via the BDC. This will allow users to tag block devices and indicate which tagged devices can be used by the BDC. Signed-off-by: kmova <kiran.mova@mayadata.io>
Ref: openebs-archive/maya#1648 A new label fixed selector (openebs.io/block-device-tag) has been added to BDC to help with narrowing down the block devices that can be claimed via the BDC. This will allow users to tag block devices and indicate which tagged devices can be used by the BDC. Signed-off-by: kmova <kiran.mova@mayadata.io>
What this PR does / why we need it:
Ref: openebs/openebs#2916
This PR makes use of the Label Selector capability
added in the BDC. More details: openebs/openebs#2921
The BDC Label Selector feature makes it possible for
administrators to
tag
a set of block devices and usethem for creating Local PV (device).
To use the Label Selector feature with Local PV (Devices)
the administrator is expected to
tag
a set ofblock devices by assigning them the label:
openebs.io/block-device-tag=< tag-value >
.The
< tag-value >
can be passed to Local PV storage classvia cas annotations. If the value is present, then Local PV
device provisioner will set the following additional selector
on the BDC:
openebs.io/block-device-tag=< tag-value >
Signed-off-by: kmova kiran.mova@mayadata.io
Special notes for your reviewer:
BDDs will be pushed in another commit or PR.
Executed the following tests:
block-device-tag
is not added to the storageclass.openebs.io/block-device-tag=mongo
.BlockDeviceTag
as follows:openebs-device-mongo
Checklist:
documentation
tag (if applicable)breaking-changes
tag (if applicable)requires-upgrade
tag (if applicable)