feat: Integrate Non-Admin Controller (NAC) with OADP #1370

mateusoliveira43 · 2024-03-20T13:53:49Z

Why the changes were made

Add Non-Admin Controller (NAC) to OADP to start testing it. Fixes migtools/oadp-non-admin#19

How the changes were made

Studied how to "reverse engineered" this PR #1171

Then applied necessary changes to migtools/oadp-non-admin repo

Created command to update non admin controller Kubernetes objects

How to test the changes made

Read migtools/oadp-non-admin#16

To test update command, change anything in newly added files in config/ folder and run

git clone --depth=1 git@github.com:mateusoliveira43/oadp-non-admin -b fix/contribution-doc
NON_ADMIN_CONTROLLER_PATH=<path_to_git_clone> make update-non-admin-manifests

Changes should be reverted

Also, check if all necessary files from NAC were included here.

Run make deploy-olm and create DPA with new feature

...
spec:
  features:
    nonAdmin:
      enable: true
...

You should be able to:

deploy Non-Admin Controller (spec.features.nonAdmin.enable true)
undeploy Non-Admin Controller (spec.features.nonAdmin.enable false or unset)

use custom Non-Admin Controller image using environment variable and unsupported override
To create custom image, run

git clone --depth=1 git@github.com:mateusoliveira43/oadp-non-admin -b fix/contribution-doc
IMG=ttl.sh/<unique_name_from_your_head>:1h make docker-build docker-push

openshift-ci · 2024-03-20T13:53:57Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

bundle/manifests/oadp-operator.clusterserviceversion.yaml

config/manager/manager.yaml

config/rbac/role.yaml

controllers/nonadmin_controller.go

Makefile

api/v1alpha1/oadp_types.go

config/manager/manager.yaml

config/non-admin-controller_rbac/role_binding.yaml

controllers/nonadmin_controller.go

api/v1alpha1/oadp_types.go

kaovilai

There are still unresolved "resolved" issues, but otherwise this lgtm.

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

shubham-pampattiwar · 2024-04-12T07:51:56Z

controllers/nonadmin_controller.go

+
+func (r *DPAReconciler) buildNonAdminDeployment(deploymentObject *appsv1.Deployment, dpa *oadpv1alpha1.DataProtectionApplication) error {
+	// TODO https://github.com/openshift/oadp-operator/pull/1316
+	nonAdminImage := r.getNonAdminImage(dpa)


Lets move this function call to ensureRequiredSpecs func and modify the ensureRequiredSpecs function to just accept the deploymentObject as the only parmeter.

That would need to make ensureRequiredSpecs a method of DPAReconciler, because today, get image function needs DPA

I believe is better to be like this

In the end, we only call buildNonAdminDeployment function, and after Sachin's PR that function will accept the deploymentObject as the only parmeter

controllers/nonadmin_controller.go

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mpryc · 2024-04-15T16:18:02Z

/lgtm

weshayutin · 2024-04-15T16:27:37Z

Let's get the details on the virt test, doesn't appear like a flake to me.

https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_oadp-operator/1370/pull-ci-openshift-oadp-operator-master-4.14-e2e-test-kubevirt-aws/1779863902008381440/artifacts/e2e-test-kubevirt-aws/e2e/build-log.txt

Please rerun ci/prow/4.14-e2e-test-kubevirt-aws

Azure tests can be overridden at this time.

openshift-ci-robot · 2024-04-15T16:45:20Z

/retest-required

Remaining retests: 0 against base HEAD e379366 and 2 for PR HEAD be0477a in total

kaovilai · 2024-04-15T19:29:40Z

config/non-admin-controller_rbac/role.yaml

+- apiGroups:
+  - velero.io
+  resources:
+  - backups


Where are the roles for restore?

restore is not yet in.. just backup

waiting this one merge to unblock this work for migtools/oadp-non-admin#42

controllers/nonadmin_controller_test.go

kaovilai · 2024-04-15T19:57:24Z

controllers/nonadmin_controller_test.go

+	deployment      *appsv1.Deployment
+}
+
+func createTestDeployment(namespace string) *appsv1.Deployment {


I would call this createTestDeploymentNeedingUpdate or something to signify that this deployment is not the final expected result.

kaovilai · 2024-04-15T19:59:44Z

/retest

see if azure works.

openshift-ci · 2024-04-16T19:27:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, mateusoliveira43, mpryc, sseago

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~~ [kaovilai,mateusoliveira43,mpryc,sseago]

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

sseago · 2024-04-16T19:29:25Z

/override ci/prow/4.13-e2e-test-azure

openshift-ci · 2024-04-16T19:29:59Z

@sseago: Overrode contexts on behalf of sseago: ci/prow/4.13-e2e-test-azure

In response to this:

/override ci/prow/4.13-e2e-test-azure

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 · 2024-04-16T19:30:01Z

@mateusoliveira43: all tests passed!

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2024

mateusoliveira43 changed the title ~~feat: Integrate Non-Admin Controller with OADP~~ feat: Integrate Non-Admin Controller (NAC) with OADP Mar 20, 2024

mateusoliveira43 requested a review from shubham-pampattiwar March 20, 2024 13:54

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2024

mateusoliveira43 requested a review from mpryc March 20, 2024 13:54

shubham-pampattiwar reviewed Mar 20, 2024

View reviewed changes

bundle/manifests/oadp-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 20, 2024

View reviewed changes

config/manager/manager.yaml Outdated Show resolved Hide resolved

mateusoliveira43 commented Mar 20, 2024

View reviewed changes

config/rbac/role.yaml Outdated Show resolved Hide resolved

mateusoliveira43 commented Mar 20, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Mar 20, 2024

View reviewed changes

controllers/nonadmin_controller.go Show resolved Hide resolved

mateusoliveira43 commented Mar 20, 2024

View reviewed changes

controllers/nonadmin_controller.go Show resolved Hide resolved

mateusoliveira43 mentioned this pull request Mar 25, 2024

fix: Add more documenation to project migtools/oadp-non-admin#16

Merged

mateusoliveira43 commented Mar 26, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Mar 27, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 commented Mar 27, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 marked this pull request as ready for review March 27, 2024 21:25

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 27, 2024

openshift-ci bot requested review from hhpatel14 and shubham-pampattiwar March 27, 2024 21:25

mateusoliveira43 commented Mar 28, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

api/v1alpha1/oadp_types.go Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

api/v1alpha1/oadp_types.go Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

config/manager/manager.yaml Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

config/non-admin-controller_rbac/role_binding.yaml Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Mar 28, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

mateusoliveira43 mentioned this pull request Mar 28, 2024

fix #1128: Removed multiple DPA get calls for Reconcilers #1316

Open

shubham-pampattiwar reviewed Apr 2, 2024

View reviewed changes

api/v1alpha1/oadp_types.go Outdated Show resolved Hide resolved

kaovilai previously approved these changes Apr 4, 2024

View reviewed changes

mateusoliveira43 added 7 commits April 11, 2024 15:44

fixup! feat: Integrate Non-Admin Controller with OADP

8d9414a

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

ca96cc6

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

d17f7ae

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

d112ce0

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

e5f404e

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

bcc484e

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

fixup! feat: Integrate Non-Admin Controller with OADP

9a8749f

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 force-pushed the feat/nac-integration branch from 21d40db to 9a8749f Compare April 11, 2024 18:45

shubham-pampattiwar reviewed Apr 12, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

shubham-pampattiwar reviewed Apr 12, 2024

View reviewed changes

controllers/nonadmin_controller.go Outdated Show resolved Hide resolved

fixup! feat: Integrate Non-Admin Controller with OADP

be0477a

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mpryc approved these changes Apr 15, 2024

View reviewed changes

openshift-ci bot assigned mpryc Apr 15, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2024

kaovilai reviewed Apr 15, 2024

View reviewed changes

controllers/nonadmin_controller_test.go Show resolved Hide resolved

kaovilai reviewed Apr 15, 2024

View reviewed changes

kaovilai approved these changes Apr 15, 2024

View reviewed changes

mateusoliveira43 mentioned this pull request Apr 16, 2024

feat: Add Non Admin Restore controller migtools/oadp-non-admin#42

Draft

sseago approved these changes Apr 16, 2024

View reviewed changes

openshift-merge-bot bot merged commit c3cbfd6 into openshift:master Apr 16, 2024
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Integrate Non-Admin Controller (NAC) with OADP #1370

feat: Integrate Non-Admin Controller (NAC) with OADP #1370

mateusoliveira43 commented Mar 20, 2024 •

edited

Loading

openshift-ci bot commented Mar 20, 2024

kaovilai left a comment

shubham-pampattiwar Apr 12, 2024

mateusoliveira43 Apr 15, 2024

mpryc commented Apr 15, 2024

weshayutin commented Apr 15, 2024

openshift-ci-robot commented Apr 15, 2024

kaovilai Apr 15, 2024

weshayutin Apr 15, 2024

mateusoliveira43 Apr 15, 2024

kaovilai Apr 15, 2024

kaovilai commented Apr 15, 2024

openshift-ci bot commented Apr 16, 2024

sseago commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

feat: Integrate Non-Admin Controller (NAC) with OADP #1370

feat: Integrate Non-Admin Controller (NAC) with OADP #1370

Conversation

mateusoliveira43 commented Mar 20, 2024 • edited Loading

Why the changes were made

How the changes were made

How to test the changes made

openshift-ci bot commented Mar 20, 2024

kaovilai left a comment

Choose a reason for hiding this comment

shubham-pampattiwar Apr 12, 2024

Choose a reason for hiding this comment

mateusoliveira43 Apr 15, 2024

Choose a reason for hiding this comment

mpryc commented Apr 15, 2024

weshayutin commented Apr 15, 2024

openshift-ci-robot commented Apr 15, 2024

kaovilai Apr 15, 2024

Choose a reason for hiding this comment

weshayutin Apr 15, 2024

Choose a reason for hiding this comment

mateusoliveira43 Apr 15, 2024

Choose a reason for hiding this comment

kaovilai Apr 15, 2024

Choose a reason for hiding this comment

kaovilai commented Apr 15, 2024

openshift-ci bot commented Apr 16, 2024

sseago commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

openshift-ci bot commented Apr 16, 2024

mateusoliveira43 commented Mar 20, 2024 •

edited

Loading