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

Separating GKE Standard and Autopilot Modules #1330

Merged
merged 23 commits into from Apr 21, 2023

Conversation

avinashkumar1289
Copy link
Collaborator

@avinashkumar1289 avinashkumar1289 commented Apr 17, 2023

This Pull request contains changes for separating GKE autopilot and Standard Modules.
I have commented out changes in the autopilot module, so it becomes easy to find what blocks I have removed before merging.

@avinashkumar1289
Copy link
Collaborator Author

@ludoo @juliocc I am facing a lot of error because of the naming convention of the module. There are a lot of reference for gke-cluster and since I have divided them to gke-autopilot-cluster and gke-standard-cluster, few of the checks are not passing ?
Please advice

@juliocc
Copy link
Collaborator

juliocc commented Apr 18, 2023

@ludoo @juliocc I am facing a lot of error because of the naming convention of the module. There are a lot of reference for gke-cluster and since I have divided them to gke-autopilot-cluster and gke-standard-cluster, few of the checks are not passing ? Please advice

Hey @avinashkumar1289, it's not a naming convention issue. You're actually removing a module that is a dependency elsewhere in the repo. The tests are failing because they depend on that module. This is exactly what should happen.

The PR should update those other parts of the codebase and ensure all the references to the existing module are updated and work as expected. The failing tests should actually help you figure out what is broken by the changes you're introducing.

If you want we can show you how to go about fixing some of those failures.

@avinashkumar1289
Copy link
Collaborator Author

Thanks @juliocc .Yeah i had a sync with ludo. I am going to replace all the dependencies from gke-cluster to gke-standard-cluster.
Thanks !!

@avinashkumar1289 avinashkumar1289 changed the title separating GKE Standard and Autopilot Modules Separating GKE Standard and Autopilot Modules Apr 19, 2023
@avinashkumar1289 avinashkumar1289 self-assigned this Apr 19, 2023
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

Super job thanks a lot for tackling this! Can you remove the commented code in the autpilot mode before merging?

@avinashkumar1289
Copy link
Collaborator Author

avinashkumar1289 commented Apr 20, 2023

@ludoo Thanks .Pushed the changes !! I will go ahead and merge the code.

@ludoo
Copy link
Collaborator

ludoo commented Apr 21, 2023

It's good to go for me, let's wait this morning so Julio can also give it a look, then you should be able to merge (do you see the green "Squash and merge" button?)

@avinashkumar1289
Copy link
Collaborator Author

@ludoo Thanks !! Yes lets wait for @juliocc to confirm !!
Yes I see the "Squash and Merge" option under "Merge Pull request".

modules/gke-autopilot-cluster/main.tf Outdated Show resolved Hide resolved
@avinashkumar1289 avinashkumar1289 enabled auto-merge (squash) April 21, 2023 11:19
@avinashkumar1289 avinashkumar1289 merged commit e881537 into master Apr 21, 2023
8 checks passed
@avinashkumar1289 avinashkumar1289 deleted the avinashjha/gke-autopilot branch April 21, 2023 12:08
lcaggio pushed a commit that referenced this pull request May 5, 2023
* separating GKE Standard and Autopilot Modules

* Changes for Updating the terraform and provide versions

* Changes for Autopilot Readme

* Changes for Autopilot Variable

* Changes for Autopilot Readme

* Changes for Autopilot Readme

* Changes for Blueprint

* Changes for Blueprint ReadMe

* Changes for gke-standard-cluster dependency

* Changes for gke-standard-cluster in gke-fleet

* Changes for gke-standard-cluster in cluster-mesh-gke-fleet-api

* python formatting

* python formatting

* python formatting

* GKE module naming convention

* Readme Changes

* test module

* Removing comment code from Autopilot
@intotecho
Copy link

Hi, What is the reason why var.monitoring_config was removed from the autopilot version? Is it not supported in GKE?

@avinashkumar1289
Copy link
Collaborator Author

avinashkumar1289 commented May 23, 2023

@intotecho monitoring config is enabled by default in GKE Autopilot and cannot be disabled. Hence we have removed the configuration.

@intotecho
Copy link

Thanks, makes sense. Are all the components enabled, like SYSTEM_COMPONENTS, APISERVER, CONTROLLER_MANAGER, and SCHEDULER?

@juliocc
Copy link
Collaborator

juliocc commented May 23, 2023

@intotecho
Copy link

intotecho commented May 23, 2023 via email

@intotecho
Copy link

I am having trouble getting the gke autopilot cluster into BALANCED mode.

Although the provider says BALANCED is the default. The console is reporting something else.

Autoscaling profile | Optimize utilization

  cluster_autoscaling {

    autoscaling_profile = "BALANCED"
    dynamic "auto_provisioning_defaults" {
      for_each = var.service_account != null ? [""] : []
      content {
        service_account = var.service_account
      }
    }
  }

I tried adding autoscaling_profile = "BALANCED" in modules/gke-cluster-autopilot/main.tf
But it didn't change the infrastructure.

I am not sure if this is user error, an issue with the google beta provider version "4.60" or the way it is called.

@wiktorn
Copy link
Collaborator

wiktorn commented Jun 13, 2023

@intotecho I can confirm your findings. As far as I can tell, there are two bugs lurking here:

  1. Providing autoscaling_profile in google_container_cluster does not change anything. As this is not editable on the cluster, it should destroy cluster and create new one. Nothing happens, because there is DiffSuppressFunc defined which for autopilot clusters ignores any changes to field. It needs to be ForceNew to force cluster recreation.
  2. When cluster is created without providing autoscaling_profile, clusters are created as OPTIMIZE_UTILIZATION which is against API documentation

So as temporary workaround, after modifying gke-cluster-autopilot module force cluster recreation and it should recreate with proper profile:

terraform plan -out tf.plan -replace 'module.cluster-1.google_container_cluster.cluster'

(your module address may differ)

@wiktorn
Copy link
Collaborator

wiktorn commented Jun 13, 2023

@intotecho Sorry, my bad. For autopilot clusters you can't modify this value.

@intotecho
Copy link

intotecho commented Jun 14, 2023 via email

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

Successfully merging this pull request may close these issues.

None yet

5 participants