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

Reconcile loop rework #54

Merged
merged 8 commits into from
May 8, 2024
Merged

Conversation

mpryc
Copy link
Collaborator

@mpryc mpryc commented Apr 29, 2024

NonAdminBackup reconcile loop major rework

Rework of the reconcile loop to include batch reconcile.

Move assignment of the Log or Context outside of the reconcile loop.

How to test

Note it's best to test this PR together with #56 which includes namespace guard (case d) in this instructions.

  1. Setup non-admin user as per instructions: https://github.com/migtools/oadp-non-admin/blob/master/docs/non_admin_user.md
  2. Create multiple NonAdminBackup objects in the namespace for which the non-admin user have namespace admin rights. After each creation see if at the end of reconcile the NonAdminBackup is correctly updated with its status and if in some cases VeleroBackup object was created in the openshift-adp namespace (as an admin).

All of the below are in the non-admin namespace nacproject:

a) Case where there is no backupSpec:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example-nospec
  namespace: nacproject
spec: {}

Result should be NAB in backingOff phase with BackupSpec is not defined condition message:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example-nospec
  namespace: nacproject
spec: {}
status:
  conditions:
    - lastTransitionTime: '2024-05-02T08:24:50Z'
      message: BackupSpec is not defined
      reason: InvalidBackupSpec
      status: 'False'
      type: Accepted
  phase: BackingOff

b) Case where there is backupSpec, but empty, should create Velero Backup with nacproject namespace in the includedNamespaces (below is not full spec, removed managedFields from it):

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: empty-backupspec
  namespace: nacproject
spec: 
  backupSpec: {}

Result should be created Velero Backup with includedNamespaces same as the origin NAB namespace and updated NonAdminBackup (below is not full spec, removed managedFields from it):

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-05-02T08:27:46Z'
  name: empty-backupspec
  namespace: nacproject
  resourceVersion: '71261871'
  uid: 624c270e-2711-490d-923e-aaebfcc2a420
spec:
  backupSpec:
    volumeSnapshotLocations:
      - velero-sample-1
    defaultVolumesToFsBackup: false
    csiSnapshotTimeout: 10m0s
    ttl: 720h0m0s
    itemOperationTimeout: 4h0m0s
    metadata: {}
    storageLocation: velero-sample-1
    hooks: {}
    includedNamespaces:
      - nacproject
    snapshotMoveData: false
status:
  conditions:
    - lastTransitionTime: '2024-05-02T08:27:47Z'
      message: backup accepted
      reason: BackupAccepted
      status: 'True'
      type: Accepted
    - lastTransitionTime: '2024-05-02T08:27:57Z'
      message: Created Velero Backup object
      reason: BackupScheduled
      status: 'True'
      type: Queued
  phase: Created
  veleroBackupName: nab-nacproject-d0b80d478b6a26
  veleroBackupNamespace: openshift-adp
  veleroBackupStatus:
    expiration: '2024-06-01T08:27:57Z'
    failureReason: >-
      unable to get credentials: unable to get key for secret: Secret
      "cloud-credentials" not found
    formatVersion: 1.1.0
    phase: Failed
    startTimestamp: '2024-05-02T08:27:57Z'
    version: 1

c) Perform step a) and then update/patch (a) to include empty backupSpec. The reconcile should be same as in b), so the Velero Backup should be created.

d) Create NonAdminBackup with includedNamespaces that does not match the namespace from the NonAdminBackup.

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  name: example-wrongnamespace
  namespace: nacproject
spec:
   backupSpec:
     includedNamespaces:
       - othernamespace

Result should be NAB in backingOff phase with spec.backupSpec.IncludedNamespaces can not contain namespaces other then: nacproject condition message:

apiVersion: nac.oadp.openshift.io/v1alpha1
kind: NonAdminBackup
metadata:
  creationTimestamp: '2024-05-02T08:33:35Z'
  generation: 1
  name: example-wrongnamespace
  namespace: nacproject
  resourceVersion: '71264312'
  uid: 2716a72d-3e44-466d-8126-5bcdeed9413b
spec:
  backupSpec:
    includedNamespaces:
      - othernamespace
status:
  conditions:
    - lastTransitionTime: '2024-05-02T08:33:45Z'
      message: >-
        spec.backupSpec.IncludedNamespaces can not contain namespaces other
        then: nacproject
      reason: InvalidBackupSpec
      status: 'False'
      type: Accepted
  phase: BackingOff

@mpryc
Copy link
Collaborator Author

mpryc commented Apr 29, 2024

need to rebase...

Moves Status outside of Spec and adjusts this to reflect
Velero Backup status as well additional Status when the
Spec within NonAdminBackup is not defined.

Signed-off-by: Michal Pryc <[email protected]>
Additional improvements to the NonAdminBackup:
 - Use generic condition names (Accepted, Queued)
 - Introduce VeleroBackupNamespace
 - Change Status: OadpVeleroBackup to VeleroBackupName
 - Use CammelCase for condition reasoan

Signed-off-by: Michal Pryc <[email protected]>
Adjust NonAdminCondition and move it to new types, so it can
be use as well in NonAdminRestore object.

Signed-off-by: Michal Pryc <[email protected]>
@mpryc mpryc force-pushed the reconcile_loop_rework branch 2 times, most recently from 4799db7 to d081047 Compare April 29, 2024 14:22
Rework of the reconcile loop to include batch
reconcile.

Move assignment of the Log or Context outisde of
the reconcile loop.

Signed-off-by: Michal Pryc <[email protected]>
Small nit to make if statement easier to read.

Signed-off-by: Michal Pryc <[email protected]>
Change of the main reconcile loop to easy logic of the return
from the reconcile batch.

We only requeue when there is need, otherwise we do exit the reconcile
loop at the same place and individual reconcile functions control
if that should happen now or should be requeued.

Signed-off-by: Michal Pryc <[email protected]>
In this implementation we don't have ReconcileBatch, instead
we do have functions with arguments that has appropriate
types.

Also there was modified implementation to first check for
errors and then continue with app flow.

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

@mateusoliveira43 mateusoliveira43 left a comment

Choose a reason for hiding this comment

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

no "how to test" section in PR description

@mpryc
Copy link
Collaborator Author

mpryc commented May 2, 2024

no "how to test" section in PR description

Added with few different use-cases.

With this change there is no separate file per reconcile function
and we move all the reconcile related functions inside
nonadminbackup_controller.

Signed-off-by: Michal Pryc <[email protected]>
@shubham-pampattiwar
Copy link
Member

Did some high level testing, 2 issues that can be addressed separately in follow up PRs:

Copy link

openshift-ci bot commented May 6, 2024

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • 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

@mpryc
Copy link
Collaborator Author

mpryc commented May 7, 2024

@mateusoliveira43 is there anything in this PR that requires rework or can we merge and continue with other PRs to improve code base?

@mpryc mpryc merged commit 15f8b12 into migtools:master May 8, 2024
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Merged / Ready for build
Development

Successfully merging this pull request may close these issues.

6 participants