Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(container_node_pool): support cgroup mode #8997

Merged

Conversation

toVersus
Copy link
Contributor

Fixes: hashicorp/terraform-provider-google#15656

This PR added support for setting cgroup version to the google_container_node_pool resource. This allows users to switch to use cgroup v2 for their existing node pools. See Linux cgroup mode configuration option and corresponding REST API doc.

Release Note Template for Downstream PRs (will be copied)

container: added `node_config.linux_node_config.cgroup_mode` field to `google_container_node_pool`

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@melinath, 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 modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Sep 19, 2023
@toVersus toVersus force-pushed the feat/container-cgroup-mode branch from 91d5913 to 18811c3 Compare September 19, 2023 08:06
@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Sep 19, 2023
@modular-magician
Copy link
Collaborator

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.

Terraform GA: Diff ( 3 files changed, 120 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 3 files changed, 120 insertions(+), 21 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (248 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_cluster" "primary" {
  node_config {
    linux_node_config {
      cgroup_mode = # value needed
    }
  }
  node_pool {
    node_config {
      linux_node_config {
        cgroup_mode = # value needed
      }
    }
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3070
Passed tests 2770
Skipped tests: 299
Affected tests: 1

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_withCgroupMode

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerNodePool_withCgroupMode[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@melinath
Copy link
Member

Reassigning review due to OOO.

@melinath melinath requested review from a team and NickElliot and removed request for melinath and a team September 21, 2023 00:30
@toVersus toVersus force-pushed the feat/container-cgroup-mode branch from 18811c3 to 3e4e0a0 Compare September 24, 2023 08:57
@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Sep 24, 2023
@modular-magician
Copy link
Collaborator

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.

Terraform GA: Diff ( 3 files changed, 88 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 3 files changed, 88 insertions(+), 21 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (246 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_cluster" "primary" {
  node_config {
    linux_node_config {
      cgroup_mode = # value needed
    }
  }
  node_pool {
    node_config {
      linux_node_config {
        cgroup_mode = # value needed
      }
    }
  }
}

Resource: google_container_node_pool (54 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_node_pool" "primary" {
  node_config {
    linux_node_config {
      cgroup_mode = # value needed
    }
  }
}

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I took so long to get to this review! Has been a busy week.

It looks like when you rebased you accidentally removed your "testAccContainerNodePool_withCgroupMode" case -- only the larger test function that calls that smaller one is in the file right now.

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2919
Passed tests 2630
Skipped tests: 289
Affected tests: 0

$\textcolor{red}{\textsf{Errors occurred during REPLAYING mode. Please fix them to complete your PR}}$
View the build log

@toVersus toVersus force-pushed the feat/container-cgroup-mode branch from 3e4e0a0 to 280d4af Compare September 26, 2023 05:21
@toVersus
Copy link
Contributor Author

Sorry I took so long to get to this review! Has been a busy week.

No problem! Thanks for making the time.

It looks like when you rebased you accidentally removed your "testAccContainerNodePool_withCgroupMode" case -- only the larger test function that calls that smaller one is in the file right now.

My bad. Fixed!

@modular-magician modular-magician added awaiting-approval Pull requests that need reviewer's approval to run presubmit tests and removed awaiting-approval Pull requests that need reviewer's approval to run presubmit tests labels Sep 26, 2023
@modular-magician
Copy link
Collaborator

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.

Terraform GA: Diff ( 3 files changed, 120 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 3 files changed, 120 insertions(+), 21 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (246 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_cluster" "primary" {
  node_config {
    linux_node_config {
      cgroup_mode = # value needed
    }
  }
  node_pool {
    node_config {
      linux_node_config {
        cgroup_mode = # value needed
      }
    }
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3102
Passed tests 2782
Skipped tests: 313
Affected tests: 7

Action taken

Found 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withCgroupMode|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerCluster_withAddons|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccDataSourceGoogleServiceAccountJwt|TestAccProjectIamPolicy_invalidMembers

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataSourceGoogleServiceAccountJwt[Debug log]
TestAccProjectIamPolicy_invalidMembers[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]
TestAccContainerNodePool_withCgroupMode[Error message] [Debug log]
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerCluster_withAddons[Error message] [Debug log]
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestAccContainerNodePool_withCgroupMode is failing with a "Error: Cannot destroy cluster because deletion_protection is set to true. Set it to false to proceed with instance deletion.", and presumably needs to have that configured to be false.

Signed-off-by: Tsubasa Nagasawa <toversus2357@gmail.com>
@toVersus toVersus force-pushed the feat/container-cgroup-mode branch from 280d4af to 1576430 Compare October 3, 2023 05:09
@modular-magician modular-magician added the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Oct 3, 2023
@toVersus
Copy link
Contributor Author

toVersus commented Oct 3, 2023

Thanks for the pointer! I think this change (#9013) is introducing the requirement to explicitly set deletion_protection = false on the google_container_cluster resource when deleting clusters in tests. Fixed.

@modular-magician modular-magician removed the awaiting-approval Pull requests that need reviewer's approval to run presubmit tests label Oct 3, 2023
@modular-magician
Copy link
Collaborator

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.

Terraform GA: Diff ( 3 files changed, 121 insertions(+), 21 deletions(-))
Terraform Beta: Diff ( 3 files changed, 121 insertions(+), 21 deletions(-))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_container_cluster (246 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_container_cluster" "primary" {
  node_config {
    linux_node_config {
      cgroup_mode = # value needed
    }
  }
  node_pool {
    node_config {
      linux_node_config {
        cgroup_mode = # value needed
      }
    }
  }
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3113
Passed tests 2794
Skipped tests: 314
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withCgroupMode|TestAccContainerCluster_withAddons|TestAccContainerNodePool_withKubeletConfig|TestAccDataprocClusterIamPolicy

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccContainerNodePool_withCgroupMode[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{red}{\textsf{Tests failed during RECORDING mode:}}$
TestAccContainerNodePool_withUpgradeSettings[Error message] [Debug log]
TestAccContainerCluster_withAddons[Error message] [Debug log]
TestAccContainerNodePool_withKubeletConfig[Error message] [Debug log]
TestAccDataprocClusterIamPolicy[Error message] [Debug log]

$\textcolor{red}{\textsf{Please fix these to complete your PR.}}$
View the build log or the debug log for each test

Copy link
Contributor

@NickElliot NickElliot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for cgroupMode option under linux_node_config for GKE nodes
4 participants