Skip to content

Commit 03f2079

Browse files
bpgbpg
authored
fix(lxc): prevent spurious dns config change when updating initialization block (#1859)
* fix(lxc): prevent spurious `dns` config change when updating `initialization` block Signed-off-by: Pavel Boldyrev <[email protected]>
1 parent f030a49 commit 03f2079

File tree

5 files changed

+154
-59
lines changed

5 files changed

+154
-59
lines changed

‎docs/resources/virtual_environment_container.md

+5
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,11 @@ resource "proxmox_virtual_environment_container" "ubuntu_container" {
3939
name = "veth0"
4040
}
4141
42+
disk {
43+
datastore_id = "local-lvm"
44+
size = 4
45+
}
46+
4247
operating_system {
4348
template_file_id = proxmox_virtual_environment_download_file.latest_ubuntu_22_jammy_lxc_img.id
4449
# Or you can use a volume ID, as obtained from a "pvesm list <storage>"

‎fwprovider/test/resource_container_test.go

+108-52
Original file line numberDiff line numberDiff line change
@@ -61,63 +61,119 @@ func TestAccResourceContainer(t *testing.T) {
6161
name string
6262
step []resource.TestStep
6363
}{
64-
{"create and start container", []resource.TestStep{{
65-
Config: te.RenderConfig(`
66-
resource "proxmox_virtual_environment_container" "test_container" {
67-
node_name = "{{.NodeName}}"
68-
vm_id = {{.TestContainerID}}
69-
disk {
70-
datastore_id = "local-lvm"
71-
size = 4
72-
}
73-
mount_point {
74-
volume = "local-lvm"
75-
size = "4G"
76-
path = "mnt/local"
77-
}
78-
device_passthrough {
79-
path = "/dev/zero"
80-
}
81-
description = <<-EOT
82-
my
83-
description
84-
value
85-
EOT
86-
initialization {
87-
hostname = "test"
88-
ip_config {
89-
ipv4 {
90-
address = "dhcp"
64+
{"create, start and update container", []resource.TestStep{
65+
{
66+
Config: te.RenderConfig(`
67+
resource "proxmox_virtual_environment_container" "test_container" {
68+
node_name = "{{.NodeName}}"
69+
vm_id = {{.TestContainerID}}
70+
timeout_delete = 10
71+
unprivileged = true
72+
disk {
73+
datastore_id = "local-lvm"
74+
size = 4
75+
}
76+
mount_point {
77+
volume = "local-lvm"
78+
size = "4G"
79+
path = "mnt/local"
80+
}
81+
device_passthrough {
82+
path = "/dev/zero"
83+
}
84+
description = <<-EOT
85+
my
86+
description
87+
value
88+
EOT
89+
initialization {
90+
hostname = "test"
91+
ip_config {
92+
ipv4 {
93+
address = "dhcp"
94+
}
9195
}
9296
}
93-
}
94-
network_interface {
95-
name = "vmbr0"
96-
}
97-
operating_system {
98-
template_file_id = "local:vztmpl/{{.ImageFileName}}"
99-
type = "ubuntu"
100-
}
101-
}`, WithRootUser()),
102-
Check: resource.ComposeTestCheckFunc(
103-
resource.TestCheckResourceAttr(accTestContainerName, "description", "my\ndescription\nvalue\n"),
104-
resource.TestCheckResourceAttr(accTestContainerName, "device_passthrough.#", "1"),
105-
func(*terraform.State) error {
106-
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
107-
defer cancel()
97+
network_interface {
98+
name = "vmbr0"
99+
}
100+
operating_system {
101+
template_file_id = "local:vztmpl/{{.ImageFileName}}"
102+
type = "ubuntu"
103+
}
104+
}`, WithRootUser()),
105+
Check: resource.ComposeTestCheckFunc(
106+
ResourceAttributes(accTestContainerName, map[string]string{
107+
"description": "my\ndescription\nvalue\n",
108+
"device_passthrough.#": "1",
109+
"initialization.0.dns.#": "0",
110+
}),
111+
func(*terraform.State) error {
112+
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
113+
defer cancel()
108114

109-
ct := te.NodeClient().Container(accTestContainerID)
110-
err := ct.WaitForContainerStatus(ctx, "running")
111-
require.NoError(te.t, err, "container did not start")
115+
ct := te.NodeClient().Container(accTestContainerID)
116+
err := ct.WaitForContainerStatus(ctx, "running")
117+
require.NoError(te.t, err, "container did not start")
112118

113-
ctInfo, err := ct.GetContainer(ctx)
114-
require.NoError(te.t, err, "failed to get container")
115-
require.NotNil(te.t, ctInfo.DevicePassthrough0)
119+
ctInfo, err := ct.GetContainer(ctx)
120+
require.NoError(te.t, err, "failed to get container")
121+
require.NotNil(te.t, ctInfo.DevicePassthrough0)
116122

117-
return nil
118-
},
119-
),
120-
}}},
123+
return nil
124+
},
125+
),
126+
},
127+
{
128+
Config: te.RenderConfig(`
129+
resource "proxmox_virtual_environment_container" "test_container" {
130+
node_name = "{{.NodeName}}"
131+
vm_id = {{.TestContainerID}}
132+
timeout_delete = 10
133+
unprivileged = true
134+
disk {
135+
datastore_id = "local-lvm"
136+
size = 4
137+
}
138+
mount_point {
139+
volume = "local-lvm"
140+
size = "4G"
141+
path = "mnt/local"
142+
}
143+
device_passthrough {
144+
path = "/dev/zero"
145+
}
146+
description = <<-EOT
147+
my
148+
description
149+
value
150+
EOT
151+
initialization {
152+
hostname = "test"
153+
ip_config {
154+
ipv4 {
155+
address = "172.16.10.10/15"
156+
gateway = "172.16.0.1"
157+
}
158+
}
159+
}
160+
network_interface {
161+
name = "vmbr0"
162+
}
163+
operating_system {
164+
template_file_id = "local:vztmpl/{{.ImageFileName}}"
165+
type = "ubuntu"
166+
}
167+
}`, WithRootUser()),
168+
Check: resource.ComposeTestCheckFunc(
169+
ResourceAttributes(accTestContainerName, map[string]string{
170+
"description": "my\ndescription\nvalue\n",
171+
"device_passthrough.#": "1",
172+
"initialization.0.dns.#": "0",
173+
}),
174+
),
175+
},
176+
}},
121177
{"update mount points", []resource.TestStep{
122178
{
123179
Config: te.RenderConfig(`

‎proxmox/nodes/containers/containers.go

+23-2
Original file line numberDiff line numberDiff line change
@@ -114,14 +114,35 @@ func (c *Client) RebootContainer(ctx context.Context, d *RebootRequestBody) erro
114114

115115
// ShutdownContainer shuts down a container.
116116
func (c *Client) ShutdownContainer(ctx context.Context, d *ShutdownRequestBody) error {
117-
err := c.DoRequest(ctx, http.MethodPost, c.ExpandPath("status/shutdown"), d, nil)
117+
taskID, err := c.ShutdownContainerAsync(ctx, d)
118118
if err != nil {
119-
return fmt.Errorf("error shutting down container: %w", err)
119+
return err
120+
}
121+
122+
err = c.Tasks().WaitForTask(ctx, *taskID)
123+
if err != nil {
124+
return fmt.Errorf("error waiting for container shut down: %w", err)
120125
}
121126

122127
return nil
123128
}
124129

130+
// ShutdownContainerAsync shuts down a container asynchronously.
131+
func (c *Client) ShutdownContainerAsync(ctx context.Context, d *ShutdownRequestBody) (*string, error) {
132+
resBody := &ShutdownResponseBody{}
133+
134+
err := c.DoRequest(ctx, http.MethodPost, c.ExpandPath("status/shutdown"), d, resBody)
135+
if err != nil {
136+
return nil, fmt.Errorf("error shutting down container: %w", err)
137+
}
138+
139+
if resBody.Data == nil {
140+
return nil, api.ErrNoDataObjectInResponse
141+
}
142+
143+
return resBody.Data, nil
144+
}
145+
125146
// StartContainer starts a container if is not already running.
126147
func (c *Client) StartContainer(ctx context.Context) error {
127148
status, err := c.GetContainerStatus(ctx)

‎proxmox/nodes/containers/containers_types.go

+2
Original file line numberDiff line numberDiff line change
@@ -159,6 +159,8 @@ type CreateResponseBody struct {
159159
Data *string `json:"data,omitempty"`
160160
}
161161

162+
type ShutdownResponseBody = CreateResponseBody
163+
162164
// GetResponseBody contains the body from a user get response.
163165
type GetResponseBody struct {
164166
Data *GetResponseData `json:"data,omitempty"`

‎proxmoxtf/resource/container/container.go

+16-5
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
2323

2424
"github.com/bpg/terraform-provider-proxmox/proxmox/api"
25+
"github.com/bpg/terraform-provider-proxmox/proxmox/helpers/ptr"
2526
"github.com/bpg/terraform-provider-proxmox/proxmox/nodes/containers"
2627
"github.com/bpg/terraform-provider-proxmox/proxmox/types"
2728
"github.com/bpg/terraform-provider-proxmox/proxmoxtf"
@@ -2997,11 +2998,14 @@ func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{})
29972998
}
29982999
}
29993000

3000-
if d.HasChange(mkInitialization) {
3001+
if d.HasChange(mkInitialization + "." + mkInitializationDNS) {
30013002
updateBody.DNSDomain = &initializationDNSDomain
30023003
updateBody.DNSServer = &initializationDNSServer
3003-
updateBody.Hostname = &initializationHostname
3004+
rebootRequired = true
3005+
}
30043006

3007+
if d.HasChange(mkInitialization + "." + mkInitializationHostname) {
3008+
updateBody.Hostname = &initializationHostname
30053009
rebootRequired = true
30063010
}
30073011

@@ -3279,11 +3283,16 @@ func containerUpdate(ctx context.Context, d *schema.ResourceData, m interface{})
32793283
}
32803284
} else {
32813285
forceStop := types.CustomBool(true)
3282-
shutdownTimeout := 300
3286+
// Using delete timeout here as we're in the similar situation
3287+
// as in the delete function, where we need to wait for the container
3288+
// to be stopped before we can proceed with the update.
3289+
// see `containerDelete` function for more details about the logic here
3290+
// Needs to be refactored to a common function
3291+
shutdownTimeoutSec := max(1, d.Get(mkTimeoutDelete).(int)-5)
32833292

32843293
e = containerAPI.ShutdownContainer(ctx, &containers.ShutdownRequestBody{
32853294
ForceStop: &forceStop,
3286-
Timeout: &shutdownTimeout,
3295+
Timeout: &shutdownTimeoutSec,
32873296
})
32883297
if e != nil {
32893298
return diag.FromErr(e)
@@ -3351,7 +3360,9 @@ func containerDelete(ctx context.Context, d *schema.ResourceData, m interface{})
33513360
ctx,
33523361
&containers.ShutdownRequestBody{
33533362
ForceStop: &forceStop,
3354-
Timeout: &deleteTimeoutSec,
3363+
// the timeout here must be less that the context timeout set above,
3364+
// otherwise the context will be cancelled before PVE forcefully stops the container
3365+
Timeout: ptr.Ptr(max(1, deleteTimeoutSec-5)),
33553366
},
33563367
)
33573368
if err != nil {

0 commit comments

Comments
 (0)