Add Support for Restoring OCS Operator CRs in ODF CLI #114
Conversation
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
9bf3003 to
96f1b48
Compare
There was a problem hiding this comment.
With this change, we really should enable the restore of any of the ODF CRs, whether from Rook, OCS operator, CSI operator, Noobaa, etc.
One approach would be to just require args[0] to include the fully qualified CRD type such as storageclusters.ocs.openshift.io, then we just extract the group name from that full name. We could return an error if they passed some group name that we don't recognize as belonging to ODF.
cmd/odf/restore/crds.go
Outdated
| pkgrestore.RestoreCrd(cmd.Context(), root.ClientSets, root.OperatorNamespace, root.StorageClusterNamespace, args) | ||
| var groupName string | ||
| var versionResource string | ||
| if args[0] == "storageclusters" { |
There was a problem hiding this comment.
Is there a check for length of args before this? If not, this could crash.
There was a problem hiding this comment.
The command already includes argument validation through Cobra with the line:
Args: cobra.RangeArgs(1, 2),
This ensures that if the number of arguments is outside the 1–2 range, Cobra will return an error and prevent further execution. As shown in the output, passing three arguments results in:
$ ./bin/odf restore deleted a b c
Error: accepts between 1 and 2 arg(s), received 3
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
96f1b48 to
15a4914
Compare
Hi @travisn , I’ve implemented the change as suggested. The restore command now requires that the first argument be a fully qualified CRD type (for example, storageclusters.ocs.openshift.io). The code parses this input to extract the resource name, group, and API version, and it validates the group against a set of supported groups (Rook, OCS, CSI, NooBaa, etc.). If the provided group isn’t recognized as part of ODF, the command returns an error with a helpful message listing the supported groups. This update should enable us to restore any of the ODF CRs regardless of the operator to which they belong. Please take a look at the changes and let me know if you have any further suggestions or feedback. Thanks, |
cmd/odf/restore/crds.go
Outdated
| var groupVersions = map[string]string{ | ||
| "ocs.openshift.io": "v1", | ||
| "ceph.rook.io": "v1", | ||
| "storage.k8s.io": "v1", | ||
| "odf.openshift.io": "v1alpha1", | ||
| "noobaa.io": "v1alpha1", | ||
| "csiaddons.openshift.io": "v1alpha1", |
There was a problem hiding this comment.
| var groupVersions = map[string]string{ | |
| "ocs.openshift.io": "v1", | |
| "ceph.rook.io": "v1", | |
| "storage.k8s.io": "v1", | |
| "odf.openshift.io": "v1alpha1", | |
| "noobaa.io": "v1alpha1", | |
| "csiaddons.openshift.io": "v1alpha1", | |
| var groupVersions = map[string]string{ | |
| "ocs.openshift.io": "v1", | |
| "ceph.rook.io": "v1", | |
| "storage.k8s.io": "v1", | |
| "odf.openshift.io": "v1alpha1", | |
| "noobaa.io": "v1alpha1", | |
| "csiaddons.openshift.io": "v1alpha1", |
could you also check csi-operator api groups name
There was a problem hiding this comment.
I've added "csi.ceph.io": "v1". Is that what you meant for this CRD?
There was a problem hiding this comment.
it is v1alpha1 please confirm and upate
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
15a4914 to
9e10de3
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: OdedViner The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cmd/odf/restore/crds.go
Outdated
| // Parse the fully qualified CRD (e.g. "cephclusters.ceph.rook.io"). | ||
| resourceName, groupName, version, err := parseFullyQualifiedCRD(args[0]) | ||
| if err != nil { | ||
| fmt.Printf("Error parsing CRD type: %v\n", err) |
There was a problem hiding this comment.
Instead of printing the error, see the helper for logging.Fatal()
cmd/odf/restore/crds.go
Outdated
| "ocs.openshift.io": "v1", | ||
| "ceph.rook.io": "v1", | ||
| "storage.k8s.io": "v1", | ||
| "csi.ceph.io": "v1", |
There was a problem hiding this comment.
In 4.19 we are pushing for v1beta1 for the csi CRDs, let's assume that for now
| "csi.ceph.io": "v1", | |
| "csi.ceph.io": "v1beta1", |
cmd/odf/restore/crds.go
Outdated
| var groupVersions = map[string]string{ | ||
| "ocs.openshift.io": "v1", | ||
| "ceph.rook.io": "v1", | ||
| "storage.k8s.io": "v1", |
There was a problem hiding this comment.
Is this for csidrivers and storageclasses? Seems like we shouldn't need to restore this type. ODF might create resources of that type, but I don't anticipate they would have finalizers that we would need to worry about.
There was a problem hiding this comment.
removed "storage.k8s.io" CRD
OdedViner
force-pushed
the
restore_ocs_cr
branch
3 times, most recently
from
684d651 to
1a49d6d
Compare
|
This PR depends on the kubectl-rook-ceph PR rook/kubectl-rook-ceph#354 that was merged today. |
That will require kubectl-rook-ceph plugin release which I'm not sure we are planning now but you can use kubectl-rook-ceph with commit that has your changes |
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
1a49d6d to
406e1ad
Compare
done |
There was a problem hiding this comment.
Changes looks @OdedViner have you verified the changes are working as expected.
|
When attempting to restore it storagecluster CR, the process encountered an error while trying to remove finalizers from the storageclusters CR I think I need to scale down the OCS operator instead of the Rook-Ceph operator. |
I don't believe the process has been documented before, thus important to test it out and confirm if it's working. Also, something doesn't look right in the execution, it shouldn't be removing all the owner references on so many resources. I thought that would only be needed for the cephcluster CR, not for the storageCluster CR. |
|
@OdedViner How's the testing looking for this? It would be good to have this work completed soon. |
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
406e1ad to
582d392
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
2 times, most recently
from
7b394b3 to
7ae8755
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
2 times, most recently
from
c0c5284 to
21f039e
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
2 times, most recently
from
f0518e0 to
bbbb77b
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
bbbb77b to
21f039e
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
from
21f039e to
f3531ba
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
2 times, most recently
from
9c46506 to
3a586fa
Compare
OdedViner
force-pushed
the
restore_ocs_cr
branch
2 times, most recently
from
daf3edd to
a978ebc
Compare
cmd/odf/restore/crds.go
Outdated
| } | ||
|
|
||
| // keys returns the keys of a string map. It is used to print out supported group names. | ||
| func keys(m map[string]string) []string { |
There was a problem hiding this comment.
| func keys(m map[string]string) []string { | |
| func groupNameKeys(m map[string]string) []string { |
Let's have name related to their work
cmd/odf/restore/crds.go
Outdated
| resourceName = parts[0] | ||
| groupName = parts[1] | ||
|
|
||
| ver, ok := groupVersions[groupName] |
There was a problem hiding this comment.
| ver, ok := groupVersions[groupName] | |
| version, ok := groupVersions[groupName] |
cmd/odf/restore/crds.go
Outdated
| newArgs[1] = args[1] | ||
| } | ||
| var customResources []pkgrestore.CustomResource | ||
| if contains(newArgs, "storageclusters") { |
There was a problem hiding this comment.
| if contains(newArgs, "storageclusters") { | |
| if contains(newArgs, "storageclusters") { |
I think we can use string.Slices here without https://go.dev/play/p/sj50jslDBUY to make code clean
|
The changes look good to me. Thanks @travisn, ready to merge? |
cmd/odf/restore/crds.go
Outdated
| var groupVersions = map[string]string{ | ||
| "ocs.openshift.io": "v1", | ||
| "ceph.rook.io": "v1", | ||
| "csi.ceph.io": "v1beta1", |
There was a problem hiding this comment.
In 4.19 we decided to keep this as v1alpha1, then in 4.20 it will change to v1.
| "csi.ceph.io": "v1beta1", | |
| "csi.ceph.io": "v1alpha1", |
go.sum
Outdated
| @@ -743,8 +743,8 @@ github.com/rogpeppe/go-internal v1.6.1/go.mod h1:xXDCJY+GAPziupqXw64V24skbSoqbTE | |||
| github.com/rogpeppe/go-internal v1.8.1/go.mod h1:JeRgkft04UBgHMgCIwADu4Pn6Mtm5d4nPKWu0nJ5d+o= | |||
| github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= | |||
| github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= | |||
| github.com/rook/kubectl-rook-ceph v0.9.3 h1:+7THA8a+S2ArJrs9jpY1eJscAjPBKjlLXSmCVPU3eoY= | |||
| github.com/rook/kubectl-rook-ceph v0.9.3/go.mod h1:dOQ+Yccc41DxZqe9jpvAUHsYTquYP/SKClrPmG70SLM= | |||
| github.com/rook/kubectl-rook-ceph v0.9.4-0.20250428051344-dbfe77cc57a1 h1:znvPe0apxkTdkdiVINk0DfUbMGt6Vv+I9mgKfsr3odY= | |||
There was a problem hiding this comment.
Discussed with @subhamkrai today that he will plan on v0.9.4 tomorrow, if you prefer to update this reference after that release.
There was a problem hiding this comment.
This PR adds support for restoring deleted Custom Resources for OCS operator via the ODF CLI. It includes logic for handling storageclusters CRs by dynamically setting groupName and versionResource, ensuring compatibility and reliability during the restore process. Signed-off-by: Oded Viner <[email protected]>
There was a problem hiding this comment.
LGTM, it seems we just need to resolve the questions in https://issues.redhat.com/browse/DFBUGS-793 before merging.
| // groupVersions defines the supported CRD groups and their corresponding API versions. | ||
| var groupVersions = map[string]string{ | ||
| "ocs.openshift.io": "v1", | ||
| "ceph.rook.io": "v1", | ||
| "csi.ceph.io": "v1alpha1", | ||
| "odf.openshift.io": "v1alpha1", | ||
| "noobaa.io": "v1alpha1", | ||
| "csiaddons.openshift.io": "v1alpha1", | ||
| } | ||
|
|
||
| // groupNameKeys returns the keys of a string map. It is used to print out supported group names. | ||
| func groupNameKeys(m map[string]string) []string { | ||
| out := make([]string, 0, len(m)) | ||
| for k := range m { | ||
| out = append(out, k) | ||
| } | ||
| return out | ||
| } | ||
|
|
||
| // parseFullyQualifiedCRD takes a fully qualified CRD type of the form "resource.group" | ||
| // (for example, "cephclusters.ceph.rook.io") and returns the resource name, group name, and | ||
| // the API version associated with that group. It returns an error if the format is invalid | ||
| // or the group is not recognized. | ||
| func parseFullyQualifiedCRD(fqcrd string) (resourceName, groupName, version string, err error) { | ||
| parts := strings.SplitN(fqcrd, ".", 2) | ||
| if len(parts) != 2 { | ||
| return "", "", "", fmt.Errorf("invalid CRD format %q; expected format <resource>.<group>", fqcrd) | ||
| } | ||
| resourceName = parts[0] | ||
| groupName = parts[1] | ||
|
|
||
| version, ok := groupVersions[groupName] | ||
| if !ok { | ||
| return "", "", "", fmt.Errorf("unsupported group %q; supported groups are: %v", groupName, groupNameKeys(groupVersions)) | ||
| } | ||
| return resourceName, groupName, version, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
any reason in not using the established GVK/GVR schema from https://github.com/kubernetes/apimachinery/blob/master/pkg/runtime/schema/group_version.go or https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/group_version.go if we want a higher level of abstraction.
is it like you don't want to import Go Structs from other repos? as of now iiuc, we already made an effort moving APIs into a submodule.
There was a problem hiding this comment.
@leelavg The input format is ./bin/odf restore deleted storageclusters.ocs.openshift.io, where only the CR name and group are provided (storageclusters.ocs.openshift.io). We do not specify the version. Please add the relevant code to improve the current implementation.
| if slices.Contains(newArgs, "storageclusters") { | ||
| customResources = []pkgrestore.CustomResource{ | ||
| // ceph.rook.io/v1 | ||
| {Group: "ceph.rook.io", Version: "v1", Resource: "cephblockpoolradosnamespaces"}, |
There was a problem hiding this comment.
how can we create a link back to the ocs-op to keep this list updated when a new resource is owned by storagecluster?
did you check that k8s primitives (secrets, configmaps etc) that are owned by storagecluster are ok to be deleted?
apart from ensuring pods are running what other checks are performed after restoration of storagecluster CR?
iirc, storagecluster UID is being used in DR related activities as well, maybe that also gets effected or we need to have a statement of not performing this on DR enabled setups?
There was a problem hiding this comment.
@leelavg I have not tested it on a DR setup or in Provider mode.
|
just dropping it here as I didn't look into the plugins that we were developing upto now (pardon), usually https://github.com/kubernetes/cli-runtime is preferred as a base when developing kubectl plugins as it abstracts api-resources, discovery if required, generic printers etc. |
@leelavg This is a downstream project, so I believe it would be more relevant for the upstream project: https://github.com/rook/kubectl-rook-ceph. |
|
We also need to scale down |
This PR adds support for restoring deleted Custom Resources (CRs) for the OCS operator via the ODF CLI. It includes logic for handling storageclusters CRs by dynamically setting groupName and versionResource, ensuring compatibility and reliability during the restore process.
Test Procedure:
SetuP:
1.Check storagecluster status:
2.Check pods status:
https://github.com/rook/kubectl-rook-ceph/blob/dbfe77cc57a16be39fc043f04baff26c5c94bba8/pkg/restore/crd.go#L63
4.Delete storagecluster CR:
5.Restore Storagecluster CR:
6.Check storagecluster CR status:
7.Check pods status: