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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mpryc
Copy link
Contributor

@mpryc mpryc commented Feb 28, 2025

Needed-by: migtools/oadp-non-admin#245

Implements migtools/oadp-non-admin#232 with additional fields for:

  • NonAdminBackup
  • NonAdminRestore
  • NonAdminBackupStorageLocation

Why the changes were made

migtools/oadp-non-admin#232 and add similar fields for the NAB and NAR objects.

How to test the changes made

  1. Deployed with local oadp-operator deployment
  2. 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                          APPROVED   BSL-STATUS   STATUS       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                                                              NABSL-NAMESPACE      NABSL-NAME               STATUS    AGE
non-admin-bsl-test-incorre-9aa22872-f39d-427c-b352-6924ff9c2175   non-admin-bsl-test   incorrect-bucket-nabsl   Pending   2m48s
non-admin-bsl-test-perfect-4413c67d-8eb8-42bc-8274-279a91895196   non-admin-bsl-test   perfect-nabsl            Pending   3m17s
non-admin-bsl-test-suspici-4fbdcebf-2277-4f44-890e-35d765815e1a   non-admin-bsl-test   suspicious-sample        Pending   47s
non-admin-bsl-test-waiting-fcae28c2-d8ea-48e2-a35c-8882bfd865e0   non-admin-bsl-test   waitingapproval-nabsl    Pending   2m11s

Approving or rejecting some:

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

Now the NABSL objects

$ oc get nabsl -n non-admin-bsl-test
NAME                          APPROVED   BSL-STATUS    STATUS       AGE
incorrect-bucket-nabsl        True       Unavailable   Created      8m25s
nonexistingsecretinns-nabsl                            BackingOff   6m53s
perfect-nabsl                 True       Available     Created      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                       BACKUP-STATUS         STATUS       AGE
backup-failed-sample       PartiallyFailed       Created      6s
incorrect-bsl-backup                             BackingOff   3m39s
some-backup                Completed             Created      13s

Restore

Nice InProgress and Completed status:

$ oc get nar -n non-admin-bsl-test
NAME               RESTORE-STATUS   STATUS       AGE
non-existing-nab                    BackingOff   46s

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

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

Implements openshift#232 with additional fields for:
 - NonAdminBackup
 - NonAdminRestore
 - NonAdminBackupStorageLocation

Signed-off-by: Michal Pryc <[email protected]>
Copy link

openshift-ci bot commented Feb 28, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mpryc

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2025
@mpryc

This comment was marked as outdated.

@mateusoliveira43

This comment was marked as outdated.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2025
Copy link

openshift-ci bot commented Mar 3, 2025

@mpryc: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.15-e2e-test-kubevirt-aws c0d412f link true /test 4.15-e2e-test-kubevirt-aws
ci/prow/4.17-e2e-test-aws c0d412f link true /test 4.17-e2e-test-aws
ci/prow/4.19-e2e-test-aws c0d412f link true /test 4.19-e2e-test-aws

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@shubham-pampattiwar
Copy link
Member

shubham-pampattiwar commented Mar 5, 2025

@mpryc Would the non-admin user get confused between the columns backup-status and status and similarly for restores and bsls ?

@weshayutin
Copy link
Contributor

@mpryc Would the non-admin user get confused between the columns backup-status and status and similarly for restores and bsls ?

hrm.. I like the way you are thinking but IMHO naming things is hard. Do you have a more clear term? Docs can handle the differences. This is a very nice update for ease of use and transparency to users.

@mpryc
Copy link
Contributor Author

mpryc commented Mar 5, 2025

we could have BACKUP-STATUS and CRD-STATUS ?

@shubham-pampattiwar
Copy link
Member

@weshayutin @mpryc How about these alternatives:

  • BACKUP-STATUS --> VELERO-STATUS OR BACKUP-PHASE OR PHASE
  • STATUS --> REQUEST-STATUS OR PROCESSING STATUS

@kaovilai
Copy link
Member

kaovilai commented Mar 5, 2025

Do you have a more clear term?

Yes I do. Velero Status and NA<C/B/R> Status

@kaovilai
Copy link
Member

kaovilai commented Mar 5, 2025

CRD-STATUS does not help. It does not tell user where signals came from.

User want to know if a status column belongs to NAC or Velero.

@weshayutin
Copy link
Contributor

CRD-STATUS does not help. It does not tell user where signals came from.

User want to know if a status column belongs to NAC or Velero.

I think what Tiger is saying makes a lot of sense. NOW PLEASE NEVER MAKE ME SAY THAT AGAIN.

@shubham-pampattiwar
Copy link
Member

just use PHASE instead of STATUS ?

  • VELERO-BACKUP-PHASE and NAB-PHASE or just PHASE

Note: I don't know how popular our CR shortname (NAB/NAR) would be and the UX of using it in printer column, I am ok with either NAB-PHASE or just PHASE

@kaovilai
Copy link
Member

kaovilai commented Mar 5, 2025

If shorter = better then my final suggestion is

Phase for nac orignated phases
Velero for velero CR originated status/phase

@mateusoliveira43

This comment was marked as outdated.

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 6, 2025
@mpryc
Copy link
Contributor Author

mpryc commented Mar 6, 2025

After reading all the comments above and suggestions.

Proposed Naming Convention:

Object Current Column Proposed Change
NABSL STATUS NAC-PHASE
NABSL BSL-STATUS VELERO-PHASE
NAB STATUS NAC-PHASE
NAB BACKUP-STATUS VELERO-PHASE
NAR STATUS NAC-PHASE
NAR RESTORE-STATUS VELERO-PHASE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants