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 logic for queue controller (Part 1) #346

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Sovietaced
Copy link
Contributor

@Sovietaced Sovietaced commented Jan 2, 2025

Pull Request Template

Description

This pull request implements logic for the queue controller but does not wire it up to actually work yet. The queue controller reconciles queue custom resources against the Armada server so that queues can be managed by GitOps instead of using armadactl manually.

I wanted to break this pull request out since I think the wiring part will be where most of the discussion and back/forth will be. The next pull request will include code for how to configure the API connection to the Armada API server and how to load that config through helm charts.

As part of this pull request I re arranged some of the common helpers so they could be used by the queue reconciling logic too.

Related to #323

Please select the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code Style Update (formatting, renaming)
  • Refactor (code changes that do not fix a bug or add a feature)
  • Documentation Update
  • Other (please describe):

How Has This Been Tested?

This pull request includes unit tests and has been tested against our production environment.

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review of my code.
  • I have commented on my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • Any dependent changes have been merged and published in downstream modules.

@Sovietaced Sovietaced force-pushed the queue-controller branch 2 times, most recently from ad25360 to 2bac68c Compare January 2, 2025 18:34
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
)

// GetObject will get the object from Kubernetes and return if it is missing or an error.
func GetObject(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this so it could be used by other controller logic

Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
Signed-off-by: Jason Parraga <sovietaced@gmail.com>
setupLog.Error(err, "unable to create controller", "controller", "Queue")
os.Exit(1)
}
//if err = (&corecontrollers.QueueReconciler{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commenting this out for now to avoid any NPEs in this intermediate state where the queue client can be nil.

@Sovietaced Sovietaced marked this pull request as ready for review January 2, 2025 19:19
@Sovietaced Sovietaced changed the title feat: Add logic for queue controller feat: Add logic for queue controller (Part 1) Jan 2, 2025
@dejanzele
Copy link
Member

Given that queues can be updated outside of Operator/CRD, how will we reconcile those changes?

@Sovietaced
Copy link
Contributor Author

Given that queues can be updated outside of Operator/CRD, how will we reconcile those changes?

I am under the impression that someone would want to use the Queue CRs for a GitOps workflow. In such a scenario manually manipulating queues through armadactl would seem like an anti-pattern.

Admittedly I'm not very familiar with building controllers so I'm not sure if reconciliation will happen without triggers from CR updates or not. If not, we might want to have some background trigger to prevent drift between the CR and reality.

@dejanzele
Copy link
Member

@Sovietaced it would be an anti-pattern to update via armadactl or via REST/GRPC API, but nothing prevents users except a convention.

Kubernetes Operators watch and reconcile changes for CRDs, but it does not know anything about queues which are a concept in Armada, so it won't do anything if the queue got updated externally.
For initial version we could even start without it, and afterwards implement a watcher process which would check CRD Queues against the ones in database, and reconcile changes at some time intervals (or a smarter approach, this is just from top of my head)

@dejanzele
Copy link
Member

@Sovietaced, on a successful reconcile, you could return ctrl.Result{RequeueAfter: requeueAfter} and control when you want to run the reconciliation loop again.

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.

2 participants