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

Add component reconciliation pipeline framework #1320

Conversation

lburgazzoli
Copy link
Contributor

Description

This PR introduces a new component reconciliation pipeline framework and a set of reusable action that can be composed according to the need of a specific component. This PR is the result of the work related to RHOAIENG-14676

The main entry point is the ComponentReconcilerBuilder:

 NewReconciler creates a ComponentReconciler for the Dashboard API.
func NewReconciler(ctx context.Context, mgr ctrl.Manager) error {
	componentName := computeComponentName()

	_, err := reconciler.ComponentReconcilerFor[*componentsv1.Dashboard](mgr, componentsv1.DashboardInstanceName, &componentsv1.Dashboard{}).
		Owns(&corev1.ConfigMap{}).
		Owns(&corev1.Secret{}).
		Owns(&appsv1.Deployment{}, builder.WithPredicates(resources.NewDeploymentPredicate())).
		Owns(&routev1.Route{}).
		Watches(&extv1.CustomResourceDefinition{}).
		WithAction(initialize).
		WithAction(devFlags).
		WithAction(render.NewAction(
			render.WithCache(true, render.DefaultCachingKeyFn),
			render.WithLabel(labels.ODH.Component(componentName), "true"),
			render.WithLabel(labels.K8SCommon.PartOf, componentName),
		)).
		WithAction(customizeResources).
		WithAction(deploy.NewAction(
			deploy.WithFieldOwner(componentsv1.DashboardInstanceName),
			deploy.WithLabel(labels.ComponentManagedBy, componentsv1.DashboardInstanceName),
		)).
		WithAction(updatestatus.NewAction(
			updatestatus.WithSelectorLabel(labels.ComponentManagedBy, componentsv1.DashboardInstanceName),
		)).
		WithAction(updateStatus).
		Build(ctx)

	if err != nil {
		return fmt.Errorf("could not create the dashboard controller: %w", err)
	}

	return nil
}

Actions currently provided out of the box are provided by the packages in github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions:

  • render
  • deploy
  • update-status
  • delete

How Has This Been Tested?

Screenshot or short clip

Merge criteria

  • You have read the contributors guide.
  • Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
  • Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

Signed-off-by: Luca Burgazzoli <lburgazzoli@gmail.com>
@lburgazzoli lburgazzoli requested review from VaishnaviHire, zdtsw, grdryn, jackdelahunt and ykaliuta and removed request for MarianMacik and biswassri October 25, 2024 13:17
@VaishnaviHire
Copy link
Member

/test opendatahub-operator-e2e

1 similar comment
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@ykaliuta
Copy link
Contributor

May be we do not need e2e mandatory for the branch?

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Oct 26, 2024

May be we do not need e2e mandatory for the branch?

I'd like to get this one successfully running as in one of the run, the error was quite strange and I'd like to be sure we are not ignoring anything relevant.

Not it seems there are some issues setting up the environment

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

4 similar comments
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

Copy link
Member

@grdryn grdryn left a comment

Choose a reason for hiding this comment

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

I haven't gotten completely through this yet, but leaving some questions to try to round out my own understanding 🙂

func updatePodSecurityRolebinding(ctx context.Context, rr *odhtypes.ReconciliationRequest) error {
switch rr.Release.Name {
case cluster.SelfManagedRhods, cluster.ManagedRhods:
if err := cluster.UpdatePodSecurityRolebinding(ctx, rr.Client, rr.DSCI.Spec.ApplicationsNamespace, "rhods-dashboard"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

I see the string "rhods-dashboard" is defined as a constant, ComponentNameDownstream, in dashboard_support.go. Shoudl that constant be used here rather than the hardcoded value?

I don't see "odh-dashboard" (for the default case on L100) being specified in the same way, so maybe even though it's the same string here, it's a different meaning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I think it has a different meaning, but that predates this PR so I'll defer to @VaishnaviHire or @zdtsw to add additional context

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

1 similar comment
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

case resources.GetAnnotation(&obj, annotations.ManagedByODHOperator) == "false":
// remove the opendatahub.io/managed as it should not be set
// to the actual object in this case
resources.RemoveAnnotation(&obj, annotations.ManagedByODHOperator)
Copy link
Member

Choose a reason for hiding this comment

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

not sure i understand this:
if the resource exist in the cluster, and with annoation to false, we dont touch it
if the resource does not exist, (like we just enable component) and component team mark the annoation in manifests with annotation to false, we remove that annoation then create it? then next time run into "run" it will for intl the first swtich...case ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is the same for each reconciliation. the annotation is removed as the behavior is controller by the component developers, not the user so I didn't want it to be set on the cluster. But is something we can talk about since that annotation is for the moment only an internal annotation (i.e. it is added to the OdhDashboardConfig)

@lburgazzoli lburgazzoli changed the title Add component reconciliation pipeline framework (take 2) Add component reconciliation pipeline framework Oct 29, 2024
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@lburgazzoli lburgazzoli force-pushed the kustomize_pipeline branch 2 times, most recently from 0f5a430 to b1bb237 Compare October 29, 2024 10:17
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 47.19731% with 471 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature-operator-refactor@9a00ff0). Learn more about missing BASE report.

Files with missing lines Patch % Lines
pkg/controller/actions/deploy/action_deploy.go 40.64% 105 Missing and 6 partials ⚠️
pkg/utils/test/matchers/jq/jq_support.go 25.00% 60 Missing and 6 partials ⚠️
pkg/utils/test/matchers/yq/yq_support.go 38.23% 38 Missing and 4 partials ⚠️
...r/actions/deploy/action_deploy_merge_deployment.go 51.38% 23 Missing and 12 partials ⚠️
pkg/manifests/kustomize/kustomize_render_opts.go 33.96% 35 Missing ⚠️
...ntroller/actions/render/action_render_manifests.go 71.11% 23 Missing and 3 partials ⚠️
pkg/manifests/kustomize/kustomize_engine.go 50.00% 17 Missing and 8 partials ⚠️
pkg/utils/test/matchers/yq/yq_matcher.go 32.14% 14 Missing and 5 partials ⚠️
pkg/utils/test/matchers/jq/jq_matcher.go 43.33% 13 Missing and 4 partials ⚠️
pkg/plugins/removerplugin.go 51.85% 11 Missing and 2 partials ⚠️
... and 10 more
Additional details and impacted files
@@                     Coverage Diff                      @@
##             feature-operator-refactor    #1320   +/-   ##
============================================================
  Coverage                             ?   24.59%           
============================================================
  Files                                ?       52           
  Lines                                ?     4265           
  Branches                             ?        0           
============================================================
  Hits                                 ?     1049           
  Misses                               ?     3087           
  Partials                             ?      129           

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

@lburgazzoli lburgazzoli requested review from grdryn and zdtsw October 29, 2024 11:50
@lburgazzoli
Copy link
Contributor Author

@zdtsw @VaishnaviHire @ykaliuta e2e are green, I guess we are ok to merge ?
@grdryn @zdtsw I'll address some of the finding in a separate PR if you are ok with that

@ykaliuta
Copy link
Contributor

yes for me

@VaishnaviHire
Copy link
Member

/lgtm

Copy link

openshift-ci bot commented Oct 29, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: VaishnaviHire

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

@openshift-merge-bot openshift-merge-bot bot merged commit 7d4613c into opendatahub-io:feature-operator-refactor Oct 29, 2024
10 checks passed
@lburgazzoli lburgazzoli deleted the kustomize_pipeline branch October 29, 2024 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants