Introduce cache for lookups of desired resource state, to reduce the amount of allocations in the controller#4568
Open
nikola-jokic wants to merge 28 commits into
Open
Introduce cache for lookups of desired resource state, to reduce the amount of allocations in the controller#4568nikola-jokic wants to merge 28 commits into
nikola-jokic wants to merge 28 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…hemeralRunnerSet and AutoscalingRunnerSet
Contributor
|
Hello! Thank you for your contribution. Please review our contribution guidelines to understand the project's testing and code conventions. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an in-memory caching layer for “desired resource state” construction in the controllers to reduce repeated allocations, and refactors several controller/API surfaces (metrics emission, CRD schema, and proxy/TLS-related fields) to support the updated behavior.
Changes:
- Add
ResourceCacheand wire it intoResourceBuilderto reuse computed desired objects safely (via deep-copy on read). - Rework integrity-hash annotation handling, proxy URL field naming, and several controller structs (embedding
*ResourceBuilder), alongside expanded metrics for EphemeralRunner phases. - Update API types and generated CRDs/charts CRDs (status fields/printcolumns/schema requirements) and rotate TLS test certificates used by controller tests.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| main.go | Instantiates and injects ResourceCache; shifts metrics publishing to EphemeralRunner reconciler. |
| github/actions/testdata/server.key | Rotates TLS test private key material for local test server. |
| github/actions/testdata/server.crt | Rotates TLS test server certificate. |
| github/actions/testdata/rootCA.crt | Rotates TLS test root CA certificate. |
| github/actions/testdata/leaf.key | Rotates TLS test leaf private key. |
| github/actions/testdata/leaf.crt | Rotates TLS test leaf certificate. |
| github/actions/testdata/intermediate.crt | Rotates TLS test intermediate certificate. |
| controllers/actions.github.com/secretresolver/secret_resolver.go | Updates proxy field usage from Url to URL. |
| controllers/actions.github.com/resourcecache.go | Adds the generic ResourceCache implementation keyed by main object + desired object identity + dependency refs. |
| controllers/actions.github.com/resourcecache_test.go | Adds tests/benchmarks validating cache behavior and allocation reduction. |
| controllers/actions.github.com/resourcebuilder.go | Integrates caching into resource construction; refactors integrity-hash annotation handling and metadata merging. |
| controllers/actions.github.com/resourcebuilder_test.go | Updates tests for renamed integrity-hash const and proxy URL field rename. |
| controllers/actions.github.com/metrics/metrics.go | Adds succeeded/outdated runner gauges and changes updates to per-runner phase delta (Add/Sub) model. |
| controllers/actions.github.com/ephemeralrunnerset_controller.go | Removes ERS metrics publishing and trims ERS status updates to phase only. |
| controllers/actions.github.com/ephemeralrunnerset_controller_test.go | Updates tests for new metrics strategy and refactored reconciler embedding; adds phase-metrics validation. |
| controllers/actions.github.com/ephemeralrunner_controller.go | Adds per-runner phase metric publishing, introduces “Succeeded” status transitions, and handles metric bookkeeping on delete. |
| controllers/actions.github.com/ephemeralrunner_controller_test.go | Updates tests for pointer ResourceBuilder embedding and proxy URL field rename. |
| controllers/actions.github.com/constants.go | Adds label key for EphemeralRunnerSet UID propagation. |
| controllers/actions.github.com/autoscalingrunnerset_controller.go | Switches integrity-hash computation and simplifies status updates; changes ERS spec update behavior. |
| controllers/actions.github.com/autoscalingrunnerset_controller_test.go | Updates tests for removed status counters and integrity-hash const rename; removes “wait for running jobs” behavior tests. |
| controllers/actions.github.com/autoscalinglistener_controller.go | Updates integrity-hash annotation comparisons to new const name. |
| controllers/actions.github.com/autoscalinglistener_controller_test.go | Updates tests for pointer ResourceBuilder usage and proxy URL field rename. |
| config/crd/bases/actions.github.com_ephemeralrunnersets.yaml | Updates schema (status fields/requirements) and removes status-related printcolumns. |
| config/crd/bases/actions.github.com_ephemeralrunners.yaml | Updates schema requirements and adds selectable fields for status.phase. |
| config/crd/bases/actions.github.com_autoscalingrunnersets.yaml | Updates schema and removes status counters/printcolumns. |
| config/crd/bases/actions.github.com_autoscalinglisteners.yaml | Updates schema requirements and adds validations (e.g., minimums). |
| charts/gha-runner-scale-set/tests/template_test.go | Updates chart unit tests for proxy field rename (Url → URL). |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml | Syncs chart CRD with base CRD status/schema changes. |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml | Syncs chart CRD with base CRD schema changes. |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml | Syncs chart CRD with base CRD schema changes. |
| charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml | Syncs chart CRD with base CRD schema changes. |
| charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml | Syncs experimental chart CRD with base CRD status/schema changes. |
| charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunners.yaml | Syncs experimental chart CRD with base CRD schema changes. |
| charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml | Syncs experimental chart CRD with base CRD schema changes. |
| charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml | Syncs experimental chart CRD with base CRD schema changes. |
| apis/actions.github.com/v1alpha1/proxy_config_test.go | Updates API tests for proxy field rename (Url → URL). |
| apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go | Changes ERS status shape (reserved replicas/patch ID; phase) and removes old count fields/printcolumns. |
| apis/actions.github.com/v1alpha1/ephemeralrunner_types.go | Adds selectable field marker for .status.phase. |
| apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go | Renames proxy URL field, fixes kubebuilder validation syntax, and removes status counters + hash method. |
| apis/actions.github.com/v1alpha1/autoscalinglistener_types.go | Updates min/max runner validations and changes JSON serialization of maxRunners/minRunners. |
Comments suppressed due to low confidence (1)
controllers/actions.github.com/autoscalingrunnerset_controller.go:298
- When the EphemeralRunnerSet integrity hash changes, the controller now patches the ERS spec immediately without checking whether there are active/pending EphemeralRunners. Previously this was explicitly gated to avoid applying spec changes while jobs were in flight. If that behavior is still required, consider reintroducing the gate by listing EphemeralRunners for this set (e.g., using
LabelKeyEphemeralRunnerSetUIDplus the new.status.phaseselectable field) and only patching once there are no Pending/Running runners.
if ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash] != desired.Annotations[AnnotationKeyIntegrityHash] {
original := ephemeralRunnerSet.DeepCopy()
ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata
ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec
ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
982
to
985
| Labels: labels, | ||
| Annotations: annotations, | ||
| Finalizers: []string{ | ||
| ephemeralRunnerFinalizerName, | ||
| ephemeralRunnerActionsFinalizerName, | ||
| }, | ||
| Finalizers: []string{ephemeralRunnerFinalizerName}, | ||
| }, |
Comment on lines
+48
to
+52
| MaxRunners int `json:"maxRunners"` | ||
|
|
||
| // +kubebuilder:validation:Minimum:=0 | ||
| // +kubebuilder:validation:Minimum=0 | ||
| // +optional | ||
| MinRunners int `json:"minRunners,omitempty"` | ||
| MinRunners int `json:"minRunners"` |
Comment on lines
+43
to
+44
| // ReservedReplicas is the controller's checkpointed scale decision for the current patch. | ||
| // It may be higher than CurrentaReplicas while the listener has not yet patched the desired count after runners complete. |
| reservedReplicas: | ||
| description: |- | ||
| ReservedReplicas is the controller's checkpointed scale decision for the current patch. | ||
| It may be higher than CurrentaReplicas while the listener has not yet patched the desired count after runners complete. |
| reservedReplicas: | ||
| description: |- | ||
| ReservedReplicas is the controller's checkpointed scale decision for the current patch. | ||
| It may be higher than CurrentaReplicas while the listener has not yet patched the desired count after runners complete. |
| reservedReplicas: | ||
| description: |- | ||
| ReservedReplicas is the controller's checkpointed scale decision for the current patch. | ||
| It may be higher than CurrentaReplicas while the listener has not yet patched the desired count after runners complete. |
Comment on lines
+302
to
306
| resourceCache := actionsgithubcom.NewResourceCache() | ||
| rb := &actionsgithubcom.ResourceBuilder{ | ||
| ExcludeLabelPropagationPrefixes: excludeLabelPropagationPrefixes, | ||
| SecretResolver: secretResolver, | ||
| Scheme: mgr.GetScheme(), |
…hemeralRunnerSet and AutoscalingRunnerSet
90658a5 to
10a95f9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Depends on #4557