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.

Sign up for GitHub

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

Jump to bottom

rbd: do deep copy for dummyVol struct #2669

Merged
merged 2 commits into from
Nov 23, 2021
Merged

rbd: do deep copy for dummyVol struct #2669

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
978c505
rbd: do deep copy for dummyVol struct
Madhu-1 Nov 22, 2021
38105e7
rbd: operate on dummy image after adding scheduling
Madhu-1 Nov 22, 2021
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
18 changes: 9 additions & 9 deletions internal/rbd/replicationcontrollerserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,9 +290,9 @@ func createDummyImage(ctx context.Context, rbdVol *rbdVolume) error {
if err != nil {
return err
}
dummyVol := rbdVol
dummyVol := *rbdVol
Copy link
Member

Choose a reason for hiding this comment

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

It probably helps to have a comment about this, as the implicit deep-copy is something that is really required here. Or create a DeepCopy() function to make it very explicit?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I can add a comment. if I make any code changes this should go through the testing again to cover all the cases to avoid regression.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have regression tests for this at all? If the e2e does not have a test-case, you can add the ci/skip/e2e label?

The current change is very fragile, and breaking seems easy. If a follow-up is planned, can you share a link to the issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nixpanic we are running the e2e to make sure nothing breaks at a cephcsi level. Yes, we can add the label here. nothing is planned an as a follow-up as code fix. #2675 a tracker to remove this workaround

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

regression is a manual testing for now

dummyVol.RbdImageName = imgName
err = createImage(ctx, dummyVol, dummyVol.conn.Creds)
err = createImage(ctx, &dummyVol, dummyVol.conn.Creds)
if err != nil && !strings.Contains(err.Error(), "File exists") {
return err
}
Expand All @@ -310,7 +310,7 @@ func tickleMirroringOnDummyImage(rbdVol *rbdVolume, mirroringMode librbd.ImageMi
if err != nil {
return err
}
dummyVol := rbdVol
dummyVol := *rbdVol
dummyVol.RbdImageName = imgName

dummyImageOpsLock.Lock()
Expand Down Expand Up @@ -523,12 +523,6 @@ func (rs *ReplicationServer) PromoteVolume(ctx context.Context,
return nil, status.Errorf(codes.Internal, "failed to get mirroring mode %s", err.Error())
}

log.DebugLog(ctx, "Attempting to tickle dummy image for restarting RBD schedules")
err = tickleMirroringOnDummyImage(rbdVol, mode)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to enable mirroring on dummy image %s", err.Error())
}

interval, startTime := getSchedulingDetails(req.GetParameters())
if interval != admin.NoInterval {
err = rbdVol.addSnapshotScheduling(interval, startTime)
Expand All @@ -543,6 +537,12 @@ func (rs *ReplicationServer) PromoteVolume(ctx context.Context,
rbdVol)
}

log.DebugLog(ctx, "attempting to tickle dummy image for restarting RBD schedules")
err = tickleMirroringOnDummyImage(rbdVol, mode)
if err != nil {
return nil, status.Errorf(codes.Internal, "failed to enable mirroring on dummy image %s", err.Error())
}

return &replication.PromoteVolumeResponse{}, nil
}

Expand Down