CSI: make plugin health_timeout configurable in csi_plugin stanza

[ggriffiths]

Fixes #13179

Test results: Configured health_timeout as 5m and introduced an artificial error to make sure Portworx was killed after 5m:

Recent Events:
Time                  Type                     Description
2022-06-10T20:05:40Z  Killed                   Task successfully killed
2022-06-10T20:05:40Z  Terminated               Exit Code: 2, Exit Message: "Docker container exited with non-zero exit code: 2"
2022-06-10T20:05:40Z  Killing                  CSI plugin did not become healthy before configured 5m0s health timeout
2022-06-10T20:05:40Z  Plugin became unhealthy  Error: CSI plugin failed probe: timeout while connecting to gRPC socket: failed to stat socket: stat /etc/nomad.d/client/csi/plugins/62bdddd8-3955-92d2-cc23-02b6f6204e62/csi.sock: no such file or directory
2022-06-10T19:58:15Z  Started                  Task started by client
2022-06-10T19:58:13Z  Task Setup               Building Task Directory
2022-06-10T19:58:13Z  Received                 Task received by client

Signed-off-by: Grant Griffiths ggriffiths@purestorage.com

[tgross]

This looks great @ggriffiths!

There's a jobspec checklist which I probably should have pointed out in the original issue, but it looks like you've already hit almost everything needed there except for the implementation for structs/diff.go.

We'll also want this new field added to the CSI plugin parameters documentation.

Thanks for the PR!

[ggriffiths]

There's a jobspec checklist which I probably should have pointed out in the original issue, but it looks like you've already hit almost everything needed there except for the implementation for structs/diff.go.

@tgross, thanks for the pointer! For structs/diff.go, should I add a new function for the CSIPluginConfig? I'm not seeing any existing logic in diff.go for this struct.

[tgross]

@tgross, thanks for the pointer! For structs/diff.go, should I add a new function for the CSIPluginConfig? I'm not seeing any existing logic in diff.go for this struct.

Oh. You've just discovered a bug in job plan output. 😊 We detect that the csi_plugin block has changed but aren't producing a diff of what fields have changed. This failing test shows the problem:

failing test

diff --git a/nomad/structs/diff_test.go b/nomad/structs/diff_test.go
index 1a5751a8c..153c5bb94 100644
--- a/nomad/structs/diff_test.go
+++ b/nomad/structs/diff_test.go
@@ -7377,6 +7377,40 @@ func TestTaskDiff(t *testing.T) {
                                },
                        },
                },
+               {
+                       Name: "CSIPlugin edited",
+                       Old: &Task{
+                               CSIPluginConfig: &TaskCSIPluginConfig{
+                                       ID:       "id0",
+                                       Type:     "monolith",
+                                       MountDir: "/csi",
+                               },
+                       },
+                       New: &Task{
+                               CSIPluginConfig: &TaskCSIPluginConfig{
+                                       ID:       "id0",
+                                       Type:     "monolith",
+                                       MountDir: "/whatever",
+                               },
+                       },
+                       Expected: &TaskDiff{
+                               Type: DiffTypeEdited,
+                               Objects: []*ObjectDiff{
+                                       {
+                                               Type: DiffTypeEdited,
+                                               Name: "CSIPluginConfig",
+                                               Fields: []*FieldDiff{
+                                                       {
+                                                               Type: DiffTypeEdited,
+                                                               Name: "MountDir",
+                                                               Old:  "/csi",
+                                                               New:  "/whatever",
+                                                       },
+                                               },
+                                       },
+                               },
+                       },
+               },
        }

        for _, c := range cases {

Because that's a bug and this PR is for a feature improvement, we'll need to handle those as separate PRs so we can backport the plan fix. So don't worry about diff for now and I'll come back for that later once this has been merged.

[tgross]

LGTM! This will ship in the upcoming Nomad 1.3.2

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.