container: fix in-place updates for `node_config.containerd_config` #12135

wyardley · 2024-10-25T18:16:37Z

It appears that in-place updates were never implemented for node_config.containerd_config, when contained within either google_container_cluster or google_container_node_pool

Fixes hashicorp/terraform-provider-google#19056
Related to hashicorp/terraform-provider-google#19225 (already closed / fixed)

The actual implementation is pretty simple, and is basically copy / paste from other fields.

The adjustments to existing tests are a little trickier to parse out.

Basically, the existing tests for both google_container_cluster and google_container_node_pool mostly covered node_config_defaults (maybe because that's the only one updates worked for), and didn't cover updates in node_config. In at least one case, the same test was used for all of node_config_defaults, node_pool.node_config, and node_config, and I've mostly preserved that, just adjusting the order. Note that in at least one case, that will likely require recreating the entire cluster and / or its node-pools; I've moved that test (named node_pool {} block in google_container_cluster) to be the last test, and attempted to add some comments.

Another approach would be to split out the tests of node_config_defaults, node_pool.node_config, and node_config.

Release Note Template for Downstream PRs (will be copied)

container: fixed in-place updates for `node_config.containerd_config` in `google_container_cluster` and `google_container_node_pool`

github-actions · 2024-10-25T18:17:15Z

Hello! I am a robot. Tests will require approval from a repository maintainer to run.

@SirGitsalot, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

wyardley · 2024-10-25T18:21:33Z

Some more context on this in @roaks3's comment here

wyardley · 2024-10-25T23:09:52Z

mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.tmpl

@@ -4862,13 +4886,12 @@ resource "google_container_node_pool" "np" {
    oauth_scopes = [
      "https://www.googleapis.com/auth/cloud-platform",
    ]
-    machine_type = "n1-standard-8"


pulling all these out because a) was seeing a lot of stockout issues with n1, b) n1 is super old, c) n1 isn't needed in any of these for a reason, and d) didn't want to trigger updates or recreation where not needed.

I imagine it probably would be good to (separately) adjust to the n1 stuff in these tests to e2 or n2/n3? There are some comments about e2 not working with pd-ssd, but afaict, it works fine now

Yeah, we've got other ticking time bombs like this scattered throughout our tests. The challenge is that APIs require that we specify a machine type/disk type/Linux distro version/etc. and it's often the case that the best we can do is use the latest available to give us the maximum useful lifetime for the test and then fix it when it inevitably breaks in a few years.

@SirGitsalot some of the comments imply it's because e2 (which I think is the default?) is required for certain tests (SSD is one example given). I think in some cases, those limitations have changed and / or the code was just copy / pasted.

Taking it out here seemed to work in my test, but happy to adjust it to a different explicit type if needed.

It appears that in-place updates were never implemented for `node_config.containerd_config`, when contained within either `google_container_cluster` or `google_container_node_pool` Fixes hashicorp/terraform-provider-google#19056 Related to hashicorp/terraform-provider-google#19225 Signed-off-by: William Yardley <[email protected]>

github-actions · 2024-10-30T09:41:44Z

@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

modular-magician · 2024-10-30T18:20:49Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 3 files changed, 277 insertions(+), 122 deletions(-))
google-beta provider: Diff ( 3 files changed, 277 insertions(+), 122 deletions(-))

SirGitsalot

LGTM, I've kicked off the tests and will approve and merge once they're done.

SirGitsalot · 2024-10-30T18:23:17Z

mmv1/third_party/terraform/services/container/resource_container_node_pool_test.go.tmpl

@@ -4862,13 +4886,12 @@ resource "google_container_node_pool" "np" {
    oauth_scopes = [
      "https://www.googleapis.com/auth/cloud-platform",
    ]
-    machine_type = "n1-standard-8"


Yeah, we've got other ticking time bombs like this scattered throughout our tests. The challenge is that APIs require that we specify a machine type/disk type/Linux distro version/etc. and it's often the case that the best we can do is use the latest available to give us the maximum useful lifetime for the test and then fix it when it inevitably breaks in a few years.

modular-magician · 2024-10-30T18:28:43Z

Tests analytics

Total tests: 215
Passed tests: 201
Skipped tests: 13
Affected tests: 1

Click here to see the affected service packages

container

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccContainerNodePool_privateRegistry

Get to know how VCR tests work

modular-magician · 2024-10-30T18:50:35Z

🟢 Tests passed during RECORDING mode:
TestAccContainerNodePool_privateRegistry [Debug log]

🟢 No issues found for passed tests after REPLAYING rerun.

🟢 All tests passed!

View the build log or the debug log for each test

…oogleCloudPlatform#12135) Signed-off-by: William Yardley <[email protected]>

github-actions bot requested a review from SirGitsalot October 25, 2024 18:17

modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Oct 25, 2024

wyardley commented Oct 25, 2024

View reviewed changes

wyardley force-pushed the wyardley/19056/nodeconfig_containerd_config_updates branch from 828ed7c to f5ce37d Compare October 26, 2024 02:08

modular-magician added service/container and removed awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests labels Oct 30, 2024

SirGitsalot reviewed Oct 30, 2024

View reviewed changes

SirGitsalot approved these changes Oct 30, 2024

View reviewed changes

SirGitsalot merged commit 07118df into GoogleCloudPlatform:main Oct 30, 2024
12 checks passed

wyardley mentioned this pull request Oct 31, 2024

container: unskipped VCR for TestAccContainerCluster_privateRegistry #12217

Merged

niharika-98 pushed a commit to niharika-98/magic-modules that referenced this pull request Nov 1, 2024

container: fix in-place updates for node_config.containerd_config (G…

be3411b

…oogleCloudPlatform#12135) Signed-off-by: William Yardley <[email protected]>

BBBmau pushed a commit to BBBmau/magic-modules that referenced this pull request Nov 5, 2024

container: fix in-place updates for node_config.containerd_config (G…

64aef33

…oogleCloudPlatform#12135) Signed-off-by: William Yardley <[email protected]>

akshat-jindal-nit pushed a commit to akshat-jindal-nit/magic-modules that referenced this pull request Nov 18, 2024

container: fix in-place updates for node_config.containerd_config (G…

e22cf97

…oogleCloudPlatform#12135) Signed-off-by: William Yardley <[email protected]>

amanMahendroo pushed a commit to amanMahendroo/magic-modules that referenced this pull request Dec 17, 2024

container: fix in-place updates for node_config.containerd_config (G…

d2e8b72

…oogleCloudPlatform#12135) Signed-off-by: William Yardley <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

container: fix in-place updates for `node_config.containerd_config` #12135

container: fix in-place updates for `node_config.containerd_config` #12135

wyardley commented Oct 25, 2024 •

edited

Loading

github-actions bot commented Oct 25, 2024

wyardley commented Oct 25, 2024

wyardley Oct 25, 2024 •

edited

Loading

SirGitsalot Oct 30, 2024

wyardley Oct 30, 2024

github-actions bot commented Oct 30, 2024

modular-magician commented Oct 30, 2024

SirGitsalot left a comment

SirGitsalot Oct 30, 2024

modular-magician commented Oct 30, 2024

modular-magician commented Oct 30, 2024

container: fix in-place updates for node_config.containerd_config #12135

container: fix in-place updates for node_config.containerd_config #12135

Conversation

wyardley commented Oct 25, 2024 • edited Loading

github-actions bot commented Oct 25, 2024

wyardley commented Oct 25, 2024

wyardley Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

SirGitsalot Oct 30, 2024

Choose a reason for hiding this comment

wyardley Oct 30, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 30, 2024

modular-magician commented Oct 30, 2024

Diff report

SirGitsalot left a comment

Choose a reason for hiding this comment

SirGitsalot Oct 30, 2024

Choose a reason for hiding this comment

modular-magician commented Oct 30, 2024

Tests analytics

Action taken

modular-magician commented Oct 30, 2024

container: fix in-place updates for `node_config.containerd_config` #12135

container: fix in-place updates for `node_config.containerd_config` #12135

wyardley commented Oct 25, 2024 •

edited

Loading

wyardley Oct 25, 2024 •

edited

Loading