-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add GCE PD CSI Driver beta support #497
Conversation
Signed-off-by: Dev <[email protected]>
Signed-off-by: Dev <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the speedy addition! Just a few thoughts/nits.
autogen/main/cluster.tf.tmpl
Outdated
@@ -152,6 +152,10 @@ resource "google_container_cluster" "primary" { | |||
dns_cache_config { | |||
enabled = var.dns_cache | |||
} | |||
|
|||
gce_persistent_disk_csi_driver_config { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make this a dynamic block, as I've observed consistent issues with the behavior of addon blocks in disabled
states.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, i based this on the initial DNS cache PR. Did you want me to fix that addon in this PR too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- cluster_gce_pd_csi_config = var.gce_pd_csi_driver ? [{ enabled = true }] : []
+ cluster_gce_pd_csi_config = var.gce_pd_csi_driver ? [{ enabled = true }] : [{ enabled = false }]
I couldn't disable this addon with the initial dynamic block approach (based on istio addon), one difference with the older addons is that newer addon config blocks (dns/csi) use enabled
rather then disabled
states. When trying to plan it doesn't indicate a change needs to be made when trying to flip back to disabled
I was only able to disable when forcing a dynamic block by proving a explicit [{ enabled = false }]
. So the dynamic block is always going to trigger now, do you still want to keep this approach then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, that's odd. It might indeed by driven by the defaults.
Can you try importing an existing cluster (with this disabled) using the static block approach? If import is successful, then I'm fine with using that approach.
Signed-off-by: Dev <[email protected]>
Co-Authored-By: Morgante Pell <[email protected]>
Signed-off-by: Dev <[email protected]>
Signed-off-by: Dev <[email protected]>
Signed-off-by: Dev <[email protected]>
Signed-off-by: Dev <[email protected]>
BREAKING CHANGE: Minimum provider change increased to 3.19.
3.19 beta provider was released today that includes the CSI addon support
This bumps the min beta provider to 3.19, GA provider is still 3.16