CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects by mpryc · Pull Request #245 · migtools/oadp-non-admin

mpryc · 2025-02-28T10:59:35Z

Depends-On: openshift/oadp-operator#1709

Implements #232 with additional fields for:

NonAdminBackup
NonAdminRestore
NonAdminBackupStorageLocation

Why the changes were made

To fix #232 and add similar fields for the NAB and NAR objects.

Deployed with local oadp-operator deployment
Tested various objects and it's states:

Prior admin approval or rejections

Created couple of BSLs - names reflect the idea behind each of them

suspicious-nabsl (reject)
nonexistingsecretinns-nabsl (do nothing - no nabslrequest)
waitingapproval-nabsl (do nothing)
incorrect-bucket-nabsl (approve)
perfect-nabsl (approve)

Before any admin action:

$ oc get nabsl -n non-admin-bsl-test
NAME                          REQUEST-APPROVED   REQUEST-PHASE  VELERO-PHASE       AGE
incorrect-bucket-nabsl        False              New                               2m11s
nonexistingsecretinns-nabsl                      BackingOff                        39s
perfect-nabsl                 False              New                               2m40s
suspicious-sample             False              New                               10s
waitingapproval-nabsl         False              New                               94s

Note the nonexistingsecretinns-nabsl have not created request - the user must create secret to get to that state

$ oc get nabslrequest
NAME                                                              REQUEST-PHASE   REQUEST-NAMESPACE     REQUEST-NAME
non-admin-bsl-test-incorre-9aa22872-f39d-427c-b352-6924ff9c2175   Pending         non-admin-bsl-test    incorrect-bucket-nabsl
non-admin-bsl-test-perfect-4413c67d-8eb8-42bc-8274-279a91895196   Pending         non-admin-bsl-test    perfect-nabsl
non-admin-bsl-test-suspici-4fbdcebf-2277-4f44-890e-35d765815e1a   Pending         non-admin-bsl-test    suspicious-sample
non-admin-bsl-test-waiting-fcae28c2-d8ea-48e2-a35c-8882bfd865e0   Pending         non-admin-bsl-test    waitingapproval-nabsl

Approving or rejecting some:

$ oc get nabslrequest
NAME                                                              REQUEST-PHASE   REQUEST-NAMESPACE     REQUEST-NAME               AGE
non-admin-bsl-test-incorre-9aa22872-f39d-427c-b352-6924ff9c2175   Approved        non-admin-bsl-test    incorrect-bucket-nabsl    4m57s
non-admin-bsl-test-perfect-4413c67d-8eb8-42bc-8274-279a91895196   Approved        non-admin-bsl-test    perfect-nabsl             5m26s
non-admin-bsl-test-suspici-4fbdcebf-2277-4f44-890e-35d765815e1a   Rejected        non-admin-bsl-test    suspicious-sample         2m56s
non-admin-bsl-test-waiting-fcae28c2-d8ea-48e2-a35c-8882bfd865e0   Pending         non-admin-bsl-test    waitingapproval-nabsl     4m20s

Now the NABSL objects

$ oc get nabsl -n non-admin-bsl-test
NAME                          REQUEST-APPROVED   REQUEST-PHASE   VELERO-PHASE   AGE
incorrect-bucket-nabsl        True               Created         Unavailable    8m25s
nonexistingsecretinns-nabsl                      BackingOff                     6m53s
perfect-nabsl                 True               Created         Available      8m54s
suspicious-sample             False              BackingOff                     6m24s
waitingapproval-nabsl         False              New                            7m48s

Backup

Couple of backup objects were created

$ oc get nab -n non-admin-bsl-test
NAME                       REQUEST-PHASE   VELERO-PHASE       AGE
backup-failed-sample       Created         PartiallyFailed    6s
incorrect-bsl-backup       BackingOff                         3m39s
some-backup                Created         Completed          13s

Restore

Nice InProgress and Completed status:

$ oc get nar -n non-admin-bsl-test
NAME               REQUEST-PHASE   VELERO-PHASE   AGE
non-existing-nab   BackingOff                     46s

$ oc get nar -n non-admin-bsl-test
NAME               REQUEST-PHASE   VELERO-PHASE   AGE
existing-nab       Created         InProgress     1s
non-existing-nab   BackingOff                     51s

$ oc get nar -n non-admin-bsl-test
NAME               REQUEST-PHASE   VELERO-PHASE   AGE
existing-nab       Created         Completed      3s
non-existing-nab   BackingOff                     53s

