Fixes and Docs (#44)

* fix typo in env var rejection * copy env_from if add_parent_env is true * more documentation * only set serviceAccount if not already defined * remove duplicate documentation * set PHX_SERVER only if not already passed * add some tests * disable nesting credo * add changelog
mruoss · Aug 27, 2024 · 44b4915 · 44b4915
1 parent 9f60142
commit 44b4915
Show file tree

Hide file tree

Showing 5 changed files with 100 additions and 67 deletions.
diff --git a/.credo.exs b/.credo.exs
@@ -130,7 +130,6 @@
           {Credo.Check.Refactor.MatchInCondition, []},
           {Credo.Check.Refactor.NegatedConditionsInUnless, []},
           {Credo.Check.Refactor.NegatedConditionsWithElse, []},
-          {Credo.Check.Refactor.Nesting, []},
           {Credo.Check.Refactor.RedundantWithClauseResult, []},
           {Credo.Check.Refactor.RejectReject, []},
           {Credo.Check.Refactor.UnlessWithElse, []},
@@ -194,6 +193,7 @@
           {Credo.Check.Refactor.MapMap, []},
           {Credo.Check.Refactor.ModuleDependencies, []},
           {Credo.Check.Refactor.NegatedIsNil, []},
+          {Credo.Check.Refactor.Nesting, []},
           {Credo.Check.Refactor.PassAsyncInTestCases, []},
           {Credo.Check.Refactor.PipeChainStart, []},
           {Credo.Check.Refactor.RejectFilter, []},

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,10 +12,22 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 <!--------------------- Don't add new entries after this line --------------------->
 
+## [0.5.0] - 2024-08-27
+
+### Fixed
+
+- `FLAMEK8sBackend.RunnerPodTemplate`: Only set `PHX_SERVER` if it is not passed.
+- `FLAMEK8sBackend.RunnerPodTemplate`: Reject `FLAME_PARENT`, not `FLAME_BACKEND` in passed env vars.
+
 ### Added
 
 - `FLAMEK8sBackend.RunnerPodTemplate`: Set `.metadata.namespace` and `.metadata.generateName` on runner pod if not set ([#43](https://github.com/mruoss/flame_k8s_backend/pull/43))
 
+### Changed
+
+- `FLAMEK8sBackend.RunnerPodTemplate`: Also copy `env_from` if `add_parent_env` is `true`
+- Improve documentation
+
 ## [0.4.3] - 2024-08-22
 
 ### Fixed

diff --git a/lib/flame_k8s_backend/runner_pod_template.ex b/lib/flame_k8s_backend/runner_pod_template.ex
@@ -101,39 +101,6 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
   end
   ```
 
-  ### Advanced Use Case - Passing a Callback Function
-
-  In some cases you might need advanced control over the runner pod manifest.
-  Maybe you want to set node affinity because you need your runners to run on
-  nodes with GPUs or you need additional volumes etc. In this case, you can pass
-  a callback via `runner_pod_tpl` option to the `FLAMEK8sBackend`.
-
-  The callback has to be of type
-  `t:FLAMEK8sBackend.RunnerPodTemplate.callback/0`. The callback will be called
-  with the manifest of the parent pod which can be used to extract information.
-  It should return a pod template as a map
-
-  Define a callback, e.g. in a separate module:
-
-  ```
-  defmodule MyApp.FLAMERunnerPodTemplate do
-    def runner_pod_manifest(parent_pod_manifest) do
-      %{
-        "metadata" => %{
-          # namespace, labels, ownerReferences,...
-        },
-        "spec" => %{
-          "containers" => [
-            %{
-              # container definition
-            }
-          ]
-        }
-      }
-    end
-  end
-  ```
-
   Register the backend:
 
   ```
@@ -162,6 +129,17 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
   >   * The container `image` (set to the image of the parent pod's app
   >     container)
 
+  > #### Automatically Defined Environment Variables {: .info}
+  >
+  > Some environment variables are defined automatically on the
+  > runner pod:
+  >
+  >   * `POD_IP` is set to the runner Pod's IP address (`.status.podIP`) - (not overridable)
+  >   * `POD_NAME` is set to the runner Pod's name (`.metadata.name`) - (not overridable)
+  >   * `POD_NAMESPACE` is set to the runner Pod's namespace (`.metadata.namespace`) - (not overridable)
+  >   * `PHX_SERVER` is set to `false` (overridable)
+  >   * `FLAME_PARENT` used internally by FLAME - (not overridable)
+
   ### Options
 
   * `:omit_owner_reference` - Omit generating and appending the parent pod as
@@ -175,6 +153,20 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
 
   defstruct [:env, :resources, add_parent_env: true]
 
+  @typedoc """
+  Describing the Runner Pod Template struct
+
+  ### Fields
+
+  * `env` - a map describing a Pod environment variable declaration
+    `%{"name" => "MY_ENV_VAR", "value" => "my_env_var_value"}`
+
+  * `resources` - Pod resource requests and limits.
+
+  * `add_parent_env` - If true, all env vars of the main container
+    including `envFrom` are copied to the runner pod.
+    default: `true`
+  """
   @type t :: %__MODULE__{
           env: map() | nil,
           resources: map() | nil,
@@ -208,10 +200,8 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
     app_container = app_container(parent_pod_manifest, opts)
     env = template_opts.env || []
 
-    parent_env =
-      if template_opts.add_parent_env,
-        do: app_container["env"],
-        else: []
+    parent_env = if template_opts.add_parent_env, do: app_container["env"]
+    parent_env_from = if template_opts.add_parent_env, do: app_container["envFrom"]
 
     runner_pod_template = %{
       "metadata" => %{
@@ -221,7 +211,8 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
         "containers" => [
           %{
             "resources" => template_opts.resources || app_container["resources"],
-            "env" => env ++ parent_env
+            "env" => env ++ List.wrap(parent_env),
+            "envFrom" => List.wrap(parent_env_from)
           }
         ]
       }
@@ -272,37 +263,48 @@ defmodule FLAMEK8sBackend.RunnerPodTemplate do
       |> Map.put("ownerReferences", object_references)
     end)
     |> put_in(~w(spec restartPolicy), "Never")
-    |> put_in(~w(spec serviceAccount), parent_pod_manifest["spec"]["serviceAccount"])
-    |> update_in(["spec", "containers", Access.at(0)], fn container ->
-      container
-      |> Map.put("image", app_container["image"])
-      |> Map.put("name", "runner")
-      |> Map.put_new("env", [])
-      |> Map.update!("env", fn env ->
-        [
-          %{
-            "name" => "POD_NAME",
-            "valueFrom" => %{"fieldRef" => %{"fieldPath" => "metadata.name"}}
-          },
-          %{
-            "name" => "POD_IP",
-            "valueFrom" => %{"fieldRef" => %{"fieldPath" => "status.podIP"}}
-          },
-          %{
-            "name" => "POD_NAMESPACE",
-            "valueFrom" => %{"fieldRef" => %{"fieldPath" => "metadata.namespace"}}
-          },
-          %{"name" => "PHX_SERVER", "value" => "false"},
-          %{"name" => "FLAME_PARENT", "value" => encoded_parent}
-          | Enum.reject(
-              env,
-              &(&1["name"] in ["FLAME_BACKEND", "POD_NAME", "POD_NAMESPACE"])
-            )
-        ]
+    |> update_in(~w(spec), fn spec ->
+      spec
+      |> Map.put("restartPolicy", "Never")
+      |> Map.put_new("serviceAccount", parent_pod_manifest["spec"]["serviceAccount"])
+      |> update_in(["containers", Access.at(0)], fn container ->
+        container
+        |> Map.put("image", app_container["image"])
+        |> Map.put("name", "runner")
+        |> Map.put_new("env", [])
+        |> Map.update!("env", fn env ->
+          [
+            %{
+              "name" => "POD_NAME",
+              "valueFrom" => %{"fieldRef" => %{"fieldPath" => "metadata.name"}}
+            },
+            %{
+              "name" => "POD_IP",
+              "valueFrom" => %{"fieldRef" => %{"fieldPath" => "status.podIP"}}
+            },
+            %{
+              "name" => "POD_NAMESPACE",
+              "valueFrom" => %{"fieldRef" => %{"fieldPath" => "metadata.namespace"}}
+            },
+            %{"name" => "FLAME_PARENT", "value" => encoded_parent}
+            | Enum.reject(
+                env,
+                &(&1["name"] in ["FLAME_PARENT", "POD_NAME", "POD_NAMESPACE", "POD_IP"])
+              )
+          ]
+          |> put_new_env("PHX_SERVER", "false")
+        end)
       end)
     end)
   end
 
+  defp put_new_env(env, name, value) do
+    case get_in(env, [Access.filter(&(&1["name"] == name))]) do
+      [] -> [%{"name" => name, "value" => value} | env]
+      _ -> env
+    end
+  end
+
   defp app_container(parent_pod_manifest, opts) do
     container_access =
       case opts[:app_container_name] do

diff --git a/test/flame_k8s_backend/runner_pod_template_test.exs b/test/flame_k8s_backend/runner_pod_template_test.exs
@@ -48,7 +48,8 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
         )
       end
 
-      MUT.manifest(parent_pod_manifest, callback, make_ref())
+      pod_manifest = MUT.manifest(parent_pod_manifest, callback, make_ref())
+      assert get_in(pod_manifest, env_var_access("PHX_SERVER")) == ["false"]
     end
 
     test "should return pod manifest with data form callback", %{
@@ -64,6 +65,9 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
                 limits:
                   memory: 500Mi
                   cpu: 500
+              env:
+                - name: PHX_SERVER
+                  value: "true"
         """
         |> put_in(
           app_container_access(~w(resources requests)),
@@ -73,6 +77,7 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
 
       pod_manifest = MUT.manifest(parent_pod_manifest, callback, make_ref())
 
+      assert get_in(pod_manifest, env_var_access("PHX_SERVER")) == ["true"]
       assert get_in(pod_manifest, app_container_access(~w(resources requests memory))) == "100Mi"
       assert get_in(pod_manifest, app_container_access(~w(resources limits memory))) == "500Mi"
     end
@@ -182,6 +187,10 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
       pod_manifest = MUT.manifest(parent_pod_manifest, template_opts, make_ref())
 
       assert get_in(pod_manifest, env_var_access("RELEASE_NODE")) == ["flame_test@$(POD_IP)"]
+
+      assert get_in(pod_manifest, app_container_access("envFrom")) == [
+               %{"configMapRef" => %{"name" => "some-config-map"}}
+             ]
     end
 
     test "Only default envs if add_parent_env is set to false", %{
@@ -193,6 +202,8 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
 
       assert get_in(pod_manifest, app_container_access(~w(resources requests memory))) == "100Mi"
       assert get_in(pod_manifest, env_var_access("PHX_SERVER")) == ["false"]
+      assert get_in(pod_manifest, app_container_access("envFrom")) == []
+
       parent = flame_parent(pod_manifest)
       assert parent.ref == ref
     end
@@ -207,6 +218,10 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
 
       assert get_in(pod_manifest, env_var_access("RELEASE_NODE")) == ["flame_test@$(POD_IP)"]
       assert get_in(pod_manifest, env_var_access("FOO")) == ["bar"]
+
+      assert get_in(pod_manifest, app_container_access("envFrom")) == [
+               %{"configMapRef" => %{"name" => "some-config-map"}}
+             ]
     end
 
     test "No parent envs if add_parent_env is set to false", %{
@@ -217,6 +232,7 @@ defmodule FLAMEK8sBackend.RunnerPodTemplateTest do
 
       assert get_in(pod_manifest, env_var_access("RELEASE_NODE")) == []
       assert get_in(pod_manifest, env_var_access("FOO")) == ["bar"]
+      assert get_in(pod_manifest, app_container_access("envFrom")) == []
     end
   end
 

diff --git a/test_support/pods.ex b/test_support/pods.ex
@@ -44,6 +44,9 @@ defmodule FLAMEK8sBackend.TestSupport.Pods do
         value: name
       - name: RELEASE_NODE
         value: flame_test@$(POD_IP)
+      envFrom:
+      - configMapRef:
+          name: some-config-map
       image: flame-test-image:0.1.0
       imagePullPolicy: IfNotPresent
       name: flame