[Issue 276] QEMU Agent configuration settings #287
Conversation
mpywell
changed the title
- Extend QEMU agent handling by introducing qemu_quest_agent block, deprecating qemu_agent bool - Enable setting the agent type - Enable toggling freeze/thaw filesystems on backup - Enable toggling fstrim after disk move or VM migration - Each added setting defaults to Proxmox's backend defaults when not configured.
mpywell
force-pushed
the
feat/276_agent_settings
branch
from
b3848f4 to
0b738ef
Compare
There was a problem hiding this comment.
Thanks for this one @mpywell!
Overall LGTM, I left a couple of suggestions, feel free to address them and I'll come back for another round of review after that.
builder/proxmox/common/config.go
Outdated
| // Sets the Agent Type. Must be `isa` or `virtio`. Defaults to `virtio` | ||
| Type string `mapstructure:"type"` | ||
| // Enable freeze/thaw of guest filesystem on backup. Defaults to `true` | ||
| Freeze config.Trilean `mapstructure:"freeze"` |
There was a problem hiding this comment.
If this is an option we're introducing here, and the default is true, in general I would suggest maybe inverting the option to something like disable_freeze, so it's clear that we freeze by default, and only don't do it if requested.
There was a problem hiding this comment.
Makes sense, have rerolled with disable_freeze as a bool
| @@ -606,7 +648,27 @@ func (c *Config) Prepare(upper interface{}, raws ...interface{}) ([]string, []st | |||
|
|
|||
| // Default qemu_agent to true | |||
| if c.Agent != config.TriFalse { | |||
| c.Agent = config.TriTrue | |||
| warnings = append(warnings, "qemu_agent is deprecated and will be removed in a future release. define QEMU agent settings in a qemu_guest_agent block instead") | |||
There was a problem hiding this comment.
I would suggest we only print that warning if qemu_agent is unset here, not if it's not explicitly false, otherwise we'll see it all the time unless we add qemu_agent = false to the template.
There was a problem hiding this comment.
It occurred to me that we don't need to set a default value for c.Agent given the replacement c.GuestAgent.Enabled has a default set further down, so I've changed the logic to warn and convert the value only if the c.Agent is not Unset.
builder/proxmox/iso/config_test.go
Outdated
| @@ -103,8 +103,8 @@ func TestBasicExampleFromDocsIsValid(t *testing.T) { | |||
| if b.config.Disks[0].CacheMode != "none" { | |||
| t.Errorf("Expected disk cache mode to be 'none', got %s", b.config.Disks[0].CacheMode) | |||
| } | |||
| if b.config.Agent.True() != true { | |||
| t.Errorf("Expected Agent to be true, got %t", b.config.Agent.True()) | |||
| if b.config.GuestAgent.Enabled.True() != true { | |||
There was a problem hiding this comment.
I would suggest simplifying this one;
| if b.config.GuestAgent.Enabled.True() != true { | |
| if !b.config.GuestAgent.Enabled.True() { |
There was a problem hiding this comment.
Done, added to reroll
- fix qemu_agent deprecation warnings and conversion - invert freeze config option for qemu_guest_agent - simplify test logic for TestBasicExampleFromDocsIsValid
|
Hi @lbajolet-hashicorp have rerolled this one and added comments in the review |
Extend QEMU agent handling by introducing the
qemu_quest_agentblockqemu_guest_agentadds the ability to set each of the QEMU Guest Agent's available settings:qemu_agentbool)Each added setting defaults to Proxmox's backend defaults when not configured.
Closes #276