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

PB-4958: RT for restore flow for non-nfs BLs #1596

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

Conversation

sgajawada-px
Copy link
Contributor

@sgajawada-px sgajawada-px commented Dec 13, 2023

This is a POC NOT FOR REVIEW

What type of PR is this?

Uncomment only one and also add the corresponding label in the PR:
bug
feature
improvement
cleanup
api-change
design
documentation
failing-test
unit-test
integration-test

What this PR does / why we need it:
It is a POC to test the resource transformation during the restore controller. As part of it we can transform all kubernetes resources in the backup object expect CRDs for the non-NFS BLs

Does this PR change a user-facing CRD or CLI?:
Yes, ApplicationRestore CRD we are accepting resourceTransformationTemplate name in the spec to transform the application specs in the backup object.

Is a release note needed?:
Yes,

Issue:
User Impact:
Resolution

Does this change need to be cherry-picked to a release branch?:

Unit Test Results
2023-12-13_15-56
2023-12-13_15-55
2023-12-13_15-54
2023-12-13_15-52
2023-12-13_15-51

@cnbu-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link

codecov bot commented Dec 13, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (56c0afc) 66.95% compared to head (47c1be7) 67.95%.
Report is 13 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1596      +/-   ##
==========================================
+ Coverage   66.95%   67.95%   +0.99%     
==========================================
  Files          43       43              
  Lines        5571     5704     +133     
==========================================
+ Hits         3730     3876     +146     
+ Misses       1502     1496       -6     
+ Partials      339      332       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sgajawada-px sgajawada-px changed the title PB-4958: v1 PB-4958: RT for restore flow for non-nfs BLs Dec 13, 2023
// rtTemplate := &storkapi.ResourceTransformation{}
// a.client.Get(context.Background(), types.NamespacedName{Name: restore.Spec.ResourceTransformationTemplate, Namespace: kdmputils.AdminNamespace}, rtTemplate)

var gvkResourceTransform map[schema.GroupVersionKind][]storkapi.TransformSpecs
Copy link
Contributor

Choose a reason for hiding this comment

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

@sgajawada-px The array nature of TransformSpecs will be effectively utilised only in future wherein we have multiple set of rules for a single type of resource and it is via a two different selector. As of current UX we don't support this via UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's just good to have support from our end.


if gvkResourceTransform != nil {
objects := make([]*unstructured.Unstructured, 0)
if err = json.Unmarshal(nsData, &namespaces); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This sound like a hack because of the way we store namespace resources., We should have fixed while we are backing up namespaces with GVK info in namespace.json. While doing actual implementation lets see if we have to chnage in backup path and how big that challenge would be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO for the actual implementation

@@ -121,6 +126,9 @@ func (r *ResourceTransformationController) handle(ctx context.Context, transform
return nil
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: During r.validateSpecPath(transform) need to remove the filter getsupportsk8stypes to allow cr and crds in rt only for the restore flow

return gvkResourceTransform, fmt.Errorf("empty name received for resource transformation/namespace")
}

resp, err := storkops.Instance().GetResourceTransformation(transformName, namespace)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As this is called n times from
verifyNamespace - download namespace.json
VolumeStage - download storageclass.json, resources.json, crd.json
ApplicationStage - download resources.json, crd.json

Is good to have a cache for RT CR object instead of reaching out to apiserver

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to check also when the RT template reaches its high watermark interms of rules how big this in-memory copy shoots up. Else in case of multiple restore with multiple RT template this in memory size will bloat the whole cluster memory consumption

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants