Skip to content

OADP-1833: Cleanup completed VSBs in backup finalize phase#263

Closed
shubham-pampattiwar wants to merge 1 commit into
openshift:oadp-1.2from
shubham-pampattiwar:del-vsb-fin
Closed

OADP-1833: Cleanup completed VSBs in backup finalize phase#263
shubham-pampattiwar wants to merge 1 commit into
openshift:oadp-1.2from
shubham-pampattiwar:del-vsb-fin

Conversation

@shubham-pampattiwar
Copy link
Copy Markdown
Member

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label May 5, 2023
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented May 5, 2023

@shubham-pampattiwar: This pull request references OADP-1833 which is a valid jira issue.

Details

In response to this:

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/test-infra repository.

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented May 5, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details 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
Copy link
Copy Markdown

openshift-ci Bot commented May 5, 2023

@shubham-pampattiwar: The following test 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/unit-test 35c2756 link false /test unit-test

Full PR test history. Your PR dashboard.

Details

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

}

// check if the VSBs associated with the backup are in completed state, if yes then clean them up
err = datamover.DeleteVSBsIfComplete(backup.Name)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

would it be better to pass r.client instead of making a new one?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if you could here since VSBs are not in the same ns

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For clientsets maybe.. this is normal kbclient
There is nothing ns specific about this. You still have to specify namespace in the object.metadata during get/create/update

func GetVolumeSnapMoverClient() (kbclient.Client, error) {
	client, err := kbclient.New(config.GetConfigOrDie(), kbclient.Options{})
	if err != nil {
		return nil, err
	}
	err = snapmoverv1alpha1.AddToScheme(client.Scheme())
	if err != nil {
		return nil, err
	}

	return client, err
}

@kaovilai
Copy link
Copy Markdown
Member

kaovilai commented May 5, 2023

make update

}

// check if the VSBs associated with the backup are in completed state, if yes then clean them up
err = datamover.DeleteVSBsIfComplete(backup.Name)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

not sure if you could here since VSBs are not in the same ns

if cond.Status == metav1.ConditionFalse && cond.Reason == ReconciledReasonError && cond.Type == ConditionReconciled {
return false, errors.Errorf("volumesnapshotbackup %s has failed status", currentVSB.Name)
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could also add in a check for VSB phase being failed or partiallyFailed. Because there are instances where conditions will be false, such as the objectModified error, but the controller will reconcile and have true conditions again.

@shubham-pampattiwar
Copy link
Copy Markdown
Member Author

Closing in favor of #268

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants