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

feat: Add Non Admin Restore controller #42

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mateusoliveira43
Copy link
Contributor

@mateusoliveira43 mateusoliveira43 commented Apr 9, 2024

Why the changes were made

Non admin users want to restore NonAdminBackups. This PR adds NonAdminRestore CRD along with its controller.

Related to #35

Blocked by #16
Blocked by #54
Blocked by #67
Blocked by openshift/oadp-operator#1388

How the changes were made

Get kubebuilder version 3.14.0 from https://github.com/kubernetes-sigs/kubebuilder/releases and run command documented in Architecture 62d5213

Run make manifests

Fix code quality issues

How to test the changes made

TODO

TODO

This PR still misses

  • write restore logic
  • [check PR size, can be separate] Adding example of configuring DPA with restricted NonAdminRestore fields
    - create/update DPA and configure non admin feature as needed, setting it to enabled
  • - TODO NonAdminRestore
    • show user how to see NonAdminRestore status
    • show user how to see NonAdminRestore queue
  • unit/integration tests for new controller. Some ideas
    • [ERROR] try to create NonAdminRestore from Velero Backup
    • [ERROR] try to create NonAdminRestore from non existing NonAdminBackup
    • [ERROR] try to create NonAdminRestore NonAdminBackup with restricted fields
    • [ERROR] try to create NonAdminRestore in non existing namespace
    • [SUCCESS] create NonAdminRestore from NonAdminBackup owned by user

Copy link

openshift-ci bot commented Apr 9, 2024

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

Copy link

openshift-ci bot commented Apr 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mateusoliveira43

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -48,6 +48,7 @@ help: ## Display this help.
.PHONY: manifests
manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects.
$(CONTROLLER_GEN) rbac:roleName=non-admin-controller-role crd webhook paths="./..." output:crd:artifacts:config=config/crd/bases
sed -i 's/Velero backup/NonAdminBackup/' ./config/crd/bases/nac.oadp.openshift.io_nonadminrestores.yaml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for better user experience

@@ -98,7 +98,8 @@ func main() {
TLSOpts: tlsOpts,
})

if len(constant.OadpNamespace) == 0 {
// TODO create get function in common :question:
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

@@ -73,6 +73,9 @@ var _ = ginkgov2.BeforeSuite(func() {
err = nacv1alpha1.AddToScheme(scheme.Scheme)
gomega.Expect(err).NotTo(gomega.HaveOccurred())

err = nacv1alpha1.AddToScheme(scheme.Scheme)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems duplication, check if it is needed

-p NAME=<NonAdminBackup-name> \
| oc create -f -
```
<!-- TODO how to track status -->
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

@@ -127,7 +128,7 @@ func GenerateVeleroBackupName(namespace, nabName string) string {
}

// UpdateNonAdminPhase updates the phase of a NonAdminBackup object with the provided phase.
func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, phase nacv1alpha1.NonAdminBackupPhase) (bool, error) {
func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, phase nacv1alpha1.NonAdminPhase) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually should be a separate change, so we don't mix restore with change of types. There is already common condition, but it's ok to move also phases to be similar. https://github.com/migtools/oadp-non-admin/blob/master/api/v1alpha1/nonadmincontroller_types.go#L21

@@ -159,7 +160,7 @@ func UpdateNonAdminPhase(ctx context.Context, r client.Client, logger logr.Logge
// based on the provided parameters. It validates the input parameters and ensures
// that the condition is set to the desired status only if it differs from the current status.
// If the condition is already set to the desired status, no update is performed.
func UpdateNonAdminBackupCondition(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, condition nacv1alpha1.NonAdminCondition, conditionStatus metav1.ConditionStatus, reason string, message string) (bool, error) {
func UpdateNonAdminBackupCondition(ctx context.Context, r client.Client, logger logr.Logger, nab *nacv1alpha1.NonAdminBackup, condition types.NonAdminCondition, conditionStatus metav1.ConditionStatus, reason string, message string) (bool, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -29,12 +29,13 @@ import (
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
apimeta "k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
apitypes "k8s.io/apimachinery/pkg/types"
Copy link
Collaborator

Choose a reason for hiding this comment

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

how about changing this to:

apimeta
apimetav1
apimetatypes

@@ -14,17 +14,8 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't know if that's valuable change. Really mixing the api with implementation types as some of them we have in api/* and now some will be in internal/* so a bit of mix that is harder to read. Definitely not part of the NAR controller, can be seaparate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see api folder as the entrypoint for kubebuilder to build CRD yamls, stored in config folder. So, there should be only things used to crate those CRDs in this folder

the type here, was not used by any CRD, so I do not think is the right place to it. Moved to internal common folder where we are storing all the things used more than once by files of the project

do you agree ❓

I can do this in separate PR yes

err := r.Get(ctx, types.NamespacedName{Namespace: req.Namespace, Name: nonAdminBackupName}, nonAdminBackup)
if err != nil {
if errors.IsNotFound(err) {
// TODO add this error message to NonAdminRestore status
Copy link
Collaborator

Choose a reason for hiding this comment

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

// Reconcile is part of the main kubernetes reconciliation loop which aims to
// move the current state of the NonAdminRestore to the desired state.
func (r *NonAdminRestoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
r.Logger = log.FromContext(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is bad. We can assign one per worker but not every reconcile. Removed number of items from NAB reconciler, see:
https://github.com/migtools/oadp-non-admin/pull/56/files#diff-98b8a647a66c83f5ee56f93b7530fb37a28301390936ecdb1fdaf38c031453c6R38-R41

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants