From 191fac9eaa8d0fa04c36abc452bc16573785e147 Mon Sep 17 00:00:00 2001 From: Nikola Jokic Date: Tue, 7 Jul 2026 16:14:21 +0200 Subject: [PATCH] Improve performance of controllers --- Makefile | 8 +- .../v1alpha1/autoscalinglistener_types.go | 38 +- .../v1alpha1/autoscalingrunnerset_types.go | 85 +- .../v1alpha1/ephemeralrunner_types.go | 17 +- .../v1alpha1/ephemeralrunnerset_types.go | 31 +- .../v1alpha1/proxy_config_test.go | 8 +- .../templates/manager_role_secrets.yaml | 1 + ...tions.github.com_autoscalinglisteners.yaml | 30 +- ...ions.github.com_autoscalingrunnersets.yaml | 44 +- .../actions.github.com_ephemeralrunners.yaml | 25 +- ...ctions.github.com_ephemeralrunnersets.yaml | 47 +- .../templates/manager_cluster_role.yaml | 1 + .../manager_single_namespace_watch_role.yaml | 1 + ...tions.github.com_autoscalinglisteners.yaml | 30 +- ...ions.github.com_autoscalingrunnersets.yaml | 44 +- .../actions.github.com_ephemeralrunners.yaml | 25 +- ...ctions.github.com_ephemeralrunnersets.yaml | 47 +- .../templates/manager_cluster_role.yaml | 1 + .../manager_single_namespace_watch_role.yaml | 1 + .../templates/_mode_kubernetes.tpl | 5 +- .../templates/manager_role.yaml | 3 + .../templates/manager_role.yaml | 3 + .../tests/template_test.go | 4 +- cmd/ghalistener/scaler/scaler.go | 14 +- cmd/ghalistener/scaler/scaler_test.go | 18 +- ...tions.github.com_autoscalinglisteners.yaml | 30 +- ...ions.github.com_autoscalingrunnersets.yaml | 44 +- .../actions.github.com_ephemeralrunners.yaml | 25 +- ...ctions.github.com_ephemeralrunnersets.yaml | 47 +- config/rbac/role.yaml | 29 +- .../autoscalinglistener_controller.go | 402 ++++++--- .../autoscalinglistener_controller_test.go | 20 +- .../autoscalingrunnerset_controller.go | 175 ++-- .../autoscalingrunnerset_controller_test.go | 275 ++++-- controllers/actions.github.com/constants.go | 1 + .../ephemeralrunner_controller.go | 441 ++++++---- .../ephemeralrunner_controller_test.go | 308 +++++-- .../ephemeralrunner_controller_unit_test.go | 139 +++ .../ephemeralrunnerset_controller.go | 815 +++++++++++++----- ...phemeralrunnerset_controller_cache_test.go | 201 +++++ .../ephemeralrunnerset_controller_test.go | 95 +- .../actions.github.com/helpers_test.go | 4 +- .../multiclient/fake/client.go | 11 + .../actions.github.com/resourcebuilder.go | 254 ++++-- .../resourcebuilder_test.go | 12 +- .../actions.github.com/resourcecache.go | 230 +++++ .../actions.github.com/resourcecache_test.go | 312 +++++++ .../secretresolver/secret_resolver.go | 8 +- controllers/actions.github.com/utils.go | 10 + main.go | 4 +- 50 files changed, 3358 insertions(+), 1065 deletions(-) create mode 100644 controllers/actions.github.com/ephemeralrunner_controller_unit_test.go create mode 100644 controllers/actions.github.com/ephemeralrunnerset_controller_cache_test.go create mode 100644 controllers/actions.github.com/resourcecache.go create mode 100644 controllers/actions.github.com/resourcecache_test.go diff --git a/Makefile b/Makefile index c52fefd2..74e6f84f 100644 --- a/Makefile +++ b/Makefile @@ -51,7 +51,8 @@ endif ifeq (${IMG_RESULT}, load) export PUSH_ARG="--load" # if load is specified, image will be built only for the build machine architecture. - export PLATFORMS="local" + # export PLATFORMS="local" + export PLATFORMS="linux/amd64" else ifeq (${IMG_RESULT}, cache) # if cache is specified, image will only be available in the build cache, it won't be pushed or loaded # therefore no PUSH_ARG will be specified @@ -78,7 +79,6 @@ test-with-deps: setup-envtest KUBEBUILDER_ASSETS="$$($(ENVTEST) use $(ENVTEST_K8S_VERSION) --bin-dir $(GOBIN) -p path)" \ make test - # Build manager binary manager: generate fmt vet go build -o bin/manager main.go @@ -365,7 +365,6 @@ SHELLCHECK=$(TOOLS_PATH)/shellcheck # find or download envtest envtest: -ifeq (, $(shell which setup-envtest)) ifeq (, $(wildcard $(GOBIN)/setup-envtest)) @{ \ set -e ;\ @@ -377,9 +376,6 @@ ifeq (, $(wildcard $(GOBIN)/setup-envtest)) } endif ENVTEST=$(GOBIN)/setup-envtest -else -ENVTEST=$(shell which setup-envtest) -endif .PHONY: setup-envtest setup-envtest: envtest diff --git a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go index 1ae599f1..990a6196 100644 --- a/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalinglistener_types.go @@ -24,36 +24,37 @@ import ( // AutoscalingListenerSpec defines the desired state of AutoscalingListener type AutoscalingListenerSpec struct { - // Required + // +optional GitHubConfigURL string `json:"githubConfigUrl,omitempty"` - // Required + // +optional GitHubConfigSecret string `json:"githubConfigSecret,omitempty"` - // Required + // +optional + // +kubebuilder:validation:Minimum=1 RunnerScaleSetID int `json:"runnerScaleSetId,omitempty"` - // Required + // +optional AutoscalingRunnerSetNamespace string `json:"autoscalingRunnerSetNamespace,omitempty"` - // Required + // +optional AutoscalingRunnerSetName string `json:"autoscalingRunnerSetName,omitempty"` - // Required + // +optional EphemeralRunnerSetName string `json:"ephemeralRunnerSetName,omitempty"` - // Required - // +kubebuilder:validation:Minimum:=0 - MaxRunners int `json:"maxRunners,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +optional + MaxRunners int `json:"maxRunners"` - // Required - // +kubebuilder:validation:Minimum:=0 - MinRunners int `json:"minRunners,omitempty"` + // +kubebuilder:validation:Minimum=0 + // +optional + MinRunners int `json:"minRunners"` - // Required + // +optional Image string `json:"image,omitempty"` - // Required + // +optional ImagePullSecrets []corev1.LocalObjectReference `json:"imagePullSecrets,omitempty"` // +optional @@ -99,17 +100,22 @@ type AutoscalingListenerStatus struct{} // AutoscalingListener is the Schema for the autoscalinglisteners API type AutoscalingListener struct { - metav1.TypeMeta `json:",inline"` + metav1.TypeMeta `json:",inline"` + // +optional metav1.ObjectMeta `json:"metadata,omitempty"` - Spec AutoscalingListenerSpec `json:"spec,omitempty"` + // +optional + Spec AutoscalingListenerSpec `json:"spec,omitempty"` + // +optional Status AutoscalingListenerStatus `json:"status,omitempty"` } +// AutoscalingListenerList is a list of AutoscalingListener resources // +kubebuilder:object:root=true // AutoscalingListenerList contains a list of AutoscalingListener type AutoscalingListenerList struct { metav1.TypeMeta `json:",inline"` + // +optional metav1.ListMeta `json:"metadata,omitempty"` Items []AutoscalingListener `json:"items"` } diff --git a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go index 4646384d..06aac809 100644 --- a/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/autoscalingrunnerset_types.go @@ -22,6 +22,7 @@ import ( "net/http" "net/url" "strings" + "sync" "github.com/actions/actions-runner-controller/hash" "github.com/actions/actions-runner-controller/vault" @@ -37,22 +38,28 @@ import ( // +kubebuilder:printcolumn:JSONPath=".spec.minRunners",name=Minimum Runners,type=integer // +kubebuilder:printcolumn:JSONPath=".spec.maxRunners",name=Maximum Runners,type=integer // +kubebuilder:printcolumn:JSONPath=".status.phase",name=Phase,type=string +// +kubebuilder:printcolumn:JSONPath=".status.pendingEphemeralRunners",name=Pending Runners,type=integer +// +kubebuilder:printcolumn:JSONPath=".status.runningEphemeralRunners",name=Running Runners,type=integer +// +kubebuilder:printcolumn:JSONPath=".status.failedEphemeralRunners",name=Failed Runners,type=integer // AutoscalingRunnerSet is the Schema for the autoscalingrunnersets API type AutoscalingRunnerSet struct { - metav1.TypeMeta `json:",inline"` + metav1.TypeMeta `json:",inline"` + // +optional metav1.ObjectMeta `json:"metadata,omitempty"` - Spec AutoscalingRunnerSetSpec `json:"spec,omitempty"` + // +optional + Spec AutoscalingRunnerSetSpec `json:"spec,omitempty"` + // +optional Status AutoscalingRunnerSetStatus `json:"status,omitempty"` } // AutoscalingRunnerSetSpec defines the desired state of AutoscalingRunnerSet type AutoscalingRunnerSetSpec struct { - // Required + // +optional GitHubConfigUrl string `json:"githubConfigUrl,omitempty"` - // Required + // +optional GitHubConfigSecret string `json:"githubConfigSecret,omitempty"` // +optional @@ -73,7 +80,7 @@ type AutoscalingRunnerSetSpec struct { // +optional VaultConfig *VaultConfig `json:"vaultConfig,omitempty"` - // Required + // +optional Template corev1.PodTemplateSpec `json:"template,omitempty"` // +optional @@ -107,16 +114,16 @@ type AutoscalingRunnerSetSpec struct { EphemeralRunnerConfigSecretMetadata *ResourceMeta `json:"ephemeralRunnerConfigSecretMetadata,omitempty"` // +optional - // +kubebuilder:validation:Minimum:=0 + // +kubebuilder:validation:Minimum=0 MaxRunners *int `json:"maxRunners,omitempty"` // +optional - // +kubebuilder:validation:Minimum:=0 + // +kubebuilder:validation:Minimum=0 MinRunners *int `json:"minRunners,omitempty"` } type TLSConfig struct { - // Required + // +required CertificateFrom *TLSCertificateSource `json:"certificateFrom,omitempty"` } @@ -153,7 +160,7 @@ func (c *TLSConfig) ToCertPool(keyFetcher func(name, key string) ([]byte, error) } type TLSCertificateSource struct { - // Required + // +required ConfigMapKeyRef *corev1.ConfigMapKeySelector `json:"configMapKeyRef,omitempty"` } @@ -168,15 +175,33 @@ type ProxyConfig struct { NoProxy []string `json:"noProxy,omitempty"` } +var parsedProxyURLCache sync.Map + +func parseProxyURLCached(rawURL string) (url.URL, error) { + if cached, ok := parsedProxyURLCache.Load(rawURL); ok { + if parsed, ok := cached.(url.URL); ok { + return parsed, nil + } + } + + parsed, err := url.Parse(rawURL) + if err != nil { + return url.URL{}, err + } + + parsedProxyURLCache.Store(rawURL, *parsed) + return *parsed, nil +} + func (c *ProxyConfig) ToHTTPProxyConfig(secretFetcher func(string) (*corev1.Secret, error)) (*httpproxy.Config, error) { config := &httpproxy.Config{ NoProxy: strings.Join(c.NoProxy, ","), } if c.HTTP != nil { - u, err := url.Parse(c.HTTP.Url) + u, err := parseProxyURLCached(c.HTTP.URL) if err != nil { - return nil, fmt.Errorf("failed to parse proxy http url %q: %w", c.HTTP.Url, err) + return nil, fmt.Errorf("failed to parse proxy http url %q: %w", c.HTTP.URL, err) } if c.HTTP.CredentialSecretRef != "" { @@ -199,9 +224,9 @@ func (c *ProxyConfig) ToHTTPProxyConfig(secretFetcher func(string) (*corev1.Secr } if c.HTTPS != nil { - u, err := url.Parse(c.HTTPS.Url) + u, err := parseProxyURLCached(c.HTTPS.URL) if err != nil { - return nil, fmt.Errorf("failed to parse proxy https url %q: %w", c.HTTPS.Url, err) + return nil, fmt.Errorf("failed to parse proxy https url %q: %w", c.HTTPS.URL, err) } if c.HTTPS.CredentialSecretRef != "" { @@ -254,8 +279,8 @@ func (c *ProxyConfig) ProxyFunc(secretFetcher func(string) (*corev1.Secret, erro } type ProxyServerConfig struct { - // Required - Url string `json:"url,omitempty"` + // +required + URL string `json:"url,omitempty"` // +optional CredentialSecretRef string `json:"credentialSecretRef,omitempty"` @@ -309,8 +334,24 @@ type HistogramMetric struct { // AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet type AutoscalingRunnerSetStatus struct { + // +optional + // +kubebuilder:validation:Minimum=0 + CurrentRunners int `json:"currentRunners"` + // +optional Phase AutoscalingRunnerSetPhase `json:"phase"` + + // EphemeralRunner counts separated by the stage ephemeral runners are in, taken from the EphemeralRunnerSet + + // +optional + // +kubebuilder:validation:Minimum=0 + PendingEphemeralRunners int `json:"pendingEphemeralRunners"` + // +optional + // +kubebuilder:validation:Minimum=0 + RunningEphemeralRunners int `json:"runningEphemeralRunners"` + // +optional + // +kubebuilder:validation:Minimum=0 + FailedEphemeralRunners int `json:"failedEphemeralRunners"` } type AutoscalingRunnerSetPhase string @@ -323,20 +364,6 @@ const ( AutoscalingRunnerSetPhaseOutdated AutoscalingRunnerSetPhase = "Outdated" ) -func (ars *AutoscalingRunnerSet) Hash() string { - type data struct { - Spec *AutoscalingRunnerSetSpec - Labels map[string]string - } - - d := &data{ - Spec: ars.Spec.DeepCopy(), - Labels: ars.Labels, - } - - return hash.ComputeTemplateHash(d) -} - func (ars *AutoscalingRunnerSet) ListenerSpecHash() string { arsSpec := ars.Spec.DeepCopy() spec := arsSpec diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go index 744afab5..7ad937fc 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunner_types.go @@ -28,6 +28,7 @@ const EphemeralRunnerContainerName = "runner" // +kubebuilder:object:root=true // +kubebuilder:subresource:status +// +kubebuilder:selectablefield:JSONPath=.status.phase // +kubebuilder:printcolumn:JSONPath=".spec.githubConfigUrl",name="GitHub Config URL",type=string // +kubebuilder:printcolumn:JSONPath=".status.runnerId",name=RunnerId,type=number // +kubebuilder:printcolumn:JSONPath=".status.phase",name=Phase,type=string @@ -41,10 +42,13 @@ const EphemeralRunnerContainerName = "runner" // EphemeralRunner is the Schema for the ephemeralrunners API type EphemeralRunner struct { - metav1.TypeMeta `json:",inline"` + metav1.TypeMeta `json:",inline"` + // +optional metav1.ObjectMeta `json:"metadata,omitempty"` - Spec EphemeralRunnerSpec `json:"spec,omitempty"` + // +optional + Spec EphemeralRunnerSpec `json:"spec,omitempty"` + // +optional Status EphemeralRunnerStatus `json:"status,omitempty"` } @@ -102,17 +106,17 @@ func (er *EphemeralRunner) VaultProxy() *ProxyConfig { // EphemeralRunnerSpec defines the desired state of EphemeralRunner type EphemeralRunnerSpec struct { - // +required + // +optional GitHubConfigURL string `json:"githubConfigUrl,omitempty"` - // +required + // +optional GitHubConfigSecret string `json:"githubConfigSecret,omitempty"` // +optional GitHubServerTLS *TLSConfig `json:"githubServerTLS,omitempty"` - // +required - RunnerScaleSetID int `json:"runnerScaleSetId,omitempty"` + // +optional + RunnerScaleSetID int `json:"runnerScaleSetId"` // +optional Proxy *ProxyConfig `json:"proxy,omitempty"` @@ -126,6 +130,7 @@ type EphemeralRunnerSpec struct { // +optional EphemeralRunnerConfigSecretMetadata *ResourceMeta `json:"ephemeralRunnerConfigSecretMetadata,omitempty"` + // +optional corev1.PodTemplateSpec `json:",inline"` } diff --git a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go index 6b0051aa..4fd0b09b 100644 --- a/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go +++ b/apis/actions.github.com/v1alpha1/ephemeralrunnerset_types.go @@ -23,10 +23,13 @@ import ( // EphemeralRunnerSetSpec defines the desired state of EphemeralRunnerSet type EphemeralRunnerSetSpec struct { // Replicas is the number of desired EphemeralRunner resources in the k8s namespace. + // +optional Replicas int `json:"replicas,omitempty"` // PatchID is the unique identifier for the patch issued by the listener app + // +optional PatchID int `json:"patchID"` // EphemeralRunnerSpec is the spec of the ephemeral runner + // +optional EphemeralRunnerSpec EphemeralRunnerSpec `json:"ephemeralRunnerSpec,omitempty"` // EphemeralRunnerMetadata is the metadata to be applied to all ephemeral runners created by this set. // If the EphemeralRunnerMetadata is updated, the update applies to new ephemeral runners created after the update, @@ -37,6 +40,27 @@ type EphemeralRunnerSetSpec struct { // EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet type EphemeralRunnerSetStatus struct { + // CurrentReplicas is the number of currently running EphemeralRunner resources being managed by this EphemeralRunnerSet. + // +kubebuilder:validation:Minimum=0 + // +optional + CurrentReplicas int `json:"currentReplicas"` + // +optional + // +kubebuilder:validation:Minimum=0 + PendingEphemeralRunners int `json:"pendingEphemeralRunners"` + // +optional + // +kubebuilder:validation:Minimum=0 + RunningEphemeralRunners int `json:"runningEphemeralRunners"` + // +optional + // +kubebuilder:validation:Minimum=0 + FailedEphemeralRunners int `json:"failedEphemeralRunners"` + // 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. + // +optional + // +kubebuilder:validation:Minimum=0 + ReservedReplicas int `json:"reservedReplicas,omitempty"` + // ReservedPatchID is the patch ID associated with ReservedReplicas. + // +optional + ReservedPatchID int `json:"reservedPatchID,omitempty"` // +optional Phase EphemeralRunnerSetPhase `json:"phase"` } @@ -58,10 +82,13 @@ const ( // EphemeralRunnerSet is the Schema for the ephemeralrunnersets API type EphemeralRunnerSet struct { - metav1.TypeMeta `json:",inline"` + metav1.TypeMeta `json:",inline"` + // +optional metav1.ObjectMeta `json:"metadata,omitempty"` - Spec EphemeralRunnerSetSpec `json:"spec,omitempty"` + // +optional + Spec EphemeralRunnerSetSpec `json:"spec,omitempty"` + // +optional Status EphemeralRunnerSetStatus `json:"status,omitempty"` } diff --git a/apis/actions.github.com/v1alpha1/proxy_config_test.go b/apis/actions.github.com/v1alpha1/proxy_config_test.go index 9291cde4..0357e5e2 100644 --- a/apis/actions.github.com/v1alpha1/proxy_config_test.go +++ b/apis/actions.github.com/v1alpha1/proxy_config_test.go @@ -14,11 +14,11 @@ import ( func TestProxyConfig_ToSecret(t *testing.T) { config := &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://proxy.example.com:8080", + URL: "http://proxy.example.com:8080", CredentialSecretRef: "my-secret", }, HTTPS: &v1alpha1.ProxyServerConfig{ - Url: "https://proxy.example.com:8080", + URL: "https://proxy.example.com:8080", CredentialSecretRef: "my-secret", }, NoProxy: []string{ @@ -48,11 +48,11 @@ func TestProxyConfig_ToSecret(t *testing.T) { func TestProxyConfig_ProxyFunc(t *testing.T) { config := &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://proxy.example.com:8080", + URL: "http://proxy.example.com:8080", CredentialSecretRef: "my-secret", }, HTTPS: &v1alpha1.ProxyServerConfig{ - Url: "https://proxy.example.com:8080", + URL: "https://proxy.example.com:8080", CredentialSecretRef: "my-secret", }, NoProxy: []string{ diff --git a/charts/actions-runner-controller/templates/manager_role_secrets.yaml b/charts/actions-runner-controller/templates/manager_role_secrets.yaml index 38037c83..4688f3cd 100644 --- a/charts/actions-runner-controller/templates/manager_role_secrets.yaml +++ b/charts/actions-runner-controller/templates/manager_role_secrets.yaml @@ -21,4 +21,5 @@ rules: {{/* See https://github.com/actions/actions-runner-controller/pull/1268/files#r917331632 */}} - create - delete + - deletecollection {{- end }} \ No newline at end of file diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml index 20e57e33..84e243f9 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalinglisteners.yaml @@ -51,10 +51,8 @@ spec: description: AutoscalingListenerSpec defines the desired state of AutoscalingListener properties: autoscalingRunnerSetName: - description: Required type: string autoscalingRunnerSetNamespace: - description: Required type: string configSecretMetadata: description: ResourceMeta carries metadata common to all internal @@ -70,21 +68,17 @@ spec: type: object type: object ephemeralRunnerSetName: - description: Required type: string githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -106,13 +100,15 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object image: - description: Required type: string imagePullSecrets: - description: Required items: description: |- LocalObjectReference contains enough information to let you locate the @@ -131,7 +127,6 @@ spec: x-kubernetes-map-type: atomic type: array maxRunners: - description: Required minimum: 0 type: integer metrics: @@ -183,7 +178,6 @@ spec: type: object type: object minRunners: - description: Required minimum: 0 type: integer proxy: @@ -193,16 +187,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -236,7 +232,7 @@ spec: type: object type: object runnerScaleSetId: - description: Required + minimum: 1 type: integer serviceAccountMetadata: description: ResourceMeta carries metadata common to all internal @@ -8782,16 +8778,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml index 1f4b63f3..0738087d 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_autoscalingrunnersets.yaml @@ -24,6 +24,15 @@ spec: - jsonPath: .status.phase name: Phase type: string + - jsonPath: .status.pendingEphemeralRunners + name: Pending Runners + type: integer + - jsonPath: .status.runningEphemeralRunners + name: Running Runners + type: integer + - jsonPath: .status.failedEphemeralRunners + name: Failed Runners + type: integer name: v1alpha1 schema: openAPIV3Schema: @@ -98,18 +107,15 @@ spec: type: object type: object githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -130,7 +136,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object listenerConfigSecretMetadata: description: ResourceMeta carries metadata common to all internal resources @@ -8351,16 +8361,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8376,7 +8388,7 @@ spec: runnerScaleSetName: type: string template: - description: Required + description: PodTemplateSpec describes the data a pod should have when created from a template properties: metadata: description: |- @@ -16502,16 +16514,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -16528,8 +16542,20 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + currentRunners: + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: type: string + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunners.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunners.yaml index 3cd90148..2a4fad2c 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunners.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunners.yaml @@ -89,10 +89,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -113,7 +112,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -144,16 +147,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8268,16 +8273,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8290,10 +8297,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object status: description: EphemeralRunnerStatus defines the observed state of EphemeralRunner @@ -8342,6 +8345,8 @@ spec: type: integer type: object type: object + selectableFields: + - jsonPath: .status.phase served: true storage: true subresources: diff --git a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml index fa706e3b..d85630d3 100644 --- a/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/crds/actions.github.com_ephemeralrunnersets.yaml @@ -83,10 +83,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -107,7 +106,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -138,16 +141,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8262,16 +8267,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8284,10 +8291,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object patchID: description: PatchID is the unique identifier for the patch issued by the listener app @@ -8295,15 +8298,35 @@ spec: replicas: description: Replicas is the number of desired EphemeralRunner resources in the k8s namespace. type: integer - required: - - patchID type: object status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + currentReplicas: + description: CurrentReplicas is the number of currently running EphemeralRunner resources being managed by this EphemeralRunnerSet. + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string + reservedPatchID: + description: ReservedPatchID is the patch ID associated with ReservedReplicas. + type: integer + 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. + minimum: 0 + type: integer + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/charts/gha-runner-scale-set-controller-experimental/templates/manager_cluster_role.yaml b/charts/gha-runner-scale-set-controller-experimental/templates/manager_cluster_role.yaml index df19824f..39723419 100644 --- a/charts/gha-runner-scale-set-controller-experimental/templates/manager_cluster_role.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/templates/manager_cluster_role.yaml @@ -92,6 +92,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set-controller-experimental/templates/manager_single_namespace_watch_role.yaml b/charts/gha-runner-scale-set-controller-experimental/templates/manager_single_namespace_watch_role.yaml index 59acd953..fde1bf77 100644 --- a/charts/gha-runner-scale-set-controller-experimental/templates/manager_single_namespace_watch_role.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/templates/manager_single_namespace_watch_role.yaml @@ -66,6 +66,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml index 20e57e33..84e243f9 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalinglisteners.yaml @@ -51,10 +51,8 @@ spec: description: AutoscalingListenerSpec defines the desired state of AutoscalingListener properties: autoscalingRunnerSetName: - description: Required type: string autoscalingRunnerSetNamespace: - description: Required type: string configSecretMetadata: description: ResourceMeta carries metadata common to all internal @@ -70,21 +68,17 @@ spec: type: object type: object ephemeralRunnerSetName: - description: Required type: string githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -106,13 +100,15 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object image: - description: Required type: string imagePullSecrets: - description: Required items: description: |- LocalObjectReference contains enough information to let you locate the @@ -131,7 +127,6 @@ spec: x-kubernetes-map-type: atomic type: array maxRunners: - description: Required minimum: 0 type: integer metrics: @@ -183,7 +178,6 @@ spec: type: object type: object minRunners: - description: Required minimum: 0 type: integer proxy: @@ -193,16 +187,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -236,7 +232,7 @@ spec: type: object type: object runnerScaleSetId: - description: Required + minimum: 1 type: integer serviceAccountMetadata: description: ResourceMeta carries metadata common to all internal @@ -8782,16 +8778,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml index 1f4b63f3..0738087d 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_autoscalingrunnersets.yaml @@ -24,6 +24,15 @@ spec: - jsonPath: .status.phase name: Phase type: string + - jsonPath: .status.pendingEphemeralRunners + name: Pending Runners + type: integer + - jsonPath: .status.runningEphemeralRunners + name: Running Runners + type: integer + - jsonPath: .status.failedEphemeralRunners + name: Failed Runners + type: integer name: v1alpha1 schema: openAPIV3Schema: @@ -98,18 +107,15 @@ spec: type: object type: object githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -130,7 +136,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object listenerConfigSecretMetadata: description: ResourceMeta carries metadata common to all internal resources @@ -8351,16 +8361,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8376,7 +8388,7 @@ spec: runnerScaleSetName: type: string template: - description: Required + description: PodTemplateSpec describes the data a pod should have when created from a template properties: metadata: description: |- @@ -16502,16 +16514,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -16528,8 +16542,20 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + currentRunners: + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: type: string + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml index 3cd90148..2a4fad2c 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunners.yaml @@ -89,10 +89,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -113,7 +112,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -144,16 +147,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8268,16 +8273,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8290,10 +8297,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object status: description: EphemeralRunnerStatus defines the observed state of EphemeralRunner @@ -8342,6 +8345,8 @@ spec: type: integer type: object type: object + selectableFields: + - jsonPath: .status.phase served: true storage: true subresources: diff --git a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml index fa706e3b..d85630d3 100644 --- a/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml +++ b/charts/gha-runner-scale-set-controller/crds/actions.github.com_ephemeralrunnersets.yaml @@ -83,10 +83,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -107,7 +106,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -138,16 +141,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8262,16 +8267,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8284,10 +8291,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object patchID: description: PatchID is the unique identifier for the patch issued by the listener app @@ -8295,15 +8298,35 @@ spec: replicas: description: Replicas is the number of desired EphemeralRunner resources in the k8s namespace. type: integer - required: - - patchID type: object status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + currentReplicas: + description: CurrentReplicas is the number of currently running EphemeralRunner resources being managed by this EphemeralRunnerSet. + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string + reservedPatchID: + description: ReservedPatchID is the patch ID associated with ReservedReplicas. + type: integer + 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. + minimum: 0 + type: integer + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/charts/gha-runner-scale-set-controller/templates/manager_cluster_role.yaml b/charts/gha-runner-scale-set-controller/templates/manager_cluster_role.yaml index cc58e3c2..5ad9febe 100644 --- a/charts/gha-runner-scale-set-controller/templates/manager_cluster_role.yaml +++ b/charts/gha-runner-scale-set-controller/templates/manager_cluster_role.yaml @@ -92,6 +92,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set-controller/templates/manager_single_namespace_watch_role.yaml b/charts/gha-runner-scale-set-controller/templates/manager_single_namespace_watch_role.yaml index ac5a2d93..47829dae 100644 --- a/charts/gha-runner-scale-set-controller/templates/manager_single_namespace_watch_role.yaml +++ b/charts/gha-runner-scale-set-controller/templates/manager_single_namespace_watch_role.yaml @@ -66,6 +66,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set-experimental/templates/_mode_kubernetes.tpl b/charts/gha-runner-scale-set-experimental/templates/_mode_kubernetes.tpl index 6589d01d..672c8495 100644 --- a/charts/gha-runner-scale-set-experimental/templates/_mode_kubernetes.tpl +++ b/charts/gha-runner-scale-set-experimental/templates/_mode_kubernetes.tpl @@ -82,7 +82,10 @@ volumeMounts: subPath: extension readOnly: true {{- end }} - {{ include "githubServerTLS.volumeMountItem" (dict "root" $ "existingVolumeMounts" (list)) | nindent 2 }} + {{- with .Values.runner.container.volumeMounts }} + {{- toYaml . | nindent 2 }} + {{- end }} + {{ include "githubServerTLS.volumeMountItem" (dict "root" $ "existingVolumeMounts" (.Values.runner.container.volumeMounts | default (list))) | nindent 2 }} {{- end }} {{- define "runner-mode-kubernetes.pod-volumes" -}} diff --git a/charts/gha-runner-scale-set-experimental/templates/manager_role.yaml b/charts/gha-runner-scale-set-experimental/templates/manager_role.yaml index 2990ccc4..c9540680 100644 --- a/charts/gha-runner-scale-set-experimental/templates/manager_role.yaml +++ b/charts/gha-runner-scale-set-experimental/templates/manager_role.yaml @@ -17,6 +17,8 @@ rules: verbs: - create - delete + - update + - patch - get - apiGroups: - "" @@ -31,6 +33,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set/templates/manager_role.yaml b/charts/gha-runner-scale-set/templates/manager_role.yaml index bbf92799..de68ee2d 100644 --- a/charts/gha-runner-scale-set/templates/manager_role.yaml +++ b/charts/gha-runner-scale-set/templates/manager_role.yaml @@ -41,6 +41,8 @@ rules: verbs: - create - delete + - update + - patch - get - apiGroups: - "" @@ -55,6 +57,7 @@ rules: verbs: - create - delete + - deletecollection - get - list - patch diff --git a/charts/gha-runner-scale-set/tests/template_test.go b/charts/gha-runner-scale-set/tests/template_test.go index ba67ea90..10a3ddfd 100644 --- a/charts/gha-runner-scale-set/tests/template_test.go +++ b/charts/gha-runner-scale-set/tests/template_test.go @@ -1364,11 +1364,11 @@ func TestTemplateRenderedWithProxy(t *testing.T) { require.NotNil(t, ars.Spec.Proxy) require.NotNil(t, ars.Spec.Proxy.HTTP) - assert.Equal(t, "http://proxy.example.com", ars.Spec.Proxy.HTTP.Url) + assert.Equal(t, "http://proxy.example.com", ars.Spec.Proxy.HTTP.URL) assert.Equal(t, "http-secret", ars.Spec.Proxy.HTTP.CredentialSecretRef) require.NotNil(t, ars.Spec.Proxy.HTTPS) - assert.Equal(t, "https://proxy.example.com", ars.Spec.Proxy.HTTPS.Url) + assert.Equal(t, "https://proxy.example.com", ars.Spec.Proxy.HTTPS.URL) assert.Equal(t, "https-secret", ars.Spec.Proxy.HTTPS.CredentialSecretRef) require.NotNil(t, ars.Spec.Proxy.NoProxy) diff --git a/cmd/ghalistener/scaler/scaler.go b/cmd/ghalistener/scaler/scaler.go index 51cb0362..4a559736 100644 --- a/cmd/ghalistener/scaler/scaler.go +++ b/cmd/ghalistener/scaler/scaler.go @@ -229,22 +229,18 @@ func (w *Scaler) setDesiredWorkerState(count int) int { dirty := w.dirty w.dirty = false - if w.patchSeq == math.MaxInt32 { + if w.patchSeq == math.MaxInt64 { w.patchSeq = 0 } - w.patchSeq++ targetRunnerCount := min(w.config.MinRunners+count, w.config.MaxRunners) oldTargetRunners := w.targetRunners w.targetRunners = targetRunnerCount - desiredPatchID := w.patchSeq - if !dirty && targetRunnerCount == oldTargetRunners && targetRunnerCount == w.config.MinRunners { - // If there were no events sent, and the target runner count - // is the same as the last patched count, we can force the state. - // - // TODO: see to remove w.config.MinRunenrs from the equation, as it is not relevant to the decision of whether to patch or not. - desiredPatchID = 0 + desiredPatchID := w.patchSeq + 1 + if dirty || targetRunnerCount != oldTargetRunners || targetRunnerCount != w.config.MinRunners { + w.patchSeq++ + desiredPatchID = w.patchSeq } w.logger.Info( diff --git a/cmd/ghalistener/scaler/scaler_test.go b/cmd/ghalistener/scaler/scaler_test.go index 7ea3e967..b1a4c855 100644 --- a/cmd/ghalistener/scaler/scaler_test.go +++ b/cmd/ghalistener/scaler/scaler_test.go @@ -129,7 +129,7 @@ func TestSetDesiredWorkerState_MinSet(t *testing.T) { assert.Equal(t, 1, w.patchSeq) }) - t.Run("desired patch is 0 but sequence continues on empty batch and min runners", func(t *testing.T) { + t.Run("desired patch repeats without advancing sequence on empty batch and min runners", func(t *testing.T) { w := newEmptyWorker() patchID := w.setDesiredWorkerState(3) assert.False(t, w.dirty) @@ -147,9 +147,9 @@ func TestSetDesiredWorkerState_MinSet(t *testing.T) { // Empty batch on min runners patchID = w.setDesiredWorkerState(0) assert.False(t, w.dirty) - assert.Equal(t, 0, patchID) // forcing the state + assert.Equal(t, 2, patchID) assert.Equal(t, 1, w.targetRunners) - assert.Equal(t, 2, w.patchSeq) + assert.Equal(t, 1, w.patchSeq) }) } @@ -234,7 +234,7 @@ func TestSetDesiredWorkerState_MaxSet(t *testing.T) { assert.Equal(t, 1, w.patchSeq) }) - t.Run("force 0 on empty batch and last patch == min runners", func(t *testing.T) { + t.Run("desired patch repeats without advancing sequence on empty batch and min runners", func(t *testing.T) { w := newEmptyWorker() patchID := w.setDesiredWorkerState(3) assert.Equal(t, 0, patchID) @@ -249,9 +249,9 @@ func TestSetDesiredWorkerState_MaxSet(t *testing.T) { // Empty batch on min runners patchID = w.setDesiredWorkerState(0) - assert.Equal(t, 0, patchID) // forcing the state + assert.Equal(t, 2, patchID) assert.Equal(t, 0, w.targetRunners) - assert.Equal(t, 2, w.patchSeq) + assert.Equal(t, 1, w.patchSeq) }) } @@ -309,7 +309,7 @@ func TestSetDesiredWorkerState_MinMaxSet(t *testing.T) { assert.Equal(t, 0, w.patchSeq) }) - t.Run("force 0 on empty batch and last patch == min runners", func(t *testing.T) { + t.Run("desired patch repeats without advancing sequence on empty batch and min runners", func(t *testing.T) { w := newEmptyWorker() patchID := w.setDesiredWorkerState(3) assert.False(t, w.dirty) @@ -327,8 +327,8 @@ func TestSetDesiredWorkerState_MinMaxSet(t *testing.T) { // Empty batch on min runners patchID = w.setDesiredWorkerState(0) assert.False(t, w.dirty) - assert.Equal(t, 0, patchID) // forcing the state + assert.Equal(t, 2, patchID) assert.Equal(t, 1, w.targetRunners) - assert.Equal(t, 2, w.patchSeq) + assert.Equal(t, 1, w.patchSeq) }) } diff --git a/config/crd/bases/actions.github.com_autoscalinglisteners.yaml b/config/crd/bases/actions.github.com_autoscalinglisteners.yaml index 20e57e33..84e243f9 100644 --- a/config/crd/bases/actions.github.com_autoscalinglisteners.yaml +++ b/config/crd/bases/actions.github.com_autoscalinglisteners.yaml @@ -51,10 +51,8 @@ spec: description: AutoscalingListenerSpec defines the desired state of AutoscalingListener properties: autoscalingRunnerSetName: - description: Required type: string autoscalingRunnerSetNamespace: - description: Required type: string configSecretMetadata: description: ResourceMeta carries metadata common to all internal @@ -70,21 +68,17 @@ spec: type: object type: object ephemeralRunnerSetName: - description: Required type: string githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -106,13 +100,15 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object image: - description: Required type: string imagePullSecrets: - description: Required items: description: |- LocalObjectReference contains enough information to let you locate the @@ -131,7 +127,6 @@ spec: x-kubernetes-map-type: atomic type: array maxRunners: - description: Required minimum: 0 type: integer metrics: @@ -183,7 +178,6 @@ spec: type: object type: object minRunners: - description: Required minimum: 0 type: integer proxy: @@ -193,16 +187,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -236,7 +232,7 @@ spec: type: object type: object runnerScaleSetId: - description: Required + minimum: 1 type: integer serviceAccountMetadata: description: ResourceMeta carries metadata common to all internal @@ -8782,16 +8778,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: diff --git a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml index 1f4b63f3..0738087d 100644 --- a/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml +++ b/config/crd/bases/actions.github.com_autoscalingrunnersets.yaml @@ -24,6 +24,15 @@ spec: - jsonPath: .status.phase name: Phase type: string + - jsonPath: .status.pendingEphemeralRunners + name: Pending Runners + type: integer + - jsonPath: .status.runningEphemeralRunners + name: Running Runners + type: integer + - jsonPath: .status.failedEphemeralRunners + name: Failed Runners + type: integer name: v1alpha1 schema: openAPIV3Schema: @@ -98,18 +107,15 @@ spec: type: object type: object githubConfigSecret: - description: Required type: string githubConfigUrl: - description: Required type: string githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -130,7 +136,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object listenerConfigSecretMetadata: description: ResourceMeta carries metadata common to all internal resources @@ -8351,16 +8361,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8376,7 +8388,7 @@ spec: runnerScaleSetName: type: string template: - description: Required + description: PodTemplateSpec describes the data a pod should have when created from a template properties: metadata: description: |- @@ -16502,16 +16514,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -16528,8 +16542,20 @@ spec: status: description: AutoscalingRunnerSetStatus defines the observed state of AutoscalingRunnerSet properties: + currentRunners: + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: type: string + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/config/crd/bases/actions.github.com_ephemeralrunners.yaml b/config/crd/bases/actions.github.com_ephemeralrunners.yaml index 3cd90148..2a4fad2c 100644 --- a/config/crd/bases/actions.github.com_ephemeralrunners.yaml +++ b/config/crd/bases/actions.github.com_ephemeralrunners.yaml @@ -89,10 +89,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -113,7 +112,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -144,16 +147,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8268,16 +8273,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8290,10 +8297,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object status: description: EphemeralRunnerStatus defines the observed state of EphemeralRunner @@ -8342,6 +8345,8 @@ spec: type: integer type: object type: object + selectableFields: + - jsonPath: .status.phase served: true storage: true subresources: diff --git a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml index fa706e3b..d85630d3 100644 --- a/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml +++ b/config/crd/bases/actions.github.com_ephemeralrunnersets.yaml @@ -83,10 +83,9 @@ spec: githubServerTLS: properties: certificateFrom: - description: Required properties: configMapKeyRef: - description: Required + description: Selects a key from a ConfigMap. properties: key: description: The key to select. @@ -107,7 +106,11 @@ spec: - key type: object x-kubernetes-map-type: atomic + required: + - configMapKeyRef type: object + required: + - certificateFrom type: object metadata: description: |- @@ -138,16 +141,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8262,16 +8267,18 @@ spec: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object https: properties: credentialSecretRef: type: string url: - description: Required type: string + required: + - url type: object noProxy: items: @@ -8284,10 +8291,6 @@ spec: It is used to identify which vault integration should be used to resolve secrets. type: string type: object - required: - - githubConfigSecret - - githubConfigUrl - - runnerScaleSetId type: object patchID: description: PatchID is the unique identifier for the patch issued by the listener app @@ -8295,15 +8298,35 @@ spec: replicas: description: Replicas is the number of desired EphemeralRunner resources in the k8s namespace. type: integer - required: - - patchID type: object status: description: EphemeralRunnerSetStatus defines the observed state of EphemeralRunnerSet properties: + currentReplicas: + description: CurrentReplicas is the number of currently running EphemeralRunner resources being managed by this EphemeralRunnerSet. + minimum: 0 + type: integer + failedEphemeralRunners: + minimum: 0 + type: integer + pendingEphemeralRunners: + minimum: 0 + type: integer phase: description: EphemeralRunnerSetPhase is the phase of the ephemeral runner set resource type: string + reservedPatchID: + description: ReservedPatchID is the patch ID associated with ReservedReplicas. + type: integer + 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. + minimum: 0 + type: integer + runningEphemeralRunners: + minimum: 0 + type: integer type: object type: object served: true diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index dc0becfd..e752c667 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -46,20 +46,30 @@ rules: - "" resources: - secrets + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - watch +- apiGroups: + - "" + resources: - serviceaccounts verbs: - create - delete - get - list - - update + - patch - watch - apiGroups: - actions.github.com resources: - autoscalinglisteners - autoscalingrunnersets - - ephemeralrunners - ephemeralrunners/finalizers - ephemeralrunnersets verbs: @@ -88,6 +98,19 @@ rules: - get - patch - update +- apiGroups: + - actions.github.com + resources: + - ephemeralrunners + verbs: + - create + - delete + - deletecollection + - get + - list + - patch + - update + - watch - apiGroups: - actions.github.com resources: @@ -167,5 +190,5 @@ rules: - delete - get - list - - update + - patch - watch diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 7d12f895..568248ee 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -17,6 +17,7 @@ limitations under the License. package actionsgithubcom import ( + "bytes" "context" "fmt" "maps" @@ -57,15 +58,15 @@ type AutoscalingListenerReconciler struct { ListenerMetricsAddr string ListenerMetricsEndpoint string - ResourceBuilder + *ResourceBuilder } // +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=pods/status,verbs=get -// +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;update -// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;list;watch;create;update -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=create;delete;get;list;watch;update -// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=create;delete;get;list;watch;update +// +kubebuilder:rbac:groups=core,resources=secrets,verbs=get;list;watch;create;patch +// +kubebuilder:rbac:groups=core,resources=serviceaccounts,verbs=get;list;watch;create;patch +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles,verbs=create;delete;get;list;watch;patch +// +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=rolebindings,verbs=create;delete;get;list;watch;patch // +kubebuilder:rbac:groups=actions.github.com,resources=autoscalinglisteners,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=actions.github.com,resources=autoscalinglisteners/status,verbs=get;update;patch // +kubebuilder:rbac:groups=actions.github.com,resources=autoscalinglisteners/finalizers,verbs=update @@ -78,7 +79,6 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. if err := r.Get(ctx, req.NamespacedName, &autoscalingListener); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingListener.DeepCopy() if !autoscalingListener.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { @@ -97,7 +97,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + if controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + original := autoscalingListener.DeepCopy() + controllerutil.RemoveFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to remove finalizer") return ctrl.Result{}, err @@ -108,7 +110,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + if !controllerutil.ContainsFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) { + original := autoscalingListener.DeepCopy() + controllerutil.AddFinalizer(&autoscalingListener, autoscalingListenerFinalizerName) if err := r.Patch(ctx, &autoscalingListener, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to add finalizer") return ctrl.Result{}, err @@ -147,6 +151,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener service account is created for the listener pod in the controller namespace var serviceAccount corev1.ServiceAccount + var desiredServiceAccount *corev1.ServiceAccount err := r.Get( ctx, types.NamespacedName{ @@ -157,34 +162,11 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: - desiredServiceAccount, err := r.newScaleSetListenerServiceAccount(&autoscalingListener) + desiredServiceAccount, err = r.newScaleSetListenerServiceAccount(&autoscalingListener) if err != nil { log.Error(err, "Failed to build desired listener service account") return ctrl.Result{}, err } - - updatedServiceAccount := serviceAccount.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(serviceAccount.Labels, desiredServiceAccount.Labels) - if !maps.Equal(serviceAccount.Labels, desiredLabels) { - updatedServiceAccount.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(serviceAccount.Annotations, desiredServiceAccount.Annotations) - if !maps.Equal(serviceAccount.Annotations, desiredAnnotations) { - updatedServiceAccount.Annotations = desiredAnnotations - shouldUpdate = true - } - if shouldUpdate { - log.Info("Updating listener service account") - - if err := r.Update(ctx, updatedServiceAccount); err != nil { - log.Error(err, "Failed to update listener service account") - return ctrl.Result{}, err - } - - return ctrl.Result{Requeue: true}, nil - } case kerrors.IsNotFound(err): // Create a service account for the listener pod in the controller namespace log.Info("Creating a service account for the listener pod") @@ -196,6 +178,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener role is created in the AutoscalingRunnerSet namespace var listenerRole rbacv1.Role + var desiredRole *rbacv1.Role err = r.Get( ctx, types.NamespacedName{ @@ -206,26 +189,19 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: - desiredRole := r.newScaleSetListenerRole(&autoscalingListener) - updatedRole := listenerRole.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(listenerRole.Labels, desiredRole.Labels) - if !maps.Equal(listenerRole.Labels, desiredLabels) { - updatedRole.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(listenerRole.Annotations, desiredRole.Annotations) - if !maps.Equal(listenerRole.Annotations, desiredAnnotations) { - updatedRole.Annotations = desiredAnnotations - shouldUpdate = true - } + desiredRole = r.newScaleSetListenerRole(&autoscalingListener) if !reflect.DeepEqual(listenerRole.Rules, desiredRole.Rules) { - updatedRole.Rules = desiredRole.Rules - shouldUpdate = true - } - if shouldUpdate { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(&listenerRole, desiredRole) log.Info("Updating listener role") - if err := r.Update(ctx, updatedRole); err != nil { + updatedRole := listenerRole.DeepCopy() + updatedRole.Rules = desiredRole.Rules + if labelsChanged { + updatedRole.Labels = labels + } + if annotationsChanged { + updatedRole.Annotations = annotations + } + if err := r.Patch(ctx, updatedRole, client.MergeFrom(&listenerRole)); err != nil { log.Error(err, "Failed to update listener role") return ctrl.Result{}, err } @@ -242,36 +218,15 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // Make sure the runner scale set listener role binding is created var listenerRoleBinding rbacv1.RoleBinding + var desiredRoleBinding *rbacv1.RoleBinding err = r.Get(ctx, types.NamespacedName{Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, Name: autoscalingListener.Name}, &listenerRoleBinding) switch { case err == nil: - desiredRoleBinding := r.newScaleSetListenerRoleBinding( + desiredRoleBinding = r.newScaleSetListenerRoleBinding( &autoscalingListener, &listenerRole, &serviceAccount, ) - updatedRoleBinding := listenerRoleBinding.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(listenerRoleBinding.Labels, desiredRoleBinding.Labels) - if !maps.Equal(listenerRoleBinding.Labels, desiredLabels) { - updatedRoleBinding.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(listenerRoleBinding.Annotations, desiredRoleBinding.Annotations) - if !maps.Equal(listenerRoleBinding.Annotations, desiredAnnotations) { - updatedRoleBinding.Annotations = desiredAnnotations - shouldUpdate = true - } - if shouldUpdate { - log.Info("Updating listener role binding") - if err := r.Update(ctx, updatedRoleBinding); err != nil { - log.Error(err, "Failed to update listener role binding") - return ctrl.Result{}, err - } - - log.Info("Updated listener role binding") - return ctrl.Result{Requeue: true}, nil - } case kerrors.IsNotFound(err): // Create a role binding for the listener pod in the AutoScalingRunnerSet namespace @@ -289,38 +244,51 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } // Create a secret containing proxy config if specified + var proxySecret *corev1.Secret + var desiredListenerProxy *corev1.Secret if autoscalingListener.Spec.Proxy != nil { - var proxySecret corev1.Secret + var currentProxySecret corev1.Secret err := r.Get( ctx, types.NamespacedName{ Namespace: autoscalingListener.Namespace, Name: proxyListenerSecretName(&autoscalingListener), }, - &proxySecret, + ¤tProxySecret, ) switch { case err == nil: - desiredListenerProxy, err := r.newAutoscalingListenerProxySecret(&autoscalingListener, proxySecret.Data) + proxySecret = ¤tProxySecret + proxySecretData, err := autoscalingListener.Spec.Proxy.ToSecretData(func(s string) (*corev1.Secret, error) { + var secret corev1.Secret + err := r.Get(ctx, types.NamespacedName{Name: s, Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace}, &secret) + if err != nil { + return nil, fmt.Errorf("failed to get secret %s: %w", s, err) + } + return &secret, nil + }) + if err != nil { + return ctrl.Result{}, fmt.Errorf("failed to convert proxy config to secret data: %w", err) + } + + desiredListenerProxy, err = r.newAutoscalingListenerProxySecret(&autoscalingListener, proxySecretData) if err != nil { log.Error(err, "Failed to build desired listener proxy secret") return ctrl.Result{}, err } - updatedProxySecret := proxySecret.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredListenerProxy.Labels) - if !maps.Equal(proxySecret.Labels, desiredLabels) { - updatedProxySecret.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredListenerProxy.Annotations) - if !maps.Equal(proxySecret.Annotations, desiredAnnotations) { - updatedProxySecret.Annotations = desiredAnnotations - shouldUpdate = true - } - if shouldUpdate { + + if !maps.EqualFunc(proxySecret.Data, proxySecretData, bytes.Equal) { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(proxySecret, desiredListenerProxy) log.Info("Updating listener proxy secret") - if err := r.Update(ctx, updatedProxySecret); err != nil { + updatedProxySecret := proxySecret.DeepCopy() + updatedProxySecret.Data = proxySecretData + if labelsChanged { + updatedProxySecret.Labels = labels + } + if annotationsChanged { + updatedProxySecret.Annotations = annotations + } + if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(proxySecret)); err != nil { log.Error(err, "Failed to update listener proxy secret") return ctrl.Result{}, err } @@ -368,6 +336,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } var listenerConfigSecret corev1.Secret + var desiredConfigSecret *corev1.Secret err = r.Get( ctx, types.NamespacedName{ @@ -389,27 +358,23 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, fmt.Errorf("failed to build GitHub server TLS certificate value for listener config: %w", err) } } - desiredSecret, err := r.newScaleSetListenerConfig(&autoscalingListener, cfg, metricsConfig, cert) + desiredConfigSecret, err = r.newScaleSetListenerConfig(&autoscalingListener, cfg, metricsConfig, cert) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to build listener config secret: %w", err) } - updatedSecret := listenerConfigSecret.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(listenerConfigSecret.Labels, desiredSecret.Labels) - if !maps.Equal(listenerConfigSecret.Labels, desiredLabels) { - updatedSecret.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(listenerConfigSecret.Annotations, desiredSecret.Annotations) - if !maps.Equal(listenerConfigSecret.Annotations, desiredAnnotations) { - updatedSecret.Annotations = desiredAnnotations - shouldUpdate = true - } - - if shouldUpdate { - log.Info("Updating listener config secret", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) - if err := r.Update(ctx, updatedSecret); err != nil { - return ctrl.Result{}, fmt.Errorf("failed to update listener config secret: %w", err) + if !maps.EqualFunc(listenerConfigSecret.Data, desiredConfigSecret.Data, bytes.Equal) { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(&listenerConfigSecret, desiredConfigSecret) + updatedSecret := listenerConfigSecret.DeepCopy() + updatedSecret.Data = desiredConfigSecret.Data + if labelsChanged { + updatedSecret.Labels = labels + } + if annotationsChanged { + updatedSecret.Annotations = annotations + } + log.Info("Updating listener config secret data", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) + if err := r.Patch(ctx, updatedSecret, client.MergeFrom(&listenerConfigSecret)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update listener config secret data: %w", err) } return ctrl.Result{Requeue: true}, nil } @@ -444,6 +409,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. } var listenerPod corev1.Pod + var desiredPod *corev1.Pod err = r.Get( ctx, client.ObjectKey{ @@ -454,7 +420,7 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. ) switch { case err == nil: - desiredPod, err := r.newScaleSetListenerPod( + desiredPod, err = r.newScaleSetListenerPod( &autoscalingListener, &listenerConfigSecret, &serviceAccount, @@ -467,9 +433,17 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - shouldReCreate := desiredPod.Annotations[annotationKeyIntegrityHash] != listenerPod.Annotations[annotationKeyIntegrityHash] - if shouldReCreate { - log.Info("Listener pod dependency changed, recreating listener pod") + if desiredPod.Annotations[AnnotationKeyIntegrityHash] != listenerPod.Annotations[AnnotationKeyIntegrityHash] { + // Since the pod is controlled by a pod controller, we tag the pod with integrity hash. + // If the integrity hash is changed, that means the new spec is different. Keep in mind, the tagged hash + // is created by hashing only the fields this controller sets. + log.Info( + "Listener pod dependency changed, recreating listener pod", + "desiredSpec", + mustJSON(desiredPod.Spec), + "currentSpec", + mustJSON(listenerPod.Spec), + ) if err := r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log); err != nil { return ctrl.Result{}, err } @@ -478,28 +452,6 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } - updatedPod := listenerPod.DeepCopy() - var shouldUpdate bool - desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) - if !maps.Equal(listenerPod.Labels, desiredLabels) { - updatedPod.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) - if !maps.Equal(listenerPod.Annotations, desiredAnnotations) { - updatedPod.Annotations = desiredAnnotations - shouldUpdate = true - } - - if shouldUpdate { - log.Info("Updating listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) - if err := r.Update(ctx, updatedPod); err != nil { - log.Error(err, "Unable to update listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) - return ctrl.Result{}, err - } - return ctrl.Result{}, nil - } - case kerrors.IsNotFound(err): if err := r.publishRunningListener(&autoscalingListener, false); err != nil { // If publish fails, URL is incorrect which means the listener pod would never be able to start @@ -519,7 +471,11 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, err } - log.Info("Creating listener pod", "namespace", desiredPod.Namespace, "name", desiredPod.Name) + log.Info( + "Creating listener pod", + "namespace", desiredPod.Namespace, + "name", desiredPod.Name, + ) if err := r.Create(ctx, desiredPod); err != nil { log.Error(err, "Unable to create listener pod", "namespace", desiredPod.Namespace, "name", desiredPod.Name) return ctrl.Result{}, err @@ -542,6 +498,24 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log) case cs == nil: + if result, updated, err := r.reconcileListenerMetadata( + ctx, + &serviceAccount, + desiredServiceAccount, + &listenerRole, + desiredRole, + &listenerRoleBinding, + desiredRoleBinding, + proxySecret, + desiredListenerProxy, + &listenerConfigSecret, + desiredConfigSecret, + &listenerPod, + desiredPod, + log, + ); err != nil || updated { + return result, err + } log.Info("Listener pod is not ready", "namespace", listenerPod.Namespace, "name", listenerPod.Name) return ctrl.Result{}, nil case cs.State.Terminated != nil: @@ -563,12 +537,170 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. // notify the reconciler again. return ctrl.Result{}, nil } + if result, updated, err := r.reconcileListenerMetadata( + ctx, + &serviceAccount, + desiredServiceAccount, + &listenerRole, + desiredRole, + &listenerRoleBinding, + desiredRoleBinding, + proxySecret, + desiredListenerProxy, + &listenerConfigSecret, + desiredConfigSecret, + &listenerPod, + desiredPod, + log, + ); err != nil || updated { + return result, err + } return ctrl.Result{}, nil } return ctrl.Result{}, nil } +func (r *AutoscalingListenerReconciler) reconcileListenerMetadata( + ctx context.Context, + serviceAccount *corev1.ServiceAccount, + desiredServiceAccount *corev1.ServiceAccount, + listenerRole *rbacv1.Role, + desiredRole *rbacv1.Role, + listenerRoleBinding *rbacv1.RoleBinding, + desiredRoleBinding *rbacv1.RoleBinding, + proxySecret *corev1.Secret, + desiredListenerProxy *corev1.Secret, + listenerConfigSecret *corev1.Secret, + desiredConfigSecret *corev1.Secret, + listenerPod *corev1.Pod, + desiredPod *corev1.Pod, + log logr.Logger, +) (ctrl.Result, bool, error) { + if desiredServiceAccount != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(serviceAccount, desiredServiceAccount) + if labelsChanged || annotationsChanged { + log.Info("Updating listener service account") + updatedServiceAccount := serviceAccount.DeepCopy() + if labelsChanged { + updatedServiceAccount.Labels = labels + } + if annotationsChanged { + updatedServiceAccount.Annotations = annotations + } + if err := r.Patch(ctx, updatedServiceAccount, client.MergeFrom(serviceAccount)); err != nil { + log.Error(err, "Failed to update listener service account") + return ctrl.Result{}, false, err + } + return ctrl.Result{Requeue: true}, true, nil + } + } + + if desiredRole != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(listenerRole, desiredRole) + if labelsChanged || annotationsChanged { + log.Info("Updating listener role metadata") + updatedRole := listenerRole.DeepCopy() + if labelsChanged { + updatedRole.Labels = labels + } + if annotationsChanged { + updatedRole.Annotations = annotations + } + if err := r.Patch(ctx, updatedRole, client.MergeFrom(listenerRole)); err != nil { + log.Error(err, "Failed to update listener role metadata") + return ctrl.Result{}, false, err + } + return ctrl.Result{Requeue: true}, true, nil + } + } + + if desiredRoleBinding != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(listenerRoleBinding, desiredRoleBinding) + if labelsChanged || annotationsChanged { + log.Info("Updating listener role binding") + updatedRoleBinding := listenerRoleBinding.DeepCopy() + if labelsChanged { + updatedRoleBinding.Labels = labels + } + if annotationsChanged { + updatedRoleBinding.Annotations = annotations + } + if err := r.Patch(ctx, updatedRoleBinding, client.MergeFrom(listenerRoleBinding)); err != nil { + log.Error(err, "Failed to update listener role binding") + return ctrl.Result{}, false, err + } + + log.Info("Updated listener role binding") + return ctrl.Result{Requeue: true}, true, nil + } + } + + if proxySecret != nil && desiredListenerProxy != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(proxySecret, desiredListenerProxy) + if labelsChanged || annotationsChanged { + log.Info("Updating listener proxy secret") + updatedProxySecret := proxySecret.DeepCopy() + if labelsChanged { + updatedProxySecret.Labels = labels + } + if annotationsChanged { + updatedProxySecret.Annotations = annotations + } + if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(proxySecret)); err != nil { + log.Error(err, "Failed to update listener proxy secret") + return ctrl.Result{}, false, err + } + return ctrl.Result{Requeue: true}, true, nil + } + } + + if desiredConfigSecret != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(listenerConfigSecret, desiredConfigSecret) + if labelsChanged || annotationsChanged { + updatedSecret := listenerConfigSecret.DeepCopy() + if labelsChanged { + updatedSecret.Labels = labels + } + if annotationsChanged { + updatedSecret.Annotations = annotations + } + log.Info("Updating listener config secret", "namespace", updatedSecret.Namespace, "name", updatedSecret.Name) + if err := r.Patch(ctx, updatedSecret, client.MergeFrom(listenerConfigSecret)); err != nil { + return ctrl.Result{}, false, fmt.Errorf("failed to update listener config secret: %w", err) + } + return ctrl.Result{Requeue: true}, true, nil + } + } + + if desiredPod != nil { + labels, annotations, labelsChanged, annotationsChanged := r.desiredObjectMetadata(listenerPod, desiredPod) + if labelsChanged || annotationsChanged { + updatedPod := listenerPod.DeepCopy() + if labelsChanged { + updatedPod.Labels = labels + } + if annotationsChanged { + updatedPod.Annotations = annotations + } + log.Info("Updating listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) + if err := r.Patch(ctx, updatedPod, client.MergeFrom(listenerPod)); err != nil { + log.Error(err, "Unable to update listener pod", "namespace", updatedPod.Namespace, "name", updatedPod.Name) + return ctrl.Result{}, false, err + } + return ctrl.Result{}, true, nil + } + } + + return ctrl.Result{}, false, nil +} + +func (r *AutoscalingListenerReconciler) desiredObjectMetadata(current client.Object, desired client.Object) (map[string]string, map[string]string, bool, bool) { + labels := r.filterAndMergeLabels(current.GetLabels(), desired.GetLabels()) + annotations := r.filterAndMergeAnnotations(current.GetAnnotations(), desired.GetAnnotations()) + return labels, annotations, !maps.Equal(current.GetLabels(), labels), !r.annotationsEqual(current.GetAnnotations(), annotations) +} + func (r *AutoscalingListenerReconciler) deleteListenerPod(ctx context.Context, autoscalingListener *v1alpha1.AutoscalingListener, listenerPod *corev1.Pod, log logr.Logger) error { if err := r.publishRunningListener(autoscalingListener, false); err != nil { log.Error(err, "Unable to publish runner listener down metric", "namespace", listenerPod.Namespace, "name", listenerPod.Name) diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index d48d613e..fea9f04c 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -49,7 +49,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { scalefake.NewMultiClient(), ) - rb := ResourceBuilder{ + rb := &ResourceBuilder{ SecretResolver: secretResolver, } @@ -516,7 +516,7 @@ var _ = Describe("Test AutoScalingListener controller", func() { }, }, } - err := k8sClient.Status().Update(ctx, updated) + err := k8sClient.Status().Patch(ctx, updated, client.MergeFrom(pod)) Expect(err).NotTo(HaveOccurred(), "failed to update test pod") // Waiting for the new pod is created @@ -592,7 +592,7 @@ var _ = Describe("Test AutoScalingListener customization", func() { secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) - rb := ResourceBuilder{ + rb := &ResourceBuilder{ SecretResolver: secretResolver, } @@ -785,7 +785,7 @@ var _ = Describe("Test AutoScalingListener customization", func() { }, }, } - err := k8sClient.Status().Update(ctx, updated) + err := k8sClient.Status().Patch(ctx, updated, client.MergeFrom(pod)) Expect(err).NotTo(HaveOccurred(), "failed to update pod status") pod = new(corev1.Pod) @@ -831,7 +831,7 @@ var _ = Describe("Test AutoScalingListener customization", func() { updated := pod.DeepCopy() oldPodUID := string(pod.UID) updated.Status.Reason = "Evicted" - err := k8sClient.Status().Update(ctx, updated) + err := k8sClient.Status().Patch(ctx, updated, client.MergeFrom(pod)) Expect(err).NotTo(HaveOccurred(), "failed to update pod status") pod = new(corev1.Pod) @@ -921,7 +921,7 @@ var _ = Describe("Test AutoScalingListener controller with proxy", func() { configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) - rb := ResourceBuilder{ + rb := &ResourceBuilder{ SecretResolver: secretResolver, } @@ -954,11 +954,11 @@ var _ = Describe("Test AutoScalingListener controller with proxy", func() { proxy := &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://localhost:8080", + URL: "http://localhost:8080", CredentialSecretRef: "proxy-credentials", }, HTTPS: &v1alpha1.ProxyServerConfig{ - Url: "https://localhost:8443", + URL: "https://localhost:8443", CredentialSecretRef: "proxy-credentials", }, NoProxy: []string{ @@ -1126,7 +1126,7 @@ var _ = Describe("Test AutoScalingListener controller with template modification secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) - rb := ResourceBuilder{ + rb := &ResourceBuilder{ SecretResolver: secretResolver, } @@ -1231,7 +1231,7 @@ var _ = Describe("Test GitHub Server TLS configuration", func() { secretResolver := secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()) - rb := ResourceBuilder{ + rb := &ResourceBuilder{ SecretResolver: secretResolver, } diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller.go b/controllers/actions.github.com/autoscalingrunnerset_controller.go index f565a147..8a472af0 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller.go @@ -55,7 +55,7 @@ type AutoscalingRunnerSetReconciler struct { ControllerNamespace string DefaultRunnerScaleSetListenerImage string DefaultRunnerScaleSetListenerImagePullSecrets []string - ResourceBuilder + *ResourceBuilder } // +kubebuilder:rbac:groups=actions.github.com,resources=autoscalingrunnersets,verbs=get;list;watch;create;update;patch;delete @@ -74,7 +74,6 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl if err := r.Get(ctx, req.NamespacedName, &autoscalingRunnerSet); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := autoscalingRunnerSet.DeepCopy() if !autoscalingRunnerSet.DeletionTimestamp.IsZero() { if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { @@ -99,7 +98,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + original := autoscalingRunnerSet.DeepCopy() + controllerutil.RemoveFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) log.Info("Removing finalizer") if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil && !kerrors.IsNotFound(err) { log.Error(err, "Failed to update autoscaling runner set without finalizer") @@ -129,7 +130,9 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) { + original := autoscalingRunnerSet.DeepCopy() + controllerutil.AddFinalizer(&autoscalingRunnerSet, autoscalingRunnerSetFinalizerName) log.Info("Adding finalizer") if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { @@ -142,13 +145,12 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } // Something has changed, we need to re-apply the pending phase and change hash annotation to trigger the update of runner scale set and listener. - if targetHash := autoscalingRunnerSet.Hash(); autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] != targetHash { - // TODO: apply the version label + if targetHash := autoscalingRunnerSetIntegrityHash(&autoscalingRunnerSet); autoscalingRunnerSet.Annotations[AnnotationKeyIntegrityHash] != targetHash { original := autoscalingRunnerSet.DeepCopy() if autoscalingRunnerSet.Annotations == nil { autoscalingRunnerSet.Annotations = map[string]string{} } - autoscalingRunnerSet.Annotations[annotationKeyIntegrityHash] = targetHash + autoscalingRunnerSet.Annotations[AnnotationKeyIntegrityHash] = targetHash if err := r.Patch(ctx, &autoscalingRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update autoscaling runner set with new change hash and pending phase") return ctrl.Result{}, err @@ -204,12 +206,14 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.Replicas = 0 - ephemeralRunnerSet.Spec.PatchID = 0 - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") - return ctrl.Result{}, err + if ephemeralRunnerSet.Spec.Replicas > 0 || ephemeralRunnerSet.Spec.PatchID > 0 { + original := ephemeralRunnerSet.DeepCopy() + ephemeralRunnerSet.Spec.Replicas = 0 + ephemeralRunnerSet.Spec.PatchID = 0 + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to patch ephemeral runner set with 0 replicas and reset patch ID for the outdated runner set") + return ctrl.Result{}, err + } } return ctrl.Result{}, nil @@ -236,6 +240,7 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet + var desiredEphemeralRunnerSet *v1alpha1.EphemeralRunnerSet err := r.Get( ctx, types.NamespacedName{ @@ -285,42 +290,18 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil default: - desired, err := r.newEphemeralRunnerSet(&autoscalingRunnerSet) + desiredEphemeralRunnerSet, err = r.newEphemeralRunnerSet(&autoscalingRunnerSet) if err != nil { log.Error(err, "Failed to generate ephemeral runner set spec") return ctrl.Result{}, nil } - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != desired.Annotations[annotationKeyIntegrityHash] { - // When runners are actively processing jobs, defer the spec update: - // delete the listener to stop accepting new jobs, but leave the ERS - // (and its running pods) untouched until all jobs have drained. - var ephemeralRunnerList v1alpha1.EphemeralRunnerList - if err := r.List(ctx, &ephemeralRunnerList, - client.InNamespace(ephemeralRunnerSet.Namespace), - client.MatchingFields{resourceOwnerKey: ephemeralRunnerSet.Name}, - ); err != nil { - log.Error(err, "Failed to list ephemeral runners") - return ctrl.Result{}, err - } - - buckets := AggregateEphemeralRunnerLifecycle(ephemeralRunnerList.Items) - activeRunners := buckets.Pending + buckets.Running - - if activeRunners > 0 { - log.Info("Ephemeral runner set spec changed but runners are still active; deleting listener to stop new jobs") - if _, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log); err != nil { - log.Error(err, "Failed to clean up listener while waiting for runners to drain") - return ctrl.Result{}, err - } - return ctrl.Result{RequeueAfter: 1 * time.Second}, nil - } - + if !cmp.Equal(desiredEphemeralRunnerSet.Spec.EphemeralRunnerSpec, ephemeralRunnerSet.Spec.EphemeralRunnerSpec) { original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata - ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desired.Spec.EphemeralRunnerSpec - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerSet.Spec.EphemeralRunnerSpec = desiredEphemeralRunnerSet.Spec.EphemeralRunnerSpec + ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desiredEphemeralRunnerSet.Spec.EphemeralRunnerMetadata + ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desiredEphemeralRunnerSet.Labels) + ephemeralRunnerSet.Annotations = r.filterAndMergeAnnotations(ephemeralRunnerSet.Annotations, desiredEphemeralRunnerSet.Annotations) log.Info("Updating ephemeral runner set spec to match the desired spec") if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { @@ -329,18 +310,17 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } log.Info("Successfully patched ephemeral runner set spec") - return ctrl.Result{}, nil } - ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desired.Spec.EphemeralRunnerMetadata) - ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerAnnotationsModified := !maps.Equal(ephemeralRunnerSet.Annotations, desired.Annotations) + ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desiredEphemeralRunnerSet.Spec.EphemeralRunnerMetadata) + ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desiredEphemeralRunnerSet.Labels) + ephemeralRunnerAnnotationsModified := !r.annotationsEqual(ephemeralRunnerSet.Annotations, desiredEphemeralRunnerSet.Annotations) if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { original := ephemeralRunnerSet.DeepCopy() - ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desired.Labels) - ephemeralRunnerSet.Annotations = r.mergeAnnotations(ephemeralRunnerSet.Annotations, desired.Annotations) - ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desired.Spec.EphemeralRunnerMetadata + ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desiredEphemeralRunnerSet.Labels) + ephemeralRunnerSet.Annotations = r.filterAndMergeAnnotations(ephemeralRunnerSet.Annotations, desiredEphemeralRunnerSet.Annotations) + ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desiredEphemeralRunnerSet.Spec.EphemeralRunnerMetadata log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to patch ephemeral runner set metadata to match desired labels and annotations") @@ -348,11 +328,13 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl } log.Info("Successfully patched ephemeral runner set metadata") - return ctrl.Result{}, nil } } + activeRunners := ephemeralRunnerSet.Status.PendingEphemeralRunners + ephemeralRunnerSet.Status.RunningEphemeralRunners + var listener v1alpha1.AutoscalingListener + var desiredListener *v1alpha1.AutoscalingListener err = r.Get( ctx, types.NamespacedName{ @@ -363,13 +345,18 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl ) switch { case kerrors.IsNotFound(err): + if activeRunners > 0 { + log.Info("AutoscalingListener does not exist but runners are still active; waiting for runners to drain") + return ctrl.Result{RequeueAfter: 1 * time.Second}, nil + } + log.Info("AutoscalingListener does not exist, creating autoscaling listener") return r.createAutoScalingListenerForRunnerSet(ctx, &autoscalingRunnerSet, &ephemeralRunnerSet, log) case err != nil: log.Error(err, "Failed to get AutoscalingListener resource") return ctrl.Result{}, err default: - desired, err := r.newAutoscalingListener( + desiredListener, err = r.newAutoscalingListener( &autoscalingRunnerSet, &ephemeralRunnerSet, r.ControllerNamespace, @@ -381,15 +368,26 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, nil } - if !cmp.Equal(listener.Spec, desired.Spec) || - !cmp.Equal(listener.Labels, desired.Labels) || - !cmp.Equal(listener.Annotations, desired.Annotations) { - log.Info("Deleting AutoscalingListener to re-create with updated spec") - if err := r.Delete(ctx, &listener); err != nil { - log.Error(err, "Failed to delete AutoscalingListener for re-creation") + if !cmp.Equal(listener.Spec, desiredListener.Spec) { + if activeRunners > 0 { + log.Info("Listener spec changed but runners are still active; deleting listener to stop new jobs") + if _, err := r.cleanupListener(ctx, &autoscalingRunnerSet, log); err != nil { + log.Error(err, "Failed to clean up listener while waiting for runners to drain") + return ctrl.Result{}, err + } + return ctrl.Result{RequeueAfter: 1 * time.Second}, nil + } + + log.Info("Updating listener") + original := listener.DeepCopy() + listener.Spec = desiredListener.Spec + listener.Labels = r.filterAndMergeLabels(listener.Labels, desiredListener.Labels) + listener.Annotations = r.filterAndMergeAnnotations(listener.Annotations, desiredListener.Annotations) + if err := r.Patch(ctx, &listener, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to update AutoscalingListener with new spec") return ctrl.Result{}, err } - log.Info("Deleted AutoscalingListener, will re-create on next reconcile") + log.Info("Successfully updated AutoscalingListener with new spec") return ctrl.Result{}, nil } } @@ -406,6 +404,48 @@ func (r *AutoscalingRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } + if desiredEphemeralRunnerSet != nil { + ephemeralRunnerMetadataModified := !cmp.Equal(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata, desiredEphemeralRunnerSet.Spec.EphemeralRunnerMetadata) + ephemeralRunnerLabelsModified := !maps.Equal(ephemeralRunnerSet.Labels, desiredEphemeralRunnerSet.Labels) + ephemeralRunnerAnnotationsModified := !r.annotationsEqual(ephemeralRunnerSet.Annotations, desiredEphemeralRunnerSet.Annotations) + + if ephemeralRunnerLabelsModified || ephemeralRunnerAnnotationsModified || ephemeralRunnerMetadataModified { + original := ephemeralRunnerSet.DeepCopy() + ephemeralRunnerSet.Labels = r.filterAndMergeLabels(ephemeralRunnerSet.Labels, desiredEphemeralRunnerSet.Labels) + ephemeralRunnerSet.Annotations = r.filterAndMergeAnnotations(ephemeralRunnerSet.Annotations, desiredEphemeralRunnerSet.Annotations) + ephemeralRunnerSet.Spec.EphemeralRunnerMetadata = desiredEphemeralRunnerSet.Spec.EphemeralRunnerMetadata + log.Info("Updating ephemeral runner set metadata to match desired labels and annotations") + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to patch ephemeral runner set metadata to match desired labels and annotations") + return ctrl.Result{}, err + } + + log.Info("Successfully patched ephemeral runner set metadata") + return ctrl.Result{}, nil + } + } + + if desiredListener != nil { + listenerLabelsModified := !maps.Equal(listener.Labels, desiredListener.Labels) + listenerAnnotationsModified := !r.annotationsEqual(listener.Annotations, desiredListener.Annotations) + if listenerLabelsModified || listenerAnnotationsModified { + log.Info("Updating listener metadata") + original := listener.DeepCopy() + if listenerLabelsModified { + listener.Labels = r.filterAndMergeLabels(listener.Labels, desiredListener.Labels) + } + if listenerAnnotationsModified { + listener.Annotations = r.filterAndMergeAnnotations(listener.Annotations, desiredListener.Annotations) + } + if err := r.Patch(ctx, &listener, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to update AutoscalingListener metadata") + return ctrl.Result{}, err + } + log.Info("Successfully updated AutoscalingListener metadata") + return ctrl.Result{}, nil + } + } + return ctrl.Result{}, nil } @@ -445,13 +485,21 @@ func (r *AutoscalingRunnerSetReconciler) cleanUpResources(ctx context.Context, a // Update the status of autoscaling runner set if necessary func (r *AutoscalingRunnerSetReconciler) updateStatus(ctx context.Context, autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, phase v1alpha1.AutoscalingRunnerSetPhase, log logr.Logger) error { - phaseDiff := phase != autoscalingRunnerSet.Status.Phase - if !phaseDiff { + desiredStatus := autoscalingRunnerSet.Status + desiredStatus.Phase = phase + if ephemeralRunnerSet != nil { + desiredStatus.CurrentRunners = ephemeralRunnerSet.Status.CurrentReplicas + desiredStatus.PendingEphemeralRunners = ephemeralRunnerSet.Status.PendingEphemeralRunners + desiredStatus.RunningEphemeralRunners = ephemeralRunnerSet.Status.RunningEphemeralRunners + desiredStatus.FailedEphemeralRunners = ephemeralRunnerSet.Status.FailedEphemeralRunners + } + + if autoscalingRunnerSet.Status == desiredStatus { return nil } original := autoscalingRunnerSet.DeepCopy() - autoscalingRunnerSet.Status.Phase = phase + autoscalingRunnerSet.Status = desiredStatus if err := r.Status().Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to patch autoscaling runner set status") @@ -711,6 +759,9 @@ func (r *AutoscalingRunnerSetReconciler) updateRunnerScaleSetName(ctx context.Co logger.Info("Updating runner scale set name as an annotation") original := autoscalingRunnerSet.DeepCopy() + if autoscalingRunnerSet.Annotations == nil { + autoscalingRunnerSet.Annotations = make(map[string]string, 1) + } autoscalingRunnerSet.Annotations[AnnotationKeyGitHubRunnerScaleSetName] = updatedRunnerScaleSet.Name if err := r.Patch(ctx, autoscalingRunnerSet, client.MergeFrom(original)); err != nil { logger.Error(err, "Failed to update runner scale set name annotation") diff --git a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go index 0de00581..f4d0d4c0 100644 --- a/controllers/actions.github.com/autoscalingrunnerset_controller_test.go +++ b/controllers/actions.github.com/autoscalingrunnerset_controller_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "sync" + "sync/atomic" "time" corev1 "k8s.io/api/core/v1" @@ -72,7 +73,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -461,7 +462,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -472,13 +480,21 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalRunnerSetHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] + originalResourceVersion := runnerSet.ResourceVersion patched := autoscalingRunnerSet.DeepCopy() patched.Spec.Template.Spec.Containers[0].Image = "ghcr.io/actions/runner:updated" @@ -492,7 +508,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") g.Expect(current.Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image).To(Equal("ghcr.io/actions/runner:updated")) - g.Expect(current.Annotations[annotationKeyIntegrityHash]).NotTo(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec hash should change") + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet hash integrity key should not be modified") + g.Expect(current.ResourceVersion).NotTo(Equal(originalResourceVersion), "EphemeralRunnerSet ResourceVersion should change after update") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -504,34 +521,50 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should not be recreated") - g.Expect(current.ResourceVersion).To(Equal(originalListenerResourceVersion), "Listener should not be updated") + g.Expect(current.ResourceVersion).To(Equal(originalListenerResourceVersion), "Listener ResourceVersion should not change after update") }, - time.Second*5, + autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed()) }) - It("recreates only the Listener when max runners changes", func() { + It("Updates only the Listener when max runners changes", func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") originalListenerUID := listener.UID + originalListenerResourceVersion := listener.ResourceVersion + originalListenerIntegrityHash := listener.Annotations[AnnotationKeyIntegrityHash] runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") - originalRunnerSetUID := runnerSet.UID - originalRunnerSetHash := runnerSet.Annotations[annotationKeyIntegrityHash] + originalERSRunnerSetUID := runnerSet.UID + originalERSResourceVersion := runnerSet.ResourceVersion patched := autoscalingRunnerSet.DeepCopy() max := 20 @@ -544,8 +577,10 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") - g.Expect(current.UID).NotTo(Equal(originalListenerUID), "Listener should be recreated") + g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should be updated") + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalListenerIntegrityHash), "Listener hash integrity key should not be modified") g.Expect(current.Spec.MaxRunners).To(Equal(max)) + g.Expect(current.ResourceVersion).NotTo(Equal(originalListenerResourceVersion), "Listener ResourceVersion should change after update") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -556,8 +591,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") - g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should not be recreated") - g.Expect(current.Annotations[annotationKeyIntegrityHash]).To(Equal(originalRunnerSetHash), "EphemeralRunnerSet spec should not change") + g.Expect(current.UID).To(Equal(originalERSRunnerSetUID), "EphemeralRunnerSet should not be recreated") + g.Expect(current.ResourceVersion).To(Equal(originalERSResourceVersion), "EphemeralRunnerSet spec should not change") }, time.Second*5, autoscalingRunnerSetTestInterval, @@ -568,7 +603,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) if err != nil { return "", err } @@ -586,7 +628,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { Eventually( func() (string, error) { current := new(v1alpha1.EphemeralRunnerSet) - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + current, + ) if err != nil { return "", err } @@ -601,7 +650,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() (string, error) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) if err != nil { return "", err } @@ -614,6 +670,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { patched := autoscalingRunnerSet.DeepCopy() patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/metadata-annotation"] = "updated" patched.Spec.EphemeralRunnerSetMetadata.Annotations["arc.test/new-metadata-annotation"] = "added" + originalERSIntegrityHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] + patched.Spec.EphemeralRunnerSetMetadata.Annotations[AnnotationKeyIntegrityHash] = "must-not-be-modified" err := k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet EphemeralRunnerSet metadata") @@ -624,6 +682,7 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(current.Annotations["arc.test/metadata-annotation"]).To(Equal("updated")) g.Expect(current.Annotations["arc.test/new-metadata-annotation"]).To(Equal("added")) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalERSIntegrityHash), "EphemeralRunnerSet hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -634,7 +693,14 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func(g Gomega) { - err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + err := k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") g.Expect(runnerSet.Spec.EphemeralRunnerMetadata).NotTo(BeNil()) g.Expect(runnerSet.Spec.EphemeralRunnerMetadata.Labels["arc.test/runner-metadata-label"]).To(Equal("initial")) @@ -719,22 +785,38 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { listener := new(v1alpha1.AutoscalingListener) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: autoscalingRunnerSet.Namespace, + }, + listener, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "Listener should be created") originalListenerUID := listener.UID + originalListenerIntegrityHash := listener.Annotations[AnnotationKeyIntegrityHash] runnerSet := new(v1alpha1.EphemeralRunnerSet) Eventually( func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, runnerSet) + return k8sClient.Get( + ctx, + client.ObjectKey{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + runnerSet, + ) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, ).Should(Succeed(), "EphemeralRunnerSet should be created") - originalRunnerSetUID := runnerSet.UID + originalEphemeralRunnerSetUID := runnerSet.UID + originalEphemeralRunnerSetIntegrityHash := runnerSet.Annotations[AnnotationKeyIntegrityHash] patched := autoscalingRunnerSet.DeepCopy() patched.Spec.GitHubConfigSecret = updatedSecret.Name @@ -757,8 +839,9 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: autoscalingRunnerSet.Name, Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") - g.Expect(current.UID).To(Equal(originalRunnerSetUID), "EphemeralRunnerSet should be updated in place") + g.Expect(current.UID).To(Equal(originalEphemeralRunnerSetUID), "EphemeralRunnerSet should be updated in place") g.Expect(current.Spec.EphemeralRunnerSpec.GitHubConfigSecret).To(Equal(updatedSecret.Name)) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalEphemeralRunnerSetIntegrityHash), "EphemeralRunnerSet hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -769,8 +852,9 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { current := new(v1alpha1.AutoscalingListener) err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) g.Expect(err).NotTo(HaveOccurred(), "failed to get Listener") - g.Expect(current.UID).NotTo(Equal(originalListenerUID), "Listener should be recreated") - g.Expect(current.Spec.GitHubConfigSecret).To(Equal(updatedSecret.Name)) + g.Expect(current.UID).To(Equal(originalListenerUID), "Listener should be updated in place") + g.Expect(updatedSecret.Name).To(Equal(current.Spec.GitHubConfigSecret)) + g.Expect(current.Annotations[AnnotationKeyIntegrityHash]).To(Equal(originalListenerIntegrityHash), "Listener hash integrity key should not be modified") }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, @@ -836,9 +920,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { ).Should(BeEquivalentTo("testgroup2"), "AutoScalingRunnerSet should have the runner group in its annotation") }) }) - Context("When updating an AutoscalingRunnerSet with running or pending jobs", func() { - It("It should wait for running and pending jobs to finish before applying the update.", func() { + It("patches EphemeralRunnerSet spec without stopping the Listener when Listener fields are unchanged", func() { // Wait till the listener is created listener := new(v1alpha1.AutoscalingListener) Eventually( @@ -872,9 +955,13 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { runnerSet := runnerSetList.Items[0] activeRunnerSet := runnerSet.DeepCopy() activeRunnerSet.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + activeRunnerSet.Status.CurrentReplicas = 1 + activeRunnerSet.Status.RunningEphemeralRunners = 1 desiredStatus := v1alpha1.AutoscalingRunnerSetStatus{ - Phase: v1alpha1.AutoscalingRunnerSetPhaseRunning, + CurrentRunners: 1, + Phase: v1alpha1.AutoscalingRunnerSetPhaseRunning, + RunningEphemeralRunners: 1, } err = k8sClient.Status().Patch(ctx, activeRunnerSet, client.MergeFrom(&runnerSet)) @@ -893,13 +980,8 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { autoscalingRunnerSetTestInterval, ).Should(BeEquivalentTo(desiredStatus), "AutoScalingRunnerSet status should be updated") - // Patch the AutoScalingRunnerSet image which should trigger - // the recreation of the Listener and EphemeralRunnerSet + // Patch the AutoScalingRunnerSet image, which only changes the EphemeralRunnerSet spec. patched := autoscalingRunnerSet.DeepCopy() - if patched.Annotations == nil { - patched.Annotations = make(map[string]string) - } - patched.Annotations[annotationKeyIntegrityHash] = "testgroup2" patched.Spec.Template.Spec = corev1.PodSpec{ Containers: []corev1.Container{ { @@ -912,29 +994,92 @@ var _ = Describe("Test AutoScalingRunnerSet controller", Ordered, func() { Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") autoscalingRunnerSet = patched.DeepCopy() - // The EphemeralRunnerSet should not be recreated - Consistently( - func() (string, error) { + // The EphemeralRunnerSet should be patched in place. + Eventually( + func(g Gomega) { runnerSetList := new(v1alpha1.EphemeralRunnerSetList) err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) - Expect(err).NotTo(HaveOccurred(), "failed to fetch AutoScalingRunnerSet") - return runnerSetList.Items[0].Name, nil + g.Expect(err).NotTo(HaveOccurred(), "failed to list EphemeralRunnerSets") + g.Expect(runnerSetList.Items).To(HaveLen(1), "only one EphemeralRunnerSet should exist") + g.Expect(runnerSetList.Items[0].Name).To(Equal(activeRunnerSet.Name), "EphemeralRunnerSet should be updated in place") + g.Expect(runnerSetList.Items[0].Spec.EphemeralRunnerSpec.PodTemplateSpec.Spec.Containers[0].Image).To(Equal("ghcr.io/actions/abcd:1.1.1")) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, - ).Should(Equal(activeRunnerSet.Name), "The EphemeralRunnerSet should not be recreated") + ).Should(Succeed()) - // The listener should not be recreated + // The listener should not be deleted when its own spec is unchanged. Consistently( func() error { return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) }, autoscalingRunnerSetTestTimeout, autoscalingRunnerSetTestInterval, - ).ShouldNot(Succeed(), "Listener should not be recreated") + ).Should(Succeed(), "Listener should remain when listener fields are unchanged") + }) + + It("stops the Listener when Listener fields change while runners are active", func() { + listener := new(v1alpha1.AutoscalingListener) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, listener) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(Succeed(), "Listener should be created") + + runnerSetList := new(v1alpha1.EphemeralRunnerSetList) + Eventually( + func() (int, error) { + err := k8sClient.List(ctx, runnerSetList, client.InNamespace(autoscalingRunnerSet.Namespace)) + if err != nil { + return 0, err + } + + return len(runnerSetList.Items), nil + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeEquivalentTo(1), "Only one EphemeralRunnerSet should be created") + + runnerSet := runnerSetList.Items[0] + activeRunnerSet := runnerSet.DeepCopy() + activeRunnerSet.Status.Phase = v1alpha1.EphemeralRunnerSetPhaseRunning + activeRunnerSet.Status.CurrentReplicas = 1 + activeRunnerSet.Status.RunningEphemeralRunners = 1 + + err := k8sClient.Status().Patch(ctx, activeRunnerSet, client.MergeFrom(&runnerSet)) + Expect(err).NotTo(HaveOccurred(), "Failed to patch runner set status") + + maxRunners := 20 + patched := autoscalingRunnerSet.DeepCopy() + patched.Spec.MaxRunners = &maxRunners + err = k8sClient.Patch(ctx, patched, client.MergeFrom(autoscalingRunnerSet)) + Expect(err).NotTo(HaveOccurred(), "failed to patch AutoScalingRunnerSet") + autoscalingRunnerSet = patched.DeepCopy() + + Eventually( + func() bool { + current := new(v1alpha1.AutoscalingListener) + err := k8sClient.Get(ctx, client.ObjectKey{Name: scaleSetListenerName(autoscalingRunnerSet), Namespace: autoscalingRunnerSet.Namespace}, current) + return errors.IsNotFound(err) + }, + autoscalingRunnerSetTestTimeout, + autoscalingRunnerSetTestInterval, + ).Should(BeTrue(), "Listener should be stopped while active runners drain") + + Consistently( + func(g Gomega) { + current := new(v1alpha1.EphemeralRunnerSet) + err := k8sClient.Get(ctx, client.ObjectKey{Name: activeRunnerSet.Name, Namespace: activeRunnerSet.Namespace}, current) + g.Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") + g.Expect(current.Spec).To(Equal(runnerSet.Spec), "EphemeralRunnerSet spec should not change for listener-only updates") + }, + 5*time.Second, + autoscalingRunnerSetTestInterval, + ).Should(Succeed()) }) }) - It("Should update Status on EphemeralRunnerSet status Update", func() { ars := new(v1alpha1.AutoscalingRunnerSet) Eventually( @@ -1058,7 +1203,7 @@ var _ = Describe("Test AutoScalingController updates", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), multiClient), }, } @@ -1175,7 +1320,7 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1197,10 +1342,11 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() }, }, Spec: v1alpha1.AutoscalingRunnerSetSpec{ - GitHubConfigUrl: "https://github.com/owner/repo", - MaxRunners: &max, - MinRunners: &min, - RunnerGroup: "testgroup", + GitHubConfigUrl: "https://github.com/owner/repo", + GitHubConfigSecret: "secret1", + MaxRunners: &max, + MinRunners: &min, + RunnerGroup: "testgroup", Template: corev1.PodTemplateSpec{ Spec: corev1.PodSpec{ Containers: []corev1.Container{ @@ -1234,8 +1380,9 @@ var _ = Describe("Test AutoscalingController creation failures", Ordered, func() autoscalingRunnerSetTestInterval, ).Should(BeEquivalentTo(autoscalingRunnerSetFinalizerName), "AutoScalingRunnerSet should have a finalizer") - ars.Annotations = make(map[string]string) - err = k8sClient.Update(ctx, ars) + updated := ars.DeepCopy() + updated.Annotations = make(map[string]string) + err = k8sClient.Patch(ctx, updated, client.MergeFrom(ars)) Expect(err).NotTo(HaveOccurred(), "Update autoscaling runner set without annotation should be successful") Eventually( @@ -1302,7 +1449,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } @@ -1314,9 +1461,9 @@ var _ = Describe("Test client optional configuration", Ordered, func() { }) It("should be able to make requests to a server using a proxy", func() { - serverSuccessfullyCalled := false + var serverSuccessfullyCalled atomic.Bool proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - serverSuccessfullyCalled = true + serverSuccessfullyCalled.Store(true) w.WriteHeader(http.StatusOK) })) defer proxy.Close() @@ -1339,7 +1486,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { RunnerGroup: "testgroup", Proxy: &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: proxy.URL, + URL: proxy.URL, }, }, Template: corev1.PodTemplateSpec{ @@ -1361,7 +1508,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { // wait for server to be called Eventually( func() (bool, error) { - return serverSuccessfullyCalled, nil + return serverSuccessfullyCalled.Load(), nil }, autoscalingRunnerSetTestTimeout, 1*time.Nanosecond, @@ -1417,7 +1564,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { RunnerGroup: "testgroup", Proxy: &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://test:password@" + proxy.Listener.Addr().String(), + URL: "http://test:password@" + proxy.Listener.Addr().String(), CredentialSecretRef: "proxy-credentials", }, }, @@ -1497,7 +1644,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -1529,9 +1676,9 @@ var _ = Describe("Test client optional configuration", Ordered, func() { certPath := filepath.Join(certsFolder, "server.crt") keyPath := filepath.Join(certsFolder, "server.key") - serverSuccessfullyCalled := false + var serverSuccessfullyCalled atomic.Bool server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - serverSuccessfullyCalled = true + serverSuccessfullyCalled.Store(true) w.WriteHeader(http.StatusOK) })) cert, err := tls.LoadX509KeyPair(certPath, keyPath) @@ -1586,7 +1733,7 @@ var _ = Describe("Test client optional configuration", Ordered, func() { // wait for server to be called Eventually( func() (bool, error) { - return serverSuccessfullyCalled, nil + return serverSuccessfullyCalled.Load(), nil }, autoscalingRunnerSetTestTimeout, 1*time.Nanosecond, @@ -1744,7 +1891,7 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1904,7 +2051,7 @@ var _ = Describe("Test external permissions cleanup", Ordered, func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -2114,7 +2261,7 @@ var _ = Describe("Test resource version and build version mismatch", func() { Log: logf.Log, ControllerNamespace: autoscalingNS.Name, DefaultRunnerScaleSetListenerImage: "ghcr.io/actions/arc", - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } diff --git a/controllers/actions.github.com/constants.go b/controllers/actions.github.com/constants.go index 588c95be..ef59f3cc 100644 --- a/controllers/actions.github.com/constants.go +++ b/controllers/actions.github.com/constants.go @@ -40,6 +40,7 @@ const ( LabelKeyGitHubEnterprise = "actions.github.com/enterprise" LabelKeyGitHubOrganization = "actions.github.com/organization" LabelKeyGitHubRepository = "actions.github.com/repository" + LabelKeyEphemeralRunnerSetUID = "actions.github.com/ephemeral-runner-set-uid" ) // AutoscalingRunnerSetCleanupFinalizerName is a finalizer used to protect resources diff --git a/controllers/actions.github.com/ephemeralrunner_controller.go b/controllers/actions.github.com/ephemeralrunner_controller.go index 5ce0c444..d52ef5fd 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller.go +++ b/controllers/actions.github.com/ephemeralrunner_controller.go @@ -33,15 +33,20 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" ) const ( - ephemeralRunnerFinalizerName = "ephemeralrunner.actions.github.com/finalizer" - ephemeralRunnerActionsFinalizerName = "ephemeralrunner.actions.github.com/runner-registration-finalizer" + ephemeralRunnerFinalizerName = "ephemeralrunner.actions.github.com/finalizer" + terminalPodUpdatePriority = 100 ) // EphemeralRunnerReconciler reconciles a EphemeralRunner object @@ -50,7 +55,7 @@ type EphemeralRunnerReconciler struct { Log logr.Logger Scheme *runtime.Scheme PublishMetrics bool - ResourceBuilder + *ResourceBuilder } // precompute backoff durations for failed ephemeral runners @@ -71,7 +76,7 @@ const maxFailures = 5 // +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunners/finalizers,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=core,resources=pods/status,verbs=get -// +kubebuilder:rbac:groups=core,resources=secrets,verbs=create;get;list;watch;delete +// +kubebuilder:rbac:groups=core,resources=secrets,verbs=create;get;list;watch;delete;deletecollection // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -85,7 +90,6 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err := r.Get(ctx, req.NamespacedName, &ephemeralRunner); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunner.DeepCopy() if !ephemeralRunner.DeletionTimestamp.IsZero() { r.emitLifecycleMetrics(ctx, &ephemeralRunner, log) @@ -94,27 +98,11 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - if controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { - log.Info("Trying to clean up runner from the service") - ok, err := r.cleanupRunnerFromService(ctx, &ephemeralRunner, log) - if err != nil { - log.Error(err, "Failed to clean up runner from service") - return ctrl.Result{}, err + if ephemeralRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseSucceeded && ephemeralRunner.Status.RunnerID != 0 { + log.Info("Trying to remove runner from the service before finalizing") + if err := r.deleteRunnerFromService(ctx, &ephemeralRunner, log); err != nil { + log.Error(err, "Failed to remove runner from the service before finalizing") } - if !ok { - log.Info("Runner is not finished yet, retrying in 30s") - return ctrl.Result{RequeueAfter: 30 * time.Second}, nil - } - - log.Info("Runner is cleaned up from the service, removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) { - log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner after removing finalizer") - return ctrl.Result{}, err - } - } - log.Info("Removed finalizer from ephemeral runner") } log.Info("Finalizing ephemeral runner") @@ -134,12 +122,12 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) { - log.Info("Removed finalizer from ephemeral runner") - if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { - log.Error(err, "Failed to update ephemeral runner after removing finalizer") - return ctrl.Result{}, err - } + original := ephemeralRunner.DeepCopy() + controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) + log.Info("Removed finalizer from ephemeral runner") + if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); client.IgnoreNotFound(err) != nil { + log.Error(err, "Failed to update ephemeral runner after removing finalizer") + return ctrl.Result{}, err } log.Info("Successfully removed finalizer after cleanup") @@ -154,18 +142,30 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, err } - // Stop reconciling on this object. - // The EphemeralRunnerSet is responsible for cleaning it up. - log.Info("EphemeralRunner has already finished. Stopping reconciliation and waiting for EphemeralRunnerSet to clean it up", "phase", ephemeralRunner.Status.Phase) + if ephemeralRunner.HasContainerHookConfigured() { + log.Info("Runner has container hook configured, cleaning up container hook resources") + err = r.cleanupContainerHooksResources(ctx, &ephemeralRunner, log) + if err != nil { + log.Error(err, "Failed to clean up container hooks resources") + return ctrl.Result{}, err + } + } + + log.Info("EphemeralRunner has already finished. Requesting deletion after cleanup", "phase", ephemeralRunner.Status.Phase) + if err := r.deleteCompletedEphemeralRunner(ctx, &ephemeralRunner, log); err != nil { + log.Error(err, "Failed to delete completed ephemeral runner") + return ctrl.Result{}, err + } + return ctrl.Result{}, nil } - addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) || !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) + addFinalizers := !controllerutil.ContainsFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) if addFinalizers { log.Info("Adding finalizers") + original := ephemeralRunner.DeepCopy() var addedFinalizers bool addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerFinalizerName) - addedFinalizers = addedFinalizers || controllerutil.AddFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) if addedFinalizers { if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update with finalizer set") @@ -175,8 +175,20 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ log.Info("Successfully added finalizers") } - secret := new(corev1.Secret) - if err := r.Get(ctx, req.NamespacedName, secret); err != nil { + if ephemeralRunner.Status.RunnerID != 0 { + var pod corev1.Pod + err := r.Get(ctx, req.NamespacedName, &pod) + switch { + case err == nil: + return r.reconcilePod(ctx, &ephemeralRunner, &pod, log) + case !kerrors.IsNotFound(err): + log.Error(err, "Failed to fetch the pod") + return ctrl.Result{}, err + } + } + + var secret corev1.Secret + if err := r.Get(ctx, req.NamespacedName, &secret); err != nil { if !kerrors.IsNotFound(err) { log.Error(err, "Failed to fetch secret") return ctrl.Result{}, err @@ -192,7 +204,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, fmt.Errorf("failed to create secret: %w", err) } log.Info("Created new ephemeral runner secret for jitconfig.") - secret = jitSecret + secret = *jitSecret case errors.Is(err, retryableError): log.Info("Encountered retryable error, requeueing", "error", err.Error()) @@ -216,7 +228,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ if err != nil { log.Error(err, "Runner config secret is corrupted: missing runnerId") log.Info("Deleting corrupted runner config secret") - if err := r.Delete(ctx, secret); err != nil { + if err := r.Delete(ctx, &secret); err != nil { return ctrl.Result{}, fmt.Errorf("failed to delete the corrupted runner config secret") } log.Info("Corrupted runner config secret has been deleted") @@ -244,33 +256,35 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ return ctrl.Result{}, nil } - now := metav1.Now() lastFailure := ephemeralRunner.Status.LastFailure() - backoffDuration := failedRunnerBackoff[len(ephemeralRunner.Status.Failures)] - nextReconciliation := lastFailure.Add(backoffDuration) - if !lastFailure.IsZero() && now.Before(&metav1.Time{Time: nextReconciliation}) { - requeueAfter := nextReconciliation.Sub(now.Time) - log.Info( - "Backing off the next reconciliation due to failure", - "lastFailure", lastFailure, - "nextReconciliation", nextReconciliation, - "requeueAfter", requeueAfter, - ) - return ctrl.Result{ - Requeue: true, - RequeueAfter: requeueAfter, - }, nil + if !lastFailure.IsZero() { + now := metav1.Now() + backoffDuration := failedRunnerBackoff[len(ephemeralRunner.Status.Failures)] + nextReconciliation := lastFailure.Add(backoffDuration) + if now.Before(&metav1.Time{Time: nextReconciliation}) { + requeueAfter := nextReconciliation.Sub(now.Time) + log.Info( + "Backing off the next reconciliation due to failure", + "lastFailure", lastFailure, + "nextReconciliation", nextReconciliation, + "requeueAfter", requeueAfter, + ) + return ctrl.Result{ + Requeue: true, + RequeueAfter: requeueAfter, + }, nil + } } - pod := new(corev1.Pod) - if err := r.Get(ctx, req.NamespacedName, pod); err != nil { + var pod corev1.Pod + if err := r.Get(ctx, req.NamespacedName, &pod); err != nil { if !kerrors.IsNotFound(err) { log.Error(err, "Failed to fetch the pod") return ctrl.Result{}, err } log.Info("Ephemeral runner pod does not exist. Creating new ephemeral runner") - result, err := r.createPod(ctx, &ephemeralRunner, secret, log) + result, err := r.createPod(ctx, &ephemeralRunner, &secret, log) switch { case err == nil: return result, nil @@ -318,6 +332,10 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ } } + return r.reconcilePod(ctx, &ephemeralRunner, &pod, log) +} + +func (r *EphemeralRunnerReconciler) reconcilePod(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, pod *corev1.Pod, log logr.Logger) (ctrl.Result, error) { cs := runnerContainerStatus(pod) switch { case pod.Status.Phase == corev1.PodFailed: // All containers are stopped @@ -331,7 +349,7 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ // Therefore, we should try to restart it. if cs == nil || cs.State.Terminated == nil { log.Info("Runner container does not have state set, deleting pod as failed so it can be restarted") - return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, &ephemeralRunner, pod, log) + return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, ephemeralRunner, pod, log) } switch cs.State.Terminated.ExitCode { @@ -341,13 +359,13 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ // If the runner container exits with 0, we assume that the runner has finished successfully. // If side-car container exits with non-zero, it shouldn't affect the runner. Runner exit code // drives the controller's inference of whether the job has succeeded or failed. - if err := r.Delete(ctx, &ephemeralRunner); err != nil { - log.Error(err, "Failed to delete ephemeral runner after successful completion") + if err := r.markAsSucceededAndCleanup(ctx, ephemeralRunner, pod, log); err != nil { + log.Error(err, "Failed to clean up ephemeral runner resources after successful completion") return ctrl.Result{}, err } return ctrl.Result{}, nil case 7: - if err := r.markAsOutdated(ctx, &ephemeralRunner, log); err != nil { + if err := r.markAsOutdated(ctx, ephemeralRunner, log); err != nil { log.Error(err, "Failed to set ephemeral runner to phase Outdated") return ctrl.Result{}, err } @@ -359,14 +377,14 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ "Ephemeral runner container has failed, and runner container termination exit code is non-zero", "containerTerminatedState", cs.State.Terminated, ) - return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, &ephemeralRunner, pod, log) + return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, ephemeralRunner, pod, log) case initContainerFailed(pod): log.Info( "Pod has a failed init container, deleting pod as failed so it can be restarted", "initContainerStatuses", pod.Status.InitContainerStatuses, ) - return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, &ephemeralRunner, pod, log) + return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, ephemeralRunner, pod, log) case cs == nil: // starting, no container state yet @@ -375,14 +393,14 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ case cs.State.Terminated == nil: // container is not terminated and pod phase is not failed, so runner is still running log.Info("Runner container is still running; updating ephemeral runner status") - if err := r.updateRunStatusFromPod(ctx, &ephemeralRunner, pod, log); err != nil { + if err := r.updateRunStatusFromPod(ctx, ephemeralRunner, pod, log); err != nil { log.Info("Failed to update ephemeral runner status. Requeue to not miss this event") return ctrl.Result{}, err } return ctrl.Result{}, nil case cs.State.Terminated.ExitCode == 7: // outdated - if err := r.markAsOutdated(ctx, &ephemeralRunner, log); err != nil { + if err := r.markAsOutdated(ctx, ephemeralRunner, log); err != nil { log.Error(err, "Failed to set ephemeral runner to phase Outdated") return ctrl.Result{}, err } @@ -390,12 +408,12 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ case cs.State.Terminated.ExitCode != 0: // failed log.Info("Ephemeral runner container failed", "exitCode", cs.State.Terminated.ExitCode) - return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, &ephemeralRunner, pod, log) + return ctrl.Result{}, r.deleteEphemeralRunnerOrPod(ctx, ephemeralRunner, pod, log) default: // succeeded - log.Info("Ephemeral runner has finished successfully, deleting ephemeral runner", "exitCode", cs.State.Terminated.ExitCode) - if err := r.Delete(ctx, &ephemeralRunner); err != nil { - log.Error(err, "Failed to delete ephemeral runner after successful completion") + log.Info("Ephemeral runner has finished successfully, cleaning up runner resources", "exitCode", cs.State.Terminated.ExitCode) + if err := r.markAsSucceededAndCleanup(ctx, ephemeralRunner, pod, log); err != nil { + log.Error(err, "Failed to clean up ephemeral runner resources after successful completion") return ctrl.Result{}, err } return ctrl.Result{}, nil @@ -408,24 +426,34 @@ func (r *EphemeralRunnerReconciler) deleteEphemeralRunnerOrPod(ctx context.Conte errors.New("ephemeral runner has a job assigned, but the pod has failed"), "Ephemeral runner either has faulty entrypoint or something external killing the runner", ) + + if ephemeralRunner.Status.RunnerID != 0 { + log.Info("Trying to remove the runner from the service") + if err := r.deleteRunnerFromService(ctx, ephemeralRunner, log); err != nil { + log.Error(err, "Failed to remove the runner from the service") + } + } + log.Info("Deleting the ephemeral runner that has a job assigned but the pod has failed") if err := r.Delete(ctx, ephemeralRunner); err != nil { log.Error(err, "Failed to delete the ephemeral runner that has a job assigned but the pod has failed") return err } - log.Info("Deleted the ephemeral runner that has a job assigned but the pod has failed") - log.Info("Trying to remove the runner from the service") - actionsClient, err := r.GetActionsService(ctx, ephemeralRunner) - if err != nil { - log.Error(err, "Failed to get actions client for removing the runner from the service") - return nil + return nil + } + + failureCount := len(ephemeralRunner.Status.Failures) + if _, ok := ephemeralRunner.Status.Failures[string(pod.UID)]; !ok { + failureCount++ + } + if failureCount > maxFailures { + log.Info(fmt.Sprintf("EphemeralRunner has failed more than %d times. Deleting ephemeral runner so it can be re-created", maxFailures)) + if err := r.Delete(ctx, ephemeralRunner); err != nil { + log.Error(fmt.Errorf("failed to delete ephemeral runner after %d failures: %w", maxFailures, err), "Failed to delete ephemeral runner") + return err } - if err := actionsClient.RemoveRunner(ctx, int64(ephemeralRunner.Status.RunnerID)); err != nil { - log.Error(err, "Failed to remove the runner from the service") - return nil - } - log.Info("Removed the runner from the service") + return nil } @@ -437,59 +465,53 @@ func (r *EphemeralRunnerReconciler) deleteEphemeralRunnerOrPod(ctx context.Conte return nil } -func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (ok bool, err error) { - if err := r.deleteRunnerFromService(ctx, ephemeralRunner, log); err != nil { - if errors.Is(err, scaleset.JobStillRunningError) { - log.Info("Runner job is still running, cannot remove the runner from the service yet") - return false, nil - } - - return false, err - } - - return true, nil +func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { + return r.cleanupResourcesForPod(ctx, ephemeralRunner, nil, log) } -func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { +func (r *EphemeralRunnerReconciler) cleanupResourcesForPod(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, pod *corev1.Pod, log logr.Logger) error { log.Info("Cleaning up the runner pod") - pod := new(corev1.Pod) - err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, pod) - switch { - case err == nil: + if pod != nil { if pod.DeletionTimestamp.IsZero() { log.Info("Deleting the runner pod") - if err := r.Delete(ctx, pod); err != nil && !kerrors.IsNotFound(err) { + if err := r.deletePodForCleanup(ctx, pod, log); err != nil { return fmt.Errorf("failed to delete pod: %w", err) } log.Info("Deleted the runner pod") } else { - log.Info("Pod contains deletion timestamp") + log.Info("Runner pod is already being deleted") } - case kerrors.IsNotFound(err): - log.Info("Runner pod is deleted") - default: - return err + } else { + var pod corev1.Pod + err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, &pod) + switch { + case err == nil && pod.DeletionTimestamp.IsZero(): + log.Info("Deleting the runner pod") + if err := r.deletePodForCleanup(ctx, &pod, log); err != nil { + return fmt.Errorf("failed to delete pod: %w", err) + } + log.Info("Deleted the runner pod") + case err == nil && !pod.DeletionTimestamp.IsZero(): + log.Info("Runner pod is already being deleted") + case kerrors.IsNotFound(err): + log.Info("Runner pod is deleted") + default: + return fmt.Errorf("failed to get pod: %w", err) + } + } + log.Info("Cleaning up the runner jitconfig secret") + secret := corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: ephemeralRunner.Name, + Namespace: ephemeralRunner.Namespace, + }, } - log.Info("Cleaning up the runner jitconfig secret") - secret := new(corev1.Secret) - err = r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, secret) - switch { - case err == nil: - if secret.DeletionTimestamp.IsZero() { - log.Info("Deleting the jitconfig secret") - if err := r.Delete(ctx, secret); err != nil && !kerrors.IsNotFound(err) { - return fmt.Errorf("failed to delete secret: %w", err) - } - log.Info("Deleted jitconfig secret") - } else { - log.Info("Secret contains deletion timestamp") - } - case kerrors.IsNotFound(err): - log.Info("Runner jitconfig secret is deleted") - default: - return err + log.Info("Deleting the jitconfig secret") + if err := r.Delete(ctx, &secret); err != nil && !kerrors.IsNotFound(err) { + return fmt.Errorf("failed to delete secret: %w", err) } + log.Info("Deleted jitconfig secret") return nil } @@ -535,7 +557,7 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerLinkedPods(ctx context.Context, } log.Info("Deleting container hooks runner-linked pod", "name", linkedPod.Name) - if err := r.Delete(ctx, linkedPod); err != nil && !kerrors.IsNotFound(err) { + if err := r.deletePodForCleanup(ctx, linkedPod, log); err != nil { errs = append(errs, fmt.Errorf("failed to delete runner linked pod %q: %w", linkedPod.Name, err)) } } @@ -549,32 +571,13 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerLinkedSecrets(ctx context.Conte "runner-pod": ephemeralRunner.Name, }, ) - var runnerLinkedSecretList corev1.SecretList - if err := r.List(ctx, &runnerLinkedSecretList, client.InNamespace(ephemeralRunner.Namespace), runnerLinkedLabels); err != nil { - return fmt.Errorf("failed to list runner-linked secrets: %w", err) + log.Info("Deleting container hooks runner-linked secrets") + if err := r.DeleteAllOf(ctx, &corev1.Secret{}, client.InNamespace(ephemeralRunner.Namespace), runnerLinkedLabels); err != nil { + return fmt.Errorf("failed to delete runner-linked secrets: %w", err) } - if len(runnerLinkedSecretList.Items) == 0 { - log.Info("Runner-linked secrets are deleted") - return nil - } - - log.Info("Deleting container hooks runner-linked secrets", "count", len(runnerLinkedSecretList.Items)) - - var errs []error - for i := range runnerLinkedSecretList.Items { - s := &runnerLinkedSecretList.Items[i] - if !s.DeletionTimestamp.IsZero() { - continue - } - - log.Info("Deleting container hooks runner-linked secret", "name", s.Name) - if err := r.Delete(ctx, s); err != nil && !kerrors.IsNotFound(err) { - errs = append(errs, fmt.Errorf("failed to delete runner linked secret %q: %w", s.Name, err)) - } - } - - return errors.Join(errs...) + log.Info("Runner-linked secrets are deleted") + return nil } func (r *EphemeralRunnerReconciler) markAsFailed(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, errMessage string, reason string, log logr.Logger) error { @@ -620,6 +623,41 @@ func (r *EphemeralRunnerReconciler) markAsOutdated(ctx context.Context, ephemera return nil } +func (r *EphemeralRunnerReconciler) markAsSucceededAndCleanup(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, pod *corev1.Pod, log logr.Logger) error { + if ephemeralRunner.Status.Phase != v1alpha1.EphemeralRunnerPhaseSucceeded { + log.Info("Updating ephemeral runner status to Succeeded") + original := ephemeralRunner.DeepCopy() + ephemeralRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + ephemeralRunner.Status.Ready = false + ephemeralRunner.Status.Reason = "" + ephemeralRunner.Status.Message = "" + + if err := r.Status().Patch(ctx, ephemeralRunner, client.MergeFrom(original)); err != nil { + return fmt.Errorf("failed to update ephemeral runner status to Succeeded: %w", err) + } + } + + if err := r.cleanupResourcesForPod(ctx, ephemeralRunner, pod, log); err != nil { + return fmt.Errorf("failed to clean up ephemeral runner resources after successful completion: %w", err) + } + + if err := r.deleteCompletedEphemeralRunner(ctx, ephemeralRunner, log); err != nil { + return fmt.Errorf("failed to delete ephemeral runner after successful completion: %w", err) + } + + return nil +} + +func (r *EphemeralRunnerReconciler) deleteCompletedEphemeralRunner(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error { + log.Info("Deleting completed ephemeral runner") + if err := r.Delete(ctx, ephemeralRunner); err != nil && !kerrors.IsNotFound(err) { + return err + } + log.Info("Deleted completed ephemeral runner") + + return nil +} + // deletePodAsFailed is responsible for deleting the pod and updating the .Status.Failures for tracking failure count. // It should not be responsible for setting the status to Failed. // @@ -627,7 +665,7 @@ func (r *EphemeralRunnerReconciler) markAsOutdated(ctx context.Context, ephemera func (r *EphemeralRunnerReconciler) deletePodAsFailed(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, pod *corev1.Pod, log logr.Logger) error { if pod.DeletionTimestamp.IsZero() { log.Info("Deleting the ephemeral runner pod", "podId", pod.UID) - if err := r.Delete(ctx, pod); err != nil && !kerrors.IsNotFound(err) { + if err := r.deletePodForCleanup(ctx, pod, log); err != nil { return fmt.Errorf("failed to delete pod with status failed: %w", err) } } @@ -652,6 +690,25 @@ func (r *EphemeralRunnerReconciler) deletePodAsFailed(ctx context.Context, ephem return nil } +var ( + defaultPodDeleteOptions = []client.DeleteOption{} + forcePodDeleteOptions = []client.DeleteOption{client.GracePeriodSeconds(0)} +) + +func (r *EphemeralRunnerReconciler) deletePodForCleanup(ctx context.Context, pod *corev1.Pod, log logr.Logger) error { + deleteOptions := defaultPodDeleteOptions + if shouldForceDeletePod(pod) { + log.Info("Force deleting terminal pod", "pod", types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}) + deleteOptions = forcePodDeleteOptions + } + + if err := r.Delete(ctx, pod, deleteOptions...); err != nil && !kerrors.IsNotFound(err) { + return err + } + + return nil +} + func (r *EphemeralRunnerReconciler) createRunnerJitConfig(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (*scaleset.RunnerScaleSetJitRunnerConfig, error) { // Runner is not registered with the service. We need to register it first log.Info("Creating ephemeral runner JIT config") @@ -875,12 +932,88 @@ func (r *EphemeralRunnerReconciler) SetupWithManager(mgr ctrl.Manager, opts ...O return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.EphemeralRunner{}). - Owns(&corev1.Pod{}). + Watches(&corev1.Pod{}, newEphemeralRunnerPodEventHandler(mgr)). WithEventFilter(predicate.ResourceVersionChangedPredicate{}), opts, ).Complete(r) } +func newEphemeralRunnerPodEventHandler(mgr ctrl.Manager) handler.EventHandler { + return &prioritizedPodEventHandler{ + owner: handler.EnqueueRequestForOwner(mgr.GetScheme(), mgr.GetRESTMapper(), &v1alpha1.EphemeralRunner{}, handler.OnlyControllerOwner()), + } +} + +type prioritizedPodEventHandler struct { + owner handler.EventHandler +} + +func (h *prioritizedPodEventHandler) Create(ctx context.Context, evt event.CreateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + h.owner.Create(ctx, evt, q) +} + +func (h *prioritizedPodEventHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + if podBecameCleanupCandidate(evt.ObjectOld, evt.ObjectNew) { + h.owner.Update(ctx, evt, prioritizedWorkQueue{TypedRateLimitingInterface: q, priority: terminalPodUpdatePriority}) + return + } + + h.owner.Update(ctx, evt, q) +} + +func (h *prioritizedPodEventHandler) Delete(ctx context.Context, evt event.DeleteEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + h.owner.Delete(ctx, evt, q) +} + +func (h *prioritizedPodEventHandler) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + h.owner.Generic(ctx, evt, q) +} + +type prioritizedWorkQueue struct { + workqueue.TypedRateLimitingInterface[reconcile.Request] + priority int +} + +func (q prioritizedWorkQueue) Add(item reconcile.Request) { + priorityQueue, ok := q.TypedRateLimitingInterface.(priorityqueue.PriorityQueue[reconcile.Request]) + if !ok { + q.TypedRateLimitingInterface.Add(item) + return + } + + priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: &q.priority}, item) +} + +func (q prioritizedWorkQueue) AddAfter(item reconcile.Request, after time.Duration) { + priorityQueue, ok := q.TypedRateLimitingInterface.(priorityqueue.PriorityQueue[reconcile.Request]) + if !ok { + q.TypedRateLimitingInterface.AddAfter(item, after) + return + } + + priorityQueue.AddWithOpts(priorityqueue.AddOpts{After: after, Priority: &q.priority}, item) +} + +func (q prioritizedWorkQueue) AddRateLimited(item reconcile.Request) { + priorityQueue, ok := q.TypedRateLimitingInterface.(priorityqueue.PriorityQueue[reconcile.Request]) + if !ok { + q.TypedRateLimitingInterface.AddRateLimited(item) + return + } + + priorityQueue.AddWithOpts(priorityqueue.AddOpts{RateLimited: true, Priority: &q.priority}, item) +} + +func podBecameCleanupCandidate(oldObj, newObj client.Object) bool { + newPod, ok := newObj.(*corev1.Pod) + if !ok || newPod == nil || !shouldForceDeletePod(newPod) { + return false + } + + oldPod, ok := oldObj.(*corev1.Pod) + return !ok || oldPod == nil || !shouldForceDeletePod(oldPod) +} + func runnerContainerStatus(pod *corev1.Pod) *corev1.ContainerStatus { for i := range pod.Status.ContainerStatuses { cs := &pod.Status.ContainerStatuses[i] @@ -974,3 +1107,15 @@ func (r *EphemeralRunnerReconciler) emitLifecycleMetrics(ctx context.Context, ep "deleting", buckets.Deleting, ) } + +func shouldForceDeletePod(pod *corev1.Pod) bool { + if pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed { + return true + } + + if cs := runnerContainerStatus(pod); cs != nil && cs.State.Terminated != nil { + return true + } + + return initContainerFailed(pod) +} diff --git a/controllers/actions.github.com/ephemeralrunner_controller_test.go b/controllers/actions.github.com/ephemeralrunner_controller_test.go index 80c27134..a96a7e33 100644 --- a/controllers/actions.github.com/ephemeralrunner_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunner_controller_test.go @@ -10,6 +10,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "time" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" @@ -110,7 +111,7 @@ var _ = Describe("EphemeralRunner", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -155,7 +156,7 @@ var _ = Describe("EphemeralRunner", func() { }, ephemeralRunnerTimeout, ephemeralRunnerInterval, - ).Should(BeEquivalentTo([]string{ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName})) + ).Should(BeEquivalentTo([]string{ephemeralRunnerFinalizerName})) Eventually( func() (bool, error) { @@ -223,8 +224,9 @@ var _ = Describe("EphemeralRunner", func() { ).Should(Succeed(), "failed to get ephemeral runner") // update job id to simulate job assigned - er.Status.JobID = "1" - err := k8sClient.Status().Update(ctx, er) + updatedER := er.DeepCopy() + updatedER.Status.JobID = "1" + err := k8sClient.Status().Patch(ctx, updatedER, client.MergeFrom(er)) Expect(err).To(BeNil(), "failed to update ephemeral runner status") er = new(v1alpha1.EphemeralRunner) @@ -249,7 +251,8 @@ var _ = Describe("EphemeralRunner", func() { }).Should(BeEquivalentTo(true)) // delete pod to simulate failure - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -257,7 +260,7 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - err = k8sClient.Status().Update(ctx, pod) + err = k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") er = new(v1alpha1.EphemeralRunner) @@ -277,8 +280,9 @@ var _ = Describe("EphemeralRunner", func() { return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er) }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(Succeed(), "failed to get ephemeral runner") - er.Status.JobID = "1" - err := k8sClient.Status().Update(ctx, er) + updatedER := er.DeepCopy() + updatedER.Status.JobID = "1" + err := k8sClient.Status().Patch(ctx, updatedER, client.MergeFrom(er)) Expect(err).To(BeNil(), "failed to update ephemeral runner status") Eventually(func() (string, error) { @@ -297,9 +301,10 @@ var _ = Describe("EphemeralRunner", func() { return true, nil }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(BeEquivalentTo(true)) - pod.Status.Phase = corev1.PodFailed - pod.Status.ContainerStatuses = nil - err = k8sClient.Status().Update(ctx, pod) + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.ContainerStatuses = nil + err = k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually(func() bool { @@ -320,9 +325,10 @@ var _ = Describe("EphemeralRunner", func() { oldPodUID := pod.UID - pod.Status.Phase = corev1.PodFailed - pod.Status.ContainerStatuses = nil - err := k8sClient.Status().Update(ctx, pod) + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.ContainerStatuses = nil + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually( @@ -369,8 +375,9 @@ var _ = Describe("EphemeralRunner", func() { // Simulate init container failure without PodFailed phase. // This can happen when the kubelet has not yet transitioned the pod phase. - pod.Status.Phase = corev1.PodPending - pod.Status.InitContainerStatuses = []corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodPending + updatedPod.Status.InitContainerStatuses = []corev1.ContainerStatus{ { Name: "setup", State: corev1.ContainerState{ @@ -382,7 +389,7 @@ var _ = Describe("EphemeralRunner", func() { }, }, } - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually( @@ -422,8 +429,9 @@ var _ = Describe("EphemeralRunner", func() { return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er) }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(Succeed(), "failed to get ephemeral runner") - er.Status.JobID = "1" - err := k8sClient.Status().Update(ctx, er) + updatedER := er.DeepCopy() + updatedER.Status.JobID = "1" + err := k8sClient.Status().Patch(ctx, updatedER, client.MergeFrom(er)) Expect(err).To(BeNil(), "failed to update ephemeral runner status") Eventually(func() (string, error) { @@ -443,8 +451,9 @@ var _ = Describe("EphemeralRunner", func() { }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(BeEquivalentTo(true)) // Simulate init container failure with job assigned - pod.Status.Phase = corev1.PodPending - pod.Status.InitContainerStatuses = []corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodPending + updatedPod.Status.InitContainerStatuses = []corev1.ContainerStatus{ { Name: "setup", State: corev1.ContainerState{ @@ -455,7 +464,7 @@ var _ = Describe("EphemeralRunner", func() { }, }, } - err = k8sClient.Status().Update(ctx, pod) + err = k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually(func() bool { @@ -471,8 +480,9 @@ var _ = Describe("EphemeralRunner", func() { return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er) }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(Succeed(), "failed to get ephemeral runner") - er.Status.JobID = "1" - err := k8sClient.Status().Update(ctx, er) + updatedER := er.DeepCopy() + updatedER.Status.JobID = "1" + err := k8sClient.Status().Patch(ctx, updatedER, client.MergeFrom(er)) Expect(err).To(BeNil(), "failed to update ephemeral runner status") pod := new(corev1.Pod) @@ -487,8 +497,9 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(Succeed(), "failed to get pod") - pod.Status.Phase = corev1.PodFailed - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -496,18 +507,40 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - err = k8sClient.Status().Update(ctx, pod) + err = k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually( - func() bool { + func() (bool, error) { check := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check); err != nil { + return kerrors.IsNotFound(err), client.IgnoreNotFound(err) + } + return check.Status.Phase == v1alpha1.EphemeralRunnerPhaseSucceeded, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner should be marked as succeeded or deleted") + + Eventually( + func() bool { + check := new(corev1.Pod) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) return kerrors.IsNotFound(err) }, ephemeralRunnerTimeout, ephemeralRunnerInterval, - ).Should(BeTrue(), "Ephemeral runner should eventually be deleted") + ).Should(BeTrue(), "Ephemeral runner pod should eventually be deleted") + + Eventually( + func() bool { + check := new(corev1.Secret) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner secret should eventually be deleted") }) It("It should treat pod failed with runner container exit 0 as success with no job id", func() { @@ -523,8 +556,9 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(Succeed(), "failed to get pod") - pod.Status.Phase = corev1.PodFailed - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -532,18 +566,40 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually( - func() bool { + func() (bool, error) { check := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check); err != nil { + return kerrors.IsNotFound(err), client.IgnoreNotFound(err) + } + return check.Status.Phase == v1alpha1.EphemeralRunnerPhaseSucceeded, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner should be marked as succeeded or deleted") + + Eventually( + func() bool { + check := new(corev1.Pod) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) return kerrors.IsNotFound(err) }, ephemeralRunnerTimeout, ephemeralRunnerInterval, - ).Should(BeTrue(), "Ephemeral runner should eventually be deleted") + ).Should(BeTrue(), "Ephemeral runner pod should eventually be deleted") + + Eventually( + func() bool { + check := new(corev1.Secret) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner secret should eventually be deleted") }) It("It should mark as failed when job is not assigned and pod is failed", func() { @@ -568,9 +624,10 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(Succeed(), "failed to get pod") - pod.Status.Phase = corev1.PodFailed + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed oldPodUID := pod.UID - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -579,7 +636,7 @@ var _ = Describe("EphemeralRunner", func() { }, }) - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") Eventually( @@ -779,6 +836,74 @@ var _ = Describe("EphemeralRunner", func() { ).Should(BeEquivalentTo(true)) }) + It("It should clean up runner-linked secrets when the runner is done", func() { + Eventually( + func() (bool, error) { + pod := new(corev1.Pod) + if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod); err != nil { + return false, err + } + return true, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(true)) + + runnerLinkedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner-linked-secret-done", + Namespace: ephemeralRunner.Namespace, + Labels: map[string]string{ + "runner-pod": ephemeralRunner.Name, + }, + }, + Data: map[string][]byte{"test": []byte("test")}, + } + + err := k8sClient.Create(ctx, runnerLinkedSecret) + Expect(err).To(BeNil(), "failed to create runner linked secret") + + updated := new(v1alpha1.EphemeralRunner) + Eventually( + func() error { + return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(Succeed(), "failed to get ephemeral runner") + + finished := updated.DeepCopy() + finished.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + err = k8sClient.Status().Patch(ctx, finished, client.MergeFrom(updated)) + Expect(err).To(BeNil(), "failed to mark ephemeral runner as succeeded") + + Eventually( + func() (bool, error) { + secret := new(corev1.Secret) + err = k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, secret) + if err == nil { + return false, nil + } + return kerrors.IsNotFound(err), nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(true)) + + Eventually( + func() (bool, error) { + secret := new(corev1.Secret) + err = k8sClient.Get(ctx, client.ObjectKey{Name: runnerLinkedSecret.Name, Namespace: runnerLinkedSecret.Namespace}, secret) + if err == nil { + return false, nil + } + return kerrors.IsNotFound(err), nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeEquivalentTo(true)) + }) + It("It should eventually have runner id set", func() { Eventually( func() (int, error) { @@ -939,8 +1064,9 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) - pod.Status.Phase = corev1.PodRunning - err := k8sClient.Status().Update(ctx, pod) + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodRunning + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "failed to patch pod status") Consistently( @@ -975,7 +1101,8 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(Succeed(), "failed to get ephemeral runner pod") - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -983,10 +1110,10 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "Failed to update pod status") - return pod + return updatedPod } for i := range 5 { @@ -1065,13 +1192,14 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) - pod.Status.Phase = corev1.PodFailed - pod.Status.Reason = "Evicted" - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.Reason = "Evicted" + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{}, }) - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "failed to patch pod status") updated := new(v1alpha1.EphemeralRunner) @@ -1111,13 +1239,14 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunnerInterval, ).Should(BeEquivalentTo(true)) - pod.Status.Phase = corev1.PodFailed - pod.Status.Reason = "OutOfpods" - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.Phase = corev1.PodFailed + updatedPod.Status.Reason = "OutOfpods" + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{}, }) - err := k8sClient.Status().Update(ctx, pod) + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "failed to patch pod status") updated := new(v1alpha1.EphemeralRunner) @@ -1157,7 +1286,8 @@ var _ = Describe("EphemeralRunner", func() { ).Should(BeEquivalentTo(true)) // first set phase to running - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Running: &corev1.ContainerStateRunning{ @@ -1165,8 +1295,8 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - pod.Status.Phase = corev1.PodRunning - err := k8sClient.Status().Update(ctx, pod) + updatedPod.Status.Phase = corev1.PodRunning + err := k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil()) Eventually( @@ -1182,8 +1312,9 @@ var _ = Describe("EphemeralRunner", func() { ).Should(BeEquivalentTo(v1alpha1.EphemeralRunnerPhaseRunning)) // set phase to succeeded - pod.Status.Phase = corev1.PodSucceeded - err = k8sClient.Status().Update(ctx, pod) + nextPod := updatedPod.DeepCopy() + nextPod.Status.Phase = corev1.PodSucceeded + err = k8sClient.Status().Patch(ctx, nextPod, client.MergeFrom(updatedPod)) Expect(err).To(BeNil()) Consistently( @@ -1215,7 +1346,7 @@ var _ = Describe("EphemeralRunner", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New( mgr.GetClient(), scalefake.NewMultiClient( @@ -1244,7 +1375,7 @@ var _ = Describe("EphemeralRunner", func() { startManagers(GinkgoT(), mgr) }) - It("It should delete EphemeralRunner when pod exits successfully", func() { + It("It should mark EphemeralRunner as Succeeded and clean up resources when pod exits successfully", func() { ephemeralRunner := newExampleRunner("test-runner", autoscalingNS.Name, configSecret.Name) err := k8sClient.Create(ctx, ephemeralRunner) @@ -1258,7 +1389,8 @@ var _ = Describe("EphemeralRunner", func() { return true, nil }, ephemeralRunnerTimeout, ephemeralRunnerInterval).Should(BeEquivalentTo(true)) - pod.Status.ContainerStatuses = append(pod.Status.ContainerStatuses, corev1.ContainerStatus{ + updatedPod := pod.DeepCopy() + updatedPod.Status.ContainerStatuses = append(updatedPod.Status.ContainerStatuses, corev1.ContainerStatus{ Name: v1alpha1.EphemeralRunnerContainerName, State: corev1.ContainerState{ Terminated: &corev1.ContainerStateTerminated{ @@ -1266,22 +1398,44 @@ var _ = Describe("EphemeralRunner", func() { }, }, }) - err = k8sClient.Status().Update(ctx, pod) + err = k8sClient.Status().Patch(ctx, updatedPod, client.MergeFrom(pod)) Expect(err).To(BeNil(), "failed to update pod status") - updated := new(v1alpha1.EphemeralRunner) Eventually( - func() bool { - err := k8sClient.Get( + func() (bool, error) { + updated := new(v1alpha1.EphemeralRunner) + if err := k8sClient.Get( ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, updated, - ) + ); err != nil { + return kerrors.IsNotFound(err), client.IgnoreNotFound(err) + } + return updated.Status.Phase == v1alpha1.EphemeralRunnerPhaseSucceeded, nil + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner should be marked as succeeded or deleted") + + Eventually( + func() bool { + check := new(corev1.Pod) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) return kerrors.IsNotFound(err) }, ephemeralRunnerTimeout, ephemeralRunnerInterval, - ).Should(BeTrue()) + ).Should(BeTrue(), "Ephemeral runner pod should eventually be deleted") + + Eventually( + func() bool { + check := new(corev1.Secret) + err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, check) + return kerrors.IsNotFound(err) + }, + ephemeralRunnerTimeout, + ephemeralRunnerInterval, + ).Should(BeTrue(), "Ephemeral runner secret should eventually be deleted") }) }) @@ -1301,7 +1455,7 @@ var _ = Describe("EphemeralRunner", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient( scalefake.WithClient( scalefake.NewClient( @@ -1325,14 +1479,14 @@ var _ = Describe("EphemeralRunner", func() { It("uses an actions client with proxy transport", func() { // Use an actual client - controller.ResourceBuilder = ResourceBuilder{ + controller.ResourceBuilder = &ResourceBuilder{ SecretResolver: secretresolver.New( mgr.GetClient(), multiclient.NewScaleset(), ), } - proxySuccessfulllyCalled := false + var proxySuccessfulllyCalled atomic.Bool proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { header := r.Header.Get("Proxy-Authorization") Expect(header).NotTo(BeEmpty()) @@ -1342,7 +1496,7 @@ var _ = Describe("EphemeralRunner", func() { Expect(err).NotTo(HaveOccurred()) Expect(string(decoded)).To(Equal("test:password")) - proxySuccessfulllyCalled = true + proxySuccessfulllyCalled.Store(true) w.WriteHeader(http.StatusOK) })) GinkgoT().Cleanup(func() { @@ -1367,7 +1521,7 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunner.Spec.GitHubConfigURL = "http://example.com/org/repo" ephemeralRunner.Spec.Proxy = &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: proxy.URL, + URL: proxy.URL, CredentialSecretRef: "proxy-credentials", }, } @@ -1377,7 +1531,7 @@ var _ = Describe("EphemeralRunner", func() { Eventually( func() bool { - return proxySuccessfulllyCalled + return proxySuccessfulllyCalled.Load() }, 2*time.Second, ephemeralRunnerInterval, @@ -1388,10 +1542,10 @@ var _ = Describe("EphemeralRunner", func() { ephemeralRunner := newExampleRunner("test-runner", autoScalingNS.Name, configSecret.Name) ephemeralRunner.Spec.Proxy = &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://proxy.example.com:8080", + URL: "http://proxy.example.com:8080", }, HTTPS: &v1alpha1.ProxyServerConfig{ - Url: "http://proxy.example.com:8080", + URL: "http://proxy.example.com:8080", }, NoProxy: []string{"example.com"}, } @@ -1484,7 +1638,7 @@ var _ = Describe("EphemeralRunner", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient()), }, } @@ -1505,9 +1659,9 @@ var _ = Describe("EphemeralRunner", func() { certPath := filepath.Join(certsFolder, "server.crt") keyPath := filepath.Join(certsFolder, "server.key") - serverSuccessfullyCalled := false + var serverSuccessfullyCalled atomic.Bool server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - serverSuccessfullyCalled = true + serverSuccessfullyCalled.Store(true) w.WriteHeader(http.StatusOK) })) cert, err := tls.LoadX509KeyPair(certPath, keyPath) @@ -1518,7 +1672,7 @@ var _ = Describe("EphemeralRunner", func() { defer server.Close() // Use an actual client - controller.ResourceBuilder = ResourceBuilder{ + controller.ResourceBuilder = &ResourceBuilder{ SecretResolver: secretresolver.New( mgr.GetClient(), multiclient.NewScaleset(), @@ -1543,7 +1697,7 @@ var _ = Describe("EphemeralRunner", func() { Eventually( func() bool { - return serverSuccessfullyCalled + return serverSuccessfullyCalled.Load() }, 2*time.Second, ephemeralRunnerInterval, diff --git a/controllers/actions.github.com/ephemeralrunner_controller_unit_test.go b/controllers/actions.github.com/ephemeralrunner_controller_unit_test.go new file mode 100644 index 00000000..58c38637 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunner_controller_unit_test.go @@ -0,0 +1,139 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" + "sigs.k8s.io/controller-runtime/pkg/reconcile" +) + +func TestCleanupResourcesForKnownTerminalPodDeletesPodAndSecret(t *testing.T) { + t.Parallel() + + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + runner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner", + Namespace: "default", + }, + } + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: runner.Name, + Namespace: runner.Namespace, + }, + Status: corev1.PodStatus{Phase: corev1.PodSucceeded}, + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: runner.Name, + Namespace: runner.Namespace, + }, + } + + reconciler := &EphemeralRunnerReconciler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(pod, secret).Build(), + } + + require.NoError(t, reconciler.cleanupResourcesForPod(ctx, runner, pod, logr.Discard())) + + key := types.NamespacedName{Namespace: runner.Namespace, Name: runner.Name} + require.True(t, kerrors.IsNotFound(reconciler.Get(ctx, key, &corev1.Pod{}))) + require.True(t, kerrors.IsNotFound(reconciler.Get(ctx, key, &corev1.Secret{}))) +} + +func TestCleanupRunnerLinkedSecretsDeletesByRunnerPodLabel(t *testing.T) { + t.Parallel() + + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, corev1.AddToScheme(scheme)) + + runner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner", + Namespace: "default", + }, + } + linkedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "linked-secret", + Namespace: runner.Namespace, + Labels: map[string]string{ + "runner-pod": runner.Name, + }, + }, + } + unrelatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-secret", + Namespace: runner.Namespace, + Labels: map[string]string{ + "runner-pod": "other-runner", + }, + }, + } + otherNamespaceSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "linked-secret-other-namespace", + Namespace: "other-namespace", + Labels: map[string]string{ + "runner-pod": runner.Name, + }, + }, + } + + reconciler := &EphemeralRunnerReconciler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(linkedSecret, unrelatedSecret, otherNamespaceSecret).Build(), + } + + require.NoError(t, reconciler.cleanupRunnerLinkedSecrets(ctx, runner, logr.Discard())) + + require.True(t, kerrors.IsNotFound(reconciler.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: linkedSecret.Name}, &corev1.Secret{}))) + require.NoError(t, reconciler.Get(ctx, types.NamespacedName{Namespace: runner.Namespace, Name: unrelatedSecret.Name}, &corev1.Secret{})) + require.NoError(t, reconciler.Get(ctx, types.NamespacedName{Namespace: otherNamespaceSecret.Namespace, Name: otherNamespaceSecret.Name}, &corev1.Secret{})) +} + +func TestPodBecameCleanupCandidate(t *testing.T) { + t.Parallel() + + pendingPod := &corev1.Pod{Status: corev1.PodStatus{Phase: corev1.PodPending}} + runningPod := &corev1.Pod{Status: corev1.PodStatus{Phase: corev1.PodRunning}} + succeededPod := &corev1.Pod{Status: corev1.PodStatus{Phase: corev1.PodSucceeded}} + failedPod := &corev1.Pod{Status: corev1.PodStatus{Phase: corev1.PodFailed}} + + require.False(t, podBecameCleanupCandidate(pendingPod, runningPod)) + require.True(t, podBecameCleanupCandidate(runningPod, succeededPod)) + require.True(t, podBecameCleanupCandidate(runningPod, failedPod)) + require.False(t, podBecameCleanupCandidate(succeededPod, failedPod)) +} + +func TestPrioritizedWorkQueueAddsTerminalPodPriority(t *testing.T) { + t.Parallel() + + queue := priorityqueue.New[reconcile.Request]("test-prioritized-workqueue") + defer queue.ShutDown() + + request := reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "default", Name: "test-runner"}} + prioritizedWorkQueue{TypedRateLimitingInterface: queue, priority: terminalPodUpdatePriority}.Add(request) + + item, priority, shutdown := queue.GetWithPriority() + defer queue.Done(item) + + require.False(t, shutdown) + require.Equal(t, request, item) + require.Equal(t, terminalPodUpdatePriority, priority) +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller.go b/controllers/actions.github.com/ephemeralrunnerset_controller.go index 4a1ed903..c3636f29 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller.go @@ -24,20 +24,24 @@ import ( "maps" "sort" "strconv" + "sync" "time" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/controllers/actions.github.com/metrics" "github.com/actions/actions-runner-controller/controllers/actions.github.com/multiclient" + "github.com/actions/actions-runner-controller/github/actions" "github.com/actions/scaleset" "github.com/go-logr/logr" - "go.uber.org/multierr" corev1 "k8s.io/api/core/v1" kerrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -52,14 +56,29 @@ type EphemeralRunnerSetReconciler struct { Log logr.Logger Scheme *runtime.Scheme PublishMetrics bool - ResourceBuilder + *ResourceBuilder + specHashCache sync.Map + scaleStateCache sync.Map +} + +type specHashCacheEntry struct { + uid types.UID + generation int64 + hash string +} + +type scaleStateCacheEntry struct { + uid types.UID + patchID int + replicas int } // +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunnersets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunnersets/status,verbs=get;update;patch // +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunnersets/finalizers,verbs=update;patch -// +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunners,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunners,verbs=get;list;watch;create;update;patch;delete;deletecollection // +kubebuilder:rbac:groups=actions.github.com,resources=ephemeralrunners/status,verbs=get +// +kubebuilder:rbac:groups=core,resources=secrets,verbs=create;delete;get;list;patch;watch // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. @@ -78,9 +97,12 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R var ephemeralRunnerSet v1alpha1.EphemeralRunnerSet if err := r.Get(ctx, req.NamespacedName, &ephemeralRunnerSet); err != nil { + if kerrors.IsNotFound(err) { + r.clearSpecHashCache(req.NamespacedName) + r.clearScaleStateCache(req.NamespacedName) + } return ctrl.Result{}, client.IgnoreNotFound(err) } - original := ephemeralRunnerSet.DeepCopy() // Requested deletion does not need reconciled. if !ephemeralRunnerSet.DeletionTimestamp.IsZero() { @@ -110,7 +132,9 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } log.Info("Removing finalizer") - if controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + if controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + original := ephemeralRunnerSet.DeepCopy() + controllerutil.RemoveFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update ephemeral runner set with removed finalizer") return ctrl.Result{}, err @@ -118,12 +142,16 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } log.Info("Successfully removed finalizer after cleanup") + r.clearSpecHashCache(req.NamespacedName) + r.clearScaleStateCache(req.NamespacedName) return ctrl.Result{}, nil } // Add finalizer if not present - if controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { + if !controllerutil.ContainsFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) { log.Info("Adding finalizer") + original := ephemeralRunnerSet.DeepCopy() + controllerutil.AddFinalizer(&ephemeralRunnerSet, EphemeralRunnerSetFinalizerName) if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { log.Error(err, "Failed to update ephemeral runner set with new finalizer") return ctrl.Result{}, err @@ -133,34 +161,96 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, nil } + patchEphemeralRunnerSetProxySecretData := func(proxySecret *corev1.Secret) (bool, error) { + proxySecretData, err := ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy.ToSecretData(func(s string) (*corev1.Secret, error) { + secret := new(corev1.Secret) + err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: s}, secret) + return secret, err + }) + if err != nil { + return false, fmt.Errorf("failed to convert proxy config to secret data: %w", err) + } + + if maps.EqualFunc(proxySecret.Data, proxySecretData, bytes.Equal) { + return false, nil + } + + desiredProxySecret, err := r.newEphemeralRunnerSetProxySecret(&ephemeralRunnerSet, proxySecretData) + if err != nil { + return false, fmt.Errorf("failed to build ephemeralRunnerSet proxy secret: %w", err) + } + + updatedProxySecret := proxySecret.DeepCopy() + updatedProxySecret.Data = proxySecretData + updatedProxySecret.Labels = r.filterAndMergeLabels(proxySecret.Labels, desiredProxySecret.Labels) + updatedProxySecret.Annotations = r.filterAndMergeAnnotations(proxySecret.Annotations, desiredProxySecret.Annotations) + + log.Info("Updating ephemeralRunnerSet proxy secret") + if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(proxySecret)); err != nil { + return false, fmt.Errorf("failed to update ephemeralRunnerSet proxy secret: %w", err) + } + return true, nil + } + // If hash spec has changed, delete idle ephemeral runners // in order to apply the change to the runners that did not yet receive a job. - ephemeralRunnerIntegrityHash := ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet) - if ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] != ephemeralRunnerIntegrityHash { - log.Info("EphemeralRunnerSpec has changed, deleting idle ephemeral runners to apply the new spec") - if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { - log.Error(err, "Failed to clean up EphemeralRunners") - return ctrl.Result{}, err + storedIntegrityHash := ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash] + if !r.hasSpecHashCache(req.NamespacedName, ephemeralRunnerSet.UID, ephemeralRunnerSet.Generation, storedIntegrityHash) { + ephemeralRunnerIntegrityHash := ephemeralRunnerSetIntegrityHash(&ephemeralRunnerSet) + if storedIntegrityHash != ephemeralRunnerIntegrityHash { + log.Info("EphemeralRunnerSpec has changed, deleting idle ephemeral runners to apply the new spec") + if _, err := r.cleanUpEphemeralRunners(ctx, &ephemeralRunnerSet, log); err != nil { + log.Error(err, "Failed to clean up EphemeralRunners") + return ctrl.Result{}, err + } + + if ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy != nil { + var proxySecret corev1.Secret + err := r.Get( + ctx, + types.NamespacedName{ + Namespace: ephemeralRunnerSet.Namespace, + Name: proxyEphemeralRunnerSetSecretName(&ephemeralRunnerSet), + }, + &proxySecret, + ) + switch { + case err == nil: + updated, err := patchEphemeralRunnerSetProxySecretData(&proxySecret) + if err != nil { + return ctrl.Result{}, err + } + if updated { + return ctrl.Result{RequeueAfter: 1}, nil + } + case kerrors.IsNotFound(err): + log.Info("Creating a ephemeralRunnerSet proxy secret for the runner pods") + if err := r.createProxySecret(ctx, &ephemeralRunnerSet, log); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to create ephemeralRunnerSet proxy secret: %w", err) + } + default: + log.Error(err, "Unable to get ephemeralRunnerSet proxy secret", "namespace", ephemeralRunnerSet.Namespace, "name", proxyEphemeralRunnerSetSecretName(&ephemeralRunnerSet)) + return ctrl.Result{}, err + } + } + + log.Info("Updating EphemeralRunnerSet with new spec hash") + original := ephemeralRunnerSet.DeepCopy() + if ephemeralRunnerSet.Annotations == nil { + ephemeralRunnerSet.Annotations = make(map[string]string) + } + ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash] = ephemeralRunnerIntegrityHash + if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + log.Error(err, "Failed to update ephemeral runner set with new spec hash") + return ctrl.Result{}, err + } + r.setSpecHashCache(req.NamespacedName, ephemeralRunnerSet.UID, ephemeralRunnerSet.Generation, ephemeralRunnerIntegrityHash) + + log.Info("Updated ephemeral runner set with new spec hash") + return ctrl.Result{}, nil } - if _, _, err := r.reconcileEphemeralRunnerSetProxySecret(ctx, &ephemeralRunnerSet, log); err != nil { - log.Error(err, "Failed to update EphemeralRunnerSet proxy secret") - return ctrl.Result{}, err - } - - log.Info("Updating EphemeralRunnerSet with new spec hash") - original := ephemeralRunnerSet.DeepCopy() - if ephemeralRunnerSet.Annotations == nil { - ephemeralRunnerSet.Annotations = make(map[string]string) - } - ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerIntegrityHash - if err := r.Patch(ctx, &ephemeralRunnerSet, client.MergeFrom(original)); err != nil { - log.Error(err, "Failed to update ephemeral runner set with new spec hash") - return ctrl.Result{}, err - } - - log.Info("Updated ephemeral runner set with new spec hash") - return ctrl.Result{}, nil + r.setSpecHashCache(req.NamespacedName, ephemeralRunnerSet.UID, ephemeralRunnerSet.Generation, ephemeralRunnerIntegrityHash) } if ephemeralRunnerSet.Status.Phase == v1alpha1.EphemeralRunnerSetPhaseOutdated { @@ -171,12 +261,38 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, nil } - // Create or update proxy secret if needed - if _, updated, err := r.reconcileEphemeralRunnerSetProxySecret(ctx, &ephemeralRunnerSet, log); err != nil { - log.Error(err, "Unable to reconcile ephemeralRunnerSet proxy secret", "namespace", ephemeralRunnerSet.Namespace, "name", proxyEphemeralRunnerSetSecretName(&ephemeralRunnerSet)) - return ctrl.Result{}, err - } else if updated { - return ctrl.Result{RequeueAfter: 1 * time.Second}, nil + var proxySecret *corev1.Secret + if ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy != nil { + var secret corev1.Secret + err := r.Get( + ctx, + types.NamespacedName{ + Namespace: ephemeralRunnerSet.Namespace, + Name: proxyEphemeralRunnerSetSecretName(&ephemeralRunnerSet), + }, + &secret, + ) + proxySecret = &secret + switch { + case err == nil: + updated, err := patchEphemeralRunnerSetProxySecretData(proxySecret) + if err != nil { + return ctrl.Result{}, err + } + if updated { + return ctrl.Result{RequeueAfter: 1}, nil + } + case kerrors.IsNotFound(err): + // Create a compiled secret for the runner pods in the runnerset namespace + log.Info("Creating a ephemeralRunnerSet proxy secret for the runner pods") + if err := r.createProxySecret(ctx, &ephemeralRunnerSet, log); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to create ephemeralRunnerSet proxy secret: %w", err) + } + return ctrl.Result{}, nil + default: + log.Error(err, "Unable to get ephemeralRunnerSet proxy secret", "namespace", ephemeralRunnerSet.Namespace, "name", proxyEphemeralRunnerSetSecretName(&ephemeralRunnerSet)) + return ctrl.Result{}, err + } } // Find all EphemeralRunner with matching namespace and own by this EphemeralRunnerSet. @@ -191,34 +307,72 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R return ctrl.Result{}, err } - ephemeralRunnersByState := newEphemeralRunnersByStates(&ephemeralRunnerList) + runnerState := newEphemeralRunnersByStates(&ephemeralRunnerList, false, ephemeralRunnerSet.Spec.PatchID != 0) log.Info( "Ephemeral runner counts", - "outdated", len(ephemeralRunnersByState.outdated), - "pending", len(ephemeralRunnersByState.pending), - "running", len(ephemeralRunnersByState.running), - "finished", len(ephemeralRunnersByState.finished), - "failed", len(ephemeralRunnersByState.failed), - "deleting", len(ephemeralRunnersByState.deleting), + "outdated", runnerState.outdatedCount, + "pending", runnerState.pendingCount, + "running", runnerState.runningCount, + "finished", runnerState.finishedCount, + "failed", runnerState.failedCount, + "deleting", runnerState.deletingCount, ) - total := ephemeralRunnersByState.scaleTotal() - if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != ephemeralRunnersByState.latestPatchID { - defer func() { - if err := r.cleanupFinishedEphemeralRunners(ctx, ephemeralRunnersByState.finished, log); err != nil { - log.Error(err, "failed to cleanup finished ephemeral runners") + if r.PublishMetrics { + githubConfigURL := ephemeralRunnerSet.Spec.EphemeralRunnerSpec.GitHubConfigURL + parsedURL, err := actions.ParseGitHubConfigFromURL(githubConfigURL) + if err != nil { + log.Error(err, "Github Config URL is invalid", "URL", githubConfigURL) + // stop reconciling on this object + return ctrl.Result{}, nil + } + + metrics.SetEphemeralRunnerCountsByLifecycle( + metrics.CommonLabels{ + Name: ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName], + Namespace: ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace], + Repository: parsedURL.Repository, + Organization: parsedURL.Organization, + Enterprise: parsedURL.Enterprise, + }, + runnerState.pendingCount, + runnerState.runningCount, + runnerState.finishedCount, + runnerState.failedCount, + runnerState.outdatedCount, + runnerState.deletingCount, + ) + } + + total := runnerState.scaleTotal() + scaleState := r.getScaleState(req.NamespacedName, &ephemeralRunnerSet, total) + scalingTotal := total + if ephemeralRunnerSet.Spec.PatchID > 0 { + scalingTotal = scaleState.replicas + } + if ephemeralRunnerSet.Spec.PatchID == 0 || ephemeralRunnerSet.Spec.PatchID != runnerState.latestPatchID { + var fullState *ephemeralRunnersByState + getFullState := func() *ephemeralRunnersByState { + if fullState == nil { + fullState = newEphemeralRunnersByStates(&ephemeralRunnerList, true, ephemeralRunnerSet.Spec.PatchID != 0) } - }() - log.Info("Scaling comparison", "current", total, "desired", ephemeralRunnerSet.Spec.Replicas) + return fullState + } + + log.Info("Scaling comparison", "current", scalingTotal, "clusterCurrent", total, "desired", ephemeralRunnerSet.Spec.Replicas) switch { - case total < ephemeralRunnerSet.Spec.Replicas: // Handle scale up - count := ephemeralRunnerSet.Spec.Replicas - total + case scalingTotal < ephemeralRunnerSet.Spec.Replicas: // Handle scale up + count := ephemeralRunnerSet.Spec.Replicas - scalingTotal log.Info("Creating new ephemeral runners (scale up)", "count", count) if err := r.createEphemeralRunners(ctx, &ephemeralRunnerSet, count, log); err != nil { log.Error(err, "failed to make ephemeral runner") return ctrl.Result{}, err } + if ephemeralRunnerSet.Spec.PatchID > 0 { + scaleState.replicas += count + r.setScaleStateCache(req.NamespacedName, scaleState) + } case ephemeralRunnerSet.Spec.PatchID > 0 && total >= ephemeralRunnerSet.Spec.Replicas: // Handle scale down scenario. // If ephemeral runner did not yet update the phase to succeeded, but the scale down @@ -226,13 +380,14 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R // Eventually, the ephemeral runner will be cleaned up on the next patch request, which happens // on the next batch case ephemeralRunnerSet.Spec.PatchID == 0 && total > ephemeralRunnerSet.Spec.Replicas: + stateWithSlices := getFullState() count := total - ephemeralRunnerSet.Spec.Replicas log.Info("Deleting ephemeral runners (scale down)", "count", count) if err := r.deleteIdleEphemeralRunners( ctx, &ephemeralRunnerSet, - ephemeralRunnersByState.pending, - ephemeralRunnersByState.running, + stateWithSlices.pending, + stateWithSlices.running, count, log, ); err != nil { @@ -242,14 +397,98 @@ func (r *EphemeralRunnerSetReconciler) Reconcile(ctx context.Context, req ctrl.R } } - return ctrl.Result{}, r.updateStatus(ctx, &ephemeralRunnerSet, ephemeralRunnersByState, log) + if err := r.updateStatus(ctx, &ephemeralRunnerSet, runnerState, scaleState, log); err != nil { + return ctrl.Result{}, err + } + + if proxySecret != nil { + expectedScaleSetName := ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName] + expectedScaleSetNamespace := ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace] + labelsChanged := proxySecret.Labels[LabelKeyGitHubScaleSetName] != expectedScaleSetName || proxySecret.Labels[LabelKeyGitHubScaleSetNamespace] != expectedScaleSetNamespace + + if labelsChanged { + updatedProxySecret := proxySecret.DeepCopy() + updatedProxySecret.Labels = r.filterAndMergeLabels(proxySecret.Labels, map[string]string{ + LabelKeyGitHubScaleSetName: expectedScaleSetName, + LabelKeyGitHubScaleSetNamespace: expectedScaleSetNamespace, + }) + + log.Info("Updating ephemeralRunnerSet proxy secret metadata") + if err := r.Patch(ctx, updatedProxySecret, client.MergeFrom(proxySecret)); err != nil { + return ctrl.Result{}, fmt.Errorf("failed to update ephemeralRunnerSet proxy secret metadata: %w", err) + } + return ctrl.Result{RequeueAfter: 1}, nil + } + } + + return ctrl.Result{}, nil } -func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, log logr.Logger) error { - original := ephemeralRunnerSet.DeepCopy() +func (r *EphemeralRunnerSetReconciler) setSpecHashCache(namespacedName types.NamespacedName, uid types.UID, generation int64, hash string) { + r.specHashCache.Store(namespacedName, specHashCacheEntry{uid: uid, generation: generation, hash: hash}) +} + +func (r *EphemeralRunnerSetReconciler) hasSpecHashCache(namespacedName types.NamespacedName, uid types.UID, generation int64, hash string) bool { + entry, ok := r.specHashCache.Load(namespacedName) + if !ok { + return false + } + + specHashEntry, ok := entry.(specHashCacheEntry) + if !ok { + return false + } + + return specHashEntry.uid == uid && specHashEntry.generation == generation && specHashEntry.hash == hash +} + +func (r *EphemeralRunnerSetReconciler) clearSpecHashCache(namespacedName types.NamespacedName) { + r.specHashCache.Delete(namespacedName) +} + +func (r *EphemeralRunnerSetReconciler) setScaleStateCache(namespacedName types.NamespacedName, entry scaleStateCacheEntry) { + r.scaleStateCache.Store(namespacedName, entry) +} + +func (r *EphemeralRunnerSetReconciler) getScaleState(namespacedName types.NamespacedName, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, clusterReplicas int) scaleStateCacheEntry { + if ephemeralRunnerSet.Spec.PatchID == 0 { + return scaleStateCacheEntry{uid: ephemeralRunnerSet.UID, patchID: 0, replicas: clusterReplicas} + } + + entry, ok := r.scaleStateCache.Load(namespacedName) + if ok { + stateEntry, ok := entry.(scaleStateCacheEntry) + if ok && stateEntry.uid == ephemeralRunnerSet.UID && stateEntry.patchID == ephemeralRunnerSet.Spec.PatchID { + if stateEntry.replicas < clusterReplicas { + stateEntry.replicas = clusterReplicas + r.setScaleStateCache(namespacedName, stateEntry) + } + return stateEntry + } + } + + replicas := clusterReplicas + if ephemeralRunnerSet.Status.ReservedPatchID == ephemeralRunnerSet.Spec.PatchID && ephemeralRunnerSet.Status.ReservedReplicas > replicas { + replicas = ephemeralRunnerSet.Status.ReservedReplicas + } + if replicas > ephemeralRunnerSet.Spec.Replicas { + replicas = ephemeralRunnerSet.Spec.Replicas + } + + stateEntry := scaleStateCacheEntry{uid: ephemeralRunnerSet.UID, patchID: ephemeralRunnerSet.Spec.PatchID, replicas: replicas} + r.setScaleStateCache(namespacedName, stateEntry) + return stateEntry +} + +func (r *EphemeralRunnerSetReconciler) clearScaleStateCache(namespacedName types.NamespacedName) { + r.scaleStateCache.Delete(namespacedName) +} + +func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, state *ephemeralRunnersByState, scaleState scaleStateCacheEntry, log logr.Logger) error { + total := state.scaleTotal() var phase v1alpha1.EphemeralRunnerSetPhase switch { - case len(state.outdated) > 0: + case state.outdatedCount > 0: phase = v1alpha1.EphemeralRunnerSetPhaseOutdated case ephemeralRunnerSet.Status.Phase == "": phase = v1alpha1.EphemeralRunnerSetPhaseRunning @@ -257,37 +496,31 @@ func (r *EphemeralRunnerSetReconciler) updateStatus(ctx context.Context, ephemer phase = ephemeralRunnerSet.Status.Phase } desiredStatus := v1alpha1.EphemeralRunnerSetStatus{ - Phase: phase, + CurrentReplicas: total, + Phase: phase, + PendingEphemeralRunners: state.pendingCount, + RunningEphemeralRunners: state.runningCount, + FailedEphemeralRunners: state.failedCount, + } + if ephemeralRunnerSet.Spec.PatchID > 0 { + desiredStatus.ReservedReplicas = scaleState.replicas + desiredStatus.ReservedPatchID = scaleState.patchID } // Update the status if needed. if ephemeralRunnerSet.Status != desiredStatus { - ephemeralRunnerSet.Status = desiredStatus - if err := r.Status().Patch(ctx, ephemeralRunnerSet, client.MergeFrom(original)); err != nil { + updated := ephemeralRunnerSet.DeepCopy() + updated.Status = desiredStatus + if err := r.Status().Patch(ctx, updated, client.MergeFrom(ephemeralRunnerSet)); err != nil { log.Error(err, "Failed to update EphemeralRunnerSet status") return err } - log.Info("Updated EphemeralRunnerSet status", "status", ephemeralRunnerSet.Status) + log.Info("Updated EphemeralRunnerSet status", "status", updated.Status) } return nil } -func (r *EphemeralRunnerSetReconciler) cleanupFinishedEphemeralRunners(ctx context.Context, finishedEphemeralRunners []*v1alpha1.EphemeralRunner, log logr.Logger) error { - // cleanup finished runners and proceed - var errs []error - for i := range finishedEphemeralRunners { - log.Info("Deleting finished ephemeral runner", "name", finishedEphemeralRunners[i].Name) - if err := r.Delete(ctx, finishedEphemeralRunners[i]); err != nil { - if !kerrors.IsNotFound(err) { - errs = append(errs, err) - } - } - } - - return multierr.Combine(errs...) -} - func (r *EphemeralRunnerSetReconciler) cleanUpProxySecret(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) error { if ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy == nil { return nil @@ -324,53 +557,47 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte return true, nil } - ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList) + ephemeralRunnerState := newEphemeralRunnersByStates(ephemeralRunnerList, true, false) log.Info( "Clean up runner counts", - "pending", len(ephemeralRunnerState.pending), - "running", len(ephemeralRunnerState.running), - "finished", len(ephemeralRunnerState.finished), - "failed", len(ephemeralRunnerState.failed), - "deleting", len(ephemeralRunnerState.deleting), - "outdated", len(ephemeralRunnerState.outdated), + "pending", ephemeralRunnerState.pendingCount, + "running", ephemeralRunnerState.runningCount, + "finished", ephemeralRunnerState.finishedCount, + "failed", ephemeralRunnerState.failedCount, + "deleting", ephemeralRunnerState.deletingCount, + "outdated", ephemeralRunnerState.outdatedCount, ) - log.Info("Cleanup terminated ephemeral runners") - var errs []error - for _, ephemeralRunner := range ephemeralRunnerState.terminated() { - log.Info("Deleting ephemeral runner", "name", ephemeralRunner.Name) - if err := r.Delete(ctx, ephemeralRunner); err != nil && !kerrors.IsNotFound(err) { - errs = append(errs, err) - } - } - - if len(errs) > 0 { - mergedErrs := multierr.Combine(errs...) - log.Error(mergedErrs, "Failed to delete ephemeral runners") - return false, mergedErrs - } - // avoid fetching the client if we have nothing left to do if len(ephemeralRunnerState.running) == 0 && len(ephemeralRunnerState.pending) == 0 { + if ephemeralRunnerState.finishedCount+ephemeralRunnerState.failedCount+ephemeralRunnerState.outdatedCount > 0 { + log.Info("Cleanup terminated ephemeral runners") + if err := r.deleteAllEphemeralRunnersForSet(ctx, ephemeralRunnerSet, log); err != nil { + log.Error(err, "Failed to delete terminated ephemeral runners") + return false, err + } + } + return false, nil } - actionsClient, err := r.GetActionsService(ctx, ephemeralRunnerSet) - if err != nil { + log.Info("Cleanup terminated ephemeral runners") + if err := r.deleteEphemeralRunners(ctx, ephemeralRunnerState.finished, log); err != nil { + log.Error(err, "Failed to delete finished ephemeral runners") + return false, err + } + if err := r.deleteEphemeralRunners(ctx, ephemeralRunnerState.failed, log); err != nil { + log.Error(err, "Failed to delete failed ephemeral runners") + return false, err + } + if err := r.deleteEphemeralRunners(ctx, ephemeralRunnerState.outdated, log); err != nil { + log.Error(err, "Failed to delete outdated ephemeral runners") return false, err } log.Info("Cleanup pending or running ephemeral runners") - errs = errs[0:0] - for _, ephemeralRunner := range ephemeralRunnerState.pending { - log.Info("Removing the ephemeral runner from the service", "name", ephemeralRunner.Name) - _, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) - if err != nil { - errs = append(errs, err) - } - } - + candidates := append([]*v1alpha1.EphemeralRunner{}, ephemeralRunnerState.pending...) for _, ephemeralRunner := range ephemeralRunnerState.running { if ephemeralRunner.HasJob() { log.Info( @@ -381,18 +608,20 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunners(ctx context.Conte ) continue } - - log.Info("Removing the idle ephemeral runner from the service", "name", ephemeralRunner.Name) - _, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) - if err != nil { - errs = append(errs, err) - } + candidates = append(candidates, ephemeralRunner) + } + if len(candidates) == 0 { + return false, nil } - if len(errs) > 0 { - mergedErrs := multierr.Combine(errs...) - log.Error(mergedErrs, "Failed to remove ephemeral runners from the service") - return false, mergedErrs + actionsClient, err := r.GetActionsService(ctx, ephemeralRunnerSet) + if err != nil { + return false, err + } + + if _, err := r.deleteEphemeralRunnersWithActionsClient(ctx, candidates, actionsClient, log); err != nil { + log.Error(err, "Failed to remove ephemeral runners from the service") + return false, err } return false, nil @@ -436,98 +665,28 @@ func (r *EphemeralRunnerSetReconciler) cleanUpEphemeralRunnerSetProxySecret(ctx } } -func (r *EphemeralRunnerSetReconciler) reconcileEphemeralRunnerSetProxySecret(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) (secret *corev1.Secret, updated bool, err error) { - if ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy == nil { - return nil, false, nil - } - - var proxySecret corev1.Secret - err = r.Get( - ctx, - types.NamespacedName{ - Namespace: ephemeralRunnerSet.Namespace, - Name: proxyEphemeralRunnerSetSecretName(ephemeralRunnerSet), - }, - &proxySecret, - ) - switch { - case err == nil: - proxySecretData, err := ephemeralRunnerSet.Spec.EphemeralRunnerSpec.Proxy.ToSecretData(func(s string) (*corev1.Secret, error) { - secret := new(corev1.Secret) - err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunnerSet.Namespace, Name: s}, secret) - return secret, err - }) - if err != nil { - return nil, false, fmt.Errorf("failed to convert proxy config to secret data: %w", err) - } - - desiredRunnerSetProxy, err := r.newEphemeralRunnerSetProxySecret(ephemeralRunnerSet, proxySecretData) - if err != nil { - return nil, false, fmt.Errorf("failed to build desired ephemeralRunnerSet proxy secret: %w", err) - } - - updatedProxySecret := proxySecret.DeepCopy() - var shouldUpdate bool - if !maps.EqualFunc(proxySecret.Data, desiredRunnerSetProxy.Data, bytes.Equal) { - updatedProxySecret.Data = desiredRunnerSetProxy.Data - shouldUpdate = true - } - desiredLabels := r.filterAndMergeLabels(proxySecret.Labels, desiredRunnerSetProxy.Labels) - if !maps.Equal(proxySecret.Labels, desiredLabels) { - updatedProxySecret.Labels = desiredLabels - shouldUpdate = true - } - desiredAnnotations := r.mergeAnnotations(proxySecret.Annotations, desiredRunnerSetProxy.Annotations) - if !maps.Equal(proxySecret.Annotations, desiredAnnotations) { - updatedProxySecret.Annotations = desiredAnnotations - shouldUpdate = true - } - if shouldUpdate { - log.Info("Updating ephemeralRunnerSet proxy secret") - if err := r.Update(ctx, updatedProxySecret); err != nil { - return nil, false, fmt.Errorf("failed to update ephemeralRunnerSet proxy secret: %w", err) - } - return updatedProxySecret, true, nil - } - return &proxySecret, false, nil - case kerrors.IsNotFound(err): - // Create a compiled secret for the runner pods in the runnerset namespace - log.Info("Creating a ephemeralRunnerSet proxy secret for the runner pods") - if err := r.createProxySecret(ctx, ephemeralRunnerSet, log); err != nil { - return nil, false, fmt.Errorf("failed to create ephemeralRunnerSet proxy secret: %w", err) - } - return nil, false, nil - default: - return nil, false, err - } -} - // createEphemeralRunners provisions `count` number of v1alpha1.EphemeralRunner resources in the cluster. func (r *EphemeralRunnerSetReconciler) createEphemeralRunners(ctx context.Context, runnerSet *v1alpha1.EphemeralRunnerSet, count int, log logr.Logger) error { - // Track multiple errors at once and return the bundle. - errs := make([]error, 0) - for i := range count { + r.setSchemeIfUnset(r.Scheme) + for i := 0; i < count; i++ { ephemeralRunner, err := r.newEphemeralRunner(runnerSet) if err != nil { log.Error(err, "failed to build ephemeral runner") - errs = append(errs, err) - continue + return err } if runnerSet.Spec.EphemeralRunnerSpec.Proxy != nil { ephemeralRunner.Spec.ProxySecretRef = proxyEphemeralRunnerSetSecretName(runnerSet) } - log.Info("Creating new ephemeral runner", "progress", i+1, "total", count) if err := r.Create(ctx, ephemeralRunner); err != nil { log.Error(err, "failed to make ephemeral runner") - errs = append(errs, err) - continue + return err } - log.Info("Created new ephemeral runner", "runner", ephemeralRunner.Name) + log.Info("Created new ephemeral runner", "progress", i+1, "total", count, "runner", ephemeralRunner.Name) } - return multierr.Combine(errs...) + return nil } func (r *EphemeralRunnerSetReconciler) createProxySecret(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) error { @@ -572,12 +731,7 @@ func (r *EphemeralRunnerSetReconciler) deleteIdleEphemeralRunners(ctx context.Co log.Info("No pending or running ephemeral runners running at this time for scale down") return nil } - actionsClient, err := r.GetActionsService(ctx, ephemeralRunnerSet) - if err != nil { - return fmt.Errorf("failed to create actions client for ephemeral runner replica set: %w", err) - } - var errs []error - deletedCount := 0 + candidates := make([]*v1alpha1.EphemeralRunner, 0, runners.len()) for runners.next() { ephemeralRunner := runners.object() isDone := ephemeralRunner.IsDone() @@ -596,22 +750,75 @@ func (r *EphemeralRunnerSetReconciler) deleteIdleEphemeralRunners(ctx context.Co continue } - log.Info("Removing the idle ephemeral runner", "name", ephemeralRunner.Name) - ok, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) - if err != nil { - errs = append(errs, err) - } - if !ok { - continue + candidates = append(candidates, ephemeralRunner) + } + if len(candidates) == 0 { + return nil + } + + actionsClient, err := r.GetActionsService(ctx, ephemeralRunnerSet) + if err != nil { + return fmt.Errorf("failed to create actions client for ephemeral runner replica set: %w", err) + } + remaining := count + for _, ephemeralRunner := range candidates { + if remaining <= 0 { + return nil } - deletedCount++ - if deletedCount == count { - break + log.Info("Removing the ephemeral runner from the service", "name", ephemeralRunner.Name) + deleted, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) + if err != nil { + return err + } + if deleted { + remaining-- } } - return multierr.Combine(errs...) + return nil +} + +func (r *EphemeralRunnerSetReconciler) deleteAllEphemeralRunnersForSet(ctx context.Context, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, log logr.Logger) error { + log.Info("Deleting all ephemeral runners", "ephemeralRunnerSetUID", ephemeralRunnerSet.UID) + if err := r.DeleteAllOf( + ctx, + &v1alpha1.EphemeralRunner{}, + client.InNamespace(ephemeralRunnerSet.Namespace), + client.MatchingLabels{LabelKeyEphemeralRunnerSetUID: string(ephemeralRunnerSet.UID)}, + ); err != nil { + return err + } + + log.Info("Deleted all ephemeral runners", "ephemeralRunnerSetUID", ephemeralRunnerSet.UID) + return nil +} + +func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunners(ctx context.Context, ephemeralRunners []*v1alpha1.EphemeralRunner, log logr.Logger) error { + for _, ephemeralRunner := range ephemeralRunners { + log.Info("Deleting ephemeral runner", "name", ephemeralRunner.Name) + if err := r.Delete(ctx, ephemeralRunner); err != nil && !kerrors.IsNotFound(err) { + return err + } + } + + return nil +} + +func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunnersWithActionsClient(ctx context.Context, ephemeralRunners []*v1alpha1.EphemeralRunner, actionsClient multiclient.Client, log logr.Logger) (int, error) { + deletedCount := 0 + for _, ephemeralRunner := range ephemeralRunners { + log.Info("Removing the ephemeral runner from the service", "name", ephemeralRunner.Name) + ok, err := r.deleteEphemeralRunnerWithActionsClient(ctx, ephemeralRunner, actionsClient, log) + if err != nil { + return deletedCount, err + } + if ok { + deletedCount++ + } + } + + return deletedCount, nil } func (r *EphemeralRunnerSetReconciler) deleteEphemeralRunnerWithActionsClient(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, actionsClient multiclient.Client, log logr.Logger) (bool, error) { @@ -640,54 +847,134 @@ func (r *EphemeralRunnerSetReconciler) SetupWithManager(mgr ctrl.Manager, opts . return builderWithOptions( ctrl.NewControllerManagedBy(mgr). For(&v1alpha1.EphemeralRunnerSet{}). - Owns(&v1alpha1.EphemeralRunner{}). + Owns(&v1alpha1.EphemeralRunner{}, builder.WithPredicates(ephemeralRunnerSetChildPredicate())). WithEventFilter(predicate.ResourceVersionChangedPredicate{}), opts, ).Complete(r) } +func ephemeralRunnerSetChildPredicate() predicate.Predicate { + return predicate.Funcs{ + CreateFunc: func(event.CreateEvent) bool { + return true + }, + DeleteFunc: func(event.DeleteEvent) bool { + return true + }, + UpdateFunc: func(e event.UpdateEvent) bool { + return ephemeralRunnerUpdateAffectsScale(e.ObjectOld, e.ObjectNew) + }, + GenericFunc: func(event.GenericEvent) bool { + return false + }, + } +} + +func ephemeralRunnerUpdateAffectsScale(oldObj, newObj client.Object) bool { + newRunner, ok := newObj.(*v1alpha1.EphemeralRunner) + if !ok || newRunner == nil { + return false + } + + oldRunner, ok := oldObj.(*v1alpha1.EphemeralRunner) + if !ok || oldRunner == nil { + return true + } + + if oldRunner.Annotations[AnnotationKeyPatchID] != newRunner.Annotations[AnnotationKeyPatchID] { + return true + } + + if oldRunner.Status.Phase != newRunner.Status.Phase { + return true + } + + if oldRunner.Status.RunnerID != newRunner.Status.RunnerID { + return true + } + + if ephemeralRunnerIsDeleting(oldRunner) != ephemeralRunnerIsDeleting(newRunner) { + return true + } + + return ephemeralRunnerCountsTowardScale(oldRunner) != ephemeralRunnerCountsTowardScale(newRunner) +} + +func ephemeralRunnerIsDeleting(ephemeralRunner *v1alpha1.EphemeralRunner) bool { + deletionTimestamp := ephemeralRunner.GetDeletionTimestamp() + return deletionTimestamp != nil && !deletionTimestamp.IsZero() +} + +func ephemeralRunnerCountsTowardScale(ephemeralRunner *v1alpha1.EphemeralRunner) bool { + if ephemeralRunnerIsDeleting(ephemeralRunner) { + return false + } + + switch ephemeralRunner.Status.Phase { + case v1alpha1.EphemeralRunnerPhaseSucceeded, v1alpha1.EphemeralRunnerPhaseOutdated: + return false + default: + return true + } +} + type ephemeralRunnerStepper struct { - items []*v1alpha1.EphemeralRunner - index int + buckets [][]*v1alpha1.EphemeralRunner + bucketIndex int + itemIndex int } func newEphemeralRunnerStepper(primary []*v1alpha1.EphemeralRunner, othersOrdered ...[]*v1alpha1.EphemeralRunner) *ephemeralRunnerStepper { sort.Slice(primary, func(i, j int) bool { return primary[i].GetCreationTimestamp().Time.Before(primary[j].GetCreationTimestamp().Time) }) + + buckets := make([][]*v1alpha1.EphemeralRunner, 0, len(othersOrdered)+1) + buckets = append(buckets, primary) + for _, bucket := range othersOrdered { sort.Slice(bucket, func(i, j int) bool { return bucket[i].GetCreationTimestamp().Time.Before(bucket[j].GetCreationTimestamp().Time) }) - } - - for _, bucket := range othersOrdered { - primary = append(primary, bucket...) + buckets = append(buckets, bucket) } return &ephemeralRunnerStepper{ - items: primary, - index: -1, + buckets: buckets, + bucketIndex: 0, + itemIndex: -1, } } func (s *ephemeralRunnerStepper) next() bool { - if s.index+1 < len(s.items) { - s.index++ - return true + for s.bucketIndex < len(s.buckets) { + if s.itemIndex+1 < len(s.buckets[s.bucketIndex]) { + s.itemIndex++ + return true + } + + s.bucketIndex++ + s.itemIndex = -1 } + return false } func (s *ephemeralRunnerStepper) object() *v1alpha1.EphemeralRunner { - if s.index >= 0 && s.index < len(s.items) { - return s.items[s.index] + if s.bucketIndex >= 0 && s.bucketIndex < len(s.buckets) && s.itemIndex >= 0 && s.itemIndex < len(s.buckets[s.bucketIndex]) { + return s.buckets[s.bucketIndex][s.itemIndex] } + return nil } func (s *ephemeralRunnerStepper) len() int { - return len(s.items) + var total int + for i := range s.buckets { + total += len(s.buckets[i]) + } + + return total } type ephemeralRunnersByState struct { @@ -698,18 +985,61 @@ type ephemeralRunnersByState struct { deleting []*v1alpha1.EphemeralRunner outdated []*v1alpha1.EphemeralRunner + pendingCount int + runningCount int + finishedCount int + failedCount int + deletingCount int + outdatedCount int + latestPatchID int } -func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList) *ephemeralRunnersByState { +func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerList, includeSlices bool, trackLatestPatchID bool) *ephemeralRunnersByState { var ephemeralRunnerState ephemeralRunnersByState + latestPatchIDValue := "" for i := range ephemeralRunnerList.Items { r := &ephemeralRunnerList.Items[i] - patchID, err := strconv.Atoi(r.Annotations[AnnotationKeyPatchID]) - if err == nil && patchID > ephemeralRunnerState.latestPatchID { - ephemeralRunnerState.latestPatchID = patchID + if trackLatestPatchID { + latestPatchIDValue = trackLatestPatchIDValue(r.Annotations[AnnotationKeyPatchID], latestPatchIDValue, &ephemeralRunnerState.latestPatchID) } + if !r.DeletionTimestamp.IsZero() { + ephemeralRunnerState.deletingCount++ + continue + } + + switch r.Status.Phase { + case v1alpha1.EphemeralRunnerPhaseRunning: + ephemeralRunnerState.runningCount++ + case v1alpha1.EphemeralRunnerPhaseSucceeded: + ephemeralRunnerState.finishedCount++ + case v1alpha1.EphemeralRunnerPhaseFailed: + ephemeralRunnerState.failedCount++ + case v1alpha1.EphemeralRunnerPhaseOutdated: + ephemeralRunnerState.outdatedCount++ + default: + // Pending or no phase should be considered as pending. + // + // If field is not set, that means that the EphemeralRunner + // did not yet have chance to update the Status.Phase field. + ephemeralRunnerState.pendingCount++ + } + } + + if !includeSlices { + return &ephemeralRunnerState + } + + ephemeralRunnerState.pending = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.pendingCount) + ephemeralRunnerState.running = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.runningCount) + ephemeralRunnerState.finished = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.finishedCount) + ephemeralRunnerState.failed = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.failedCount) + ephemeralRunnerState.deleting = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.deletingCount) + ephemeralRunnerState.outdated = make([]*v1alpha1.EphemeralRunner, 0, ephemeralRunnerState.outdatedCount) + + for i := range ephemeralRunnerList.Items { + r := &ephemeralRunnerList.Items[i] if !r.DeletionTimestamp.IsZero() { ephemeralRunnerState.deleting = append(ephemeralRunnerState.deleting, r) continue @@ -725,20 +1055,27 @@ func newEphemeralRunnersByStates(ephemeralRunnerList *v1alpha1.EphemeralRunnerLi case v1alpha1.EphemeralRunnerPhaseOutdated: ephemeralRunnerState.outdated = append(ephemeralRunnerState.outdated, r) default: - // Pending or no phase should be considered as pending. - // - // If field is not set, that means that the EphemeralRunner - // did not yet have chance to update the Status.Phase field. ephemeralRunnerState.pending = append(ephemeralRunnerState.pending, r) } } + return &ephemeralRunnerState } -func (s *ephemeralRunnersByState) terminated() []*v1alpha1.EphemeralRunner { - return append(s.finished, append(s.failed, s.outdated...)...) +func trackLatestPatchIDValue(value, latestValue string, latestPatchID *int) string { + if value == "" || len(value) < len(latestValue) || (len(value) == len(latestValue) && value <= latestValue) { + return latestValue + } + + patchID, err := strconv.Atoi(value) + if err != nil || patchID <= *latestPatchID { + return latestValue + } + + *latestPatchID = patchID + return value } func (s *ephemeralRunnersByState) scaleTotal() int { - return len(s.pending) + len(s.running) + len(s.failed) + return s.pendingCount + s.runningCount + s.failedCount } diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_cache_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_cache_test.go new file mode 100644 index 00000000..4adc9675 --- /dev/null +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_cache_test.go @@ -0,0 +1,201 @@ +package actionsgithubcom + +import ( + "context" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + kerrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestEphemeralRunnerSetSpecHashCache_HitRequiresMatchingUID(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + + reconciler.setSpecHashCache(name, types.UID("uid-1"), 7, "hash-1") + + require.True(t, reconciler.hasSpecHashCache(name, types.UID("uid-1"), 7, "hash-1"), "expected cache hit for matching uid, generation, and hash") + + require.False(t, reconciler.hasSpecHashCache(name, types.UID("uid-2"), 7, "hash-1"), "expected cache miss when uid changes") +} + +func TestEphemeralRunnerSetSpecHashCache_ClearRemovesEntry(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + + reconciler.setSpecHashCache(name, types.UID("uid-1"), 3, "hash-3") + require.True(t, reconciler.hasSpecHashCache(name, types.UID("uid-1"), 3, "hash-3"), "expected cache hit before clear") + + reconciler.clearSpecHashCache(name) + require.False(t, reconciler.hasSpecHashCache(name, types.UID("uid-1"), 3, "hash-3"), "expected cache miss after clear") +} + +func TestEphemeralRunnerSetScaleStateCache_InitializesFromStatus(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + runnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{UID: types.UID("uid-1")}, + Spec: v1alpha1.EphemeralRunnerSetSpec{PatchID: 7, Replicas: 3}, + Status: v1alpha1.EphemeralRunnerSetStatus{ReservedPatchID: 7, ReservedReplicas: 3}, + } + + state := reconciler.getScaleState(name, runnerSet, 0) + require.Equal(t, runnerSet.UID, state.uid) + require.Equal(t, 7, state.patchID) + require.Equal(t, 3, state.replicas) +} + +func TestEphemeralRunnerSetScaleStateCache_UIDMismatchReinitializes(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + reconciler.setScaleStateCache(name, scaleStateCacheEntry{uid: types.UID("uid-1"), patchID: 7, replicas: 4}) + runnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{UID: types.UID("uid-2")}, + Spec: v1alpha1.EphemeralRunnerSetSpec{PatchID: 7, Replicas: 2}, + } + + state := reconciler.getScaleState(name, runnerSet, 2) + require.Equal(t, runnerSet.UID, state.uid) + require.Equal(t, 7, state.patchID) + require.Equal(t, 2, state.replicas) +} + +func TestEphemeralRunnerSetScaleStateCache_ClusterCountIsFloor(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + runnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{UID: types.UID("uid-1")}, + Spec: v1alpha1.EphemeralRunnerSetSpec{PatchID: 7, Replicas: 3}, + Status: v1alpha1.EphemeralRunnerSetStatus{ReservedPatchID: 7, ReservedReplicas: 1}, + } + + state := reconciler.getScaleState(name, runnerSet, 2) + require.Equal(t, 2, state.replicas, "expected cluster count to be a floor for reserved replicas") +} + +func TestEphemeralRunnerSetScaleStateCache_CapsAtDesiredReplicas(t *testing.T) { + t.Parallel() + + reconciler := &EphemeralRunnerSetReconciler{} + name := types.NamespacedName{Namespace: "default", Name: "test-ers"} + runnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{UID: types.UID("uid-1")}, + Spec: v1alpha1.EphemeralRunnerSetSpec{PatchID: 7, Replicas: 1}, + Status: v1alpha1.EphemeralRunnerSetStatus{ReservedPatchID: 7, ReservedReplicas: 3}, + } + + state := reconciler.getScaleState(name, runnerSet, 2) + require.Equal(t, 1, state.replicas, "expected reserved replicas to be capped by desired replicas") +} + +func TestEphemeralRunnerSetStateTracksLatestPatchID(t *testing.T) { + t.Parallel() + + runners := &v1alpha1.EphemeralRunnerList{ + Items: []v1alpha1.EphemeralRunner{ + {ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyPatchID: "7"}}}, + {ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyPatchID: "12"}}}, + {ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyPatchID: "8"}}}, + {ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{AnnotationKeyPatchID: "invalid"}}}, + }, + } + + state := newEphemeralRunnersByStates(runners, false, true) + require.Equal(t, 12, state.latestPatchID) +} + +func TestEphemeralRunnerUpdateAffectsScale(t *testing.T) { + t.Parallel() + + runningRunner := &v1alpha1.EphemeralRunner{Status: v1alpha1.EphemeralRunnerStatus{Phase: v1alpha1.EphemeralRunnerPhaseRunning}} + readyRunner := runningRunner.DeepCopy() + readyRunner.Status.Ready = true + succeededRunner := runningRunner.DeepCopy() + succeededRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded + failedRunner := runningRunner.DeepCopy() + failedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseFailed + deletingRunner := runningRunner.DeepCopy() + now := metav1.Now() + deletingRunner.DeletionTimestamp = &now + newPatchRunner := runningRunner.DeepCopy() + newPatchRunner.Annotations = map[string]string{AnnotationKeyPatchID: "9"} + + require.False(t, ephemeralRunnerUpdateAffectsScale(runningRunner, readyRunner), "expected readiness-only runner update to be ignored") + require.True(t, ephemeralRunnerUpdateAffectsScale(runningRunner, succeededRunner), "expected succeeded runner update to affect scale") + require.True(t, ephemeralRunnerUpdateAffectsScale(runningRunner, failedRunner), "expected failed runner update to refresh status counts") + require.True(t, ephemeralRunnerUpdateAffectsScale(runningRunner, deletingRunner), "expected deleting runner update to affect scale") + require.True(t, ephemeralRunnerUpdateAffectsScale(runningRunner, newPatchRunner), "expected patch ID update to affect scale") + registeredRunner := runningRunner.DeepCopy() + registeredRunner.Status.RunnerID = 1 + require.True(t, ephemeralRunnerUpdateAffectsScale(runningRunner, registeredRunner), "expected runner registration update to affect scale") +} + +func TestDeleteAllEphemeralRunnersForSetDeletesByUIDLabel(t *testing.T) { + t.Parallel() + + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, v1alpha1.AddToScheme(scheme)) + + runnerSet := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-runner-set", + Namespace: "default", + UID: types.UID("runner-set-uid"), + }, + } + linkedRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "linked-runner", + Namespace: runnerSet.Namespace, + Labels: map[string]string{ + LabelKeyEphemeralRunnerSetUID: string(runnerSet.UID), + }, + }, + } + unrelatedRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unrelated-runner", + Namespace: runnerSet.Namespace, + Labels: map[string]string{ + LabelKeyEphemeralRunnerSetUID: "other-runner-set-uid", + }, + }, + } + otherNamespaceRunner := &v1alpha1.EphemeralRunner{ + ObjectMeta: metav1.ObjectMeta{ + Name: "other-namespace-runner", + Namespace: "other-namespace", + Labels: map[string]string{ + LabelKeyEphemeralRunnerSetUID: string(runnerSet.UID), + }, + }, + } + + reconciler := &EphemeralRunnerSetReconciler{ + Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(linkedRunner, unrelatedRunner, otherNamespaceRunner).Build(), + } + + require.NoError(t, reconciler.deleteAllEphemeralRunnersForSet(ctx, runnerSet, logr.Discard())) + + err := reconciler.Get(ctx, types.NamespacedName{Namespace: linkedRunner.Namespace, Name: linkedRunner.Name}, &v1alpha1.EphemeralRunner{}) + require.True(t, kerrors.IsNotFound(err), "expected linked runner to be deleted, got %v", err) + require.NoError(t, reconciler.Get(ctx, types.NamespacedName{Namespace: unrelatedRunner.Namespace, Name: unrelatedRunner.Name}, &v1alpha1.EphemeralRunner{}), "expected unrelated runner to remain") + require.NoError(t, reconciler.Get(ctx, types.NamespacedName{Namespace: otherNamespaceRunner.Namespace, Name: otherNamespaceRunner.Name}, &v1alpha1.EphemeralRunner{}), "expected other namespace runner to remain") +} diff --git a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go index 8eb485d6..2eefd6e3 100644 --- a/controllers/actions.github.com/ephemeralrunnerset_controller_test.go +++ b/controllers/actions.github.com/ephemeralrunnerset_controller_test.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -54,7 +55,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), fake.NewMultiClient( fake.WithClient( fake.NewClient( @@ -136,9 +137,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // Check if the status stay 0 Consistently( func() (int, error) { - var runnerList v1alpha1.EphemeralRunnerList - err := k8sClient.List(ctx, &runnerList, client.InNamespace(ephemeralRunnerSet.Namespace), client.MatchingFields{resourceOwnerKey: ephemeralRunnerSet.Name}) - if err != nil { + runnerList := new(v1alpha1.EphemeralRunnerList) + if err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace); err != nil { return -1, err } return len(runnerList.Items), nil @@ -150,7 +150,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // Scaling up the EphemeralRunnerSet updated := created.DeepCopy() updated.Spec.Replicas = 5 - err := k8sClient.Update(ctx, updated) + err := k8sClient.Patch(ctx, updated, client.MergeFrom(created)) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") // Check if the number of ephemeral runners are created @@ -189,9 +189,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // Check if the status is updated Eventually( func() (int, error) { - var runnerList v1alpha1.EphemeralRunnerList - err := k8sClient.List(ctx, &runnerList, client.InNamespace(ephemeralRunnerSet.Namespace), client.MatchingFields{resourceOwnerKey: ephemeralRunnerSet.Name}) - if err != nil { + runnerList := new(v1alpha1.EphemeralRunnerList) + if err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace); err != nil { return -1, err } return len(runnerList.Items), nil @@ -211,7 +210,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // Scale up the EphemeralRunnerSet updated := created.DeepCopy() updated.Spec.Replicas = 5 - err = k8sClient.Update(ctx, updated) + err = k8sClient.Patch(ctx, updated, client.MergeFrom(created)) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") // Wait for the EphemeralRunnerSet to be scaled up @@ -452,13 +451,15 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[0])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + err = k8sClient.Delete(ctx, updatedRunner) + Expect(err).NotTo(HaveOccurred(), "failed to delete completed EphemeralRunner") updatedRunner = runnerList.Items[1].DeepCopy() updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[1])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") - // Keep the ephemeral runner until the next patch + // The completed runner deletes itself before the listener patch arrives. runnerList = new(v1alpha1.EphemeralRunnerList) Eventually( func() (int, error) { @@ -471,7 +472,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, - ).Should(BeEquivalentTo(2), "1 EphemeralRunner should be up") + ).Should(BeEquivalentTo(1), "1 EphemeralRunner should be up") // The listener was slower to patch the completed, but we should still have 1 running ers = new(v1alpha1.EphemeralRunnerSet) @@ -500,7 +501,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeEquivalentTo(1), "1 Ephemeral runner should be up") }) - It("Should keep finished ephemeral runners until patch id changes", func() { + It("Should not recreate completed ephemeral runners before patch id changes", func() { ers := new(v1alpha1.EphemeralRunnerSet) err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunnerSet.Name, Namespace: ephemeralRunnerSet.Namespace}, ers) Expect(err).NotTo(HaveOccurred(), "failed to get EphemeralRunnerSet") @@ -530,13 +531,15 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[0])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + err = k8sClient.Delete(ctx, updatedRunner) + Expect(err).NotTo(HaveOccurred(), "failed to delete completed EphemeralRunner") updatedRunner = runnerList.Items[1].DeepCopy() updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhasePending err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[1])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") - // confirm they are not deleted + // confirm the completed runner is not recreated before the next listener patch runnerList = new(v1alpha1.EphemeralRunnerList) Consistently( func() (int, error) { @@ -549,7 +552,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { }, 5*time.Second, ephemeralRunnerSetTestInterval, - ).Should(BeEquivalentTo(2), "2 EphemeralRunner should be created") + ).Should(BeEquivalentTo(1), "1 EphemeralRunner should remain") }) It("Should handle double scale up", func() { @@ -582,6 +585,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[0])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + err = k8sClient.Delete(ctx, updatedRunner) + Expect(err).NotTo(HaveOccurred(), "failed to delete completed EphemeralRunner") updatedRunner = runnerList.Items[1].DeepCopy() updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseRunning @@ -601,7 +606,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet") runnerList = new(v1alpha1.EphemeralRunnerList) - // We should have 3 runners, and have no Succeeded ones + // We should have 3 active runners; the completed runner deleted itself. Eventually( func() error { err := listEphemeralRunnersAndRemoveFinalizers(ctx, k8sClient, runnerList, ephemeralRunnerSet.Namespace) @@ -613,17 +618,21 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) } + succeeded := 0 for _, runner := range runnerList.Items { if runner.Status.Phase == v1alpha1.EphemeralRunnerPhaseSucceeded { - return fmt.Errorf("Runner %s is in Succeeded phase", runner.Name) + succeeded++ } } + if succeeded != 0 { + return fmt.Errorf("Expected 0 runners in Succeeded phase, got %d", succeeded) + } return nil }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, - ).Should(BeNil(), "3 EphemeralRunner should be created and none should be in Succeeded phase") + ).Should(BeNil(), "3 active EphemeralRunners should be created") }) It("Should handle scale down without removing pending runners", func() { @@ -656,6 +665,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[0])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + err = k8sClient.Delete(ctx, updatedRunner) + Expect(err).NotTo(HaveOccurred(), "failed to delete completed EphemeralRunner") updatedRunner = runnerList.Items[1].DeepCopy() updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhasePending @@ -681,15 +692,15 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { } } - if pending != 1 && succeeded != 1 { - return fmt.Errorf("Expected 1 runner in Pending and 1 in Succeeded, got %d in Pending and %d in Succeeded", pending, succeeded) + if pending != 1 || succeeded != 0 { + return fmt.Errorf("Expected 1 runner in Pending and 0 in Succeeded, got %d in Pending and %d in Succeeded", pending, succeeded) } return nil }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, - ).Should(BeNil(), "1 EphemeralRunner should be in Pending and 1 in Succeeded phase") + ).Should(BeNil(), "1 EphemeralRunner should be in Pending phase") // Scale down to 0, while 1 is still pending. This simulates the difference between the desired and actual state ers = new(v1alpha1.EphemeralRunnerSet) @@ -731,6 +742,8 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { updatedRunner.Status.Phase = v1alpha1.EphemeralRunnerPhaseSucceeded err = k8sClient.Status().Patch(ctx, updatedRunner, client.MergeFrom(&runnerList.Items[0])) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunner") + err = k8sClient.Delete(ctx, updatedRunner) + Expect(err).NotTo(HaveOccurred(), "failed to delete completed EphemeralRunner") Eventually( func() (int, error) { @@ -960,21 +973,25 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { return err } - if len(runnerList.Items) != 2 { - return fmt.Errorf("Expected 2 runners, got %d", len(runnerList.Items)) + if len(runnerList.Items) != 3 { + return fmt.Errorf("Expected 3 runners, got %d", len(runnerList.Items)) } + succeeded := 0 for _, runner := range runnerList.Items { if runner.Status.Phase == v1alpha1.EphemeralRunnerPhaseSucceeded { - return fmt.Errorf("Expected no runners in Succeeded phase, got one") + succeeded++ } } + if succeeded != 1 { + return fmt.Errorf("Expected 1 runner in Succeeded phase, got %d", succeeded) + } return nil }, ephemeralRunnerSetTestTimeout, ephemeralRunnerSetTestInterval, - ).Should(BeNil(), "2 EphemeralRunner should be created and none should be in Succeeded phase") + ).Should(BeNil(), "2 active EphemeralRunners should be created and 1 Succeeded EphemeralRunner should be retained") }) It("Should delete idle runners, keep busy runners, and create new runners when the spec changes", func() { @@ -1099,7 +1116,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { // Scale up the EphemeralRunnerSet updated := created.DeepCopy() updated.Spec.Replicas = 3 - err := k8sClient.Update(ctx, updated) + err := k8sClient.Patch(ctx, updated, client.MergeFrom(created)) Expect(err).NotTo(HaveOccurred(), "failed to update EphemeralRunnerSet replica count") runnerList := new(v1alpha1.EphemeralRunnerList) @@ -1165,7 +1182,7 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { refetch = true failedOriginal = empty[0] - failed := pendingOriginal.DeepCopy() + failed := failedOriginal.DeepCopy() failed.Status.RunnerID = 103 failed.Status.Phase = v1alpha1.EphemeralRunnerPhaseFailed @@ -1182,7 +1199,11 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeTrue(), "Failed to eventually update to one pending, one running and one failed") desiredStatus := v1alpha1.EphemeralRunnerSetStatus{ - Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + CurrentReplicas: 3, + PendingEphemeralRunners: 1, + RunningEphemeralRunners: 1, + FailedEphemeralRunners: 1, + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, } Eventually( func() (v1alpha1.EphemeralRunnerSetStatus, error) { @@ -1221,7 +1242,9 @@ var _ = Describe("Test EphemeralRunnerSet controller", func() { ).Should(BeEquivalentTo(1), "Failed to eventually scale down") desiredStatus = v1alpha1.EphemeralRunnerSetStatus{ - Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, + CurrentReplicas: 1, + FailedEphemeralRunners: 1, + Phase: v1alpha1.EphemeralRunnerSetPhaseRunning, } Eventually( @@ -1275,7 +1298,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } @@ -1313,11 +1336,11 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( RunnerScaleSetID: 100, Proxy: &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: "http://proxy.example.com", + URL: "http://proxy.example.com", CredentialSecretRef: secretCredentials.Name, }, HTTPS: &v1alpha1.ProxyServerConfig{ - Url: "https://proxy.example.com", + URL: "https://proxy.example.com", CredentialSecretRef: secretCredentials.Name, }, NoProxy: []string{"example.com", "example.org"}, @@ -1466,7 +1489,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( err := k8sClient.Create(ctx, secretCredentials) Expect(err).NotTo(HaveOccurred(), "failed to create secret credentials") - proxySuccessfulllyCalled := false + var proxySuccessfulllyCalled atomic.Bool proxy := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { header := r.Header.Get("Proxy-Authorization") Expect(header).NotTo(BeEmpty()) @@ -1476,7 +1499,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( Expect(err).NotTo(HaveOccurred()) Expect(string(decoded)).To(Equal("test:password")) - proxySuccessfulllyCalled = true + proxySuccessfulllyCalled.Store(true) w.WriteHeader(http.StatusOK) })) GinkgoT().Cleanup(func() { @@ -1496,7 +1519,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( RunnerScaleSetID: 100, Proxy: &v1alpha1.ProxyConfig{ HTTP: &v1alpha1.ProxyServerConfig{ - Url: proxy.URL, + URL: proxy.URL, CredentialSecretRef: "proxy-credentials", }, }, @@ -1548,7 +1571,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with proxy settings", func( Eventually( func() bool { - return proxySuccessfulllyCalled + return proxySuccessfulllyCalled.Load() }, 2*time.Second, ephemeralRunnerInterval, @@ -1593,7 +1616,7 @@ var _ = Describe("Test EphemeralRunnerSet controller with custom root CA", func( Client: mgr.GetClient(), Scheme: mgr.GetScheme(), Log: logf.Log, - ResourceBuilder: ResourceBuilder{ + ResourceBuilder: &ResourceBuilder{ SecretResolver: secretresolver.New(mgr.GetClient(), multiclient.NewScaleset()), }, } diff --git a/controllers/actions.github.com/helpers_test.go b/controllers/actions.github.com/helpers_test.go index b7989586..02cbb211 100644 --- a/controllers/actions.github.com/helpers_test.go +++ b/controllers/actions.github.com/helpers_test.go @@ -21,9 +21,7 @@ const defaultGitHubToken = "gh_token" func startManagers(t ginkgo.GinkgoTInterface, first manager.Manager, others ...manager.Manager) { for _, mgr := range append([]manager.Manager{first}, others...) { - if err := SetupIndexers(mgr); err != nil { - t.Fatalf("failed to setup indexers: %v", err) - } + require.NoError(t, SetupIndexers(mgr)) ctx, cancel := context.WithCancel(context.Background()) g, ctx := errgroup.WithContext(ctx) diff --git a/controllers/actions.github.com/multiclient/fake/client.go b/controllers/actions.github.com/multiclient/fake/client.go index 737092a8..4b194792 100644 --- a/controllers/actions.github.com/multiclient/fake/client.go +++ b/controllers/actions.github.com/multiclient/fake/client.go @@ -71,6 +71,13 @@ func WithRemoveRunner(err error) ClientOption { } } +// WithRemoveRunnerFunc configures a function to handle RemoveRunner calls dynamically. +func WithRemoveRunnerFunc(fn func(context.Context, int64) error) ClientOption { + return func(c *Client) { + c.removeRunnerFunc = fn + } +} + // WithGenerateJitRunnerConfig configures the result of GenerateJitRunnerConfig func WithGenerateJitRunnerConfig(result *scaleset.RunnerScaleSetJitRunnerConfig, err error) ClientOption { return func(c *Client) { @@ -121,6 +128,7 @@ type Client struct { systemInfo scaleset.SystemInfo createRunnerScaleSetFunc func(context.Context, *scaleset.RunnerScaleSet) (*scaleset.RunnerScaleSet, error) updateRunnerScaleSetFunc func(context.Context, int, *scaleset.RunnerScaleSet) (*scaleset.RunnerScaleSet, error) + removeRunnerFunc func(context.Context, int64) error getRunnerScaleSetResult struct { *scaleset.RunnerScaleSet @@ -204,6 +212,9 @@ func (c *Client) GetRunnerByName(ctx context.Context, runnerName string) (*scale } func (c *Client) RemoveRunner(ctx context.Context, runnerID int64) error { + if c.removeRunnerFunc != nil { + return c.removeRunnerFunc(ctx, runnerID) + } return c.removeRunnerResult.err } diff --git a/controllers/actions.github.com/resourcebuilder.go b/controllers/actions.github.com/resourcebuilder.go index fd456b56..c4773501 100644 --- a/controllers/actions.github.com/resourcebuilder.go +++ b/controllers/actions.github.com/resourcebuilder.go @@ -46,14 +46,14 @@ var commonLabelKeys = [...]string{ LabelKeyGitHubRepository, } -// annotationKeyIntegrityHash is used as a hash of the important fields +// AnnotationKeyIntegrityHash is used as a hash of the important fields // of each resource to determine if more drastic action should be taken. // // For example, annotations/labels are not something that should modify // the behavior of a resource, while the change in spec is. Therefore, // the spec hash should contain the spec fields in order to determine // modifications. -const annotationKeyIntegrityHash = "actions.github.com/integrity-hash" +const AnnotationKeyIntegrityHash = "actions.github.com/integrity-hash" const labelValueKubernetesPartOf = "gha-runner-scale-set" @@ -95,7 +95,8 @@ type SecretResolver interface { type ResourceBuilder struct { ExcludeLabelPropagationPrefixes []string SecretResolver - Scheme *runtime.Scheme + Scheme *runtime.Scheme + ResourceCache *ResourceCache } func (b *ResourceBuilder) setSchemeIfUnset(scheme *runtime.Scheme) { @@ -115,20 +116,46 @@ func (b *ResourceBuilder) setControllerReference(owner client.Object, object cli return ctrl.SetControllerReference(owner, object, b.Scheme) } +func autoscalingRunnerSetIntegrityHash(ars *v1alpha1.AutoscalingRunnerSet) string { + return hash.ComputeTemplateHash(&ars.Spec) +} + func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet, ephemeralRunnerSet *v1alpha1.EphemeralRunnerSet, namespace, image string, imagePullSecrets []corev1.LocalObjectReference) (*v1alpha1.AutoscalingListener, error) { runnerScaleSetID, err := strconv.Atoi(autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey]) if err != nil { return nil, err } + cacheKeyObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: scaleSetListenerName(autoscalingRunnerSet), + Namespace: namespace, + }, + } + inputDependency := resourceCacheInputObject("autoscaling-listener-inputs", struct { + Namespace string + Image string + ImagePullSecrets []corev1.LocalObjectReference + }{ + Namespace: namespace, + Image: image, + ImagePullSecrets: imagePullSecrets, + }) + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.autoscalingListener.Get(autoscalingRunnerSet, cacheKeyObject, ephemeralRunnerSet, inputDependency); ok { + return cached, nil + } + } + effectiveMinRunners := 0 + if autoscalingRunnerSet.Spec.MinRunners != nil { + effectiveMinRunners = *autoscalingRunnerSet.Spec.MinRunners + } + effectiveMaxRunners := math.MaxInt32 if autoscalingRunnerSet.Spec.MaxRunners != nil { effectiveMaxRunners = *autoscalingRunnerSet.Spec.MaxRunners } - if autoscalingRunnerSet.Spec.MinRunners != nil { - effectiveMinRunners = *autoscalingRunnerSet.Spec.MinRunners - } spec := v1alpha1.AutoscalingListenerSpec{ GitHubConfigURL: autoscalingRunnerSet.Spec.GitHubConfigUrl, @@ -165,12 +192,12 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. } annotations := map[string]string{ - annotationKeyIntegrityHash: spec.Hash(), + AnnotationKeyIntegrityHash: spec.Hash(), } if autoscalingRunnerSet.Spec.AutoscalingListenerMetadata != nil { labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) + annotations = b.filterAndMergeAnnotations(autoscalingRunnerSet.Spec.AutoscalingListenerMetadata.Annotations, annotations) } autoscalingListener := &v1alpha1.AutoscalingListener{ @@ -182,10 +209,22 @@ func (b *ResourceBuilder) newAutoscalingListener(autoscalingRunnerSet *v1alpha1. }, Spec: spec, } + if b.ResourceCache != nil { + b.ResourceCache.autoscalingListener.Upsert(autoscalingRunnerSet, autoscalingListener, ephemeralRunnerSet, inputDependency) + } return autoscalingListener, nil } +func resourceCacheInputObject(name string, value any) client.Object { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + ResourceVersion: hash.ComputeTemplateHash(value), + }, + } +} + type listenerMetricsServerConfig struct { addr string endpoint string @@ -278,7 +317,7 @@ func (b *ResourceBuilder) newScaleSetListenerConfig(autoscalingListener *v1alpha }, } - desiredSecret.Annotations[annotationKeyIntegrityHash] = scaleSetListenerConfigIntegrityHash(desiredSecret) + desiredSecret.Annotations[AnnotationKeyIntegrityHash] = scaleSetListenerConfigIntegrityHash(desiredSecret) if err := b.setControllerReference(autoscalingListener, desiredSecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener config secret: %w", err) @@ -307,6 +346,18 @@ func (b *ResourceBuilder) newScaleSetListenerPod( roleBinding *rbacv1.RoleBinding, metricsConfig *listenerMetricsServerConfig, ) (*corev1.Pod, error) { + cacheKeyObject := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + }, + } + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.listenerPod.Get(autoscalingListener, cacheKeyObject, podConfig, serviceAccount, role, roleBinding); ok { + return cached, nil + } + } + envs := []corev1.EnvVar{ { Name: "LISTENER_CONFIG_PATH", @@ -426,7 +477,7 @@ func (b *ResourceBuilder) newScaleSetListenerPod( Spec: podSpec, } - newRunnerScaleSetListenerPod.Annotations[annotationKeyIntegrityHash] = scaleSetListenerPodIntegrity( + newRunnerScaleSetListenerPod.Annotations[AnnotationKeyIntegrityHash] = scaleSetListenerPodIntegrity( newRunnerScaleSetListenerPod, autoscalingListener, podConfig, @@ -443,6 +494,9 @@ func (b *ResourceBuilder) newScaleSetListenerPod( if autoscalingListener.Spec.Template != nil { mergeListenerPodWithTemplate(newRunnerScaleSetListenerPod, autoscalingListener.Spec.Template) } + if b.ResourceCache != nil { + b.ResourceCache.listenerPod.Upsert(autoscalingListener, newRunnerScaleSetListenerPod, podConfig, serviceAccount, role, roleBinding) + } return newRunnerScaleSetListenerPod, nil } @@ -468,11 +522,11 @@ func scaleSetListenerPodIntegrity( d := data{ ListenerPodSpec: &pod.Spec, - AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[annotationKeyIntegrityHash], - ConfigSecretIntegrityHash: podConfig.Annotations[annotationKeyIntegrityHash], - ServiceAccountIntegrityHash: serviceAccount.Annotations[annotationKeyIntegrityHash], - RoleIntegrityHash: role.Annotations[annotationKeyIntegrityHash], - RoleBindingIntegrityHash: roleBinding.Annotations[annotationKeyIntegrityHash], + AutoscalingListenerIntegrityHash: autoscalingListener.Annotations[AnnotationKeyIntegrityHash], + ConfigSecretIntegrityHash: podConfig.Annotations[AnnotationKeyIntegrityHash], + ServiceAccountIntegrityHash: serviceAccount.Annotations[AnnotationKeyIntegrityHash], + RoleIntegrityHash: role.Annotations[AnnotationKeyIntegrityHash], + RoleBindingIntegrityHash: roleBinding.Annotations[AnnotationKeyIntegrityHash], MetricsConfig: metricsConfig, } @@ -597,6 +651,18 @@ func mergeListenerContainer(base, from *corev1.Container) { } func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener *v1alpha1.AutoscalingListener) (*corev1.ServiceAccount, error) { + cacheKeyObject := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Namespace, + }, + } + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.listenerServiceAccount.Get(autoscalingListener, cacheKeyObject); ok { + return cached, nil + } + } + base := &corev1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ Name: autoscalingListener.Name, @@ -611,14 +677,17 @@ func (b *ResourceBuilder) newScaleSetListenerServiceAccount(autoscalingListener if autoscalingListener.Spec.ServiceAccountMetadata != nil { base.Labels = b.filterAndMergeLabels(autoscalingListener.Spec.ServiceAccountMetadata.Labels, base.Labels) - base.Annotations = b.mergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, base.Annotations) + base.Annotations = b.filterAndMergeAnnotations(autoscalingListener.Spec.ServiceAccountMetadata.Annotations, base.Annotations) } - base.Annotations[annotationKeyIntegrityHash] = scaleSetListenerServiceAccountIntegrityHash(base) + base.Annotations[AnnotationKeyIntegrityHash] = scaleSetListenerServiceAccountIntegrityHash(base) if err := b.setControllerReference(autoscalingListener, base); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener service account: %w", err) } + if b.ResourceCache != nil { + b.ResourceCache.listenerServiceAccount.Upsert(autoscalingListener, base) + } return base, nil } @@ -640,6 +709,18 @@ func scaleSetListenerServiceAccountIntegrityHash(sa *corev1.ServiceAccount) stri } func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1.AutoscalingListener) *rbacv1.Role { + cacheKeyObject := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + }, + } + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.listenerRole.Get(autoscalingListener, cacheKeyObject); ok { + return cached + } + } + labels := b.filterAndMergeLabels(autoscalingListener.Labels, map[string]string{ LabelKeyGitHubScaleSetNamespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, LabelKeyGitHubScaleSetName: autoscalingListener.Spec.AutoscalingRunnerSetName, @@ -650,7 +731,7 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. annotations := make(map[string]string) if autoscalingListener.Spec.RoleMetadata != nil { labels = b.filterAndMergeLabels(autoscalingListener.Spec.RoleMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingListener.Spec.RoleMetadata.Annotations, nil) + annotations = b.filterAndMergeAnnotations(autoscalingListener.Spec.RoleMetadata.Annotations, nil) } newRole := &rbacv1.Role{ @@ -663,7 +744,10 @@ func (b *ResourceBuilder) newScaleSetListenerRole(autoscalingListener *v1alpha1. Rules: rulesForListenerRole([]string{autoscalingListener.Spec.EphemeralRunnerSetName}), } - newRole.Annotations[annotationKeyIntegrityHash] = scaleSetRoleIntegrityHash(newRole) + newRole.Annotations[AnnotationKeyIntegrityHash] = scaleSetRoleIntegrityHash(newRole) + if b.ResourceCache != nil { + b.ResourceCache.listenerRole.Upsert(autoscalingListener, newRole) + } return newRole } @@ -681,6 +765,18 @@ func scaleSetRoleIntegrityHash(role *rbacv1.Role) string { } func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1alpha1.AutoscalingListener, listenerRole *rbacv1.Role, serviceAccount *corev1.ServiceAccount) *rbacv1.RoleBinding { + cacheKeyObject := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingListener.Name, + Namespace: autoscalingListener.Spec.AutoscalingRunnerSetNamespace, + }, + } + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.listenerRoleBinding.Get(autoscalingListener, cacheKeyObject, listenerRole, serviceAccount); ok { + return cached + } + } + roleRef := rbacv1.RoleRef{ Kind: "Role", Name: listenerRole.Name, @@ -718,7 +814,10 @@ func (b *ResourceBuilder) newScaleSetListenerRoleBinding(autoscalingListener *v1 Subjects: subjects, } - newRoleBinding.Annotations[annotationKeyIntegrityHash] = scaleSetListenerRoleBindingIntegrityHash(newRoleBinding) + newRoleBinding.Annotations[AnnotationKeyIntegrityHash] = scaleSetListenerRoleBindingIntegrityHash(newRoleBinding) + if b.ResourceCache != nil { + b.ResourceCache.listenerRoleBinding.Upsert(autoscalingListener, newRoleBinding, listenerRole, serviceAccount) + } return newRoleBinding } @@ -743,6 +842,18 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A return nil, err } + cacheKeyObject := &v1alpha1.EphemeralRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: autoscalingRunnerSet.Name, + Namespace: autoscalingRunnerSet.Namespace, + }, + } + if b.ResourceCache != nil { + if cached, ok := b.ResourceCache.ephemeralRunnerSet.Get(autoscalingRunnerSet, cacheKeyObject); ok { + return cached, nil + } + } + spec := v1alpha1.EphemeralRunnerSetSpec{ Replicas: 0, EphemeralRunnerSpec: v1alpha1.EphemeralRunnerSpec{ @@ -777,7 +888,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A if autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata != nil { labels = b.filterAndMergeLabels(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Labels, labels) - annotations = b.mergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) + annotations = b.filterAndMergeAnnotations(autoscalingRunnerSet.Spec.EphemeralRunnerSetMetadata.Annotations, annotations) } newEphemeralRunnerSet := &v1alpha1.EphemeralRunnerSet{ @@ -791,11 +902,14 @@ func (b *ResourceBuilder) newEphemeralRunnerSet(autoscalingRunnerSet *v1alpha1.A Spec: spec, } - newEphemeralRunnerSet.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetIntegrityHash(newEphemeralRunnerSet) + newEphemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash] = ephemeralRunnerSetIntegrityHash(newEphemeralRunnerSet) if err := b.setControllerReference(autoscalingRunnerSet, newEphemeralRunnerSet); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set: %w", err) } + if b.ResourceCache != nil { + b.ResourceCache.ephemeralRunnerSet.Upsert(autoscalingRunnerSet, newEphemeralRunnerSet) + } return newEphemeralRunnerSet, nil } @@ -825,7 +939,7 @@ func (b *ResourceBuilder) newAutoscalingListenerProxySecret(autoscalingListener Data: data, } - newProxySecret.Annotations[annotationKeyIntegrityHash] = autoscalingListenerProxySecretIntegrityHash(newProxySecret) + newProxySecret.Annotations[AnnotationKeyIntegrityHash] = autoscalingListenerProxySecretIntegrityHash(newProxySecret) if err := b.setControllerReference(autoscalingListener, newProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for listener proxy secret: %w", err) @@ -857,8 +971,9 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme if ephemeralRunnerSet.Spec.EphemeralRunnerMetadata != nil { labels = b.filterAndMergeLabels(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Labels, labels) - annotations = b.mergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) + annotations = b.filterAndMergeAnnotations(ephemeralRunnerSet.Spec.EphemeralRunnerMetadata.Annotations, annotations) } + labels[LabelKeyEphemeralRunnerSetUID] = string(ephemeralRunnerSet.UID) ephemeralRunner := &v1alpha1.EphemeralRunner{ ObjectMeta: metav1.ObjectMeta{ @@ -866,10 +981,7 @@ func (b *ResourceBuilder) newEphemeralRunner(ephemeralRunnerSet *v1alpha1.Epheme Namespace: ephemeralRunnerSet.Namespace, Labels: labels, Annotations: annotations, - Finalizers: []string{ - ephemeralRunnerFinalizerName, - ephemeralRunnerActionsFinalizerName, - }, + Finalizers: []string{ephemeralRunnerFinalizerName}, }, Spec: ephemeralRunnerSet.Spec.EphemeralRunnerSpec, } @@ -992,7 +1104,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v Data: data, } - runnerPodProxySecret.Annotations[annotationKeyIntegrityHash] = ephemeralRunnerSetProxySecretZIdentityHash(runnerPodProxySecret) + runnerPodProxySecret.Annotations[AnnotationKeyIntegrityHash] = ephemeralRunnerSetProxySecretIdentityHash(runnerPodProxySecret) if err := b.setControllerReference(ephemeralRunnerSet, runnerPodProxySecret); err != nil { return nil, fmt.Errorf("failed to set controller reference for ephemeral runner set proxy secret: %w", err) @@ -1001,7 +1113,7 @@ func (b *ResourceBuilder) newEphemeralRunnerSetProxySecret(ephemeralRunnerSet *v return runnerPodProxySecret, nil } -func ephemeralRunnerSetProxySecretZIdentityHash(secret *corev1.Secret) string { +func ephemeralRunnerSetProxySecretIdentityHash(secret *corev1.Secret) string { type data struct { Data map[string][]byte `json:"data"` } @@ -1093,40 +1205,78 @@ func trimLabelValue(val string) string { return strings.Trim(val, "-_.") } +func (b *ResourceBuilder) filterLabels(k, v string) bool { + for _, prefix := range b.ExcludeLabelPropagationPrefixes { + if strings.HasPrefix(k, prefix) { + return true + } + } + return false +} + func (b *ResourceBuilder) filterAndMergeLabels(base, overwrite map[string]string) map[string]string { + return filterAndMergeMaps(base, overwrite, b.filterLabels) +} + +func filterAndMergeMaps(base, overwrite map[string]string, filter func(k, v string) bool) map[string]string { if base == nil && overwrite == nil { return nil } + var result map[string]string + if len(base) == 0 { + result = make(map[string]string) + } else { + result = maps.Clone(base) + } + if len(overwrite) > 0 { + maps.Copy(result, overwrite) + } + maps.DeleteFunc(result, filter) + return result +} - mergedLabels := make(map[string]string, len(base)) -base: - for k, v := range base { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue base - } - } - mergedLabels[k] = v +func (b *ResourceBuilder) filterAndMergeAnnotations(base, overwrite map[string]string) map[string]string { + if base == nil && overwrite == nil { + return nil + } + var result map[string]string + if len(base) == 0 { + result = make(map[string]string) + } else { + result = maps.Clone(base) } -overwrite: for k, v := range overwrite { - for _, prefix := range b.ExcludeLabelPropagationPrefixes { - if strings.HasPrefix(k, prefix) { - continue overwrite - } + if k == AnnotationKeyIntegrityHash { + continue } - mergedLabels[k] = v + result[k] = v } - return mergedLabels + return result } -func (b *ResourceBuilder) mergeAnnotations(base, overwrite map[string]string) map[string]string { - if base == nil && overwrite == nil { - return nil +// compareAnnotations compares two maps of annotations, ignoring the integrity hash annotation. +func (b *ResourceBuilder) annotationsEqual(m1, m2 map[string]string) bool { + l1 := len(m1) + if _, ok := m1[AnnotationKeyIntegrityHash]; !ok { + l1++ } - base = maps.Clone(base) - maps.Copy(base, overwrite) - return base + l2 := len(m2) + if _, ok := m2[AnnotationKeyIntegrityHash]; !ok { + l2++ + } + if l1 != l2 { + return false + } + + for k, v1 := range m1 { + if k == AnnotationKeyIntegrityHash { + continue + } + if v2, ok := m2[k]; !ok || v1 != v2 { + return false + } + } + return true } diff --git a/controllers/actions.github.com/resourcebuilder_test.go b/controllers/actions.github.com/resourcebuilder_test.go index d0885117..d67fd4a6 100644 --- a/controllers/actions.github.com/resourcebuilder_test.go +++ b/controllers/actions.github.com/resourcebuilder_test.go @@ -113,7 +113,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, labelValueKubernetesPartOf, ephemeralRunnerSet.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-set", ephemeralRunnerSet.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], ephemeralRunnerSet.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotEmpty(t, ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash]) assert.Equal(t, autoscalingRunnerSet.Name, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, ephemeralRunnerSet.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", ephemeralRunnerSet.Labels[LabelKeyGitHubEnterprise]) @@ -130,7 +130,7 @@ func TestMetadataPropagation(t *testing.T) { assert.Equal(t, labelValueKubernetesPartOf, listener.Labels[LabelKeyKubernetesPartOf]) assert.Equal(t, "runner-scale-set-listener", listener.Labels[LabelKeyKubernetesComponent]) assert.Equal(t, autoscalingRunnerSet.Labels[LabelKeyKubernetesVersion], listener.Labels[LabelKeyKubernetesVersion]) - assert.NotEmpty(t, ephemeralRunnerSet.Annotations[annotationKeyIntegrityHash]) + assert.NotEmpty(t, ephemeralRunnerSet.Annotations[AnnotationKeyIntegrityHash]) assert.Equal(t, autoscalingRunnerSet.Name, listener.Labels[LabelKeyGitHubScaleSetName]) assert.Equal(t, autoscalingRunnerSet.Namespace, listener.Labels[LabelKeyGitHubScaleSetNamespace]) assert.Equal(t, "", listener.Labels[LabelKeyGitHubEnterprise]) @@ -221,13 +221,13 @@ func TestEphemeralRunnerSetProxySecretZIdentityHash(t *testing.T) { }) require.NoError(t, err) - actualHash := proxySecret.Annotations[annotationKeyIntegrityHash] + actualHash := proxySecret.Annotations[AnnotationKeyIntegrityHash] assert.NotEmpty(t, actualHash) - assert.Equal(t, ephemeralRunnerSetProxySecretZIdentityHash(proxySecret), actualHash) + assert.Equal(t, ephemeralRunnerSetProxySecretIdentityHash(proxySecret), actualHash) changedProxySecret := proxySecret.DeepCopy() changedProxySecret.Data["http_proxy"] = []byte("http://updated-proxy.example.com") - assert.NotEqual(t, actualHash, ephemeralRunnerSetProxySecretZIdentityHash(changedProxySecret)) + assert.NotEqual(t, actualHash, ephemeralRunnerSetProxySecretIdentityHash(changedProxySecret)) } func TestGitHubURLTrimLabelValues(t *testing.T) { @@ -313,7 +313,7 @@ func TestOwnershipRelationships(t *testing.T) { runnerScaleSetIDAnnotationKey: "1", AnnotationKeyGitHubRunnerGroupName: "test-group", AnnotationKeyGitHubRunnerScaleSetName: "test-scale-set", - annotationKeyIntegrityHash: "test-hash", + AnnotationKeyIntegrityHash: "test-hash", }, }, Spec: v1alpha1.AutoscalingRunnerSetSpec{ diff --git a/controllers/actions.github.com/resourcecache.go b/controllers/actions.github.com/resourcecache.go new file mode 100644 index 00000000..2441f91c --- /dev/null +++ b/controllers/actions.github.com/resourcecache.go @@ -0,0 +1,230 @@ +package actionsgithubcom + +import ( + "reflect" + "slices" + "strings" + "sync" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/actions/actions-runner-controller/hash" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +var resourceCacheObjectTypes sync.Map + +type ResourceCacheObjectRef struct { + ObjectType string + Namespace string + Name string + UID types.UID + ResourceVersion string +} + +type ResourceCacheKey struct { + MainUID types.UID + Namespace string + Name string +} + +type ResourceCacheValue[T client.Object] struct { + MainObject ResourceCacheObjectRef + ResourceVersion string + Dependencies []ResourceCacheObjectRef + Object T +} + +type ResourceCache struct { + autoscalingListener *resourceCacheState[*v1alpha1.AutoscalingListener] + ephemeralRunnerSet *resourceCacheState[*v1alpha1.EphemeralRunnerSet] + listenerPod *resourceCacheState[*corev1.Pod] + listenerServiceAccount *resourceCacheState[*corev1.ServiceAccount] + listenerRole *resourceCacheState[*rbacv1.Role] + listenerRoleBinding *resourceCacheState[*rbacv1.RoleBinding] +} + +func NewResourceCache() ResourceCache { + return ResourceCache{ + autoscalingListener: newResourceCacheState[*v1alpha1.AutoscalingListener](), + ephemeralRunnerSet: newResourceCacheState[*v1alpha1.EphemeralRunnerSet](), + listenerPod: newResourceCacheState[*corev1.Pod](), + listenerServiceAccount: newResourceCacheState[*corev1.ServiceAccount](), + listenerRole: newResourceCacheState[*rbacv1.Role](), + listenerRoleBinding: newResourceCacheState[*rbacv1.RoleBinding](), + } +} + +type resourceCacheState[T client.Object] struct { + mu sync.RWMutex + entries map[ResourceCacheKey]ResourceCacheValue[T] +} + +func newResourceCacheState[T client.Object]() *resourceCacheState[T] { + return &resourceCacheState[T]{ + entries: make(map[ResourceCacheKey]ResourceCacheValue[T], 512), + } +} + +func (s *resourceCacheState[T]) Get( + mainObject client.Object, + desiredObject T, + dependencies ...client.Object, +) (T, bool) { + key := newResourceCacheKey(mainObject, desiredObject) + + s.mu.RLock() + value, ok := s.entries[key] + s.mu.RUnlock() + if !ok || !value.Matches(mainObject, dependencies...) { + var zero T + return zero, false + } + + return cloneResourceCacheObject(value.Object), true +} + +func (s *resourceCacheState[T]) Upsert( + mainObject client.Object, + desiredObject T, + dependencies ...client.Object, +) (ResourceCacheValue[T], bool) { + key := newResourceCacheKey(mainObject, desiredObject) + mainObjectRef := newResourceCacheObjectRef(mainObject) + resourceVersion := desiredObject.GetResourceVersion() + + s.mu.RLock() + previous, ok := s.entries[key] + if ok && previous.MainObject == mainObjectRef && previous.ResourceVersion == resourceVersion && previous.dependenciesMatch(dependencies...) { + s.mu.RUnlock() + return previous, false + } + s.mu.RUnlock() + + s.mu.Lock() + defer s.mu.Unlock() + + previous, ok = s.entries[key] + if ok && previous.MainObject == mainObjectRef && previous.ResourceVersion == resourceVersion && previous.dependenciesMatch(dependencies...) { + return previous, false + } + + dependencyRefs := newResourceCacheObjectRefs(dependencies...) + value := newResourceCacheValue(mainObjectRef, resourceVersion, dependencyRefs, cloneResourceCacheObject(desiredObject)) + s.entries[key] = value + return value, true +} + +func (v ResourceCacheValue[T]) Matches(mainObject client.Object, dependencies ...client.Object) bool { + if v.MainObject != newResourceCacheObjectRef(mainObject) { + return false + } + + return v.dependenciesMatch(dependencies...) +} + +func newResourceCacheKey(mainObject client.Object, desiredObject client.Object) ResourceCacheKey { + return ResourceCacheKey{ + MainUID: mainObject.GetUID(), + Namespace: desiredObject.GetNamespace(), + Name: resourceCacheObjectName(desiredObject), + } +} + +func newResourceCacheValue[T client.Object]( + mainObjectRef ResourceCacheObjectRef, + resourceVersion string, + dependencyRefs []ResourceCacheObjectRef, + object T, +) ResourceCacheValue[T] { + return ResourceCacheValue[T]{ + MainObject: mainObjectRef, + ResourceVersion: resourceVersion, + Dependencies: dependencyRefs, + Object: object, + } +} + +func cloneResourceCacheObject[T client.Object](object T) T { + return object.DeepCopyObject().(T) +} + +func newResourceCacheObjectRefs(objects ...client.Object) []ResourceCacheObjectRef { + refs := make([]ResourceCacheObjectRef, 0, len(objects)) + for _, object := range objects { + refs = append(refs, newResourceCacheObjectRef(object)) + } + slices.SortFunc(refs, func(a, b ResourceCacheObjectRef) int { + return compareResourceCacheObjectRefs(a, b) + }) + return refs +} + +func (v ResourceCacheValue[T]) dependenciesMatch(objects ...client.Object) bool { + if len(v.Dependencies) != len(objects) { + return false + } + + for _, object := range objects { + ref := newResourceCacheObjectRef(object) + if !slices.Contains(v.Dependencies, ref) { + return false + } + } + + return true +} + +func newResourceCacheObjectRef(object client.Object) ResourceCacheObjectRef { + resourceVersion := object.GetResourceVersion() + if resourceVersion == "" { + resourceVersion = hash.ComputeTemplateHash(object) + } + + return ResourceCacheObjectRef{ + ObjectType: resourceCacheObjectType(object), + Namespace: object.GetNamespace(), + Name: resourceCacheObjectName(object), + UID: object.GetUID(), + ResourceVersion: resourceVersion, + } +} + +func compareResourceCacheObjectRefs(a, b ResourceCacheObjectRef) int { + if c := strings.Compare(a.ObjectType, b.ObjectType); c != 0 { + return c + } + if c := strings.Compare(a.Namespace, b.Namespace); c != 0 { + return c + } + if c := strings.Compare(a.Name, b.Name); c != 0 { + return c + } + if c := strings.Compare(string(a.UID), string(b.UID)); c != 0 { + return c + } + return strings.Compare(a.ResourceVersion, b.ResourceVersion) +} + +func resourceCacheObjectType(object client.Object) string { + t := reflect.TypeOf(object) + if t.Kind() == reflect.Pointer { + t = t.Elem() + } + if objectType, ok := resourceCacheObjectTypes.Load(t); ok { + return objectType.(string) + } + + objectType := t.PkgPath() + "." + t.Name() + actual, _ := resourceCacheObjectTypes.LoadOrStore(t, objectType) + return actual.(string) +} + +func resourceCacheObjectName(object client.Object) string { + if object.GetName() != "" { + return object.GetName() + } + return object.GetGenerateName() +} diff --git a/controllers/actions.github.com/resourcecache_test.go b/controllers/actions.github.com/resourcecache_test.go new file mode 100644 index 00000000..36b2f5cc --- /dev/null +++ b/controllers/actions.github.com/resourcecache_test.go @@ -0,0 +1,312 @@ +package actionsgithubcom + +import ( + "fmt" + "testing" + + "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +var benchmarkEphemeralRunnerSetSink *v1alpha1.EphemeralRunnerSet + +func TestResourceCacheUpsertReplacesByDependencyResourceVersion(t *testing.T) { + mainObject := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "listener-uid", + ResourceVersion: "10", + }, + } + desiredPod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + ResourceVersion: "1", + Labels: map[string]string{ + "app": "listener", + }, + }, + } + configSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener-config", + Namespace: "controller-ns", + UID: "config-secret-uid", + ResourceVersion: "1", + }, + } + serviceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "service-account-uid", + ResourceVersion: "1", + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-uid", + ResourceVersion: "1", + }, + } + + cache := NewResourceCache() + value, replaced := cache.listenerPod.Upsert(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, replaced) + _, ok := cache.listenerPod.Get(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, ok) + assert.Equal(t, "1", value.ResourceVersion) + + _, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, role, configSecret, serviceAccount) + assert.False(t, replaced, "dependency ordering should not affect the cache value") + _, ok = cache.listenerPod.Get(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, ok) + + configSecret.ResourceVersion = "2" + value, replaced = cache.listenerPod.Upsert(mainObject, desiredPod, configSecret, serviceAccount, role) + assert.True(t, replaced) + assert.Contains(t, value.Dependencies, ResourceCacheObjectRef{ + ObjectType: resourceCacheObjectType(configSecret), + Namespace: "controller-ns", + Name: "listener-config", + UID: "config-secret-uid", + ResourceVersion: "2", + }) + + desiredPod.Labels["mutated"] = "after-cache" + cachedPod := value.Object + assert.NotContains(t, cachedPod.Labels, "mutated") +} + +func TestResourceBuilderCachesListenerPodDependencies(t *testing.T) { + listener := &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "listener-uid", + Annotations: map[string]string{ + AnnotationKeyIntegrityHash: "listener-hash", + }, + }, + Spec: v1alpha1.AutoscalingListenerSpec{ + Image: "listener:latest", + AutoscalingRunnerSetName: "scale-set", + AutoscalingRunnerSetNamespace: "scale-set-ns", + EphemeralRunnerSetName: "scale-set", + }, + } + podConfig := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener-config", + Namespace: "controller-ns", + UID: "config-secret-uid", + ResourceVersion: "11", + Annotations: map[string]string{ + AnnotationKeyIntegrityHash: "config-hash", + }, + }, + } + serviceAccount := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "controller-ns", + UID: "service-account-uid", + ResourceVersion: "12", + Annotations: map[string]string{ + AnnotationKeyIntegrityHash: "service-account-hash", + }, + }, + } + role := &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-uid", + ResourceVersion: "13", + Annotations: map[string]string{ + AnnotationKeyIntegrityHash: "role-hash", + }, + }, + } + roleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: "listener", + Namespace: "scale-set-ns", + UID: "role-binding-uid", + ResourceVersion: "14", + Annotations: map[string]string{ + AnnotationKeyIntegrityHash: "role-binding-hash", + }, + }, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + listenerPod, err := b.newScaleSetListenerPod(listener, podConfig, serviceAccount, role, roleBinding, nil) + require.NoError(t, err) + + cachedPod, ok := b.ResourceCache.listenerPod.Get(listener, listenerPod, podConfig, serviceAccount, role, roleBinding) + require.True(t, ok) + assert.IsType(t, &corev1.Pod{}, cachedPod) + + role.ResourceVersion = "changed" + _, ok = b.ResourceCache.listenerPod.Get(listener, listenerPod, podConfig, serviceAccount, role, roleBinding) + assert.False(t, ok) +} + +func TestResourceBuilderCachesEphemeralRunnerSet(t *testing.T) { + autoscalingRunnerSet := v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "scale-set", + Namespace: "default", + UID: "scale-set-uid", + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "1", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/actions/actions-runner-controller", + }, + } + + cache := NewResourceCache() + b := ResourceBuilder{ResourceCache: &cache} + runnerSet, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + + cachedRunnerSet, ok := b.ResourceCache.ephemeralRunnerSet.Get(&autoscalingRunnerSet, runnerSet) + require.True(t, ok) + assert.Equal(t, runnerSet.Spec, cachedRunnerSet.Spec) + + runnerSet.Labels["mutated"] = "after-cache" + assert.NotContains(t, cachedRunnerSet.Labels, "mutated") + + fromBuilder, err := b.newEphemeralRunnerSet(&autoscalingRunnerSet) + require.NoError(t, err) + assert.NotContains(t, fromBuilder.Labels, "mutated") + + autoscalingRunnerSet.Annotations[runnerScaleSetIDAnnotationKey] = "2" + _, ok = b.ResourceCache.ephemeralRunnerSet.Get(&autoscalingRunnerSet, runnerSet) + assert.False(t, ok) +} + +func BenchmarkNewEphemeralRunnerSetResourceCache(b *testing.B) { + autoscalingRunnerSet := newBenchmarkAutoscalingRunnerSet() + + b.Run("no_cache", func(b *testing.B) { + builder := ResourceBuilder{} + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + runnerSet, err := builder.newEphemeralRunnerSet(autoscalingRunnerSet) + if err != nil { + b.Fatal(err) + } + benchmarkEphemeralRunnerSetSink = runnerSet + } + }) + + b.Run("cache_hit", func(b *testing.B) { + cache := NewResourceCache() + builder := ResourceBuilder{ResourceCache: &cache} + if _, err := builder.newEphemeralRunnerSet(autoscalingRunnerSet); err != nil { + b.Fatal(err) + } + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + runnerSet, err := builder.newEphemeralRunnerSet(autoscalingRunnerSet) + if err != nil { + b.Fatal(err) + } + benchmarkEphemeralRunnerSetSink = runnerSet + } + }) + + b.Run("cache_miss", func(b *testing.B) { + cache := NewResourceCache() + builder := ResourceBuilder{ResourceCache: &cache} + autoscalingRunnerSet := autoscalingRunnerSet.DeepCopy() + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + autoscalingRunnerSet.ResourceVersion = fmt.Sprint(i) + runnerSet, err := builder.newEphemeralRunnerSet(autoscalingRunnerSet) + if err != nil { + b.Fatal(err) + } + benchmarkEphemeralRunnerSetSink = runnerSet + } + }) +} + +func newBenchmarkAutoscalingRunnerSet() *v1alpha1.AutoscalingRunnerSet { + return &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "benchmark-scale-set", + Namespace: "benchmark-namespace", + UID: "benchmark-scale-set-uid", + ResourceVersion: "1", + Labels: map[string]string{ + LabelKeyKubernetesVersion: "0.12.0", + "example.com/label-1": "value-1", + "example.com/label-2": "value-2", + }, + Annotations: map[string]string{ + runnerScaleSetIDAnnotationKey: "123", + AnnotationKeyGitHubRunnerGroupName: "benchmark-runner-group", + AnnotationKeyGitHubRunnerScaleSetName: "benchmark-scale-set", + }, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/actions/actions-runner-controller", + EphemeralRunnerSetMetadata: &v1alpha1.ResourceMeta{ + Labels: map[string]string{ + "example.com/runner-set-label": "runner-set-value", + }, + Annotations: map[string]string{ + "example.com/runner-set-annotation": "runner-set-value", + }, + }, + EphemeralRunnerMetadata: &v1alpha1.ResourceMeta{ + Labels: map[string]string{ + "example.com/runner-label": "runner-value", + }, + Annotations: map[string]string{ + "example.com/runner-annotation": "runner-value", + }, + }, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "example.com/template-label": "template-value", + }, + }, + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: v1alpha1.EphemeralRunnerContainerName, + Image: "ghcr.io/actions/actions-runner:latest", + Env: []corev1.EnvVar{ + {Name: "ACTIONS_RUNNER_REQUIRE_JOB_CONTAINER", Value: "false"}, + }, + }, + }, + }, + }, + }, + } +} diff --git a/controllers/actions.github.com/secretresolver/secret_resolver.go b/controllers/actions.github.com/secretresolver/secret_resolver.go index 5f3e9561..b9f9d9cc 100644 --- a/controllers/actions.github.com/secretresolver/secret_resolver.go +++ b/controllers/actions.github.com/secretresolver/secret_resolver.go @@ -85,9 +85,9 @@ func (sr *SecretResolver) GetActionsService(ctx context.Context, obj object.Acti } if proxy.HTTP != nil { - u, err := url.Parse(proxy.HTTP.Url) + u, err := url.Parse(proxy.HTTP.URL) if err != nil { - return nil, fmt.Errorf("failed to parse proxy http url %q: %w", proxy.HTTP.Url, err) + return nil, fmt.Errorf("failed to parse proxy http url %q: %w", proxy.HTTP.URL, err) } if ref := proxy.HTTP.CredentialSecretRef; ref != "" { @@ -101,9 +101,9 @@ func (sr *SecretResolver) GetActionsService(ctx context.Context, obj object.Acti } if proxy.HTTPS != nil { - u, err := url.Parse(proxy.HTTPS.Url) + u, err := url.Parse(proxy.HTTPS.URL) if err != nil { - return nil, fmt.Errorf("failed to parse proxy https url %q: %w", proxy.HTTPS.Url, err) + return nil, fmt.Errorf("failed to parse proxy https url %q: %w", proxy.HTTPS.URL, err) } if ref := proxy.HTTPS.CredentialSecretRef; ref != "" { diff --git a/controllers/actions.github.com/utils.go b/controllers/actions.github.com/utils.go index a77b24ba..da87eb7b 100644 --- a/controllers/actions.github.com/utils.go +++ b/controllers/actions.github.com/utils.go @@ -1,6 +1,8 @@ package actionsgithubcom import ( + "encoding/json" + "k8s.io/apimachinery/pkg/util/rand" ) @@ -25,3 +27,11 @@ func RandStringRunes(n int) string { } return string(b) } + +func mustJSON(v any) string { + val, err := json.Marshal(v) + if err != nil { + panic(err) + } + return string(val) +} diff --git a/main.go b/main.go index 80c047e6..dd31d247 100644 --- a/main.go +++ b/main.go @@ -299,10 +299,12 @@ func main() { secretresolver.WithLogger(slogLogger), ) - rb := actionsgithubcom.ResourceBuilder{ + resourceCache := actionsgithubcom.NewResourceCache() + rb := &actionsgithubcom.ResourceBuilder{ ExcludeLabelPropagationPrefixes: excludeLabelPropagationPrefixes, SecretResolver: secretResolver, Scheme: mgr.GetScheme(), + ResourceCache: &resourceCache, } log.Info("Resource builder initializing")