-
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
(WIP)fix(cspc, webhook): add validations for cStor pool expansions scenarios #1588
Conversation
pkg/cstor/poolcluster/v1alpha1/cstorpoolspecs/cstorpoolspecs.go
Outdated
Show resolved
Hide resolved
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
@@ -131,12 +136,12 @@ func (wh *webhook) validateCSPCCreateRequest(req *v1beta1.AdmissionRequest) *v1b | |||
func cspcValidation(cspc *apis.CStorPoolCluster) (bool, string) { |
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.
lets check that all the raidGroup Types in a poolSpec are same, and in the case of stripe, only one raidgroup should be there
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.
In the case of stripe, we are checking the length of raidGroup
https://github.com/openebs/maya/blob/35af198f6c25f87cd9ab39e6f9cbc34ac154e31f/pkg/webhook/cspc.go#L208
But, I will add a check all raidGroup in the spec should be of same pool type
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.
Added the check that all the raidGroups in the spec should be of same pool type
@@ -342,16 +350,18 @@ func (wh *webhook) validateCSPCUpdateRequest(req *v1beta1.AdmissionRequest) *v1b | |||
return response | |||
} | |||
|
|||
bdr := NewBlockDeviceReplacement().WithNewCSPC(&cspcNew).WithOldCSPC(cspcOld) | |||
pOps := NewPoolOperations().WithNewCSPC(&cspcNew).WithOldCSPC(cspcOld) |
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.
do we have check that no two poolspecs should be on same node?
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.
for _, oldRg := range oldPoolSpec.RaidGroups { | ||
isRaidGroupExist := false | ||
for _, newRg := range newPoolSpec.RaidGroups { | ||
if IsRaidGroupCommon(oldRg, newRg) { |
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.
lets check length of oldRg and newRg are of same in IsRaidGroupCommon
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.
ok fine.. we are just getting the common ones
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.
lets check length of oldRg and newRg are of same in IsRaidGroupCommon
We are already checking it https://github.com/openebs/maya/blob/35af198f6c25f87cd9ab39e6f9cbc34ac154e31f/pkg/webhook/cspc_operation.go#L220
// changes case | ||
func (bdr *BlockDeviceReplacement) IsPoolSpecChangeValid(oldPoolSpec, newPoolSpec *apis.PoolSpec) (bool, string) { | ||
func (pOps *PoolOperations) ArePoolSpecChangesValid(oldPoolSpec, newPoolSpec *apis.PoolSpec) (bool, string) { |
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.
please provide comments at each step for better review
This PR does the following changes: - Fix comments regarding naming conventions from PR: openebs-archive#1543 - Fix a bug in general validations of CSPC - add validations to validate CSPC pool expansion scenarios Signed-off-by: mittachaitu <[email protected]>
Signed-off-by: mittachaitu <[email protected]>
Signed-off-by: mittachaitu <[email protected]>
35af198
to
956f6ca
Compare
Signed-off-by: mittachaitu <[email protected]>
// IsStripePoolSpec returns true if pool spec is stripe pool or else it will | ||
// return false | ||
func IsStripePoolSpec(poolSpec *apisv1alpha1.PoolSpec) bool { | ||
if len(poolSpec.RaidGroups[0].Type) != 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.
we need to check for first non-writecache, non-readcache, non-spare
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.
As discussed first let schema need to fix then I will add validations as discussed
- All the blockdevices of the data type raid groups (or) read/write/spare type raid groups should have the same capacity.
- No.of blockdevices in the corresponding raid groups should be the same.
- No.of blockdevices across the raid group should be the same.
These changes have been addressed via openebs-archive/cstor-operators#46. |
Signed-off-by: mittachaitu [email protected]
What this PR does / why we need it:
This PR does the following changes:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
Checklist:
documentation
tagbreaking-changes
tagrequires-upgrade
tag