-
Notifications
You must be signed in to change notification settings - Fork 19
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
Changes to support sync replication for Powerstore #348
Conversation
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 address lint errors.
Addressed |
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
@@ -302,15 +318,24 @@ func (s *Service) CreateVolume(ctx context.Context, req *csi.CreateVolumeRequest | |||
if apiError, ok := err.(gopowerstore.APIError); ok && apiError.NotFound() { | |||
log.Infof("Volume group with name %s not found, creating it", vgName) | |||
|
|||
// Attribute that indicates whether snapshot sets of the volumegroup will be write-order consistent. | |||
isWriteOrderConsistent := false |
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.
This will make write-order-consistent to be false for ASYNC mode while the API/UI's enables it by default for volume group creation.
CC @alankar-verma
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.
How about enabling it (sending true) irrespective of the mode?
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.
Hi @santhoshatdell Currently for async we are not passing is-write-consistent ... so by default the value is going as false due to boolean default value (though from API perspective by default it is true ). For Sync, it should be true always so used this variable to pass as true.. Can we discuss on this and handle as part of upcoming PR s ?
// Zero indicates value zero for RPO | ||
Zero = "Zero" | ||
// Metro indicates Metro mode | ||
Metro = "METRO" |
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.
'MetroMode' - to be consistent with other mode names? Or an enum that defines the modes.
please address comments by @santhoshatdell
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
Description
GitHub Issues
List the GitHub issues impacted by this PR:
Checklist:
How Has This Been Tested?
Ran unit tests and the results look good
Ran sanity to check Sync replication is working as expected. ensured the following scenarios
Details with screenshots are captured and attached as part of 28497