diff --git a/chart/pelican-wings/Chart.yaml b/chart/pelican-wings/Chart.yaml index 9d518e05..8f018a45 100644 --- a/chart/pelican-wings/Chart.yaml +++ b/chart/pelican-wings/Chart.yaml @@ -2,8 +2,8 @@ apiVersion: v2 name: pelican-wings description: Helm chart for Pelican Wings — the game server management daemon with Kubernetes support type: application -version: 0.1.0 -appVersion: "1.0.0" +version: 1.0.2 +appVersion: "1.0.2" keywords: - pelican - wings diff --git a/chart/pelican-wings/README.md b/chart/pelican-wings/README.md index a33339be..6e507f4d 100644 --- a/chart/pelican-wings/README.md +++ b/chart/pelican-wings/README.md @@ -12,13 +12,21 @@ networking support. ## Quick Start +Put your node credentials from the Panel in a local values file (kept out of +version control) rather than on the command line, where `--set` would leak them +into shell history and process listings: + +```yaml +# values.local.yaml (do not commit) +wings: + panelUrl: https://panel.example.com + token: YOUR_TOKEN + tokenId: YOUR_TOKEN_ID + uuid: YOUR_NODE_UUID +``` + ```bash -# Add your node credentials from the Panel -helm install wings ./chart/pelican-wings \ - --set wings.panelUrl=https://panel.example.com \ - --set wings.token=YOUR_TOKEN \ - --set wings.tokenId=YOUR_TOKEN_ID \ - --set wings.uuid=YOUR_NODE_UUID +helm install wings ./chart/pelican-wings -f values.local.yaml ``` ## Configuration @@ -33,13 +41,26 @@ See [values.yaml](values.yaml) for the full list of configurable values. | `wings.token` | Panel authentication token | `""` | | `wings.tokenId` | Panel token ID | `""` | | `wings.uuid` | Node UUID from Panel | `""` | -| `wings.kubernetes.networkMode` | Port exposure: `hostport` or `nodeport` | `nodeport` | +| `wings.kubernetes.networkMode` | Port exposure: `hostport`, `nodeport`, or `loadbalancer` | `nodeport` | | `wings.kubernetes.storageMode` | Storage: `hostpath` or `pvc` | `pvc` | | `wings.kubernetes.storageClass` | StorageClass for PVCs | `""` (cluster default) | | `wings.kubernetes.storageSize` | Default PVC size | `10Gi` | +| `wings.kubernetes.imagePullPolicy` | Pull policy for game server Pods/install Jobs: `Always`, `IfNotPresent`, `Never` | `""` (smart default) | | `gameNamespace` | Namespace for game server resources | `pelican` | | `rbac.create` | Create RBAC resources | `true` | +| `rbac.kubeletMetricsFallback` | Grant cluster-wide `nodes/proxy` for the kubelet stats fallback (broad permission; prefer metrics-server) | `false` | | `serviceAccount.create` | Create ServiceAccount | `true` | +| `serviceAccount.name` | ServiceAccount name (**required** when `serviceAccount.create=false`) | `""` | + +> **Namespace:** Wings schedules game-server workloads into `gameNamespace`, and +> the chart creates the namespaced RBAC there. `wings.kubernetes.namespace` is +> therefore derived from `gameNamespace`; if you set it explicitly it must match +> `gameNamespace` or the chart will fail to render. + +> **Credentials:** `wings.token`, `wings.tokenId`, and `wings.uuid` are rendered +> into a Kubernetes **Secret** (not a ConfigMap). Supply them via a private +> values file or `--set`, e.g. `helm install ... -f my-creds.yaml`, and keep that +> file out of version control. ### Storage @@ -75,13 +96,39 @@ wings: networkMode: hostport ``` +LoadBalancer mode provisions a `Service` of type `LoadBalancer` per game server +(for use with MetalLB, Cilium LB-IPAM, or a cloud LB). LB IP/sharing-key +annotations can be auto-populated from the allocation IP: + +```yaml +wings: + kubernetes: + networkMode: loadbalancer +``` + +### Image pulling + +Game server Pods and installation Jobs default to `imagePullPolicy: Always` +for remote images, so updated tags are re-pulled rather than reusing a stale +copy cached on the node (matching the Docker backend). `~`-prefixed local +images are never pulled. Override this for air-gapped clusters: + +```yaml +wings: + kubernetes: + imagePullPolicy: IfNotPresent # or "Never" +``` + +This is independent of `image.pullPolicy`, which applies to the Wings daemon +image itself. + ## What Gets Created - **Namespace** — `pelican` (configurable) - **ServiceAccount** — For Wings and game server Pods - **Role + RoleBinding** — Namespace-scoped permissions (Pods, Services, Jobs, PVCs) -- **ClusterRole + ClusterRoleBinding** — Metrics API access -- **ConfigMap** — Wings configuration file +- **ClusterRole + ClusterRoleBinding** — Metrics API access (`nodes/proxy` only when `rbac.kubeletMetricsFallback=true`) +- **Secret** — Wings configuration file (contains Panel token) - **Deployment** — Wings daemon with health probes - **Service** — Exposes Wings API within the cluster diff --git a/chart/pelican-wings/templates/_helpers.tpl b/chart/pelican-wings/templates/_helpers.tpl index b7d93818..dbde1679 100644 --- a/chart/pelican-wings/templates/_helpers.tpl +++ b/chart/pelican-wings/templates/_helpers.tpl @@ -54,7 +54,9 @@ Create the name of the service account to use. {{- define "pelican-wings.serviceAccountName" -}} {{- if .Values.serviceAccount.create }} {{- default (include "pelican-wings.fullname" .) .Values.serviceAccount.name }} +{{- else if .Values.serviceAccount.name }} +{{- .Values.serviceAccount.name }} {{- else }} -{{- default "default" .Values.serviceAccount.name }} +{{- fail "serviceAccount.name must be set when serviceAccount.create is false: binding RBAC to the namespace 'default' ServiceAccount would grant Wings' permissions to every workload using it." }} {{- end }} {{- end }} diff --git a/chart/pelican-wings/templates/clusterrole-metrics.yaml b/chart/pelican-wings/templates/clusterrole-metrics.yaml index aaf02289..399f0bbb 100644 --- a/chart/pelican-wings/templates/clusterrole-metrics.yaml +++ b/chart/pelican-wings/templates/clusterrole-metrics.yaml @@ -12,9 +12,12 @@ rules: - apiGroups: [""] resources: ["nodes"] verbs: ["get"] + {{- if .Values.rbac.kubeletMetricsFallback }} # Proxy to kubelet stats/summary API for resource metrics when - # metrics-server is not installed. + # metrics-server is not installed. Opt-in: this is a broad cluster-scoped + # permission (rbac.kubeletMetricsFallback). - apiGroups: [""] resources: ["nodes/proxy"] verbs: ["get"] + {{- end }} {{- end }} diff --git a/chart/pelican-wings/templates/configmap.yaml b/chart/pelican-wings/templates/configmap.yaml index 0128d4fc..a332fae0 100644 --- a/chart/pelican-wings/templates/configmap.yaml +++ b/chart/pelican-wings/templates/configmap.yaml @@ -1,11 +1,15 @@ +{{- if and .Values.wings.kubernetes.namespace (ne .Values.wings.kubernetes.namespace .Values.gameNamespace) }} +{{- fail "wings.kubernetes.namespace must equal gameNamespace: the chart's namespaced RBAC is created in gameNamespace, so Wings must schedule game-server workloads there too. Leave wings.kubernetes.namespace empty or set it equal to gameNamespace." }} +{{- end }} apiVersion: v1 -kind: ConfigMap +kind: Secret metadata: name: {{ include "pelican-wings.fullname" . }}-config namespace: {{ .Values.gameNamespace }} labels: {{- include "pelican-wings.labels" . | nindent 4 }} -data: +type: Opaque +stringData: config.yml: | debug: false app_name: pelican @@ -26,7 +30,7 @@ data: tmp_directory: {{ .Values.wings.system.tmpDirectory | quote }} kubernetes: enabled: {{ .Values.wings.kubernetes.enabled }} - namespace: {{ .Values.wings.kubernetes.namespace | quote }} + namespace: {{ .Values.gameNamespace | quote }} network_mode: {{ .Values.wings.kubernetes.networkMode | quote }} {{- if .Values.wings.kubernetes.lbAnnotations }} lb_annotations: @@ -71,3 +75,6 @@ data: image_pull_secrets: {{- toYaml .Values.wings.kubernetes.imagePullSecrets | nindent 8 }} {{- end }} + {{- if .Values.wings.kubernetes.imagePullPolicy }} + image_pull_policy: {{ .Values.wings.kubernetes.imagePullPolicy | quote }} + {{- end }} diff --git a/chart/pelican-wings/templates/deployment.yaml b/chart/pelican-wings/templates/deployment.yaml index e7de1499..f0bb4944 100644 --- a/chart/pelican-wings/templates/deployment.yaml +++ b/chart/pelican-wings/templates/deployment.yaml @@ -91,8 +91,8 @@ spec: {{- end }} volumes: - name: config-source - configMap: - name: {{ include "pelican-wings.fullname" . }}-config + secret: + secretName: {{ include "pelican-wings.fullname" . }}-config - name: config emptyDir: {} {{- if .Values.persistence.enabled }} diff --git a/chart/pelican-wings/templates/limitrange.yaml b/chart/pelican-wings/templates/limitrange.yaml index 13d03582..20243f37 100644 --- a/chart/pelican-wings/templates/limitrange.yaml +++ b/chart/pelican-wings/templates/limitrange.yaml @@ -1,4 +1,7 @@ {{- if .Values.limitRange.enabled }} +{{- if not (or .Values.limitRange.defaultCPULimit .Values.limitRange.defaultMemoryLimit .Values.limitRange.defaultCPURequest .Values.limitRange.defaultMemoryRequest .Values.limitRange.maxCPU .Values.limitRange.maxMemory) }} +{{- fail "limitRange.enabled is true but no limits are set; configure at least one of defaultCPULimit, defaultMemoryLimit, defaultCPURequest, defaultMemoryRequest, maxCPU or maxMemory (an empty LimitRange has no effect)." }} +{{- end }} apiVersion: v1 kind: LimitRange metadata: diff --git a/chart/pelican-wings/templates/resourcequota.yaml b/chart/pelican-wings/templates/resourcequota.yaml index 8ec70eb4..4ac5877f 100644 --- a/chart/pelican-wings/templates/resourcequota.yaml +++ b/chart/pelican-wings/templates/resourcequota.yaml @@ -1,4 +1,7 @@ {{- if .Values.resourceQuota.enabled }} +{{- if not (or .Values.resourceQuota.cpuLimit .Values.resourceQuota.memoryLimit .Values.resourceQuota.cpuRequest .Values.resourceQuota.memoryRequest .Values.resourceQuota.maxPods .Values.resourceQuota.maxPVCs .Values.resourceQuota.maxStorage) }} +{{- fail "resourceQuota.enabled is true but no quota fields are set; configure at least one of cpuLimit, memoryLimit, cpuRequest, memoryRequest, maxPods, maxPVCs or maxStorage (an empty ResourceQuota would block all Pods from scheduling)." }} +{{- end }} apiVersion: v1 kind: ResourceQuota metadata: diff --git a/chart/pelican-wings/values.yaml b/chart/pelican-wings/values.yaml index f211fa65..6f26c07c 100644 --- a/chart/pelican-wings/values.yaml +++ b/chart/pelican-wings/values.yaml @@ -30,6 +30,11 @@ serviceAccount: rbac: # -- Whether to create RBAC resources (Role, RoleBinding, ClusterRole, ClusterRoleBinding) create: true + # -- Grant the cluster-wide nodes/proxy permission so Wings can read resource + # metrics directly from the kubelet stats/summary API when metrics-server is + # not installed. This is a broad, cluster-scoped permission; leave it off and + # install metrics-server unless you specifically need the kubelet fallback. + kubeletMetricsFallback: false # -- Namespace for game server resources (Pods, Services, Jobs, PVCs) gameNamespace: pelican @@ -119,6 +124,10 @@ wings: tolerations: [] # -- Image pull secrets for game server Pods imagePullSecrets: [] + # -- Override pull policy for game server Pods and install Jobs: + # "Always", "IfNotPresent", or "Never". Empty = remote images use Always + # (re-pull updated tags, matching Docker), ~-local images use IfNotPresent. + imagePullPolicy: "" # -- Pod-level security context for the Wings Deployment podSecurityContext: {} diff --git a/config/config_kubernetes.go b/config/config_kubernetes.go index 319ef4a5..525eb62e 100644 --- a/config/config_kubernetes.go +++ b/config/config_kubernetes.go @@ -48,6 +48,13 @@ type KubernetesConfiguration struct { // images in game server Pods. ImagePullSecrets []string `json:"image_pull_secrets" yaml:"image_pull_secrets"` + // ImagePullPolicy overrides the pull policy applied to game server Pods + // and installation Jobs. Valid values are "Always", "IfNotPresent", and + // "Never". When empty, remote images use "Always" (so updated tags are + // re-pulled, matching the Docker backend) and ~-prefixed local images use + // "IfNotPresent". Set to "IfNotPresent" or "Never" for air-gapped clusters. + ImagePullPolicy string `default:"" json:"image_pull_policy" yaml:"image_pull_policy"` + // ServiceAccount is the name of the Kubernetes ServiceAccount assigned to // game server Pods. ServiceAccount string `default:"" json:"service_account" yaml:"service_account"` diff --git a/environment/kubernetes/api.go b/environment/kubernetes/api.go index 4c80e63e..de503a04 100644 --- a/environment/kubernetes/api.go +++ b/environment/kubernetes/api.go @@ -14,29 +14,29 @@ import ( var ( _konce sync.Once _client kubernetes.Interface + _kerr error ) // Client returns a shared Kubernetes clientset. The client is created once // and reused for all subsequent calls. It uses in-cluster config when no // kubeconfig path is specified, falling back to the provided kubeconfig file. func Client() (kubernetes.Interface, error) { - var err error _konce.Do(func() { var cfg *rest.Config kubeconfig := config.Get().Kubernetes.Kubeconfig if kubeconfig != "" { - cfg, err = clientcmd.BuildConfigFromFlags("", kubeconfig) + cfg, _kerr = clientcmd.BuildConfigFromFlags("", kubeconfig) } else { - cfg, err = rest.InClusterConfig() + cfg, _kerr = rest.InClusterConfig() } - if err != nil { - err = errors.Wrap(err, "environment/kubernetes: failed to build config") + if _kerr != nil { + _kerr = errors.Wrap(_kerr, "environment/kubernetes: failed to build config") return } - _client, err = kubernetes.NewForConfig(cfg) - if err != nil { - err = errors.Wrap(err, "environment/kubernetes: failed to create clientset") + _client, _kerr = kubernetes.NewForConfig(cfg) + if _kerr != nil { + _kerr = errors.Wrap(_kerr, "environment/kubernetes: failed to create clientset") } }) - return _client, err + return _client, _kerr } diff --git a/environment/kubernetes/container.go b/environment/kubernetes/container.go index 31f3d1cb..12afb647 100644 --- a/environment/kubernetes/container.go +++ b/environment/kubernetes/container.go @@ -11,25 +11,47 @@ import ( "emperror.dev/errors" "github.com/apex/log" corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" - "k8s.io/client-go/tools/remotecommand" "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" + "k8s.io/client-go/tools/remotecommand" "github.com/exonical/wings/config" "github.com/exonical/wings/environment" "github.com/exonical/wings/system" ) +// resolveImagePullPolicy returns the cleaned image reference (without the ~ +// local-image prefix) and the pull policy to apply. It mirrors the Docker +// backend, which always attempts to pull remote images before starting a +// server so updated tags are picked up, while never pulling ~-prefixed local +// images. A non-empty kubernetes.image_pull_policy config value overrides the +// default (useful for air-gapped clusters). +func resolveImagePullPolicy(image string) (string, corev1.PullPolicy) { + cleaned := strings.TrimPrefix(image, "~") + if override := config.Get().Kubernetes.ImagePullPolicy; override != "" { + return cleaned, corev1.PullPolicy(override) + } + if strings.HasPrefix(image, "~") { + return cleaned, corev1.PullIfNotPresent + } + return cleaned, corev1.PullAlways +} + // Create builds and creates the Pod for this game server in Kubernetes. // If the Pod already exists this is a no-op. func (e *Environment) Create() error { ctx := context.Background() // If the Pod already exists, return immediately. - if exists, _ := e.Exists(); exists { + exists, err := e.Exists() + if err != nil { + return errors.Wrap(err, "environment/kubernetes: failed to check pod existence") + } + if exists { return nil } @@ -67,8 +89,8 @@ func (e *Environment) Create() error { volumeMounts = append(volumeMounts, idMounts...) } - // Determine image (strip ~ prefix used for local Docker images). - image := strings.TrimPrefix(e.meta.Image, "~") + // Determine image and pull policy (mirrors the Docker backend). + image, pullPolicy := resolveImagePullPolicy(e.meta.Image) // Build container ports from allocations. containerPorts := e.buildContainerPorts() @@ -131,7 +153,7 @@ func (e *Environment) Create() error { VolumeMounts: volumeMounts, Stdin: true, TTY: true, - ImagePullPolicy: corev1.PullIfNotPresent, + ImagePullPolicy: pullPolicy, }, }, Volumes: volumes, @@ -194,7 +216,7 @@ func (e *Environment) Create() error { e.log().WithField("image", image).Info("creating pod for server") - _, err := e.client.CoreV1().Pods(e.namespace()).Create(ctx, pod, metav1.CreateOptions{}) + _, err = e.client.CoreV1().Pods(e.namespace()).Create(ctx, pod, metav1.CreateOptions{}) if err != nil { return errors.Wrap(err, "environment/kubernetes: failed to create pod") } @@ -235,12 +257,12 @@ func (e *Environment) Destroy() error { e.log().WithField("error", cmErr).Warn("failed to delete identity ConfigMap during destroy") } - e.SetState(environment.ProcessOfflineState) - if err != nil && !isNotFound(err) { return errors.Wrap(err, "environment/kubernetes: failed to delete pod") } + e.SetState(environment.ProcessOfflineState) + return nil } @@ -331,13 +353,15 @@ func (e *Environment) Attach(ctx context.Context) error { // SendCommand writes a command string to the attached Pod's stdin. func (e *Environment) SendCommand(c string) error { - if !e.IsAttached() { - return errors.New("environment/kubernetes: not attached to pod") - } - e.mu.RLock() defer e.mu.RUnlock() + // Check the stream under the lock so it cannot be cleared between an + // attachment check and the write below. + if e.stream == nil { + return errors.New("environment/kubernetes: not attached to pod") + } + // If this is the stop command, mark the server as stopping. if e.meta.Stop.Type == "command" && c == e.meta.Stop.Value { e.SetState(environment.ProcessStoppingState) @@ -496,12 +520,17 @@ func (e *Environment) buildContainerPorts() []corev1.ContainerPort { cfg := config.Get() allocs := e.Configuration.Allocations() var ports []corev1.ContainerPort + seen := make(map[int]struct{}) for _, allocPorts := range allocs.Mappings { for _, port := range allocPorts { if port < 1 || port > 65535 { continue } + if _, ok := seen[port]; ok { + continue + } + seen[port] = struct{}{} tcpPort := corev1.ContainerPort{ Name: fmt.Sprintf("tcp-%d", port), @@ -543,7 +572,7 @@ func (e *Environment) getPod(ctx context.Context) (*corev1.Pod, error) { // isNotFound checks if the error is a Kubernetes NotFound error. func isNotFound(err error) bool { - return strings.Contains(err.Error(), "not found") + return apierrors.IsNotFound(err) } // isPodRunning checks if a Pod is in Running phase with a ready container. @@ -558,5 +587,3 @@ func isPodRunning(pod *corev1.Pod) bool { } return false } - - diff --git a/environment/kubernetes/container_test.go b/environment/kubernetes/container_test.go new file mode 100644 index 00000000..5e9a0e7f --- /dev/null +++ b/environment/kubernetes/container_test.go @@ -0,0 +1,54 @@ +package kubernetes + +import ( + "testing" + + . "github.com/franela/goblin" + corev1 "k8s.io/api/core/v1" + + "github.com/exonical/wings/config" +) + +// TestResolveImagePullPolicy verifies pull-policy resolution for remote +// images, ~-prefixed local images, and configured overrides. +func TestResolveImagePullPolicy(t *testing.T) { + g := Goblin(t) + + g.Describe("resolveImagePullPolicy", func() { + g.BeforeEach(func() { + config.Update(func(c *config.Configuration) { + c.Kubernetes.ImagePullPolicy = "" + }) + }) + + g.It("always pulls remote images so updated tags are picked up", func() { + image, policy := resolveImagePullPolicy("ghcr.io/pelican-eggs/games:latest") + g.Assert(image).Equal("ghcr.io/pelican-eggs/games:latest") + g.Assert(policy).Equal(corev1.PullAlways) + }) + + g.It("does not pull ~-prefixed local images and strips the prefix", func() { + image, policy := resolveImagePullPolicy("~local/custom:dev") + g.Assert(image).Equal("local/custom:dev") + g.Assert(policy).Equal(corev1.PullIfNotPresent) + }) + + g.It("honors a configured override for remote images", func() { + config.Update(func(c *config.Configuration) { + c.Kubernetes.ImagePullPolicy = "IfNotPresent" + }) + image, policy := resolveImagePullPolicy("ghcr.io/pelican-eggs/games:latest") + g.Assert(image).Equal("ghcr.io/pelican-eggs/games:latest") + g.Assert(policy).Equal(corev1.PullIfNotPresent) + }) + + g.It("honors a configured override and still strips the local prefix", func() { + config.Update(func(c *config.Configuration) { + c.Kubernetes.ImagePullPolicy = "Never" + }) + image, policy := resolveImagePullPolicy("~local/custom:dev") + g.Assert(image).Equal("local/custom:dev") + g.Assert(policy).Equal(corev1.PullNever) + }) + }) +} diff --git a/environment/kubernetes/environment_test.go b/environment/kubernetes/environment_test.go index 484e055b..1523b32c 100644 --- a/environment/kubernetes/environment_test.go +++ b/environment/kubernetes/environment_test.go @@ -563,5 +563,25 @@ func TestEnvironment(t *testing.T) { // Only 8080 is valid, so 2 ports (TCP + UDP). g.Assert(len(ports)).Equal(2) }) + + g.It("should deduplicate ports shared across allocation IPs", func() { + allocs := environment.Allocations{ + Mappings: map[string][]int{ + "1.1.1.1": {25565}, + "2.2.2.2": {25565}, + }, + } + settings := environment.Settings{Allocations: allocs} + cfg := environment.NewConfiguration(settings, nil) + env := &Environment{Configuration: cfg} + + config.Update(func(c *config.Configuration) { + c.Kubernetes.NetworkMode = config.KubeNetworkNodePort + }) + + ports := env.buildContainerPorts() + // 25565 appears under two IPs but must yield a single TCP + UDP pair. + g.Assert(len(ports)).Equal(2) + }) }) } diff --git a/environment/kubernetes/installer.go b/environment/kubernetes/installer.go index c4019b29..fd5523bc 100644 --- a/environment/kubernetes/installer.go +++ b/environment/kubernetes/installer.go @@ -62,10 +62,17 @@ func (ip *InstallerProcess) configMapName() string { // Run executes the full installation process: creates a Job, streams its logs, // waits for completion, and cleans up. func (ip *InstallerProcess) Run(ctx context.Context) error { - // Clean up any existing installer Job from a previous run. - _ = ip.client.BatchV1().Jobs(ip.namespace).Delete(ctx, ip.jobName(), metav1.DeleteOptions{ - PropagationPolicy: propagationBackground(), - }) + // Clean up any existing installer Job from a previous run. Use foreground + // propagation and wait for the Job to disappear so the Create below does not + // race a still-terminating Job and fail with AlreadyExists. + if err := ip.client.BatchV1().Jobs(ip.namespace).Delete(ctx, ip.jobName(), metav1.DeleteOptions{ + PropagationPolicy: propagationForeground(), + }); err != nil && !isNotFound(err) { + return errors.Wrap(err, "environment/kubernetes: failed to delete existing installer job") + } + if err := ip.waitForJobDeletion(ctx, 30*time.Second); err != nil { + return errors.Wrap(err, "environment/kubernetes: timed out waiting for old installer job deletion") + } // Build environment variables. var envVars []corev1.EnvVar @@ -81,6 +88,9 @@ func (ip *InstallerProcess) Run(ctx context.Context) error { cfg := config.Get() + // Determine install image and pull policy (mirrors the Docker backend). + installImage, installPullPolicy := resolveImagePullPolicy(ip.Script.ContainerImage) + // Build the Job spec. backoffLimit := int32(0) ttl := int32(300) // Auto-clean Job after 5 minutes. @@ -111,7 +121,7 @@ func (ip *InstallerProcess) Run(ctx context.Context) error { Containers: []corev1.Container{ { Name: "installer", - Image: ip.Script.ContainerImage, + Image: installImage, Command: []string{ip.Script.Entrypoint, "/mnt/install/install.sh"}, Env: envVars, VolumeMounts: []corev1.VolumeMount{ @@ -121,7 +131,7 @@ func (ip *InstallerProcess) Run(ctx context.Context) error { MountPath: "/mnt/install", }, }, - ImagePullPolicy: corev1.PullIfNotPresent, + ImagePullPolicy: installPullPolicy, }, }, Volumes: ip.buildJobVolumes(cfg), @@ -280,7 +290,7 @@ func (ip *InstallerProcess) Cleanup(ctx context.Context) error { // WriteInstallScript creates a ConfigMap containing the installation script. // The ConfigMap is mounted into the Job Pod at /mnt/install, eliminating the // need for a shared filesystem between Wings and the Job Pod. -func (ip *InstallerProcess) WriteInstallScript() error { +func (ip *InstallerProcess) WriteInstallScript(ctx context.Context) error { content := strings.ReplaceAll(ip.Script.Script, "\r\n", "\n") cm := &corev1.ConfigMap{ @@ -298,8 +308,6 @@ func (ip *InstallerProcess) WriteInstallScript() error { }, } - ctx := context.Background() - // Delete any existing ConfigMap from a previous run. _ = ip.client.CoreV1().ConfigMaps(ip.namespace).Delete(ctx, ip.configMapName(), metav1.DeleteOptions{}) @@ -370,7 +378,10 @@ func (ip *InstallerProcess) buildServerDataMount() corev1.VolumeMount { if cfg.Kubernetes.DataPVC != "" { serverPath := filepath.Join(cfg.System.Data, ip.ServerID) rel, err := filepath.Rel(cfg.System.RootDirectory, serverPath) - if err != nil { + // filepath.Rel can yield a path escaping the volume root ("..") when + // System.Data is outside RootDirectory; such a SubPath is rejected by + // Kubernetes, so fall back to the default layout in that case. + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { vm.SubPath = filepath.Join("volumes", ip.ServerID) } else { vm.SubPath = rel @@ -379,6 +390,34 @@ func (ip *InstallerProcess) buildServerDataMount() corev1.VolumeMount { return vm } +// waitForJobDeletion blocks until the installer Job is fully deleted or the +// timeout elapses. +func (ip *InstallerProcess) waitForJobDeletion(ctx context.Context, timeout time.Duration) error { + dctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + + for { + _, err := ip.client.BatchV1().Jobs(ip.namespace).Get(dctx, ip.jobName(), metav1.GetOptions{}) + if err != nil && isNotFound(err) { + return nil + } + select { + case <-dctx.Done(): + return dctx.Err() + case <-ticker.C: + } + } +} + +// propagationForeground returns a pointer to the Foreground propagation policy. +func propagationForeground() *metav1.DeletionPropagation { + p := metav1.DeletePropagationForeground + return &p +} + // propagationBackground returns a pointer to the Background propagation policy. func propagationBackground() *metav1.DeletionPropagation { p := metav1.DeletePropagationBackground diff --git a/environment/kubernetes/installer_test.go b/environment/kubernetes/installer_test.go index c794e1e6..63ecfd9b 100644 --- a/environment/kubernetes/installer_test.go +++ b/environment/kubernetes/installer_test.go @@ -2,6 +2,7 @@ package kubernetes import ( "context" + "errors" "os" "path/filepath" "testing" @@ -51,7 +52,7 @@ func TestInstaller(t *testing.T) { }, } - err := ip.WriteInstallScript() + err := ip.WriteInstallScript(context.Background()) g.Assert(err).IsNil() cm, err := client.CoreV1().ConfigMaps("pelican").Get(context.Background(), "write-cm-uuid-install-script", metav1.GetOptions{}) @@ -79,7 +80,7 @@ func TestInstaller(t *testing.T) { }, } - err := ip.WriteInstallScript() + err := ip.WriteInstallScript(context.Background()) g.Assert(err).IsNil() cm, err := client.CoreV1().ConfigMaps("pelican").Get(context.Background(), "rewrite-uuid-install-script", metav1.GetOptions{}) @@ -179,9 +180,9 @@ func TestInstaller(t *testing.T) { // Verify backoff limit. g.Assert(*job.Spec.BackoffLimit).Equal(int32(0)) - // Cancel to stop waitForJob. + // Cancel to stop waitForJob and assert the cancellation propagates. cancel() - <-errCh + g.Assert(errors.Is(<-errCh, context.Canceled)).IsTrue() }) g.It("should succeed when Job completes successfully", func() { diff --git a/environment/kubernetes/network.go b/environment/kubernetes/network.go index e0459a97..0ec14d2d 100644 --- a/environment/kubernetes/network.go +++ b/environment/kubernetes/network.go @@ -139,15 +139,16 @@ func (e *Environment) EnsureService(ctx context.Context) error { } // Service exists; update it with the desired spec while preserving - // existing NodePort assignments where possible. + // existing NodePort assignments where possible. Type and annotations must + // also be reconciled so a networkMode switch (e.g. NodePort <-> LoadBalancer) + // is applied and stale LB annotations are cleared. + existing.Spec.Type = desired.Spec.Type existing.Spec.Selector = desired.Spec.Selector existing.Spec.Ports = mergeServicePorts(existing.Spec.Ports, desired.Spec.Ports) existing.Labels = labels + existing.Annotations = annotations e.log().WithField("service", svcName).Infof("updating %s service for server", svcType) - if isLB { - existing.Annotations = annotations - } _, err = e.client.CoreV1().Services(ns).Update(ctx, existing, metav1.UpdateOptions{}) if err != nil { return errors.Wrap(err, "environment/kubernetes: failed to update service") diff --git a/environment/kubernetes/network_test.go b/environment/kubernetes/network_test.go index 7df5d379..e5fcef13 100644 --- a/environment/kubernetes/network_test.go +++ b/environment/kubernetes/network_test.go @@ -129,6 +129,42 @@ func TestNetwork(t *testing.T) { g.Assert(len(svc.Spec.Ports)).Equal(4) }) + g.It("should reconcile Service type and annotations when network mode changes", func() { + // Pre-create a LoadBalancer Service with stale LB annotations. + existingSvc := &corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gs-test-server-uuid", + Namespace: "pelican", + Annotations: map[string]string{"lbipam.cilium.io/ips": "10.0.0.1"}, + }, + Spec: corev1.ServiceSpec{ + Type: corev1.ServiceTypeLoadBalancer, + Selector: map[string]string{"pelican.dev/server-id": "test-server-uuid"}, + }, + } + client := fake.NewSimpleClientset(existingSvc) + allocs := environment.Allocations{ + Mappings: map[string][]int{"0.0.0.0": {25565}}, + } + env := newTestEnv(client, allocs) + + // Switch to NodePort mode. + config.Update(func(c *config.Configuration) { + c.Kubernetes.NetworkMode = config.KubeNetworkNodePort + c.Kubernetes.Namespace = "pelican" + }) + + err := env.EnsureService(context.Background()) + g.Assert(err).IsNil() + + svc, err := client.CoreV1().Services("pelican").Get(context.Background(), "gs-test-server-uuid", metav1.GetOptions{}) + g.Assert(err).IsNil() + // Type must be updated to NodePort and the stale LB annotation cleared. + g.Assert(svc.Spec.Type).Equal(corev1.ServiceTypeNodePort) + _, hasStale := svc.Annotations["lbipam.cilium.io/ips"] + g.Assert(hasStale).IsFalse() + }) + g.It("should be a no-op with empty allocations", func() { client := fake.NewSimpleClientset() allocs := environment.Allocations{ @@ -242,9 +278,9 @@ func TestNetwork(t *testing.T) { Name: "gs-test-server-uuid", Namespace: "pelican", Annotations: map[string]string{ - "io.cilium/lb-ipam-pool": "game-servers", - "lbipam.cilium.io/ips": "23.227.184.222", - "lbipam.cilium.io/sharing-key": "23.227.184.222", + "io.cilium/lb-ipam-pool": "game-servers", + "lbipam.cilium.io/ips": "23.227.184.222", + "lbipam.cilium.io/sharing-key": "23.227.184.222", }, }, Spec: corev1.ServiceSpec{ diff --git a/environment/kubernetes/power.go b/environment/kubernetes/power.go index 69d4e3a6..bf3abdca 100644 --- a/environment/kubernetes/power.go +++ b/environment/kubernetes/power.go @@ -20,12 +20,17 @@ import ( func (e *Environment) OnBeforeStart(ctx context.Context) error { // Delete any existing Pod to ensure fresh config is applied. gracePeriod := int64(0) - _ = e.client.CoreV1().Pods(e.namespace()).Delete(ctx, e.Id, metav1.DeleteOptions{ + if err := e.client.CoreV1().Pods(e.namespace()).Delete(ctx, e.Id, metav1.DeleteOptions{ GracePeriodSeconds: &gracePeriod, - }) + }); err != nil && !isNotFound(err) { + return errors.Wrap(err, "environment/kubernetes: failed to delete existing pod before start") + } - // Wait briefly for deletion to propagate. - e.waitForPodDeletion(ctx, 10*time.Second) + // Wait for the old Pod to be fully removed so the recreate below does not + // no-op against a stale Pod. + if err := e.waitForPodDeletion(ctx, 10*time.Second); err != nil { + return errors.Wrap(err, "environment/kubernetes: timed out waiting for old pod deletion") + } // Create the Pod with current configuration. if err := e.Create(); err != nil { @@ -87,22 +92,23 @@ func (e *Environment) Stop(ctx context.Context) error { s := e.meta.Stop e.mu.RUnlock() - // If using a signal-based stop, terminate with that signal. - if s.Type == "" || s.Type == remote.ProcessStopSignal { - return e.Terminate(ctx, "SIGTERM") - } - if e.st.Load() != environment.ProcessOfflineState { e.SetState(environment.ProcessStoppingState) } // If using a command-based stop and we're attached, send the command. - if e.IsAttached() && s.Type == remote.ProcessStopCommand { + if s.Type == remote.ProcessStopCommand && e.IsAttached() { return e.SendCommand(s.Value) } - // Default: delete the Pod with a 30-second grace period. + // Otherwise (including signal-based stops) delete the Pod gracefully so the + // kubelet sends SIGTERM to PID 1 and the process can shut down within the + // grace period before being force-killed. An explicit SIGKILL maps to an + // immediate force-delete. gracePeriod := int64(30) + if strings.EqualFold(s.Value, "SIGKILL") { + gracePeriod = 0 + } err := e.client.CoreV1().Pods(e.namespace()).Delete(ctx, e.Id, metav1.DeleteOptions{ GracePeriodSeconds: &gracePeriod, }) @@ -153,8 +159,10 @@ func (e *Environment) WaitForStop(ctx context.Context, duration time.Duration, t return err } - // Wait for the Pod to be gone. - if err := e.waitForPodDeletion(tctx, duration); err != nil { + // Wait for the Pod to be gone or to stop running. A command-based stop + // leaves the Pod in a terminal Succeeded/Failed phase without deleting it, + // so waiting only for deletion would block until the timeout. + if err := e.waitForPodStoppedOrDeleted(tctx, duration); err != nil { if terminate { e.log().Warn("pod did not terminate in time, forcing deletion") return e.Terminate(ctx, "SIGKILL") @@ -259,6 +267,36 @@ func (e *Environment) waitForPodRunning(ctx context.Context) error { } } +// waitForPodStoppedOrDeleted blocks until the Pod is deleted or has stopped +// running (a terminal Succeeded/Failed phase), whichever happens first, or the +// timeout elapses. +func (e *Environment) waitForPodStoppedOrDeleted(ctx context.Context, timeout time.Duration) error { + dctx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + + ticker := time.NewTicker(500 * time.Millisecond) + defer ticker.Stop() + + for { + select { + case <-dctx.Done(): + return dctx.Err() + case <-ticker.C: + pod, err := e.getPod(dctx) + if err != nil { + if isNotFound(err) { + return nil + } + continue + } + if !isPodRunning(pod) { + e.markOffline() + return nil + } + } + } +} + // waitForPodDeletion blocks until the Pod is fully deleted or the timeout // elapses. func (e *Environment) waitForPodDeletion(ctx context.Context, timeout time.Duration) error { diff --git a/environment/kubernetes/quota.go b/environment/kubernetes/quota.go index 3573c748..4d28e460 100644 --- a/environment/kubernetes/quota.go +++ b/environment/kubernetes/quota.go @@ -12,7 +12,7 @@ import ( ) const ( - quotaName = "pelican-wings" + quotaName = "pelican-wings" limitRangeName = "pelican-wings" ) @@ -25,9 +25,15 @@ func (e *Environment) EnsureResourceQuota(ctx context.Context) error { } ns := e.namespace() - quota := buildResourceQuota(ns, &cfg.Kubernetes.ResourceQuota) + quota, err := buildResourceQuota(ns, &cfg.Kubernetes.ResourceQuota) + if err != nil { + return err + } existing, err := e.client.CoreV1().ResourceQuotas(ns).Get(ctx, quotaName, metav1.GetOptions{}) + if err != nil && !isNotFound(err) { + return errors.Wrap(err, "environment/kubernetes: failed to get ResourceQuota") + } if err == nil { // Update existing. existing.Spec = quota.Spec @@ -56,9 +62,15 @@ func (e *Environment) EnsureLimitRange(ctx context.Context) error { } ns := e.namespace() - lr := buildLimitRange(ns, &cfg.Kubernetes.LimitRange) + lr, err := buildLimitRange(ns, &cfg.Kubernetes.LimitRange) + if err != nil { + return err + } existing, err := e.client.CoreV1().LimitRanges(ns).Get(ctx, limitRangeName, metav1.GetOptions{}) + if err != nil && !isNotFound(err) { + return errors.Wrap(err, "environment/kubernetes: failed to get LimitRange") + } if err == nil { // Update existing. existing.Spec = lr.Spec @@ -78,21 +90,39 @@ func (e *Environment) EnsureLimitRange(ctx context.Context) error { return nil } +// setQuantity parses a quantity string into the resource list under key when +// the value is non-empty. Unlike resource.MustParse it returns an error for +// invalid (user-configurable) values instead of panicking the process. +func setQuantity(list corev1.ResourceList, key corev1.ResourceName, field, value string) error { + if value == "" { + return nil + } + q, err := resource.ParseQuantity(value) + if err != nil { + return errors.Wrapf(err, "environment/kubernetes: invalid quantity %q for %s", value, field) + } + list[key] = q + return nil +} + // buildResourceQuota constructs the ResourceQuota spec from config values. -func buildResourceQuota(namespace string, cfg *config.KubeResourceQuota) *corev1.ResourceQuota { +func buildResourceQuota(namespace string, cfg *config.KubeResourceQuota) (*corev1.ResourceQuota, error) { hard := corev1.ResourceList{} - if cfg.CPULimit != "" { - hard[corev1.ResourceLimitsCPU] = resource.MustParse(cfg.CPULimit) - } - if cfg.MemoryLimit != "" { - hard[corev1.ResourceLimitsMemory] = resource.MustParse(cfg.MemoryLimit) - } - if cfg.CPURequest != "" { - hard[corev1.ResourceRequestsCPU] = resource.MustParse(cfg.CPURequest) - } - if cfg.MemoryRequest != "" { - hard[corev1.ResourceRequestsMemory] = resource.MustParse(cfg.MemoryRequest) + for _, q := range []struct { + key corev1.ResourceName + field string + value string + }{ + {corev1.ResourceLimitsCPU, "cpu_limit", cfg.CPULimit}, + {corev1.ResourceLimitsMemory, "memory_limit", cfg.MemoryLimit}, + {corev1.ResourceRequestsCPU, "cpu_request", cfg.CPURequest}, + {corev1.ResourceRequestsMemory, "memory_request", cfg.MemoryRequest}, + {corev1.ResourceRequestsStorage, "max_storage", cfg.MaxStorage}, + } { + if err := setQuantity(hard, q.key, q.field, q.value); err != nil { + return nil, err + } } if cfg.MaxPods > 0 { hard[corev1.ResourcePods] = *resource.NewQuantity(cfg.MaxPods, resource.DecimalSI) @@ -100,9 +130,6 @@ func buildResourceQuota(namespace string, cfg *config.KubeResourceQuota) *corev1 if cfg.MaxPVCs > 0 { hard[corev1.ResourcePersistentVolumeClaims] = *resource.NewQuantity(cfg.MaxPVCs, resource.DecimalSI) } - if cfg.MaxStorage != "" { - hard[corev1.ResourceRequestsStorage] = resource.MustParse(cfg.MaxStorage) - } return &corev1.ResourceQuota{ ObjectMeta: metav1.ObjectMeta{ @@ -115,11 +142,11 @@ func buildResourceQuota(namespace string, cfg *config.KubeResourceQuota) *corev1 Spec: corev1.ResourceQuotaSpec{ Hard: hard, }, - } + }, nil } // buildLimitRange constructs the LimitRange spec from config values. -func buildLimitRange(namespace string, cfg *config.KubeLimitRange) *corev1.LimitRange { +func buildLimitRange(namespace string, cfg *config.KubeLimitRange) (*corev1.LimitRange, error) { containerLimit := corev1.LimitRangeItem{ Type: corev1.LimitTypeContainer, Default: corev1.ResourceList{}, @@ -127,23 +154,22 @@ func buildLimitRange(namespace string, cfg *config.KubeLimitRange) *corev1.Limit Max: corev1.ResourceList{}, } - if cfg.DefaultCPULimit != "" { - containerLimit.Default[corev1.ResourceCPU] = resource.MustParse(cfg.DefaultCPULimit) - } - if cfg.DefaultMemoryLimit != "" { - containerLimit.Default[corev1.ResourceMemory] = resource.MustParse(cfg.DefaultMemoryLimit) - } - if cfg.DefaultCPURequest != "" { - containerLimit.DefaultRequest[corev1.ResourceCPU] = resource.MustParse(cfg.DefaultCPURequest) - } - if cfg.DefaultMemoryRequest != "" { - containerLimit.DefaultRequest[corev1.ResourceMemory] = resource.MustParse(cfg.DefaultMemoryRequest) - } - if cfg.MaxCPU != "" { - containerLimit.Max[corev1.ResourceCPU] = resource.MustParse(cfg.MaxCPU) - } - if cfg.MaxMemory != "" { - containerLimit.Max[corev1.ResourceMemory] = resource.MustParse(cfg.MaxMemory) + for _, q := range []struct { + list corev1.ResourceList + key corev1.ResourceName + field string + value string + }{ + {containerLimit.Default, corev1.ResourceCPU, "default_cpu_limit", cfg.DefaultCPULimit}, + {containerLimit.Default, corev1.ResourceMemory, "default_memory_limit", cfg.DefaultMemoryLimit}, + {containerLimit.DefaultRequest, corev1.ResourceCPU, "default_cpu_request", cfg.DefaultCPURequest}, + {containerLimit.DefaultRequest, corev1.ResourceMemory, "default_memory_request", cfg.DefaultMemoryRequest}, + {containerLimit.Max, corev1.ResourceCPU, "max_cpu", cfg.MaxCPU}, + {containerLimit.Max, corev1.ResourceMemory, "max_memory", cfg.MaxMemory}, + } { + if err := setQuantity(q.list, q.key, q.field, q.value); err != nil { + return nil, err + } } return &corev1.LimitRange{ @@ -157,5 +183,5 @@ func buildLimitRange(namespace string, cfg *config.KubeLimitRange) *corev1.Limit Spec: corev1.LimitRangeSpec{ Limits: []corev1.LimitRangeItem{containerLimit}, }, - } + }, nil } diff --git a/environment/kubernetes/quota_test.go b/environment/kubernetes/quota_test.go index 49ffe2bd..ea8925d8 100644 --- a/environment/kubernetes/quota_test.go +++ b/environment/kubernetes/quota_test.go @@ -2,19 +2,24 @@ package kubernetes import ( "context" + "errors" "testing" . "github.com/franela/goblin" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" "github.com/exonical/wings/config" "github.com/exonical/wings/environment" "github.com/exonical/wings/system" ) +// TestQuota covers building and reconciling ResourceQuota and LimitRange +// objects from configuration, including invalid-quantity error handling. func TestQuota(t *testing.T) { g := Goblin(t) @@ -146,6 +151,26 @@ func TestQuota(t *testing.T) { memLimit := rq.Spec.Hard[corev1.ResourceLimitsMemory] g.Assert(memLimit.Cmp(resource.MustParse("64Gi"))).Equal(0) }) + + g.It("should fail fast on a non-NotFound Get error instead of creating", func() { + client := fake.NewSimpleClientset() + client.PrependReactor("get", "resourcequotas", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("boom: api server unavailable") + }) + env := &Environment{ + Id: "quota-geterr-uuid", + client: client, + st: system.NewAtomicString(environment.ProcessOfflineState), + } + config.Update(func(c *config.Configuration) { + c.Kubernetes.Namespace = "pelican" + c.Kubernetes.ResourceQuota.Enabled = true + c.Kubernetes.ResourceQuota.CPULimit = "16" + }) + + err := env.EnsureResourceQuota(context.Background()) + g.Assert(err != nil).IsTrue() + }) }) g.Describe("EnsureLimitRange", func() { @@ -254,6 +279,26 @@ func TestQuota(t *testing.T) { defaultCPU := lr.Spec.Limits[0].Default[corev1.ResourceCPU] g.Assert(defaultCPU.Cmp(resource.MustParse("4"))).Equal(0) }) + + g.It("should fail fast on a non-NotFound Get error instead of creating", func() { + client := fake.NewSimpleClientset() + client.PrependReactor("get", "limitranges", func(action k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("boom: api server unavailable") + }) + env := &Environment{ + Id: "lr-geterr-uuid", + client: client, + st: system.NewAtomicString(environment.ProcessOfflineState), + } + config.Update(func(c *config.Configuration) { + c.Kubernetes.Namespace = "pelican" + c.Kubernetes.LimitRange.Enabled = true + c.Kubernetes.LimitRange.DefaultCPULimit = "2" + }) + + err := env.EnsureLimitRange(context.Background()) + g.Assert(err != nil).IsTrue() + }) }) g.Describe("buildResourceQuota", func() { @@ -264,7 +309,8 @@ func TestQuota(t *testing.T) { MaxPods: 10, MaxPVCs: 0, } - rq := buildResourceQuota("test-ns", cfg) + rq, err := buildResourceQuota("test-ns", cfg) + g.Assert(err).IsNil() g.Assert(rq.Namespace).Equal("test-ns") _, hasCPU := rq.Spec.Hard[corev1.ResourceLimitsCPU] @@ -289,7 +335,8 @@ func TestQuota(t *testing.T) { MaxCPU: "8", MaxMemory: "", } - lr := buildLimitRange("test-ns", cfg) + lr, err := buildLimitRange("test-ns", cfg) + g.Assert(err).IsNil() g.Assert(lr.Namespace).Equal("test-ns") g.Assert(len(lr.Spec.Limits)).Equal(1) @@ -306,6 +353,20 @@ func TestQuota(t *testing.T) { _, hasMaxMem := item.Max[corev1.ResourceMemory] g.Assert(hasMaxMem).IsFalse() }) + + g.It("should return an error for an invalid quantity instead of panicking", func() { + lr, err := buildLimitRange("test-ns", &config.KubeLimitRange{DefaultCPULimit: "not-a-quantity"}) + g.Assert(err != nil).IsTrue() + g.Assert(lr == nil).IsTrue() + }) + }) + + g.Describe("buildResourceQuota invalid input", func() { + g.It("should return an error for an invalid quantity instead of panicking", func() { + rq, err := buildResourceQuota("test-ns", &config.KubeResourceQuota{CPULimit: "not-a-quantity"}) + g.Assert(err != nil).IsTrue() + g.Assert(rq == nil).IsTrue() + }) }) }) } diff --git a/environment/kubernetes/stats.go b/environment/kubernetes/stats.go index 9e80ad0b..121cea2b 100644 --- a/environment/kubernetes/stats.go +++ b/environment/kubernetes/stats.go @@ -13,6 +13,10 @@ import ( "github.com/exonical/wings/environment" ) +// statsRequestTimeout bounds each individual metrics/stats API call so a slow +// or hung endpoint cannot stall the polling loop until the next tick. +const statsRequestTimeout = 3 * time.Second + // podStats holds parsed resource metrics for a pod's "server" container. type podStats struct { cpuPercent float64 @@ -37,6 +41,14 @@ func (e *Environment) Uptime(ctx context.Context) (int64, error) { return time.Since(pod.Status.StartTime.Time).Milliseconds(), nil } +// withStatsTimeout invokes fn with a child context bounded by +// statsRequestTimeout so a single slow metrics/stats call cannot stall polling. +func withStatsTimeout[T any](ctx context.Context, fn func(context.Context) (T, error)) (T, error) { + tctx, cancel := context.WithTimeout(ctx, statsRequestTimeout) + defer cancel() + return fn(tctx) +} + // pollResources periodically fetches resource usage and publishes resource // events. It tries the Kubernetes Metrics API first (requires metrics-server) // and falls back to the kubelet stats/summary API (always available). @@ -57,7 +69,6 @@ func (e *Environment) pollResources(ctx context.Context) error { defer ticker.Stop() lastCheck := time.Now() - useKubelet := false loggedSource := false for { @@ -79,30 +90,21 @@ func (e *Environment) pollResources(ctx context.Context) error { var stats *podStats - if !useKubelet { - stats, err = e.getMetricsAPIStats(ctx) - if err != nil { - if !loggedSource { - e.log().WithField("error", err).Info("metrics API unavailable, falling back to kubelet stats") - } - useKubelet = true - } - } - - if useKubelet { - stats, err = e.getKubeletPodStats(ctx) - if err != nil { - if !loggedSource { - e.log().WithField("error", err).Warn("kubelet stats also unavailable, resource metrics will not be reported") - loggedSource = true - } - } + // Prefer the metrics API and fall back to kubelet stats. The metrics + // API is retried on every tick (the fallback is not sticky) so a + // transiently-unavailable metrics-server is picked back up once it + // recovers. Each call is bounded by statsRequestTimeout. + usingKubelet := false + stats, err = withStatsTimeout(ctx, e.getMetricsAPIStats) + if err != nil { + usingKubelet = true + stats, err = withStatsTimeout(ctx, e.getKubeletPodStats) } if stats != nil { if !loggedSource { src := "metrics API (metrics-server)" - if useKubelet { + if usingKubelet { src = "kubelet stats/summary" } e.log().WithField("source", src).Info("collecting pod resource metrics") @@ -114,10 +116,13 @@ func (e *Environment) pollResources(ctx context.Context) error { RxBytes: stats.rxBytes, TxBytes: stats.txBytes, } + } else if !loggedSource { + e.log().WithField("error", err).Warn("pod resource metrics unavailable from both metrics API and kubelet") + loggedSource = true } // Get memory limit from pod spec. - pod, err := e.getPod(ctx) + pod, err := withStatsTimeout(ctx, e.getPod) if err == nil && pod != nil { for _, c := range pod.Spec.Containers { if c.Name == "server" { diff --git a/environment/kubernetes/storage.go b/environment/kubernetes/storage.go index 7602f9d5..882e7892 100644 --- a/environment/kubernetes/storage.go +++ b/environment/kubernetes/storage.go @@ -144,6 +144,11 @@ func (e *Environment) ResizePVC(ctx context.Context, newSize string) error { if cfg.Kubernetes.StorageMode != config.KubeStoragePVC { return nil } + // When a shared DataPVC is configured, per-server PVCs are not created, so + // there is nothing to resize (mirrors EnsurePVC/DeletePVC). + if cfg.Kubernetes.DataPVC != "" { + return nil + } ns := e.namespace() name := e.pvcName() diff --git a/environment/kubernetes/storage_test.go b/environment/kubernetes/storage_test.go index d4bef292..163635fa 100644 --- a/environment/kubernetes/storage_test.go +++ b/environment/kubernetes/storage_test.go @@ -281,6 +281,46 @@ func TestStorage(t *testing.T) { err := env.ResizePVC(context.Background(), "invalid") g.Assert(err).IsNotNil() }) + + g.It("should be a no-op when a shared DataPVC is configured", func() { + existingPVC := &corev1.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "gs-shared-resize-uuid", + Namespace: "pelican", + }, + Spec: corev1.PersistentVolumeClaimSpec{ + Resources: corev1.VolumeResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("10Gi"), + }, + }, + }, + } + client := fake.NewSimpleClientset(existingPVC) + env := &Environment{ + Id: "shared-resize-uuid", + client: client, + st: system.NewAtomicString(environment.ProcessOfflineState), + } + + config.Update(func(c *config.Configuration) { + c.Kubernetes.StorageMode = config.KubeStoragePVC + c.Kubernetes.Namespace = "pelican" + c.Kubernetes.DataPVC = "shared-data" + }) + defer config.Update(func(c *config.Configuration) { + c.Kubernetes.DataPVC = "" + }) + + err := env.ResizePVC(context.Background(), "50Gi") + g.Assert(err).IsNil() + + // The per-server PVC must be left untouched at its original size. + pvc, _ := client.CoreV1().PersistentVolumeClaims("pelican").Get(context.Background(), "gs-shared-resize-uuid", metav1.GetOptions{}) + storageReq := pvc.Spec.Resources.Requests[corev1.ResourceStorage] + original := resource.MustParse("10Gi") + g.Assert(storageReq.Equal(original)).IsTrue() + }) }) g.Describe("buildVolumes with PVC mode", func() { diff --git a/kubernetes/README.md b/kubernetes/README.md index d88a0982..9b113ce4 100644 --- a/kubernetes/README.md +++ b/kubernetes/README.md @@ -33,21 +33,27 @@ Wings requires two levels of RBAC: | Resource | Verbs | Purpose | |---------------------------|--------------------------------|------------------------------------------| -| pods | get, create, delete, list | Game server Pod lifecycle | +| pods | get, create, delete, list, watch | Game server Pod lifecycle | | pods/log | get | Stream server console output | | pods/attach | create | Interactive console (SPDY attach) | -| services | get, create, update, delete | NodePort Service management | +| services | get, create, update, delete, list | NodePort/LoadBalancer Service management | | jobs | get, create, delete | Egg installation scripts | -| configmaps | get, create, delete | Install script storage (multi-node) | +| configmaps | get, create, update, delete | Install script + identity files (multi-node) | | resourcequotas | get, create, update | Namespace resource limits (if enabled) | | limitranges | get, create, update | Container default limits (if enabled) | | persistentvolumeclaims | get, create, update, delete, list | PVC storage lifecycle (if enabled) | ### Cluster-scoped -| Resource | Verbs | Purpose | -|---------------------------------------|-------|---------------------------------| -| pods (metrics.k8s.io/v1beta1) | get | CPU/memory usage polling | +| Resource | Verbs | Purpose | +|---------------------------------------|-------|--------------------------------------------------------| +| pods (metrics.k8s.io/v1beta1) | get | CPU/memory usage polling | +| nodes | get | Read node addresses for the IP-allocation endpoint | +| nodes/proxy *(optional)* | get | Kubelet stats fallback when metrics-server is absent | + +> `nodes/proxy` is a broad, cluster-wide permission and is therefore **not** +> granted by `clusterrole-metrics.yaml`. Apply `clusterrole-metrics-kubelet.yaml` +> only if you do not run metrics-server and need the kubelet stats fallback. ## Files @@ -55,8 +61,9 @@ Wings requires two levels of RBAC: - `serviceaccount.yaml` — ServiceAccount for Wings Pods - `role.yaml` — Namespace-scoped Role with required permissions - `rolebinding.yaml` — Binds the Role to the ServiceAccount -- `clusterrole-metrics.yaml` — Cluster-scoped access to metrics API +- `clusterrole-metrics.yaml` — Cluster-scoped access to the metrics API + node addresses - `clusterrolebinding-metrics.yaml` — Binds the ClusterRole to the ServiceAccount +- `clusterrole-metrics-kubelet.yaml` — **Optional** `nodes/proxy` ClusterRole + binding for the kubelet stats fallback ## Configuration @@ -89,6 +96,19 @@ sed -i 's/namespace: pelican/namespace: my-namespace/g' kubernetes/*.yaml If you only use `storage_mode: hostpath`, you can remove the `persistentvolumeclaims` resource from `role.yaml`. +### Image pulling + +By default, game server Pods and installation Jobs use `imagePullPolicy: Always` +for remote images so updated tags are re-pulled instead of reusing a stale node +cache (matching the Docker backend); `~`-prefixed local images are never pulled. +For air-gapped clusters, pin the policy: + +```yaml +# config.yml +kubernetes: + image_pull_policy: IfNotPresent # or "Never" +``` + ### Disable metrics If you don't have metrics-server installed, you can skip the ClusterRole and @@ -98,4 +118,18 @@ ClusterRoleBinding. Wings will gracefully degrade (no CPU/memory stats). If you run multiple Wings nodes targeting different namespaces, create a Role and RoleBinding per namespace, but share the ClusterRole/ClusterRoleBinding (it's -namespace-independent). +namespace-independent). Give each instance its own ServiceAccount and use a +distinct `ClusterRoleBinding` subject per ServiceAccount so the cluster-scoped +metrics permission is granted to every Wings ServiceAccount that needs it. + +Do **not** point multiple Wings nodes at the same namespace: they share Pod/ +Service/Job names derived from server UUIDs and the namespaced ResourceQuota/ +LimitRange (`pelican-wings`), so concurrent reconciliation would conflict. + +### Helm-managed installs + +The Helm chart under `chart/pelican-wings` renders all of these resources (and a +config **Secret**) for you. When `serviceAccount.create=false` you must set +`serviceAccount.name` explicitly — the chart refuses to bind its RBAC to the +namespace `default` ServiceAccount. Enable the kubelet fallback with +`rbac.kubeletMetricsFallback=true`. diff --git a/kubernetes/clusterrole-metrics-kubelet.yaml b/kubernetes/clusterrole-metrics-kubelet.yaml new file mode 100644 index 00000000..c95eb551 --- /dev/null +++ b/kubernetes/clusterrole-metrics-kubelet.yaml @@ -0,0 +1,39 @@ +# OPTIONAL kubelet stats/summary fallback for resource metrics. +# +# Only apply this if you do NOT run metrics-server and want Wings to read CPU/ +# memory usage directly from the kubelet. nodes/proxy is a broad, cluster-wide +# permission, so it is kept out of the default clusterrole-metrics.yaml and must +# be opted into explicitly: +# +# kubectl apply -f clusterrole-metrics-kubelet.yaml +# +# Adjust the ServiceAccount name/namespace below to match your install. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: pelican-wings-metrics-kubelet + labels: + app.kubernetes.io/managed-by: pelican-wings + app.kubernetes.io/component: wings +rules: + # Proxy to the kubelet stats/summary API for resource metrics when + # metrics-server is not installed. + - apiGroups: [""] + resources: ["nodes/proxy"] + verbs: ["get"] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: pelican-wings-metrics-kubelet + labels: + app.kubernetes.io/managed-by: pelican-wings + app.kubernetes.io/component: wings +subjects: + - kind: ServiceAccount + name: pelican-wings + namespace: pelican +roleRef: + kind: ClusterRole + name: pelican-wings-metrics-kubelet + apiGroup: rbac.authorization.k8s.io diff --git a/kubernetes/clusterrole-metrics.yaml b/kubernetes/clusterrole-metrics.yaml index 88b461c8..82624ffd 100644 --- a/kubernetes/clusterrole-metrics.yaml +++ b/kubernetes/clusterrole-metrics.yaml @@ -17,8 +17,7 @@ rules: - apiGroups: [""] resources: ["nodes"] verbs: ["get"] - # Proxy to kubelet stats/summary API for resource metrics when - # metrics-server is not installed. - - apiGroups: [""] - resources: ["nodes/proxy"] - verbs: ["get"] + # NOTE: The kubelet stats/summary fallback (nodes/proxy) is intentionally NOT + # granted here because it is a broad, cluster-wide permission. If you do not + # run metrics-server and need that fallback, additionally apply + # clusterrole-metrics-kubelet.yaml. diff --git a/router/router_system.go b/router/router_system.go index 8a2df070..7e366672 100644 --- a/router/router_system.go +++ b/router/router_system.go @@ -125,6 +125,14 @@ func getSystemIps(c *gin.Context) { interfaces = append(interfaces, ip) } } + + // If IP discovery failed across the board, surface an error instead of + // returning a misleading empty-but-successful response that the Panel + // would treat as "no assignable IPs". + if len(interfaces) == 0 { + middleware.CaptureAndAbort(c, errors.New("failed to discover any assignable IP addresses in kubernetes mode; check node/LoadBalancer RBAC or configure kubernetes.system_ips")) + return + } } else { ips, err := system.GetSystemIps() if err != nil { diff --git a/server/install_kubernetes.go b/server/install_kubernetes.go index b0f5ed5c..f102ec4c 100644 --- a/server/install_kubernetes.go +++ b/server/install_kubernetes.go @@ -39,7 +39,7 @@ func (s *Server) internalInstallKubernetes(script *remote.InstallationScript) er s.Events().Publish(DaemonMessageEvent, "Starting installation process via Kubernetes Job, this could take a few minutes...") // Write the install script to disk for mounting into the Job Pod. - if err := ip.WriteInstallScript(); err != nil { + if err := ip.WriteInstallScript(s.Context()); err != nil { return errors.WithMessage(err, "install: failed to write installation script") } diff --git a/server/manager.go b/server/manager.go index 80a97c9d..77e2bee5 100644 --- a/server/manager.go +++ b/server/manager.go @@ -215,6 +215,9 @@ func (m *Manager) InitServer(data remote.ServerConfigurationResponse) (*Server, meta := kubernetes.Metadata{ Image: s.Config().Container.Image, } + if pc := s.ProcessConfiguration(); pc != nil { + meta.Stop = pc.Stop + } if env, err := kubernetes.New(s.ID(), &meta, envCfg); err != nil { return nil, err } else { diff --git a/server/update.go b/server/update.go index 596bace1..511f255e 100644 --- a/server/update.go +++ b/server/update.go @@ -4,6 +4,7 @@ import ( "time" "github.com/exonical/wings/environment/docker" + "github.com/exonical/wings/environment/kubernetes" "github.com/exonical/wings/environment" ) @@ -39,6 +40,14 @@ func (s *Server) SyncWithEnvironment() { e.SetStopConfiguration(s.ProcessConfiguration().Stop) } + // The Kubernetes environment likewise needs the configured image and stop + // configuration so command-based stops and image updates take effect. + if e, ok := s.Environment.(*kubernetes.Environment); ok { + s.Log().Debug("syncing stop configuration with configured kubernetes environment") + e.SetImage(cfg.Container.Image) + e.SetStopConfiguration(s.ProcessConfiguration().Stop) + } + // If build limits are changed, environment variables also change. Plus, any modifications to // the startup command also need to be properly propagated to this environment. s.Environment.Config().SetEnvironmentVariables(s.GetEnvironmentVariables()) diff --git a/system/system.go b/system/system.go index 8204c442..29a9f36f 100644 --- a/system/system.go +++ b/system/system.go @@ -112,7 +112,13 @@ func GetSystemInformationWithOptions(kubernetesMode bool) (*Information, error) release, err := osrelease.Read() if err != nil { - return nil, err + // In Kubernetes mode the daemon frequently runs on minimal images that + // lack /etc/os-release; treat it as best-effort and fall back to the + // runtime OS rather than failing the whole system info request. + if !kubernetesMode { + return nil, err + } + release = map[string]string{} } if kubernetesMode {