Skip to content

dnorman3/priority system - #119

Merged
dnorman3 merged 43 commits into
atlassian-labs:masterfrom
dnorman3:dnorman3/priority-system
Aug 25, 2025
Merged

dnorman3/priority system#119
dnorman3 merged 43 commits into
atlassian-labs:masterfrom
dnorman3:dnorman3/priority-system

Conversation

@dnorman3

Copy link
Copy Markdown
Contributor

Summary

Implement priority-based creation in Cyclops. On each run, only the lowest-priority NodeGroups with detected changes get CNRs. When those CNRs complete successfully, the next run creates CNRs for the next priority level.

Motivation

Reduce blast radius by ensuring lower-priority NodeGroups complete before higher ones are started.

How it works

Observer finds changed NodeGroups and drops ones with in‑progress CNRs.
Compute the minimum NodeGroup.Spec.Priority among the remaining; create CNRs only for NodeGroups at that priority.
CNRs reconcile immediately via the existing CNR controller; no separate activation phase.
Subsequent runs repeat the process and naturally move to the next priority once lower levels are done.
If multiple NodeGroups share the same lowest priority, CNRs are created for all of them in that run.

Backward compatibility

If NodeGroup.Spec.Priority is missing or negative, it’s treated as 0.
Priority is no longer stored on the CNR; ordering is decided solely from the NodeGroup when CNRs are created.
No changes required in the CNR controller.

Notes

No grouping needed; a single pass to find the min priority and filter is sufficient.
Existing dropInProgressCNRs prevents overlapping work; dedup beyond that is optional.
Metrics remain unchanged (CNRsCreated per NodeGroup).

@atlassian-cla-bot

Copy link
Copy Markdown

Thank you for your submission! Like many open source projects, we ask that you sign our CLA (Contributor License Agreement) before we can accept your contribution.
If your email is listed below, please ensure that you sign the CLA with the same email address.

The following users still need to sign our CLA:
❌dnorman3

Already signed the CLA? To re-check, try refreshing the page.

@mwhittington21

Copy link
Copy Markdown
Collaborator

I have discussed this with @dnorman3 on Slack, but for this PR there are a few things I would like to see done.

  1. The createCNRs function should be limited to performing the action of creating CNRs for a given set of node groups, and probably should not change in this PR
  2. The filtering logic for which node groups should be considered based on priority should be in a separate function, which filters down the node groups before we pass them to createCNRs
  3. The filtering logic here should work on the filtered node groups, so if there are any in progress at the highest priority then we should not try to create any CNRs for a lower priority

In addition, I think it would be good to add some metrics around total node groups with detected changes, and node groups currently being applied. This will help debug any issues with the priority system, and help ensure that we can track if the higher priority node groups are failing and blocking the lower priority node groups. I can imagine a high-churn environment somewhere where the lower priority node group is constantly blocked by the higher priority one. Metrics will help monitor for that.

Comment thread pkg/apis/atlassian/v1/cyclenoderequest_types.go Outdated
Comment thread pkg/observer/controller.go Outdated
Comment thread pkg/observer/controller.go
Comment thread pkg/generation/cnr.go Outdated
Comment thread pkg/observer/controller.go
Comment thread pkg/observer/controller.go
Comment thread pkg/observer/controller.go
Comment thread pkg/generation/cnr.go Outdated
Comment thread pkg/observer/controller.go Outdated
Comment thread pkg/observer/controller.go Outdated
Comment thread pkg/observer/controller.go Outdated
Comment thread pkg/observer/controller_test.go Outdated
Comment thread pkg/observer/controller_test.go
Comment thread pkg/observer/controller.go Outdated
@mwhittington21

Copy link
Copy Markdown
Collaborator

LGTM just waiting on the output of the lab-based tests

Comment thread pkg/observer/controller.go Outdated
name := generation.GetName(cnr.ObjectMeta)
// selectLowestPriorityNodeGroups returns only the node groups at the lowest priority value
func (c *controller) selectLowestPriorityNodeGroups(changedNodeGroups []*ListedNodeGroups) []*ListedNodeGroups {
if len(changedNodeGroups) == 0 {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is a redundant check because changedNodeGroups will never be 0 here since you already check there are some prior to running this function, and if nil were ever to be returned here anyway that would currently crash observer because we check the first element of a nil list right after the call of this function.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes please create and return an empty list if you end up keeping this check.

mwhittington21
mwhittington21 previously approved these changes Aug 21, 2025
mwhittington21
mwhittington21 previously approved these changes Aug 25, 2025

@vincentportella vincentportella left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@dnorman3
dnorman3 merged commit aeedd5f into atlassian-labs:master Aug 25, 2025
3 checks passed
@dnorman3
dnorman3 deleted the dnorman3/priority-system branch August 25, 2025 23:42
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.

4 participants