rbd: do deep copy for dummyVol struct #2669
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@BenamarMk PTAL. |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
BenamarMk
approved these changes
Nov 22, 2021
yati1998
previously approved these changes
Nov 22, 2021
Yuggupta27
previously approved these changes
Nov 22, 2021
Before the fixAfter the fix |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
added DNM. QE is still testing this one looks like we need some more fix |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
mergify
bot
dismissed stale reviews from yati1998 and Yuggupta27
Pull request has been modified.
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
Madhu-1
force-pushed
the
fix-shallow-copy
branch
from
474a342
to
e8792c1
Compare
ShyamsundarR
approved these changes
Nov 22, 2021
looks like jenkins issue restarting the tests |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e/k8s-1.21 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.21 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
|
/retest ci/centos/mini-e2e-helm/k8s-1.22 |
Rakshith-R
approved these changes
Nov 23, 2021
|
@Madhu-1 if I heard right we are waiting for QE testing, isnt it ? |
yes, added DNM for the same reason. |
|
@agarwal-mudit could you share some of the test results please? Re-adding |
nixpanic
reviewed
Nov 23, 2021
| @@ -290,9 +290,9 @@ func createDummyImage(ctx context.Context, rbdVol *rbdVolume) error { | |||
| if err != nil { | |||
| return err | |||
| } | |||
| dummyVol := rbdVol | |||
| dummyVol := *rbdVol | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
regression is a manual testing for now
It was decided not to wait for the test results. We can merge the fix and if it doesn't help QE can put it back to ON_QA. |
|
@nixpanic @humblec This is tested from @BenamarMk and Annette and its working fine as expected |
nixpanic
approved these changes
Nov 23, 2021
with shallow copy of rbdVol to dummyVol the image name update of the dummyVol is getting reflected on the rbdVol which we dont want. do deep copy to avoid this problem. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
currently we are fist operating on the dummy image to refresh the pool and then we are adding the scheduling. we think the scheduling should be added first and than we should refresh the pool. If we do this all the existing schedules will be considered from the scheduler. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Madhu-1
force-pushed
the
fix-shallow-copy
branch
from
e8792c1
to
38105e7
Compare
This was referenced Nov 23, 2021
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
with a shallow copy of rbdVol to dummyVol, the image name update of the dummyVol is getting reflected on the rbdVol which we don't want.
do a deep copy to avoid this problem.
fixes ##2656 (comment)
Signed-off-by: Madhu Rajanna madhupr007@gmail.com