From 6688c0de51f02b8e7a500985bfa33237170e7cf0 Mon Sep 17 00:00:00 2001 From: Marijn Valk Date: Mon, 13 Jan 2025 04:07:23 +0100 Subject: [PATCH] Helm: add deployment strategy to chart (#27048) ## Summary & Motivation Closes: https://github.com/dagster-io/dagster/issues/26445 ## How I Tested These Changes Added test --- .../templates/deployment-user.yaml | 4 ++ .../values.schema.json | 18 +++++++++ .../dagster-user-deployments/values.yaml | 4 ++ .../subschema/user_deployments.py | 1 + .../schema/schema/charts/utils/kubernetes.py | 9 +++++ .../schema_tests/test_user_deployments.py | 40 ++++++++++++++++++- helm/dagster/values.schema.json | 18 +++++++++ helm/dagster/values.yaml | 4 ++ 8 files changed, 97 insertions(+), 1 deletion(-) diff --git a/helm/dagster/charts/dagster-user-deployments/templates/deployment-user.yaml b/helm/dagster/charts/dagster-user-deployments/templates/deployment-user.yaml index 3f011d16d0321..926c4d6e5ea21 100644 --- a/helm/dagster/charts/dagster-user-deployments/templates/deployment-user.yaml +++ b/helm/dagster/charts/dagster-user-deployments/templates/deployment-user.yaml @@ -14,6 +14,10 @@ metadata: annotations: {{ $deployment.annotations | toYaml | nindent 4 }} spec: replicas: 1 + {{- if $deployment.deploymentStrategy }} + strategy: + {{- toYaml $deployment.deploymentStrategy | nindent 4 }} + {{- end }} selector: matchLabels: {{- include "dagster.selectorLabels" $ | nindent 6 }} diff --git a/helm/dagster/charts/dagster-user-deployments/values.schema.json b/helm/dagster/charts/dagster-user-deployments/values.schema.json index 3bd0f73c2141d..ea8c61a25ed93 100644 --- a/helm/dagster/charts/dagster-user-deployments/values.schema.json +++ b/helm/dagster/charts/dagster-user-deployments/values.schema.json @@ -27,6 +27,13 @@ "title": "Container", "type": "object" }, + "DeploymentStrategy": { + "$ref": "https://kubernetesjsonschema.dev/v1.18.0/_definitions.json#/definitions/io.k8s.api.apps.v1.DeploymentStrategy", + "additionalProperties": true, + "properties": {}, + "title": "DeploymentStrategy", + "type": "object" + }, "EnvVar": { "$ref": "https://kubernetesjsonschema.dev/v1.18.0/_definitions.json#/definitions/io.k8s.api.core.v1.EnvVar", "additionalProperties": true, @@ -519,6 +526,17 @@ ], "default": null, "title": "Sidecarcontainers" + }, + "deploymentStrategy": { + "anyOf": [ + { + "$ref": "#/$defs/DeploymentStrategy" + }, + { + "type": "null" + } + ], + "default": null } }, "required": [ diff --git a/helm/dagster/charts/dagster-user-deployments/values.yaml b/helm/dagster/charts/dagster-user-deployments/values.yaml index 2bae88fabbeff..041ed212ef3df 100644 --- a/helm/dagster/charts/dagster-user-deployments/values.yaml +++ b/helm/dagster/charts/dagster-user-deployments/values.yaml @@ -152,6 +152,10 @@ deployments: startupProbe: enabled: false + # Strategy to follow when replacing old pods with new pods. See: + # https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy + deploymentStrategy: {} + service: annotations: {} diff --git a/helm/dagster/schema/schema/charts/dagster_user_deployments/subschema/user_deployments.py b/helm/dagster/schema/schema/charts/dagster_user_deployments/subschema/user_deployments.py index 776ea26251ed6..719289cb022ec 100644 --- a/helm/dagster/schema/schema/charts/dagster_user_deployments/subschema/user_deployments.py +++ b/helm/dagster/schema/schema/charts/dagster_user_deployments/subschema/user_deployments.py @@ -41,6 +41,7 @@ class UserDeployment(BaseModel): schedulerName: Optional[str] = None initContainers: Optional[list[kubernetes.Container]] = None sidecarContainers: Optional[list[kubernetes.Container]] = None + deploymentStrategy: Optional[kubernetes.DeploymentStrategy] = None class UserDeployments(BaseModel): diff --git a/helm/dagster/schema/schema/charts/utils/kubernetes.py b/helm/dagster/schema/schema/charts/utils/kubernetes.py index 6350667672642..c99bf52c72698 100644 --- a/helm/dagster/schema/schema/charts/utils/kubernetes.py +++ b/helm/dagster/schema/schema/charts/utils/kubernetes.py @@ -194,3 +194,12 @@ class Container(BaseModel): "extra": "allow", "json_schema_extra": {"$ref": create_definition_ref("io.k8s.api.core.v1.Container")}, } + + +class DeploymentStrategy(BaseModel): + model_config = { + "extra": "allow", + "json_schema_extra": { + "$ref": create_definition_ref("io.k8s.api.apps.v1.DeploymentStrategy") + }, + } diff --git a/helm/dagster/schema/schema_tests/test_user_deployments.py b/helm/dagster/schema/schema_tests/test_user_deployments.py index 209c634f9059a..e01e0a3e37e4e 100644 --- a/helm/dagster/schema/schema_tests/test_user_deployments.py +++ b/helm/dagster/schema/schema_tests/test_user_deployments.py @@ -1,6 +1,6 @@ import json import subprocess -from typing import Union +from typing import Any, Optional, Union import pytest from dagster_k8s.models import k8s_model_from_dict, k8s_snake_case_dict @@ -1392,3 +1392,41 @@ def test_old_env(template: HelmTemplate, user_deployment_configmap_template): [cm] = user_deployment_configmap_template.render(helm_values) assert cm.data["test_env"] == "test_value" + + +@pytest.mark.parametrize( + "strategy,expected", + [ + (None, None), + ({}, None), + ( + { + "type": "RollingUpdate", + "rollingUpdate": {"maxSurge": 10, "maxUnavailable": 1}, + }, + { + "type": "RollingUpdate", + "rolling_update": {"max_surge": 10, "max_unavailable": 1}, + }, + ), + ( + {"type": "Recreate"}, + {"type": "Recreate", "rolling_update": None}, + ), + ], +) +def test_deployment_strategy( + template: HelmTemplate, + strategy: Optional[dict[str, Any]], + expected: Optional[dict[str, Any]], +): + deployment = create_simple_user_deployment("foo") + if strategy: + deployment.deploymentStrategy = kubernetes.DeploymentStrategy.model_construct(**strategy) + helm_values = DagsterHelmValues.construct( + dagsterUserDeployments=UserDeployments.model_construct(deployments=[deployment]) + ) + + dagster_user_deployment = template.render(helm_values) + assert len(dagster_user_deployment) == 1 + assert dagster_user_deployment[0].to_dict()["spec"]["strategy"] == expected diff --git a/helm/dagster/values.schema.json b/helm/dagster/values.schema.json index 4859ef982a2a7..9173be4204a5a 100644 --- a/helm/dagster/values.schema.json +++ b/helm/dagster/values.schema.json @@ -865,6 +865,13 @@ "title": "DaemonSchedulerConfig", "type": "object" }, + "DeploymentStrategy": { + "$ref": "https://kubernetesjsonschema.dev/v1.18.0/_definitions.json#/definitions/io.k8s.api.apps.v1.DeploymentStrategy", + "additionalProperties": true, + "properties": {}, + "title": "DeploymentStrategy", + "type": "object" + }, "EnvVar": { "$ref": "https://kubernetesjsonschema.dev/v1.18.0/_definitions.json#/definitions/io.k8s.api.core.v1.EnvVar", "additionalProperties": true, @@ -3086,6 +3093,17 @@ ], "default": null, "title": "Sidecarcontainers" + }, + "deploymentStrategy": { + "anyOf": [ + { + "$ref": "#/$defs/DeploymentStrategy" + }, + { + "type": "null" + } + ], + "default": null } }, "required": [ diff --git a/helm/dagster/values.yaml b/helm/dagster/values.yaml index bda1694c613fd..91af6a47ae0d7 100644 --- a/helm/dagster/values.yaml +++ b/helm/dagster/values.yaml @@ -457,6 +457,10 @@ dagster-user-deployments: startupProbe: enabled: false + # Strategy to follow when replacing old pods with new pods. See: + # https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy + deploymentStrategy: {} + service: annotations: {}