Added a plugin logging configuration to specify defaults. #285

apollo13 · 2023-09-29T10:53:07Z

This is similar to https://github.com/hashicorp/nomad/pull/10156/files and allows for a configuration like:

plugin "nomad-driver-podman" {
    config {
	extra_labels = ["job_name", "job_id", "task_group_name", "task_name", "namespace", "node_name", "node_id"]
	logging {
		driver = "journald"
		options = {
			tag = "{{index .Config.Labels \"com.hashicorp.nomad.alloc_id\" }}"
		}
	}
    }
}

apollo13 · 2023-09-29T10:54:10Z

config.go

-	Driver  string             `codec:"driver"`
-	Options hclutils.MapStrStr `codec:"options"`
+	Driver  string            `codec:"driver"`
+	Options map[string]string `codec:"options"`


I needed to switch this to a map from MaptStrStr because otherwise message pack would fail in setConfig:

2023-09-29T12:25:52.047+0200 [ERROR] agent: error starting agent: error= | failed to create plugin loader: failed to initialize plugin loader: parsing plugin configurations failed: 1 error occurred: | \t* plugin "podman" (driver): failed to dispense plugin: setting config for plugin "podman" (driver) failed: rpc error: code = Unknown desc = SetConfig failed: msgpack decode error [pos 117]: invalid byte descriptor for decoding bytes, got: 0x74 |

not exactly sure why this happens :/

Hum...I'm not sure why this would happen either. But I'm a bit worried about backwards compatibility of changing this value type as msgpack can be a bit fuzzy with types.

I'm als worried about the godoc for MapStrInt. It seems like this struct was created to support an unconventional syntax that was used in older version of Nomad: hashicorp/nomad@afb0c5a.

It's also interesting that the PR mentions that external plugins should not use this, but we don't document this anywhere. First time I hear about it 🙃

This is only required for built-in types that have backward
compatibility constraints. External drivers should use BlockAttrs
instead, as they see fit.

But, since this is already like this, modifying it would be a breaking change.

Instead, I think it may be better to create a new struct just like this but for driver config instead of task. It's kind of annoying that both need to be kept in-sync (and we should note this in their godoc string), but that's already kind the case since taskConfigSpec and configSpec would need to change as well.

apollo13 · 2023-09-29T10:55:03Z

driver_test.go

@@ -516,6 +516,55 @@ func TestPodmanDriver_logNomad(t *testing.T) {
 	must.StrContains(t, stdoutLog, stderrMagic)
 }

+// Check default plugin logger
+// TODO: Can we make this nicer?


This just tests that the default is set, preferably I'd like to provide a special PluginConfig that explicitly sets this value?

I think this would be fine for now. In the future it would be interesting to turn https://github.com/hashicorp/nomad-driver-podman/blob/main/api/api.go into an interface and inject a mock into the driver that doesn't actually calls Podman and allows us to inspect the config the driver submitted to ContainerCreate.

But that's a whole different work stream 😅

If you do feel like adding more through testing, another scenario to test would be to ensure the task config overwrites the plugin config.

This could be done with a test similar to this, but the plugin is configured with journald and the task with nomad. nomad should be picked as the correct log driver.

apollo13 · 2023-09-29T10:57:16Z

README.md

@@ -342,11 +347,9 @@ Ensure you're running Podman 3.1.0 or higher because of bugs in older versions.
 config {
  logging = {
    driver = "journald"
-    options = [


Was this really supposed to be a list?

Hum...good point, it seems like it's supposed to be map 👍

lgfa29

Thanks for the PR @apollo13!

And apologies for the delay in reviewing it. I'm slowly catching up to all the Ecosystem projects 😅

The main change here would be to avoid modifying the task config type as it causes a backwards incompatibility.

Could you also add an entry to CHANGELOG.md?

Let me know if you have any questions!

lgfa29 · 2023-11-25T02:27:18Z

config.go

-	Driver  string             `codec:"driver"`
-	Options hclutils.MapStrStr `codec:"options"`
+	Driver  string            `codec:"driver"`
+	Options map[string]string `codec:"options"`


Hum...I'm not sure why this would happen either. But I'm a bit worried about backwards compatibility of changing this value type as msgpack can be a bit fuzzy with types.

I'm als worried about the godoc for MapStrInt. It seems like this struct was created to support an unconventional syntax that was used in older version of Nomad: hashicorp/nomad@afb0c5a.

It's also interesting that the PR mentions that external plugins should not use this, but we don't document this anywhere. First time I hear about it 🙃

This is only required for built-in types that have backward
compatibility constraints. External drivers should use BlockAttrs
instead, as they see fit.

But, since this is already like this, modifying it would be a breaking change.

Instead, I think it may be better to create a new struct just like this but for driver config instead of task. It's kind of annoying that both need to be kept in-sync (and we should note this in their godoc string), but that's already kind the case since taskConfigSpec and configSpec would need to change as well.

lgfa29 · 2023-11-25T02:47:30Z

driver_test.go

@@ -516,6 +516,55 @@ func TestPodmanDriver_logNomad(t *testing.T) {
 	must.StrContains(t, stdoutLog, stderrMagic)
 }

+// Check default plugin logger
+// TODO: Can we make this nicer?


I think this would be fine for now. In the future it would be interesting to turn https://github.com/hashicorp/nomad-driver-podman/blob/main/api/api.go into an interface and inject a mock into the driver that doesn't actually calls Podman and allows us to inspect the config the driver submitted to ContainerCreate.

But that's a whole different work stream 😅

If you do feel like adding more through testing, another scenario to test would be to ensure the task config overwrites the plugin config.

This could be done with a test similar to this, but the plugin is configured with journald and the task with nomad. nomad should be picked as the correct log driver.

lgfa29 · 2023-11-25T02:47:56Z

README.md

@@ -342,11 +347,9 @@ Ensure you're running Podman 3.1.0 or higher because of bugs in older versions.
 config {
  logging = {
    driver = "journald"
-    options = [


Hum...good point, it seems like it's supposed to be map 👍

apollo13 · 2023-11-25T17:38:29Z

Hi @lgfa29, thank you for your review. Can you please explain to me why changing the type affects backwards compat (trying to learn more about how nomad works)?

lgfa29 · 2023-11-28T03:58:02Z

Can you please explain to me why changing the type affects backwards compat (trying to learn more about how nomad works)?

Sure! I should preface this by saying that I'm mostly going from the PR description on hashicorp/nomad#5321 which is where MapStrStr was introduced.

HCL supports two syntax: native HCL and JSON. HCLv1 is somewhat under-specified in some senses, which create a few ambiguities, meaning two different HCL/JSON files may be decoded into the same struct.

hashicorp/nomad#5321 lists some examples:

// hcl block
port_map {
  http = 80
  https = 443
}

// hcl assignment
port_map = {
  http  = 80
  https = 443
}

// json single element array of map (default in API response)
{"port_map": [{"http": 80, "https": 443}]}

// json array of individual maps (supported accidentally iiuc)
{"port_map: [{"http": 80}, {"https": 443}]}

HCLv2 is more strict, and makes a block (port { ... }) and a map assignment (port = { ... }) to be two different things. So, in order to support job files written with the old syntax, MapStrStr was created to manually decoded them the same way.

In this scenario, this means that a Podman driver written like this (note the missing = after options) would no longer work:

task "..." {
  driver = "podman"

  config {
    logging = {
      driver = "journald"
      options {
        "tag" = "redis"
      }
    }
  }
  # ...
}

But it's probably worth testing it by running a task like the above using the latest driver version available and then comparing it to this PR.

It would also be interesting to test an upgrade path.

Start non-dev Nomad cluster (need to retain state).
Run a Podman job with a task like the above.
Stop Nomad client.
Update Podman driver with this PR.
Start Nomad client and verify if the alloc was able to be recovered.

apollo13 · 2024-01-12T10:41:27Z

Thank you for the explanation @lgfa29. Would be great if you could take the changes for a testrun and maybe merge it :)

EDIT:// lemme fix the tests.

apollo13 · 2024-01-12T11:12:49Z

@lgfa29 So when the plugin config is constructed in tests like this: pluginConfig := PluginConfig{} the defaults from the spec obviously don't apply. I am wondering at which point I should handle an empty driver, would it be here https://github.com/hashicorp/nomad-driver-podman/pull/285/files#diff-d16a4232641fc7ed88a3737c22b4f54d7e03dd7fdf9053530fde70e5bca63fb1R514 or should the tests create a "proper" config. Not sure enough about nomad internals to know how to proceed.

apollo13 · 2024-01-12T11:22:13Z

driver.go

+	} else {
+		d.logger.Trace("no podman log driver provided, defaulting to plugin config")
+		switch d.config.Logging.Driver {
+		case "": // The default is nomad


Pushed this as the fix for the mentioned "issue", not sure if this should go somewhere else or if it would be possible to always properly initialize the Logging struct.

Yes, I think this is the correct spot. Just changed this a little in 79fa570#diff-d16a4232641fc7ed88a3737c22b4f54d7e03dd7fdf9053530fde70e5bca63fb1L521-R521 to avoid the use of fallthrough since it can be a little confusing to read.

apollo13 · 2024-01-30T19:57:56Z

@lgfa29 👋 would love a review :)

lgfa29

Hi @apollo13, apologies for the delay here. This PR fell through the cracks during the break....

The changes look good to me! I pushed a commit to add a CHANGELOG entry and also to fix how the driver config is used.

Previously only the Driver value was being checked, which is documented to default to nomad, so users may not set this value, and I don't think we were setting it, but rather assuming an empty string to be "nomad".

So if someone only set the Options in their task config they would expect Driver to be nomad and the Options applied to the logging config, which would not be case.

In practice we don't have any Options for nomad, so this is not likely to happen now, but just playing a bit safe here 😄

I will run another round of the upgrade path test I mentioned tomorro just to be sure it all works as expected, but the code LGTM!

Thanks again for the contribution!

This is similar to https://github.com/hashicorp/nomad/pull/10156/files and allows for a configuration like: ``` plugin "nomad-driver-podman" { config { extra_labels = ["job_name", "job_id", "task_group_name", "task_name", "namespace", "node_name", "node_id"] logging { driver = "journald" options = { tag = "{{index .Config.Labels \"com.hashicorp.nomad.alloc_id\" }}" } } } } ```

Since `nomad` is the default value for `Driver` users may be able to specify just `Options` in the task `config`, so we need to check if no value are set.

lgfa29 · 2024-02-08T18:08:52Z

Testing went well. I had to rebase the branch to fix a merge conflict in the README, but I think we're good to go!

apollo13 · 2024-02-08T18:19:02Z

Thank you very much and sorry for forgetting to rebase.

lgfa29 · 2024-02-08T23:21:31Z

Ah no worries. I caused the conflict when I pushed a CHANGELOG entry 😅

apollo13 requested review from shoenig, jrasell, lgfa29 and towe75 as code owners September 29, 2023 10:53

apollo13 commented Sep 29, 2023

View reviewed changes

apollo13 force-pushed the default_logging branch from 2ba5e70 to 99e3bce Compare September 29, 2023 10:55

apollo13 commented Sep 29, 2023

View reviewed changes

lgfa29 suggested changes Nov 25, 2023

View reviewed changes

lgfa29 self-assigned this Nov 25, 2023

apollo13 force-pushed the default_logging branch from e88ad1a to 132436f Compare January 12, 2024 10:40

apollo13 force-pushed the default_logging branch 2 times, most recently from c2ba6be to 3b3cf3a Compare January 12, 2024 11:18

apollo13 commented Jan 12, 2024

View reviewed changes

lgfa29 approved these changes Feb 8, 2024

View reviewed changes

apollo13 and others added 4 commits February 8, 2024 11:58

Keep backwards compat.

2cae585

Fix default logging driver option.

acba26c

config: minor fixes on driver logging config

f170f48

Since `nomad` is the default value for `Driver` users may be able to specify just `Options` in the task `config`, so we need to check if no value are set.

lgfa29 force-pushed the default_logging branch from 79fa570 to f170f48 Compare February 8, 2024 18:08

lgfa29 merged commit 4deb5c1 into hashicorp:main Feb 8, 2024
1 of 2 checks passed

apollo13 deleted the default_logging branch February 8, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added a plugin logging configuration to specify defaults. #285

Added a plugin logging configuration to specify defaults. #285

apollo13 commented Sep 29, 2023

apollo13 Sep 29, 2023

lgfa29 Nov 25, 2023

apollo13 Sep 29, 2023

lgfa29 Nov 25, 2023

apollo13 Sep 29, 2023

lgfa29 Nov 25, 2023

lgfa29 left a comment

lgfa29 Nov 25, 2023

lgfa29 Nov 25, 2023

lgfa29 Nov 25, 2023

apollo13 commented Nov 25, 2023

lgfa29 commented Nov 28, 2023

apollo13 commented Jan 12, 2024 •

edited

Loading

apollo13 commented Jan 12, 2024

apollo13 Jan 12, 2024

lgfa29 Feb 8, 2024

apollo13 commented Jan 30, 2024

lgfa29 left a comment

lgfa29 commented Feb 8, 2024

apollo13 commented Feb 8, 2024

lgfa29 commented Feb 8, 2024

Added a plugin logging configuration to specify defaults. #285

Added a plugin logging configuration to specify defaults. #285

Conversation

apollo13 commented Sep 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgfa29 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apollo13 commented Nov 25, 2023

lgfa29 commented Nov 28, 2023

apollo13 commented Jan 12, 2024 • edited Loading

apollo13 commented Jan 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apollo13 commented Jan 30, 2024

lgfa29 left a comment

Choose a reason for hiding this comment

lgfa29 commented Feb 8, 2024

apollo13 commented Feb 8, 2024

lgfa29 commented Feb 8, 2024

apollo13 commented Jan 12, 2024 •

edited

Loading