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

(WIP)fix(cspc, webhook): add validations for cStor pool expansions scenarios #1588

Closed

Conversation

mittachaitu
Copy link

@mittachaitu mittachaitu commented Jan 20, 2020

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:

  • Fixes #
  • Labelled this PR & related issue with documentation tag
  • PR messages has document related information
  • Labelled this PR & related issue with breaking-changes tag
  • PR messages has breaking changes related information
  • Labelled this PR & related issue with requires-upgrade tag
  • PR messages has upgrade related information
  • Commit has unit tests
  • Commit has integration tests

Sorry, something went wrong.

Copy link
Contributor

@shubham14bajpai shubham14bajpai left a 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) {
Copy link
Contributor

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

Copy link
Author

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

Copy link
Author

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)
Copy link
Contributor

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?

Copy link
Author

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) {
Copy link
Contributor

@vishnuitta vishnuitta Jan 29, 2020

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

Copy link
Contributor

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

Copy link
Author

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) {
Copy link
Contributor

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

mittachaitu added 3 commits January 30, 2020 10:15
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]>
// 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 {
Copy link
Contributor

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

@mittachaitu mittachaitu changed the title fix(cspc, webhook): add validations for cStor pool expansions scenarios (WIP)fix(cspc, webhook): add validations for cStor pool expansions scenarios Jan 30, 2020
Copy link
Author

@mittachaitu mittachaitu left a 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

  1. All the blockdevices of the data type raid groups (or) read/write/spare type raid groups should have the same capacity.
  2. No.of blockdevices in the corresponding raid groups should be the same.
  3. No.of blockdevices across the raid group should be the same.

@mittachaitu
Copy link
Author

These changes have been addressed via openebs-archive/cstor-operators#46.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants