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: fix namespace name update in metadata and rados object #3477

Merged
merged 3 commits into from
Oct 28, 2022
Merged

rbd: fix namespace name update in metadata and rados object #3477

merged 3 commits into from
Oct 28, 2022

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Oct 27, 2022

If a PV is reattached to a new PVC in a different namespace, we need to update the namespace name in the rbd image metadata.
If a PV is reattached to a new PVC in a different namespace, we need to update the namespace name in the rados object.

  • Create a RBD PVC in default namespace
[🎩︎]mrajanna@fedora rbd $]kubectl get pvc
NAME      STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS      AGE
rbd-pvc   Bound    pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7   1Gi        RWO            rook-ceph-block   76s
  • check metadata of the image
sh-4.4$ rbd image-meta ls replicapool/csi-vol-333a1bdc-55fb-11ed-b453-be69662ea197
There are 4 metadata on this image:

Key                               Value                                   
csi.ceph.com/cluster/name         my-prod-cluster                         
csi.storage.k8s.io/pv/name        pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7
csi.storage.k8s.io/pvc/name       rbd-pvc                                 
csi.storage.k8s.io/pvc/namespace  default
  • Attach same PV to different PVC in new namespace
[🎩︎]mrajanna@fedora rbd $]kubectl get pv
NAME                                       CAPACITY   ACCESS MODES   RECLAIM POLICY   STATUS   CLAIM                   STORAGECLASS      REASON   AGE
pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7   1Gi        RWO            Retain           Bound    rook-ceph/rbd-pvc-new   rook-ceph-block            3m47s
[🎩︎]mrajanna@fedora rbd $]
[🎩︎]mrajanna@fedora rbd $]
[🎩︎]mrajanna@fedora rbd $]kuberc get pvc
NAME          STATUS   VOLUME                                     CAPACITY   ACCESS MODES   STORAGECLASS      AGE
rbd-pvc-new   Bound    pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7   1Gi        RWO            rook-ceph-block   13s
  • Check metadata of the image
sh-4.4$ rbd image-meta ls replicapool/csi-vol-333a1bdc-55fb-11ed-b453-be69662ea197
There are 4 metadata on this image:

Key                               Value                                   
csi.ceph.com/cluster/name         my-prod-cluster                         
csi.storage.k8s.io/pv/name        pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7
csi.storage.k8s.io/pvc/name       rbd-pvc-new                             
csi.storage.k8s.io/pvc/namespace  default                                 
sh-4.4$ 
sh-4.4$ rados listomapvals csi.volume.333a1bdc-55fb-11ed-b453-be69662ea197 --pool=replicapool
csi.imageid
value (12 bytes) :
00000000  31 31 35 32 32 39 30 30  33 63 66 35              |115229003cf5|
0000000c

csi.imagename
value (44 bytes) :
00000000  63 73 69 2d 76 6f 6c 2d  33 33 33 61 31 62 64 63  |csi-vol-333a1bdc|
00000010  2d 35 35 66 62 2d 31 31  65 64 2d 62 34 35 33 2d  |-55fb-11ed-b453-|
00000020  62 65 36 39 36 36 32 65  61 31 39 37              |be69662ea197|
0000002c

csi.volname
value (40 bytes) :
00000000  70 76 63 2d 32 65 38 35  65 65 64 34 2d 62 31 33  |pvc-2e85eed4-b13|
00000010  61 2d 34 37 38 62 2d 61  66 62 30 2d 33 32 31 37  |a-478b-afb0-3217|
00000020  30 36 39 64 35 63 61 37                           |069d5ca7|
00000028

csi.volume.owner
value (7 bytes) :
00000000  64 65 66 61 75 6c 74                              |default|
00000007

As we can notice here, only the PVC name got updated not the namespace name in both metadata and in rados object also.

With this fix even the namespace name also got updated

sh-4.4$ rbd image-meta ls replicapool/csi-vol-333a1bdc-55fb-11ed-b453-be69662ea197
There are 4 metadata on this image:

Key                               Value                                   
csi.ceph.com/cluster/name         my-prod-cluster                         
csi.storage.k8s.io/pv/name        pvc-2e85eed4-b13a-478b-afb0-3217069d5ca7
csi.storage.k8s.io/pvc/name       rbd-pvc-new                             
csi.storage.k8s.io/pvc/namespace  rook-ceph                               
sh-4.4$ rados listomapvals csi.volume.333a1bdc-55fb-11ed-b453-be69662ea197  --pool=replicapool
csi.imageid
value (12 bytes) :
00000000  31 31 35 32 32 39 30 30  33 63 66 35              |115229003cf5|
0000000c

csi.imagename
value (44 bytes) :
00000000  63 73 69 2d 76 6f 6c 2d  33 33 33 61 31 62 64 63  |csi-vol-333a1bdc|
00000010  2d 35 35 66 62 2d 31 31  65 64 2d 62 34 35 33 2d  |-55fb-11ed-b453-|
00000020  62 65 36 39 36 36 32 65  61 31 39 37              |be69662ea197|
0000002c

csi.volname
value (40 bytes) :
00000000  70 76 63 2d 32 65 38 35  65 65 64 34 2d 62 31 33  |pvc-2e85eed4-b13|
00000010  61 2d 34 37 38 62 2d 61  66 62 30 2d 33 32 31 37  |a-478b-afb0-3217|
00000020  30 36 39 64 35 63 61 37                           |069d5ca7|
00000028

csi.volume.owner
value (9 bytes) :
00000000  72 6f 6f 6b 2d 63 65 70  68                       |rook-ceph|
00000009

@Madhu-1 Madhu-1 requested review from pkalever and a team October 27, 2022 14:54
@mergify mergify bot added component/rbd Issues related to RBD bug Something isn't working labels Oct 27, 2022
@pkalever
Copy link

@Madhu-1 Would it be good to have a testcase for this?

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 27, 2022

@Madhu-1 Would it be good to have a testcase for this?

i thought about it, but it required namespace creation, currently we dont create new namespaces in our E2E tests

@Madhu-1 Madhu-1 force-pushed the fix-namespace-metadata-problem branch from 8cf4250 to 0b299b3 Compare October 27, 2022 15:14
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 27, 2022

@Madhu-1 Would it be good to have a testcase for this?

@pkalever Added now. PTAL

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/mini-e2e-helm/k8s-1.22

nixpanic
nixpanic requested changes Oct 28, 2022
View reviewed changes
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

just some minor comments, looks good functionally

e2e/rbd.go Outdated
@@ -513,6 +513,7 @@ var _ = Describe("RBD", func() {
})

By("reattach the old PV to a new PVC and check if PVC metadata is updated on RBD image", func() {
reattachPVCNamespace := "reattach-cephfs"
Copy link
Member

Choose a reason for hiding this comment

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

Can you take the current name of the namespace , and append it with -2 or something instead?

internal/journal/voljournal.go Outdated
@@ -864,3 +864,14 @@ func (conn *Connection) ReserveNewUUIDMapping(ctx context.Context,

return setOMapKeys(ctx, conn, journalPool, cj.namespace, cj.csiDirectory, setKeys)
}

// ResetVolumeOwner update the owner in the rados object.
Copy link
Member

Choose a reason for hiding this comment

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

nit/typo: ... updates ...

internal/journal/voljournal.go Outdated

// ResetVolumeOwner update the owner in the rados object.
func (conn *Connection) ResetVolumeOwner(ctx context.Context, pool, reservedUUID, owner string) error {
err := setOMapKeys(ctx, conn, pool, conn.config.namespace, conn.config.cephUUIDDirectoryPrefix+reservedUUID,
Copy link
Member

Choose a reason for hiding this comment

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

maybe just return setOMapKeys(...)?

internal/rbd/rbd_journal.go Outdated
@@ -638,7 +644,7 @@ func RegenerateJournal(
return "", err
}

return rbdVol.VolID, nil
return rbdVol.VolID, err
Copy link
Member

Choose a reason for hiding this comment

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

err will always be nil here. I prefer to have an explicit no error case, and returning err here make it look like there could be still an error lingering, in which case the rbdVol.VolID should not be trusted.

@Madhu-1 Madhu-1 force-pushed the fix-namespace-metadata-problem branch from 0b299b3 to e3e415a Compare October 28, 2022 08:09
@Madhu-1 Madhu-1 requested review from nixpanic and a team October 28, 2022 08:09
nixpanic
nixpanic previously approved these changes Oct 28, 2022
View reviewed changes
@nixpanic nixpanic requested a review from a team October 28, 2022 08:28
@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Oct 28, 2022
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.22

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.22

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.22

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@pkalever
Copy link

@Madhu-1 the changes look good, but the 3rd commit (e2e one) does address Niels previous review comments rather than the 1st and 2nd. Will it be good to merge the changes to respective commits?

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

@Madhu-1 the changes look good, but the 3rd commit (e2e one) does address Niels previous review comments rather than the 1st and 2nd. Will it be good to merge the changes to respective commits?

done now, PTAL

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/mini-e2e/k8s-1.22

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/mini-e2e-helm

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/k8s-e2e-external-storage/k8s-1.22

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/k8s-e2e-external-storage/1.22

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/k8s-e2e-external-storage/1.23

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Oct 28, 2022

/test ci/centos/k8s-e2e-external-storage/1.24

@Madhu-1 Madhu-1 added ci/retry/e2e Label to retry e2e retesting on approved PR's backport-to-release-v3.7 labels Oct 28, 2022
@nixpanic
Copy link
Member

@Mergifyio rebase

@Madhu-1 @mergify
rbd: update namespace name in metadata
7d2c19b 
If a PV is reattached to a new PVC in a different
namespace we need to update the namespace name
in the rbd image metadata.

Signed-off-by: Madhu Rajanna <[email protected]>
@Madhu-1 @mergify
rbd: update namespace name in rados object
767e5df 
If a PV is reattached to a new PVC in a different
namespace we need to update the namespace name
in the rados object.

Signed-off-by: Madhu Rajanna <[email protected]>
@Madhu-1 @mergify
e2e: add e2e test for rbd reattach metadata
87056b5 
Added E2E test case to verify metadata after
PV is attached to a new PVC in different namespace.

Signed-off-by: Madhu Rajanna <[email protected]>
@mergify
Copy link
Contributor

mergify bot commented Oct 28, 2022

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 force-pushed the fix-namespace-metadata-problem branch from 623a850 to 87056b5 Compare October 28, 2022 13:59
@nixpanic nixpanic added ok-to-test Label to trigger E2E tests and removed ok-to-test Label to trigger E2E tests labels Oct 28, 2022
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.23

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.23

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@mergify mergify bot merged commit 0530134 into ceph:devel Oct 28, 2022
@mergify mergify bot mentioned this pull request Oct 28, 2022
Merged
@Madhu-1 Madhu-1 mentioned this pull request Oct 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nixpanic nixpanic nixpanic approved these changes

@pkalever pkalever pkalever approved these changes

Assignees
No one assigned
Labels
bug Something isn't working ci/retry/e2e Label to retry e2e retesting on approved PR's component/rbd Issues related to RBD ok-to-test Label to trigger E2E tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Madhu-1 @pkalever @nixpanic