mateusoliveira43 · 2025-03-06T12:35:32Z

/hold OADP PR

mateusoliveira43 · 2025-03-06T12:38:19Z

 // +kubebuilder:object:root=true
 // +kubebuilder:subresource:status
 // +kubebuilder:resource:path=nonadminbackups,shortName=nab
+// +kubebuilder:printcolumn:name="Backup-Status",type="string",JSONPath=".status.veleroBackup.status.phase"


name can not contain spaces?

from velero

$ velero backup-location get NAME PROVIDER BUCKET/PREFIX PHASE LAST VALIDATED ACCESS MODE DEFAULT velero-sample-1 aws my-bucket-name/velero Unavailable 2025-03-06 12:54:30 +0000 UTC ReadWrite true $ velero backup get NAME STATUS ERRORS WARNINGS CREATED EXPIRES STORAGE LOCATION SELECTOR backup Failed 0 0 2025-03-06 12:56:14 +0000 UTC 29d velero-sample-1 <none> $ velero restore get NAME BACKUP STATUS STARTED COMPLETED ERRORS WARNINGS CREATED SELECTOR restore backup Failed 2025-03-06 12:56:45 +0000 UTC 2025-03-06 12:56:46 +0000 UTC 0 0 2025-03-06 12:56:45 +0000 UTC <none>

it makes sense to call Backup Status and Status, but I think it would be better Backup Phase and Phase,

How about this?

openshift/oadp-operator#1648 (comment)

mateusoliveira43 · 2025-03-06T13:05:34Z

 // +kubebuilder:subresource:status
 // +kubebuilder:resource:path=nonadminbackupstoragelocations,shortName=nabsl
+// +kubebuilder:printcolumn:name="Approved",type="string",JSONPath=".status.conditions[?(@.type=='ClusterAdminApproved')].status"
+// +kubebuilder:printcolumn:name="BSL-Status",type="string",JSONPath=".status.veleroBackupStorageLocation.status.phase"


from velero

$ velero backup-location get NAME PROVIDER BUCKET/PREFIX PHASE LAST VALIDATED ACCESS MODE DEFAULT velero-sample-1 aws my-bucket-name/velero Unavailable 2025-03-06 12:54:30 +0000 UTC ReadWrite true $ velero backup get NAME STATUS ERRORS WARNINGS CREATED EXPIRES STORAGE LOCATION SELECTOR backup Failed 0 0 2025-03-06 12:56:14 +0000 UTC 29d velero-sample-1 <none>

would change BSL-Status to Storage Location Phase (if spaces are allowed) and Status to Phase

does the order matter?

currently is APPROVED -> BSL-Phase -> Phase (NABSL)

would be better to change to NABSL phase -> Approved -> Storage Location Phase?

I would really not like to have spaces as this isn't a good CLI/Shell practice in my opinion. It's hard to use awk/sed/cut for those as well they make have affect on the alignment in various terminal systems.

mateusoliveira43 · 2025-03-06T13:23:01Z

 // +kubebuilder:subresource:status
 // +kubebuilder:resource:path=nonadminbackupstoragelocationrequests,shortName=nabslrequest
+// +kubebuilder:printcolumn:name="NABSL-Namespace",type="string",JSONPath=".status.nonAdminBackupStorageLocation.namespace"
+// +kubebuilder:printcolumn:name="NABSL-Name",type="string",JSONPath=".status.nonAdminBackupStorageLocation.name"


would drop NABSL prefix in namespace title. having just namespace I think is enough m(this is a request for NAMESPACE X)

would also drop NAME suffix for NABSL

This object is mostly viewable by the cluster admin to interact with and the namespace and name is the one that requested the BSL creation, we can drop it, but would this be easy enough to understand without reading documentation ?

mpryc · 2025-03-20T10:03:36Z

Updated, please refer to the following comment for details: openshift/oadp-operator#1648 (comment)

In the PR description there are updated column names and it's order to reflect current implementation:
openshift/oadp-operator#1648 (comment)

weshayutin · 2025-04-11T00:40:05Z

/retest

Implements migtools#232 with additional fields for: - NonAdminBackup - NonAdminRestore - NonAdminBackupStorageLocation Signed-off-by: Michal Pryc <mpryc@redhat.com>

Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com>

mpryc · 2025-04-11T10:58:17Z

this one is required first: openshift/oadp-operator#1709

@weshayutin ^^ FYI

mpryc · 2025-04-11T11:01:58Z

The following one got merged:
openshift/oadp-operator#1648

Then the current PR never got merged, and the subsequent OADP update removed the above one (see openshift/oadp-operator@67020bf#diff-367ce0b764dc1ca544b1d76f9b8d8265ccbcf1a6ac552fed21b5cd0f73920515).

We need to sync those merges, otherwise there will be always removal of oadp parts during next manifest updates.

…1709) Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com>

shubham-pampattiwar

/lgtm

openshift-ci · 2025-04-11T15:56:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpryc, shubham-pampattiwar, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mpryc,shubham-pampattiwar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shubham-pampattiwar · 2025-04-14T13:44:19Z

/cherry-pick oadp-1.5

openshift-cherrypick-robot · 2025-04-14T13:44:56Z

@shubham-pampattiwar: new pull request created: #268

Details

In response to this:

/cherry-pick oadp-1.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com>

…1711) Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com> Co-authored-by: Michal Pryc <mpryc@redhat.com>

openshift-ci Bot requested review from mrnold and sseago February 28, 2025 10:59

openshift-ci Bot added the approved label Feb 28, 2025

mpryc requested review from shubham-pampattiwar and weshayutin February 28, 2025 10:59

mpryc mentioned this pull request Feb 28, 2025

CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects openshift/oadp-operator#1648

Merged

openshift-ci Bot added the do-not-merge/hold label Mar 6, 2025

mateusoliveira43 reviewed Mar 6, 2025

View reviewed changes

mpryc force-pushed the issue_232_oc_get_fields branch from bdb2859 to 4176804 Compare March 20, 2025 09:42

shubham-pampattiwar added the feature complete label Apr 10, 2025

shubham-pampattiwar moved this to Ready for Review in OADP Apr 10, 2025

shubham-pampattiwar added this to OADP Apr 10, 2025

CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects

c1fc2d5

Implements migtools#232 with additional fields for: - NonAdminBackup - NonAdminRestore - NonAdminBackupStorageLocation Signed-off-by: Michal Pryc <mpryc@redhat.com>

mpryc force-pushed the issue_232_oc_get_fields branch from 4176804 to c1fc2d5 Compare April 11, 2025 10:51

mpryc added a commit to mpryc/oadp-operator that referenced this pull request Apr 11, 2025

CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects

b52eb9f

Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com>

mpryc mentioned this pull request Apr 11, 2025

CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects openshift/oadp-operator#1709

Merged

weshayutin pushed a commit to openshift/oadp-operator that referenced this pull request Apr 11, 2025

CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects (#…

f7f0e80

…1709) Change required by the migtools/oadp-non-admin#245 Signed-off-by: Michal Pryc <mpryc@redhat.com>

weshayutin approved these changes Apr 11, 2025

View reviewed changes

shubham-pampattiwar approved these changes Apr 11, 2025

View reviewed changes

openshift-ci Bot assigned shubham-pampattiwar Apr 11, 2025

openshift-ci Bot added the lgtm label Apr 11, 2025

weshayutin removed the do-not-merge/hold label Apr 11, 2025

openshift-merge-bot Bot merged commit c8df739 into migtools:master Apr 11, 2025
11 of 12 checks passed

github-project-automation Bot moved this from Ready for Review to Merged / Ready for build in OADP Apr 11, 2025

openshift-cherrypick-robot mentioned this pull request Apr 14, 2025

[oadp-1.5] CRD updates for the oc CLI fields for the NAR, NAB and NABSL objects #268

Merged

mpryc mentioned this pull request Apr 15, 2025

OADP-4001 Self-Service openshift/openshift-docs#89043

Merged

1 task

Conversation

mpryc commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the changes were made

Prior admin approval or rejections

Created couple of BSLs - names reflect the idea behind each of them

Before any admin action:

Backup

Restore

Uh oh!

mateusoliveira43 commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpryc commented Mar 20, 2025

Uh oh!

weshayutin commented Apr 11, 2025

Uh oh!

mpryc commented Apr 11, 2025

Uh oh!

mpryc commented Apr 11, 2025

Uh oh!

shubham-pampattiwar left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Apr 11, 2025

Uh oh!

Uh oh!

shubham-pampattiwar commented Apr 14, 2025

Uh oh!

openshift-cherrypick-robot commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mpryc commented Feb 28, 2025 •

edited

Loading