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

journal: omap implementation for volumegroup #4415

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Feb 2, 2024

Implement the required function to store/retrieve the details from the omap for the snapshotgroup.

This adds a new omap object that contains the mapping of the RequestName and all the volumeID and its corresponding snapshotID belonging to a group.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@Madhu-1 Madhu-1 added the ci/skip/e2e skip running e2e CI jobs label Feb 2, 2024
@Madhu-1 Madhu-1 requested review from nixpanic and a team February 2, 2024 09:04
@Madhu-1 Madhu-1 force-pushed the vgroup-journal branch 4 times, most recently from 85287fe to 49d41ce Compare February 5, 2024 12:09
@Madhu-1 Madhu-1 changed the title journal: omap implementation for snapshotgroup journal: omap implementation for volumegroup Feb 5, 2024
log.ErrorLog(ctx, "omap not found (pool=%q, namespace=%q, name=%q): %v",
poolName, namespace, oid, err)

return nil, util.JoinErrors(util.ErrKeyNotFound, err)
Copy link
Member

Choose a reason for hiding this comment

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

I think there is no need to use the custom JoinErrors anymore. Use

fmt.Errorf("%w: %w", util.ErrKeyNotFound, err)

instead?

namespace string,
cr *util.Credentials) (*VolumeGroupJournalConfig, error)
Destroy()
setNamespace(ns string)
Copy link
Member

Choose a reason for hiding this comment

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

setNamespace() is not exported, it probably should be?

}

// VolumeGroupJournalConfig contains the configuration and connection details.
type VolumeGroupJournalConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be exported, only the functions of the interface should be available.

}
}

// SetNamespace sets the namespace for the journal.
Copy link
Member

Choose a reason for hiding this comment

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

I prefer comments for the function (if they do not include implementation details) at the interface. Have you considered that?

- If there is a valid reservation, then the corresponding VolumeGroupData for
the snapshot group is returned
- If there is a reservation that is stale (or not fully cleaned up), it is
garbage collected using the UndoReservation call, as appropriate
Copy link
Member

Choose a reason for hiding this comment

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

It is unclear to me if after garbage collecting, it creates a new reservation?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, its undo the reservation, its caller responsibility to create new reservation, or a new reservation is created in the next RPC call as the reservation is deleted.

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.

looks good to me, thanks!

Note that there still is a JoinErrors call, please clean that one up too.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 8, 2024

@nixpanic i have tested the changes with cephfs volumegroupsnapshot locally, the library works as expected.

@Madhu-1 Madhu-1 requested a review from a team February 8, 2024 16:38
@nixpanic nixpanic requested a review from a team February 8, 2024 17:00
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 9, 2024

@Mergifyio requeue

Copy link
Contributor

mergify bot commented Feb 9, 2024

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Feb 9, 2024

@Mergifyio queue

Copy link
Contributor

mergify bot commented Feb 9, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at dd235d8

Added a implementation for the listOmapVals
which list the object keys and values from
the rados omap.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Implement the required function to store/retrieve
the details from the omap for the volumegroup.

This adds a new omap object that contains the
mapping of the RequestName and all the volumeID
and its corresponding snapshotID belonging to a
group.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Feb 9, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot
Copy link
Collaborator

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

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Feb 9, 2024
@mergify mergify bot merged commit dd235d8 into ceph:devel Feb 9, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/e2e skip running e2e CI jobs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants