Merge pull request #4057 from zendesk/grosser/norep

allow not setting replicas for deployments

plugins/kubernetes/README.md

@@ -156,7 +156,7 @@ Prefer `spec.updateStrategy.type=RollingUpdate`
 ### Server-side apply
 
 Set `metadata.annotations.samson/server_side_apply='true'` and use a valid template.
-This only works for kubernetes 1.16+ clusters, but will soon be the default way samson works,
+This only works for kubernetes 1.16+ clusters,
 see [kubernetes docs](https://kubernetes.io/docs/reference/using-api/api-concepts/#server-side-apply) for details.
 
 ### Duplicate deployments
@@ -201,6 +201,15 @@ Samson sets namespaces to the deploygroups `kubernetes_namespace` if no `metadat
 
 For namespace-less resources, set `metadata.namespace:` (which will result in `nil`)
 
+### Deployments without replicas
+
+When using a `HorizontalPodAutoscaler` for your `Deployment` or `StatefulSet`, it is recommended to not set `spec.replicas`.
+
+on the `Deployment`
+- set `metadata.annotations.samson/NoReplicas: "true"`
+- set `metadata.annotations.samson/server_side_apply: "true"`
+- do not set `spec.replicas`
+
 ### Using custom resource names
 
 When not using project namespaces samson overrides each resource name in a particular role with the resource and service name set in the UI to prevent

plugins/kubernetes/app/models/kubernetes/resource.rb

@@ -131,7 +131,11 @@ def recreate?
       end
 
       def server_side_apply?
-        @template.dig(:metadata, :annotations, :"samson/server_side_apply") == "true"
+        self.class.server_side_apply?(@template)
+      end
+
+      public_class_method def self.server_side_apply?(template)
+        template.dig(:metadata, :annotations, :"samson/server_side_apply") == "true"
       end
 
       def error_location

plugins/kubernetes/app/models/kubernetes/template_filler.rb

@@ -44,7 +44,11 @@ def to_hash(verification: false)
           make_stateful_set_match_service if kind == 'StatefulSet'
           set_pre_stop if kind == 'Deployment'
           set_name
-          (set_replica_target || validate_replica_target_is_supported) if kind != 'PodTemplate'
+          if ['Deployment', 'StatefulSet'].include?(kind)
+            set_replica_target
+          else
+            validate_replica_target_is_supported
+          end
           set_spec_template_metadata
           set_docker_image unless verification
           set_resource_usage
@@ -385,17 +389,16 @@ def set_init_containers
     end
 
     def set_replica_target
-      key = [:spec, :replicas]
-      target =
-        if ['StatefulSet', 'Deployment'].include?(template[:kind])
-          template
-        else
-          # custom resource that has replicas set on itself or it's template
-          templates = [template] + (template[:spec] || {}).values_at(*RoleConfigFile.template_keys(template))
-          templates.detect { |c| c.dig(*key) }
+      if template.dig(:metadata, :annotations, :"samson/NoReplicas") == "true"
+        if template.dig(:spec, :replicas)
+          raise Samson::Hooks::UserError, "Do not set spec.replicas with NoReplicas"
         end
-
-      target&.dig_set key, @doc.replica_target
+        unless Kubernetes::Resource::Base.server_side_apply?(template)
+          raise Samson::Hooks::UserError, "Set metadata.annotations.samson/server_side_apply: 'true' with NoReplicas"
+        end
+      else
+        template.dig_set [:spec, :replicas], @doc.replica_target
+      end
     end
 
     def validate_replica_target_is_supported

plugins/kubernetes/test/models/kubernetes/template_filler_test.rb

@@ -136,16 +136,6 @@ def with_init_container(container)
       result.dig(:spec, :template, :metadata, :annotations, :"samson/deploy_url").must_be :blank?
     end
 
-    it "sets replicas for templates" do
-      raw_template[:apiVersion] = "zendesk.com/v1alpha1"
-      raw_template[:kind] = "ShardedDeployment"
-      raw_template[:spec].delete :replicas
-      raw_template[:spec][:template][:spec][:replicas] = 1
-      result = template.to_hash
-      result[:spec][:replicas].must_be_nil
-      result[:spec][:template][:spec][:replicas].must_equal 2
-    end
-
     it "does not fail when env/secrets are missing during deletion" do
       raw_template[:spec][:template][:metadata][:annotations] = {"secret/FOO": "bar"}
       doc.replica_target = 0
@@ -169,13 +159,6 @@ def with_init_container(container)
     end
 
     describe "name" do
-      it "sets name for unknown non-primary kinds" do
-        raw_template[:apiVersion] = "zendesk.com/v1alpha1"
-        raw_template[:kind] = "ShardedDeployment"
-        raw_template[:spec][:template][:spec].delete(:containers)
-        template.to_hash[:metadata][:name].must_equal "test-app-server"
-      end
-
       it "keeps resource name when keep_name is set" do
         raw_template[:metadata][:name] = "foobar"
         raw_template[:metadata][:annotations] = {"samson/keep_name": 'true'}
@@ -225,12 +208,39 @@ def with_init_container(container)
       end
 
       it "can read CRDs from currently deploying role" do
+        doc.replica_target = 1
         raw_template[:kind] = "MyCustomResource"
         doc.created_cluster_resources = {"MyCustomResource" => {"namespaced" => true}}
         template.to_hash[:metadata][:namespace].must_equal "pod1"
       end
     end
 
+    describe "NoReplicas" do
+      before do
+        raw_template[:spec].delete :replicas
+        raw_template[:metadata][:annotations] = {
+          "samson/NoReplicas": "true",
+          "samson/server_side_apply": "true"
+        }
+      end
+
+      it "can set no replicas" do
+        template.to_hash[:spec].keys.wont_include :replicas
+      end
+
+      it "warns when user set replicas which would be confusing" do
+        raw_template[:spec][:replicas] = 2
+        e = assert_raises(Samson::Hooks::UserError) { template.to_hash }
+        e.message.must_include "NoReplicas"
+      end
+
+      it "warns when server_side_apply is disabled, which would pick the default of 1" do
+        raw_template[:metadata][:annotations][:"samson/server_side_apply"] = "false"
+        e = assert_raises(Samson::Hooks::UserError) { template.to_hash }
+        e.message.must_include "NoReplicas"
+      end
+    end
+
     describe "unique deployments" do
       let(:labels) do
         hash = template.to_hash
@@ -804,6 +814,7 @@ def secret_annotations(hash)
       let(:image) { build.docker_repo_digest }
 
       before do
+        doc.replica_target = 1
         raw_template.replace(
           YAML.safe_load(
             read_kubernetes_sample_file('kubernetes_podtemplate.yml')
@@ -917,6 +928,7 @@ def lifecycle_defined?
         end
 
         it "does not add preStop to DaemonSet" do
+          doc.replica_target = 1
           raw_template[:kind] = 'DaemonSet'
           refute lifecycle_defined?
         end