NonAdminDownloadRequests implementation #233
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kaovilai 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 |
kaovilai
force-pushed
the
NADR
branch
3 times, most recently
from
85deef7 to
b61ea75
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
kaovilai
force-pushed
the
NADR
branch
2 times, most recently
from
eb873cc to
0df85a8
Compare
kaovilai
force-pushed
the
NADR
branch
5 times, most recently
from
54212e1 to
7bc176b
Compare
…nDownloadRequest Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
| func (r *NonAdminDownloadRequestReconciler) deleteExpiredDownloadRequest(ctx context.Context, veleroDR *velerov1.DownloadRequest, req *nacv1alpha1.NonAdminDownloadRequest) error { | ||
| logger := log.FromContext(ctx) | ||
| logger.V(1).Info("Deleting expired NonAdminDownloadRequest associated velero download request", req.VeleroDownloadRequestName(), req.Namespace) | ||
| if err := r.Delete(ctx, veleroDR); err != nil { |
There was a problem hiding this comment.
is not this Velero responsibility?
There was a problem hiding this comment.
No. Velero does not garbage collect.
I think they pile up.
Velero might implement gc eventually until then our logic here is fine imo.
There was a problem hiding this comment.
is this not garbage collection for it?
https://github.com/vmware-tanzu/velero/blob/main/pkg/controller/download_request_controller.go#L108
There was a problem hiding this comment.
ok maybe ive just seen some cu clusters clogged with them..
There was a problem hiding this comment.
we can let velero delete, and on velero delete event we can delete nadr.
There was a problem hiding this comment.
what do we want to do with things without expiration? ie. velero server too busy to handle and process.. no url therefore no expiration.
There was a problem hiding this comment.
shubham said GC on NADR without a corresponding velero DR.. I assume we want to only do this if NADR.status.url is populated which is already processed.
There was a problem hiding this comment.
velero once a minute reconcile and check for expiration. we can defer to velero for a successful case.
we need to handle case without expiration due to errors possibly but future enhancement/discussion.
| return err | ||
| } | ||
| } | ||
| logger.V(1).Info("Deleting expired NonAdminDownloadRequest", req.Name, req.Namespace) |
There was a problem hiding this comment.
to keep similar behavior for backup/restore, should we just add message in status that associated velero DownloadRequest was deleted?
There was a problem hiding this comment.
DownloadRequest life is very short lived.. so I don't think knowing velero is deleted is useful information to patch into the object. we already logged velero object is being deleted.
There was a problem hiding this comment.
could patch in.. but the object is being deleted right after.. I don't think you'd want this object lingering past expiration IMO.
| } | ||
| } | ||
| } | ||
| return nadr.Spec.Target.Kind != constant.EmptyString && |
There was a problem hiding this comment.
I think this does not make sense. Are not these fields required? it would always return true
There was a problem hiding this comment.
If I add +kubebuilder:validation:Required then I can replace this with a comment.
There was a problem hiding this comment.
no no, I think this is already done in velero side
There was a problem hiding this comment.
Do you want this check replaced with comment?
There was a problem hiding this comment.
just change return true, no need for comment
| func (r *NonAdminDownloadRequestReconciler) SetupWithManager(mgr ctrl.Manager) error { | ||
| return ctrl.NewControllerManagedBy(mgr). | ||
| For(&nacv1alpha1.NonAdminDownloadRequest{}, builder.WithPredicates(ctrlpredicate.Funcs{ | ||
| CreateFunc: func(tce event.TypedCreateEvent[client.Object]) bool { |
There was a problem hiding this comment.
we should always return true for create events
There was a problem hiding this comment.
only if CRD validation blocks all invalids.. do we want that?
There was a problem hiding this comment.
since this is the controller for this CRD, should it not process all objects creation?
There was a problem hiding this comment.
I don't think there would be anything to process otherwise.. so no.
There was a problem hiding this comment.
We can always return true if all creations have target/kind which it seems we do via velero crd validaiton markers. We can consider simplyfy to returing true here.
| expectedDownloadName = nadr.VeleroDownloadRequestName() | ||
|
|
||
| // Create a fake client with the NonAdminDownloadRequest object | ||
| fakeClient = fake.NewClientBuilder().WithScheme(scheme.Scheme). |
There was a problem hiding this comment.
I would not use fake client within controller tests
There was a problem hiding this comment.
on controller folder, we have access to envtest and k8sClient from suite_test.go
There was a problem hiding this comment.
kubernetes-sigs/controller-runtime#880
envtest is not always the best choice.
There was a problem hiding this comment.
gomega.Expect(createTestNamespaces(ctx, nonAdminNamespace, oadpNamespace)).To(gomega.Succeed())
How did you get these to work?
I see there is deleteNS code in your controller_tests
I just never got delete NS to work.
[FAILED] in [BeforeEach] - /Users/tiger/git/oadp-non-admin/internal/controller/nonadmindownloadrequest_controller_test.go:108 @ 03/03/25 12:11:43.251
[FAILED] in [AfterEach] - /Users/tiger/git/oadp-non-admin/internal/controller/nonadmindownloadrequest_controller_test.go:142 @ 03/03/25 12:11:48.254
<< Timeline
[FAILED] Expected success, but got an error:
<*errors.StatusError | 0x14000cb61e0>:
object is being deleted: namespaces "test-namespace" already exists
{
ErrStatus: {
TypeMeta: {Kind: "", APIVersion: ""},
ListMeta: {
SelfLink: "",
ResourceVersion: "",
Continue: "",
RemainingItemCount: nil,
},
Status: "Failure",
Message: "object is being deleted: namespaces \"test-namespace\" already exists",
Reason: "AlreadyExists",
Details: {
Name: "test-namespace",
Group: "",
Kind: "namespaces",
UID: "",
Causes: nil,
RetryAfterSeconds: 0,
},
Code: 409,
},
}
In [BeforeEach] at: /Users/tiger/git/oadp-non-admin/internal/controller/nonadmindownloadrequest_controller_test.go:108 @ 03/03/25 12:11:43.251
There was a problem hiding this comment.
we "simulate" deletion by calling delete on these namespaces, and not using them again
each test uses a different set of namespaces (usually 2: nonAdmin and OADP namespace)
| return ctrl.Result{}, nil | ||
| } | ||
| logger := log.FromContext(ctx) | ||
| logger.Info("Reconciling NonAdminDownloadRequest", "name", req.Name, "namespace", req.Namespace) |
There was a problem hiding this comment.
do not add info already present in log
example log without adding anything
{"controller": "nonadmindownloadrequest", "controllerGroup": "oadp.openshift.io", "controllerKind": "NonAdminDownloadRequest", "NonAdminDownloadRequest": {"name":"non-admin-download-request-object-1","namespace":"test-non-admin-download-request-reconcile-full-1"}, "namespace": "test-non-admin-download-request-reconcile-full-1", "name": "non-admin-download-request-object-1", "reconcileID": "5f3f2745-13b3-46c8-a340-5166350c9997"}
| logger.V(1).Info("Deleting expired NonAdminDownloadRequest associated velero download request", req.VeleroDownloadRequestName(), req.Namespace) | ||
| if err := r.Delete(ctx, veleroDR); err != nil { | ||
| if !apierrors.IsNotFound(err) { | ||
| logger.V(1).Info("Failed to delete expired NonAdminDownloadRequest associated velero download request", req.VeleroDownloadRequestName(), err) |
There was a problem hiding this comment.
when returning error, should not the log level be error?
There was a problem hiding this comment.
will log as error. should not matter too much as reconcile err should already log the err.
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Signed-off-by: Tiger Kaovilai <[email protected]>
Why the changes were made
Fixes #213
OADP PR openshift/oadp-operator#1643
How to test the changes made
See: openshift/oadp-operator#1643