fix(container_node_pool): panic interface conversion on linux_node_config.sysctls
#8981
Conversation
|
Hello! I am a robot. It looks like you are a: Community Contributor @shuyama1, 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. |
modular-magician
added
the
awaiting-approval
toVersus
force-pushed
the
fix/linux-node-config
branch
2 times, most recently
from
ceb20d9 to
bbfa215
Compare
modular-magician
removed
the
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 45 insertions(+), 18 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 45 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withSoleTenantGroup|TestAccContainerCluster_withWorkloadMetadataConfig|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_backend|TestAccContainerCluster_withBootDiskKmsKey|TestAccContainerCluster_withNodeConfigReservationAffinity|TestAccContainerCluster_withNodeConfigReservationAffinitySpecific|TestAccContainerCluster_withNodeConfigShieldedInstanceConfig|TestAccContainerCluster_withNodeConfigScopeAlias|TestAccContainerCluster_withSandboxConfig|TestAccContainerCluster_withLoggingVariantInNodePool|TestAccContainerCluster_withNodeConfig|TestAccContainerCluster_withLoggingVariantInNodeConfig|TestAccContainerCluster_customPlacementPolicy|TestAccContainerNodePool_withHostMaintenancePolicy|TestAccContainerNodePool_withConfidentialNodes|TestAccContainerNodePool_customPlacementPolicy|TestAccContainerNodePool_threadsPerCore|TestAccContainerNodePool_compactPlacement|TestAccContainerNodePool_fastSocket|TestAccContainerNodePool_gvnic|TestAccContainerNodePool_gcfsConfig|TestAccContainerNodePool_ephemeralStorageConfig|TestAccContainerNodePool_localNvmeSsdBlockConfig|TestAccContainerNodePool_ephemeralStorageLocalSsdConfig|TestAccContainerNodePool_shieldedInstanceConfig|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_withSoleTenantConfig|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_withEnablePrivateNodesToggle|TestAccContainerNodePool_withMultiNicNetworkConfig|TestAccContainerNodePool_withGPU|TestAccContainerCluster_withILBSubsetting|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withReservationAffinity|TestAccContainerCluster_withConfidentialNodes|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withReservationAffinitySpecific|TestAccContainerNodePool_withLoggingVariantUpdates|TestAccContainerNodePool_withNetworkConfig |
|
Rerun these tests in REPLAYING mode to catch issues
|
toVersus
force-pushed
the
fix/linux-node-config
branch
from
bbfa215 to
17ee96e
Compare
modular-magician
added
the
awaiting-approval
|
I removed the suspicious code that was affecting the VCR test for the existing nodepool test cases. However, I'm not sure how to handle the fact that the following change is always planned when reapplying after creating a nodepool. Terraform code: resource "google_container_cluster" "cluster" {
name = "test-cluster"
location = "us-central1-f"
initial_node_count = 1
}
resource "google_container_node_pool" "np" {
name = "test-nodepool"
location = "us-central1-f"
cluster = google_container_cluster.cluster.name
initial_node_count = 1
node_config {
image_type = "COS_CONTAINERD"
linux_node_config {
sysctls = {}
}
oauth_scopes = [
"https://www.googleapis.com/auth/logging.write",
"https://www.googleapis.com/auth/monitoring",
]
}
}Output: TF_LOG=DEBUG TF_LOG_PATH=plan.log TF_CLI_CONFIG_FILE="$HOME/tf-dev-override.tfrc" terraform apply
(...)
Terraform will perform the following actions:
# google_container_node_pool.np will be updated in-place
~ resource "google_container_node_pool" "np" {
id = "projects/nagasawa-test/locations/us-central1-f/clusters/test-cluster/nodePools/test-nodepool"
name = "test-nodepool"
# (9 unchanged attributes hidden)
~ node_config {
tags = []
# (15 unchanged attributes hidden)
+ linux_node_config {}
# (1 unchanged block hidden)
}
# (3 unchanged blocks hidden)
}
Plan: 0 to add, 1 to change, 0 to destroy.I think this warning is the route cause of this issue but how can I avoid that? 2023-09-19T11:34:07.352+0900 [WARN] Provider "registry.terraform.io/hashicorp/google" produced an invalid plan for google_container_node_pool.np, but we are tolerating it because it is using the legacy plugin SDK.
The following problems may be the cause of any confusing errors from downstream operations:
(...)
- .node_config[0].linux_node_config[0].sysctls: planned value cty.NullVal(cty.Map(cty.String)) does not match config value cty.MapValEmpty(cty.String)
(...) |
There was a problem hiding this comment.
Thanks for making a fix for this issue. Yeah, explicitly added it in the flattener to write back linux_node_config {} that will potentially break all current configurations and we should avoid doing so. I wonder if we explicitly send the empty fields would the API return it back as the current issues is that the API does not return linux_node_config {} and Terraform will try to update it since it's in the config.
| cfg, ok := ls[0].(map[string]interface{}) | ||
| if !ok { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
| cfg, ok := ls[0].(map[string]interface{}) | |
| if !ok { | |
| return nil | |
| } | |
| if ls[0] == nil { | |
| return &container.LinuxNodeConfig{} | |
| } | |
| cfg := ls[0].(map[string]interface{}) |
Can we try something like this and see if the API will return back the value?
There was a problem hiding this comment.
Thank you for your assistance! You are right; this change worked perfectly.
toVersus
force-pushed
the
fix/linux-node-config
branch
from
17ee96e to
9b8ba17
Compare
…_node_config` Signed-off-by: Tsubasa Nagasawa <[email protected]> Co-authored-by: Shuya Ma <[email protected]>
toVersus
force-pushed
the
fix/linux-node-config
branch
from
9b8ba17 to
6908782
Compare
modular-magician
removed
the
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 40 insertions(+), 17 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccContainerNodePool_withLinuxNodeConfig |
|
Rerun these tests in REPLAYING mode to catch issues
Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.
|
|
don't think the failing tests are related |
modular-magician
added
awaiting-approval
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 40 insertions(+), 17 deletions(-)) |
Tests analyticsTotal tests:
|
…onfig.sysctls` (GoogleCloudPlatform#8981) Co-authored-by: Shuya Ma <[email protected]>
Fixes: hashicorp/terraform-provider-google#15847
linux_node_config.sysctlsfieldsysctlsparameter to be changedlinux_node_config.sysctlsWithout the fix, the test case to create a nodepool with empty
linux_node_config.sysctlswill fail.Release Note Template for Downstream PRs (will be copied)