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

ROX-27130: Reconcile operators using ArgoCD #2195

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

ludydoo
Copy link
Collaborator

@ludydoo ludydoo commented Feb 11, 2025

Description

This PR moves the reconciliation of RHACS operators to ArgoCD.

  • It introduces a new top-level field in the "Gitops Config" called "Applications". Those are ArgoCD applications that Fleetshard-Sync will manage.

  • Adds validation for the "applications" field

  • It removes the rhacs_operators field from the Gitops configuration

  • It removes the fleetshard_sync_operator_health_status_images metric, which is not used anywhere.

  • SOPs will be modified once this is merged

  • GitOps config to be updated

Checklist (Definition of Done)

  • Unit and integration tests added
  • Added test description under Test manual
  • Documentation added if necessary (i.e. changes to dev setup, test execution, ...)
  • CI and all relevant tests are passing
  • Add the ticket number to the PR title if available, i.e. ROX-12345: ...
  • Discussed security and business related topics privately. Will move any security and business related topics that arise to private communication channel.
  • Add secret to app-interface Vault or Secrets Manager if necessary
  • RDS changes were e2e tested manually
  • Check AWS limits are reasonable for changes provisioning new resources
  • (If applicable) Changes to the dp-terraform Helm values have been reflected in the addon on integration environment

Test manual

  • Deployed an OSD cluster
  • Deployed Fleet* using make deploy/bootstrap deploy/dev
  • Observed that the applications defined in the GitOps configuration are installed successfully.
  • Observed that the SecuredCluster and Central CRDs are installed successfully.

securedClusterReconcilerEnabled: false
verticalPodAutoscaling:
recommenders: []
applications:
Copy link
Contributor

@kovayur kovayur Feb 12, 2025

Choose a reason for hiding this comment

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

Why should we define apps in fleet-manager?
How about adding these apps to the bootstrap app?

We can target the HEAD revision and have kustomizations/helm values for each cluster if we want changes to the gitops config to take effect immediately.
UPD: But most likely we don't want that for operators, do we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah having all manifests in the acscs-manifests would be the end-goal, but it was a bit too radical for a single PR lol. Also, we'll need to remove the e2e-upgrade-tests suite altogether when we do

@@ -0,0 +1,163 @@
package argox
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it like argo + rox ? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

lol. I meant argo + extensions

Copy link
Contributor

@kovayur kovayur left a comment

Choose a reason for hiding this comment

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

Label selector must be changed, otherwise it doesn't match the default rollout group

}
if want.Labels == nil {
want.Labels = map[string]string{}
if err := argox.ReconcileApplication(ctx, r.client, want); 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.

nice

@kurlov
Copy link
Member

kurlov commented Feb 12, 2025

/retest

@openshift-ci openshift-ci bot added the lgtm label Feb 12, 2025
Copy link
Member

@kurlov kurlov left a comment

Choose a reason for hiding this comment

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

Looks great 🎉

Copy link
Collaborator

@ebensh ebensh left a comment

Choose a reason for hiding this comment

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

Nice improvement!

}

// Ensuring the desired applications have labels matching the existingStateSelector
for k, v := range existingStateSelector {
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 plenty clear as it is but as FYI: https://pkg.go.dev/maps#Copy
IMHO it should really be called "Upsert" or "Update"

Copy link
Contributor

openshift-ci bot commented Feb 19, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ebensh, kovayur, kurlov, ludydoo

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 [ebensh,kovayur,kurlov,ludydoo]

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

@ludydoo ludydoo merged commit 3799dd3 into main Feb 20, 2025
19 checks passed
@ludydoo ludydoo deleted the ROX-27130-operators-on-argocd branch February 20, 2025 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants