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

api: add radosNamespace field to CephFsConfigSpec #165

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+38 −1
Open

api: add radosNamespace field to CephFsConfigSpec #165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24b685f
api: add radosNamespace field to CephFsConfigSpec
iPraveenParihar Nov 4, 2024
3f8b947
clientprofile: update Ceph CSI config entry with cephFS.RadosNamespace
iPraveenParihar Nov 4, 2024
85418e8
doc: update doc for cephFS.RadosNamespace API change
iPraveenParihar Nov 4, 2024
0154232
api: update rbd.radosNamespace field to immutable
iPraveenParihar Nov 5, 2024
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
5 changes: 5 additions & 0 deletions api/v1alpha1/clientprofile_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,15 @@ type CephFsConfigSpec struct {

//+kubebuilder:validation:Optional
FuseMountOptions map[string]string `json:"fuseMountOptions,omitempty"`

//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="radosNamespace is immutable"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the immutability?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#165 (comment)

//+kubebuilder:validation:Optional
RadosNamespace string `json:"radosNamespace,omitempty"`
nb-ohad marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usage of a Rados namespace for CephFs is sepcificly for OMAP data. The current proposed name for this field does not reflects that. I would suggest the following instead:

Suggested change
RadosNamespace string `json:"radosNamespace,omitempty"`
OmapNamespace string `json:"OmapNamespace,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OMAP are RADOS objects so the term radosNamespace would be more appropriate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@iPraveenParihar
I am not sure I agree with the lack of distinction. You want the user to specify a rados namespace for the location of the omap data for the cephfs usage. Not just a general rados namespace for general use.

What I am saying is that the name should capture that
Maybe we could agree on OmapRadosNamespace as an alternative?

Copy link

Choose a reason for hiding this comment

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

It's radosNamespace in cephcsi, let's keep it the same.

Currently it may well be only used for omap usage but it might change in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

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

User-facing API design is essential for me, there is already past acknowledgment that the Ceph CSI project did not select good names for some of its front-facing configurations. Bringing them as an example does not make a good case.

Regarding the other part of the comment:

  1. we are talking about CephFS, not RBD. RDS is the internal mechanism we use to isolate the omap data. It does not capture the value/intent of using the feature/nob
  2. Using the same field name as we have inside the RBD config section is misleading because in each section the nob had completely different semantics and effects.
  3. Using RadosNamesapce as the nob name will collide with any future nobs that we might need to allow configuration of additional RDS for other metadata the cephcsi or chepfs will allow us to use. We want future proofing against such a case.

Given the above 3 points, we should go with something like OmapRadosNamaspece which captures both the intent of the nob and the entity type this nob is referring to

Copy link

Choose a reason for hiding this comment

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

User-facing API design is essential for me, there is already past acknowledgment that the Ceph CSI project did not select good names for some of its front-facing configurations. Bringing them as an example does not make a good case.

I completely disagree with the statement Ceph CSI project did not select good names.
You can always open issues at cephcsi to suggest renaming them.

Changing such conventions in above layers only causes confusion.

Regarding the other part of the comment:

  1. we are talking about CephFS, not RBD. RDS is the internal mechanism we use to isolate the omap data. It does not capture the value/intent of using the feature/nob
  2. Using the same field name as we have inside the RBD config section is misleading because in each section the nob had completely different semantics and effects.
  3. Using RadosNamesapce as the nob name will collide with any future nobs that we might need to allow configuration of additional RDS for other metadata the cephcsi or chepfs will allow us to use. We want future proofing against such a case.

Given the above 3 points, we should go with something like OmapRadosNamaspece which captures both the intent of the nob and the entity type this nob is referring to

RadosNamespace is the key we are using for cephfs section in cephcsi, we should keep it the same in cephcsi Operator. It implies cephcsi will use this radosNS for operations, right now it maybe limited to just omap but might change in the future.

If you would like more specific naming, please open issue in cephcsi, we'll change it there, then in Operator. I would prefer this approach instead of introducing new names abruptly in above layers.

Future additional cases can use another specific key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Madhu-1, can you please provide your inputs here?

}

// RbdConfigSpec defines the desired RBD configuration
type RbdConfigSpec struct {
//+kubebuilder:validation:XValidation:rule="self == oldSelf",message="radosNamespace is immutable"
//+kubebuilder:validation:Optional
RadosNamespace string `json:"radosNamespace,omitempty"`
}
Expand Down
8 changes: 8 additions & 0 deletions config/crd/bases/csi.ceph.io_clientprofiles.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ spec:
additionalProperties:
type: string
type: object
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
subVolumeGroup:
type: string
type: object
Expand All @@ -85,6 +90,9 @@ spec:
properties:
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
type: object
required:
- cephConnectionRef
Expand Down
8 changes: 8 additions & 0 deletions deploy/all-in-one/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,11 @@ spec:
additionalProperties:
type: string
type: object
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
subVolumeGroup:
type: string
type: object
Expand All @@ -233,6 +238,9 @@ spec:
properties:
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
type: object
required:
- cephConnectionRef
Expand Down
8 changes: 8 additions & 0 deletions deploy/multifile/crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,11 @@ spec:
additionalProperties:
type: string
type: object
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
subVolumeGroup:
type: string
type: object
Expand All @@ -224,6 +229,9 @@ spec:
properties:
radosNamespace:
type: string
x-kubernetes-validations:
- message: radosNamespace is immutable
rule: self == oldSelf
type: object
required:
- cephConnectionRef
Expand Down
3 changes: 2 additions & 1 deletion docs/design/operator.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,8 +349,9 @@ spec:
subvolumeGroup: csi
kernelMountOptions: readdir_max_bytes=1048576,norbytes
fuseMountOptions: debug
radosNamespace: rados-test-cephfs
rbd:
radosNamespace: rados-test
radosNamespace: rados-test-rbd
status: {}
```

Expand Down
2 changes: 2 additions & 0 deletions internal/controller/clientprofile_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ type csiClusterInfoRecord struct {
SubvolumeGroup string `json:"subvolumeGroup,omitempty"`
KernelMountOptions string `json:"kernelMountOptions"`
FuseMountOptions string `json:"fuseMountOptions"`
RadosNamespace string `json:"radosNamespace,omitempty"`
} `json:"cephFS,omitempty"`
Rbd struct {
RadosNamespace string `json:"radosNamespace,omitempty"`
Expand Down Expand Up @@ -313,6 +314,7 @@ func composeCsiClusterInfoRecord(clientProfile *csiv1a1.ClientProfile, cephConn
record.Monitors = cephConn.Spec.Monitors
if cephFs := clientProfile.Spec.CephFs; cephFs != nil {
record.CephFs.SubvolumeGroup = cephFs.SubVolumeGroup
record.CephFs.RadosNamespace = cephFs.RadosNamespace
if mountOpt := cephFs.KernelMountOptions; mountOpt != nil {
record.CephFs.KernelMountOptions = utils.MapToString(mountOpt, "=", ",")
}
Expand Down
5 changes: 5 additions & 0 deletions vendor/github.com/ceph/ceph-csi-operator/api/v1alpha1/clientprofile_types.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading