From f9a9737610b4fcf87206abb1aea4724784e6a962 Mon Sep 17 00:00:00 2001 From: Miguel Bernabeu Diaz Date: Wed, 10 Jan 2024 14:48:20 +0100 Subject: [PATCH] Schedulable archs flag (#68) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add `cluster-schedulable-archs` flag # Context In some cases we have seen that the common architectures of the images do not match the default preferred architecture. In those cases, one of the common architectures is selected, which may not match the available architectures in the cluster. In those cases, the Pod is always pending scheduling waiting for a node that will never exist. # Approach Allow passing a comma-separated list of architectures schedulable in the cluster. As we detect image's architecture support, we ignore those architectures that have not been specified as supported by the cluster. This new flag is optional and defaults to empty string (which disables the feature and keeps the prior behaviour). ## Startup fail if preferred architecture is not part of the supported architectures Startup will fail if the preferred architecture selected via commandline is not part of the list of schedulable architectures to prevent misconfigurations. ## Ignore preferred arch at Pod level if unsupported If a preferred architecture is indicated at Pod level, but it does not belong to the list of schedulable architectures, we ignore the preference and treat it as not specified. A log line is generated in that case. # Non-goal ## Detect architectures present in the cluster We do not want to prevent scheduling of Pods with architectures supported but which have no nodes currently in the cluster (downscaled) to avoid interfering with scale from 0 set ups. Change-Id: I0b85ab61fb3a267b41d938bd90f2e2d2050e5447 Co-authored-by: Fabián Sellés Rosa <1088313+Fsero@users.noreply.github.com> * fixup! Add `cluster-schedulable-archs` flag * fixup! Add `cluster-schedulable-archs` flag --------- Co-authored-by: Fabián Sellés Rosa <1088313+Fsero@users.noreply.github.com> --- README.md | 12 ++++- charts/noe/templates/webhook.yaml | 1 + charts/noe/values.yaml | 1 + cmd/noe/main.go | 22 ++++++-- cmd/noe/main_test.go | 2 +- pkg/arch/hook.go | 39 +++++++++++--- pkg/arch/hook_test.go | 87 +++++++++++++++++++++++++++++++ 7 files changed, 151 insertions(+), 13 deletions(-) diff --git a/README.md b/README.md index b600264..b541f64 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ spec: targetRevision: HEAD ``` -## Hinting preferred target architecture +## Hinting preferred and supported target architectures By default, Noe will automatically select the appropriate architecture when only one is supported by all the containers in the Pod. If more than one is available, Noe will select the system-defined preferred one if available. This preference can be chosen in the command line for Noe (defaults to `amd64` if unspecified): @@ -49,3 +49,13 @@ labels: ``` Noe will always prioritize a running Pod, so if the preference is not supported by all the containers in the Pod, the common architecture will be selected. + +You can restrict the acceptable common architectures in the command line for Noe: +``` +./noe -cluster-schedulable-archs amd64,arm64 +``` + +If you specify both a preferred architecture and a list of supported architectures in the command line, the default architecture must be part of the list. Otherwise Noe will fail to start. + +If a preferred architecture is specified at the Pod level and is not compatible with the supported architectures listed in the command line, it will be ignored. + diff --git a/charts/noe/templates/webhook.yaml b/charts/noe/templates/webhook.yaml index 7e74072..6ca46e1 100644 --- a/charts/noe/templates/webhook.yaml +++ b/charts/noe/templates/webhook.yaml @@ -55,6 +55,7 @@ spec: workingDir: /workdir args: - registry-proxies={{ .Values.proxies | join "," }} + - cluster-schedulable-archs={{ .Values.schedulableArchitectures | join "," }} ports: - containerPort: 8443 name: webhook diff --git a/charts/noe/values.yaml b/charts/noe/values.yaml index 078f018..ff64914 100644 --- a/charts/noe/values.yaml +++ b/charts/noe/values.yaml @@ -6,6 +6,7 @@ image: registry: ghcr.io repository: adevinta/noe tag: latest +schedulableArchitectures: [] proxies: [] # - docker.io=docker-proxy.company.corp # - quay.io=quay-proxy.company.corp diff --git a/cmd/noe/main.go b/cmd/noe/main.go index 3c7cdc0..fcfe942 100644 --- a/cmd/noe/main.go +++ b/cmd/noe/main.go @@ -3,7 +3,10 @@ package main import ( "context" "flag" + "fmt" "os" + "slices" + "strings" "time" "github.com/adevinta/noe/pkg/httputils" @@ -22,21 +25,33 @@ import ( ) func main() { - var preferredArch, systemOS string + var preferredArch, schedulableArchs, systemOS string var metricsAddr string var registryProxies, matchNodeLabels string flag.StringVar(&preferredArch, "preferred-arch", "amd64", "Preferred architecture when placing pods") + flag.StringVar(&schedulableArchs, "cluster-schedulable-archs", "", "Comma separated list of architectures schedulable in the cluster") flag.StringVar(&systemOS, "system-os", "linux", "Sole OS supported by the system") flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.") flag.StringVar(&metricsAddr, "registry-proxies", "", "Proxies to substitute in the registry URL in the form of docker.io=docker-proxy.company.corp,quay.io=quay-proxy.company.corp") flag.StringVar(&matchNodeLabels, "match-node-labels", "", "A set of pod label keys to match against node labels in the form of key1,key2") flag.Parse() - Main(signals.SetupSignalHandler(), "./", preferredArch, systemOS, metricsAddr, registryProxies, matchNodeLabels) + Main(signals.SetupSignalHandler(), "./", preferredArch, schedulableArchs, systemOS, metricsAddr, registryProxies, matchNodeLabels) } -func Main(ctx context.Context, certDir, preferredArch, systemOS, metricsAddr, registryProxies, matchNodeLabels string) { +func Main(ctx context.Context, certDir, preferredArch, schedulableArchs, systemOS, metricsAddr, registryProxies, matchNodeLabels string) { + var schedulableArchSlice []string + + if schedulableArchs != "" { + schedulableArchSlice = strings.Split(schedulableArchs, ",") + } + + if preferredArch != "" && schedulableArchs != "" && !slices.Contains(schedulableArchSlice, preferredArch) { + err := fmt.Errorf("preferred architecture is not schedulable in the cluster") + log.DefaultLogger.WithError(err).Error("refusing to continue") + os.Exit(1) + } // Setup a Manager log.DefaultLogger.WithContext(ctx).Println("setting up manager") @@ -90,6 +105,7 @@ func Main(ctx context.Context, certDir, preferredArch, systemOS, metricsAddr, re containerRegistry, arch.WithMetricsRegistry(metrics.Registry), arch.WithArchitecture(preferredArch), + arch.WithSchedulableArchitectures(schedulableArchSlice), arch.WithOS(systemOS), arch.WithDecoder(decoder), arch.WithMatchNodeLabels(arch.ParseMatchNodeLabels(matchNodeLabels)), diff --git a/cmd/noe/main_test.go b/cmd/noe/main_test.go index 2ac1163..0a42861 100644 --- a/cmd/noe/main_test.go +++ b/cmd/noe/main_test.go @@ -105,7 +105,7 @@ func TestNoeMain(t *testing.T) { require.NoError(t, err) os.Setenv("KUBECONFIG", env.KubeconfigFile()) - go Main(ctx, "./integration-tests/", "amd64", "linux", ":8181", "", "") + go Main(ctx, "./integration-tests/", "amd64", "", "linux", ":8181", "", "") client := http.Client{ Transport: &http.Transport{ diff --git a/pkg/arch/hook.go b/pkg/arch/hook.go index 14ef598..e4099da 100644 --- a/pkg/arch/hook.go +++ b/pkg/arch/hook.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "slices" "sort" "strings" "time" @@ -110,13 +111,14 @@ func (w warning) Error() string { type HandlerOption func(*Handler) type Handler struct { - Client client.Client - Registry Registry - matchNodeLabels []string - metrics HandlerMetrics - decoder *admission.Decoder - preferredArchitecture string - systemOS string + Client client.Client + Registry Registry + matchNodeLabels []string + metrics HandlerMetrics + decoder *admission.Decoder + preferredArchitecture string + schedulableArchitectures []string + systemOS string } func NewHandler(client client.Client, registry Registry, opts ...HandlerOption) *Handler { @@ -141,6 +143,12 @@ func WithArchitecture(arch string) HandlerOption { } } +func WithSchedulableArchitectures(archs []string) HandlerOption { + return func(h *Handler) { + h.schedulableArchitectures = archs + } +} + func WithOS(os string) HandlerOption { return func(h *Handler) { h.systemOS = os @@ -295,12 +303,16 @@ func (h *Handler) updatePodSpec(ctx context.Context, namespace string, podLabels var preferredArchIsDefault bool preferredArch, preferredArchDefined := podLabels["arch.noe.adevinta.com/preferred"] + if preferredArch != "" && !h.isArchSupported(preferredArch) { + log.DefaultLogger.WithContext(ctx).WithField("preferredArch", preferredArch).Println("ignoring unsupported user preferred architecture") + preferredArch = "" + } if preferredArch == "" && h.preferredArchitecture != "" { preferredArch = h.preferredArchitecture preferredArchDefined = true preferredArchIsDefault = true ctx = log.AddLogFieldsToContext(ctx, logrus.Fields{"preferredArch": preferredArch}) - log.DefaultLogger.WithContext(ctx).Println("selecting preferred architecture") + log.DefaultLogger.WithContext(ctx).Println("selecting default preferred architecture") } commonArchitectures := map[string]struct{}{} @@ -325,6 +337,10 @@ func (h *Handler) updatePodSpec(ctx context.Context, namespace string, podLabels log.DefaultLogger.WithContext(ctx).WithField("os", platform.OS).Info("Skipped OS does not match system's") continue } + if !h.isArchSupported(platform.Architecture) { + log.DefaultLogger.WithContext(ctx).WithField("arch", platform.OS).Info("Skipped arch does not match system's") + continue + } imageArchitectures[platform.Architecture] = struct{}{} } @@ -498,6 +514,13 @@ func (h *Handler) Handle(ctx context.Context, req admission.Request) admission.R return resp } +func (h *Handler) isArchSupported(arch string) bool { + if len(h.schedulableArchitectures) == 0 { + return true + } + return slices.Contains(h.schedulableArchitectures, arch) +} + // Handler implements admission.DecoderInjector. // A decoder will be automatically injected. diff --git a/pkg/arch/hook_test.go b/pkg/arch/hook_test.go index 42d0833..e06c07f 100644 --- a/pkg/arch/hook_test.go +++ b/pkg/arch/hook_test.go @@ -365,6 +365,48 @@ func TestHookSucceedsWhenPreferredArchUnavailable(t *testing.T) { ) } +func TestHookSucceedsWhenPreferredArchUnschedulable(t *testing.T) { + resp := runWebhookTest( + t, + NewHandler( + fake.NewClientBuilder().Build(), + RegistryFunc(func(ctx context.Context, imagePullSecret, image string) ([]registry.Platform, error) { + assert.Equal(t, "ubuntu", image) + return []registry.Platform{ + {OS: "linux", Architecture: "amd64"}, + {OS: "linux", Architecture: "arm64"}, + {OS: "windows", Architecture: "amd64"}, + }, nil + }), + WithOS("linux"), + WithSchedulableArchitectures([]string{"amd64"}), + ), + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "object", + Labels: map[string]string{"arch.noe.adevinta.com/preferred": "arm64"}, + }, + + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Image: "ubuntu", + }, + }, + }, + }, + ) + assert.True(t, resp.Allowed) + assert.Equal(t, http.StatusOK, int(resp.Result.Code)) + require.Len(t, resp.Patches, 1) + assert.Contains( + t, + resp.Patches, + archNodeSelectorPatchForArchs("amd64"), + ) +} + func TestHookHonorsDefaultPreferredArch(t *testing.T) { resp := runWebhookTest( t, @@ -567,6 +609,51 @@ func TestHookRejectsMultipleImagesWithNoCommonArch(t *testing.T) { assert.Len(t, resp.Patch, 0) } +func TestHookRejectsMultipleImagesWhenCommonArchIsUnschedulable(t *testing.T) { + resp := runWebhookTest( + t, + NewHandler( + fake.NewClientBuilder().Build(), + RegistryFunc(func(ctx context.Context, imagePullSecret, image string) ([]registry.Platform, error) { + switch image { + case "ubuntu": + return []registry.Platform{ + {OS: "linux", Architecture: "arm64"}, + {OS: "windows", Architecture: "arm64"}, + }, nil + default: + return []registry.Platform{ + {OS: "linux", Architecture: "arm64"}, + {OS: "linux", Architecture: "amd64"}, + }, nil + } + }), + WithOS("linux"), + WithSchedulableArchitectures([]string{"amd64"}), + ), + &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test", + Name: "object", + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Image: "ubuntu", + }, + { + Image: "alpine", + }, + }, + }, + }, + ) + assert.False(t, resp.Allowed) + assert.Equal(t, http.StatusForbidden, int(resp.Result.Code)) + assert.Len(t, resp.Patches, 0) + assert.Len(t, resp.Patch, 0) +} + func TestUpdatePodSpecWithEmptyImage(t *testing.T) { t.Run("When one container has missing images", func(t *testing.T) { resp := runWebhookTest(