From 9e502edbbdaac91d6a3a858d2a12bc5ac32e04ff Mon Sep 17 00:00:00 2001 From: Arjun Mahishi Date: Mon, 29 Apr 2024 13:32:12 +0530 Subject: [PATCH] [CC-26984] metric-export: minor refactoring in prometheus resource * declare a seperate timeout for metric export enable/disable/status refresh * Remove debug logs * Rename retryFunc -> deletePromMetricExport --- CHANGELOG.md | 4 +- ...etric_export_prometheus_config_resource.go | 34 ++++++++++----- ..._export_prometheus_config_resource_test.go | 32 +++++++------- .../resource/schema/booldefault/doc.go | 5 --- .../schema/booldefault/static_value.go | 42 ------------------- vendor/modules.txt | 1 - 6 files changed, 41 insertions(+), 77 deletions(-) delete mode 100644 vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/doc.go delete mode 100644 vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/static_value.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f7901ec..c8ef8e2a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [Unreleased] +## [1.7.0] ## Added @@ -26,7 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 to true, attempts to delete the cluster will fail. Set to false to disable delete protection. -## [1.5.0] - 2024-05-26 +## [1.5.0] - 2024-04-26 - No changes. diff --git a/internal/provider/metric_export_prometheus_config_resource.go b/internal/provider/metric_export_prometheus_config_resource.go index 49f8041b..4b2e6b22 100644 --- a/internal/provider/metric_export_prometheus_config_resource.go +++ b/internal/provider/metric_export_prometheus_config_resource.go @@ -4,6 +4,10 @@ import ( "context" "encoding/json" "fmt" + "net/http" + "strings" + "time" + "github.com/cockroachdb/cockroach-cloud-sdk-go/pkg/client" "github.com/hashicorp/terraform-plugin-framework/path" "github.com/hashicorp/terraform-plugin-framework/resource" @@ -13,8 +17,12 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/retry" - "net/http" - "strings" +) + +const ( + metricExportEnableTimeout = time.Minute // This request just triggers an async job that rolls out the integration + metricExportDisableTimeout = time.Minute // Same as above. Different job, but the HTTP request behaves the same. + metricExportStableTimeout = 10 * time.Minute // This might take a while. Stability here means the integration is up and running ) var metricExportPrometheusConfigAttributes = map[string]schema.Attribute{ @@ -105,7 +113,7 @@ func (r *metricExportPrometheusConfigResource) Create( } apiObj := &client.PrometheusMetricExportInfo{} - err = retry.RetryContext(ctx, clusterUpdateTimeout, retryEnablePrometheusMetricExport( + err = retry.RetryContext(ctx, metricExportEnableTimeout, retryEnablePrometheusMetricExport( ctx, r.provider.service, clusterID, cluster, apiObj, )) @@ -116,7 +124,7 @@ func (r *metricExportPrometheusConfigResource) Create( return } - err = retry.RetryContext(ctx, clusterUpdateTimeout, + err = retry.RetryContext(ctx, metricExportStableTimeout, waitForPrometheusMetricExportStableFunc(ctx, clusterID, r.provider.service, apiObj)) if err != nil { resp.Diagnostics.AddError( @@ -183,7 +191,10 @@ func retryEnablePrometheusMetricExport( } func loadPrometheusMetricExportIntoTerraformState( - ID types.String, status types.String, targets types.Map, state *ClusterPrometheusMetricExportConfig, + ID types.String, + status types.String, + targets types.Map, + state *ClusterPrometheusMetricExportConfig, ) { state.ID = ID state.Status = status @@ -199,12 +210,15 @@ func waitForPrometheusMetricExportStableFunc( return func() *retry.RetryError { apiObj, httpResp, err := cl.GetPrometheusMetricExportInfo(ctx, clusterID) if err != nil { - if httpResp != nil && httpResp.StatusCode < http.StatusServiceUnavailable { + if httpResp != nil && httpResp.StatusCode < http.StatusInternalServerError { return retry.NonRetryableError(fmt.Errorf( "error getting Prometheus metric export info: %s", formatAPIErrorMessage(err))) } else { + // if this is a server error, we should retry + tflog.Error(ctx, "encountered a server error while refreshing Prometheus metric export status. Retrying...") return retry.RetryableError(fmt.Errorf( - "encountered a server error while reading Prometheus metric export status - trying again")) + "encountered a server error while refreshing Prometheus metric export status", + )) } } @@ -306,7 +320,7 @@ func (r *metricExportPrometheusConfigResource) Delete( clusterID := state.ID.ValueString() cluster := &client.Cluster{} - retryFunc := func() retry.RetryFunc { + deletePromMetricExport := func() retry.RetryFunc { return func() *retry.RetryError { apiObj, httpResp, err := r.provider.service.DeletePrometheusMetricExport(ctx, clusterID) if err != nil { @@ -341,7 +355,7 @@ func (r *metricExportPrometheusConfigResource) Delete( if apiObj.GetStatus() == client.METRICEXPORTSTATUSTYPE_DISABLING { // We are passing empty PrometheusMetricExportInfo object as we don't need it in stage management. - err = retry.RetryContext(ctx, clusterUpdateTimeout, + err = retry.RetryContext(ctx, metricExportStableTimeout, waitForPrometheusMetricExportStableFunc(ctx, clusterID, r.provider.service, client.NewPrometheusMetricExportInfoWithDefaults())) if err != nil { resp.Diagnostics.AddError( @@ -355,7 +369,7 @@ func (r *metricExportPrometheusConfigResource) Delete( } } - err := retry.RetryContext(ctx, clusterUpdateTimeout, retryFunc()) + err := retry.RetryContext(ctx, metricExportDisableTimeout, deletePromMetricExport()) if err != nil { resp.Diagnostics.AddError( "Error deleting Prometheus metric export config", err.Error(), diff --git a/internal/provider/metric_export_prometheus_config_resource_test.go b/internal/provider/metric_export_prometheus_config_resource_test.go index 9018295f..c8d51a5e 100644 --- a/internal/provider/metric_export_prometheus_config_resource_test.go +++ b/internal/provider/metric_export_prometheus_config_resource_test.go @@ -3,23 +3,19 @@ package provider import ( "context" "fmt" + "net/http" + "os" + "testing" + "github.com/cockroachdb/cockroach-cloud-sdk-go/pkg/client" mock_client "github.com/cockroachdb/terraform-provider-cockroach/mock" "github.com/golang/mock/gomock" "github.com/google/uuid" "github.com/hashicorp/terraform-plugin-testing/helper/resource" "github.com/hashicorp/terraform-plugin-testing/terraform" - "log" - "net/http" - "os" - "testing" ) -// TestAccMetricExportPrometheusConfigResource attempts to create, check, and destroy -// a real cluster. It will be skipped if TF_ACC isn't set. func TestAccMetricExportPrometheusConfigResource(t *testing.T) { - t.Skip("Skipping until we can either integrate the AWS provider " + - "or import a permanent test fixture.") t.Parallel() clusterName := fmt.Sprintf("%s-prometheus-%s", tfTestPrefix, GenerateRandomString(4)) testMetricExportPrometheusConfigResource(t, clusterName, false) @@ -45,7 +41,7 @@ func TestIntegrationMetricExportPrometheusConfigResource(t *testing.T) { Name: clusterName, CockroachVersion: "v22.2.0", Plan: "DEDICATED", - CloudProvider: "AWS", + CloudProvider: "GCP", State: "CREATED", Config: client.ClusterConfig{ Dedicated: &client.DedicatedHardwareConfig{ @@ -149,7 +145,7 @@ func testMetricExportPrometheusConfigExists( p := testAccProvider.(*provider) p.service = NewService(cl) - rs, ok := s.RootModule().Resources[resourceName] + _, ok := s.RootModule().Resources[resourceName] if !ok { return fmt.Errorf("not found: %s", resourceName) } @@ -159,14 +155,16 @@ func testMetricExportPrometheusConfigExists( } clusterID := clusterRs.Primary.Attributes["id"] - log.Printf("[DEBUG] clusterID: %s, name %s", clusterRs.Primary.Attributes["id"], clusterRs.Primary.Attributes["name"]) + config, _, err := p.service.GetPrometheusMetricExportInfo(context.TODO(), clusterID) + if err != nil { + return fmt.Errorf("metric export Prometheus config does not exist") + } - _, _, err := p.service.GetPrometheusMetricExportInfo(context.TODO(), clusterID) - if err == nil { - return nil + if config.GetStatus() != client.METRICEXPORTSTATUSTYPE_ENABLED { + return fmt.Errorf("metric export Prometheus config is not enabled") } - return fmt.Errorf("metric export Prometheus config with site %s does not exist", rs.Primary.Attributes["site"]) + return nil } } @@ -174,7 +172,7 @@ func getTestMetricExportPrometheusConfigResourceCreateConfig(name string) string return fmt.Sprintf(` resource "cockroach_cluster" "test" { name = "%s" - cloud_provider = "AWS" + cloud_provider = "GCP" dedicated = { storage_gib = 35 num_virtual_cpus = 4 @@ -195,7 +193,7 @@ func getTestMetricExportPrometheusConfigResourceUpdateConfig(name string) string return fmt.Sprintf(` resource "cockroach_cluster" "test" { name = "%s" - cloud_provider = "AWS" + cloud_provider = "GCP" dedicated = { storage_gib = 35 num_virtual_cpus = 4 diff --git a/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/doc.go b/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/doc.go deleted file mode 100644 index fe6b0f76..00000000 --- a/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/doc.go +++ /dev/null @@ -1,5 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -// Package booldefault provides default values for types.Bool attributes. -package booldefault diff --git a/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/static_value.go b/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/static_value.go deleted file mode 100644 index 797f81d0..00000000 --- a/vendor/github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault/static_value.go +++ /dev/null @@ -1,42 +0,0 @@ -// Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 - -package booldefault - -import ( - "context" - "fmt" - - "github.com/hashicorp/terraform-plugin-framework/resource/schema/defaults" - "github.com/hashicorp/terraform-plugin-framework/types" -) - -// StaticBool returns a static boolean value default handler. -// -// Use StaticBool if a static default value for a boolean should be set. -func StaticBool(defaultVal bool) defaults.Bool { - return staticBoolDefault{ - defaultVal: defaultVal, - } -} - -// staticBoolDefault is static value default handler that -// sets a value on a boolean attribute. -type staticBoolDefault struct { - defaultVal bool -} - -// Description returns a human-readable description of the default value handler. -func (d staticBoolDefault) Description(_ context.Context) string { - return fmt.Sprintf("value defaults to %t", d.defaultVal) -} - -// MarkdownDescription returns a markdown description of the default value handler. -func (d staticBoolDefault) MarkdownDescription(_ context.Context) string { - return fmt.Sprintf("value defaults to `%t`", d.defaultVal) -} - -// DefaultBool implements the static default value logic. -func (d staticBoolDefault) DefaultBool(_ context.Context, req defaults.BoolRequest, resp *defaults.BoolResponse) { - resp.PlanValue = types.BoolValue(d.defaultVal) -} diff --git a/vendor/modules.txt b/vendor/modules.txt index f71b0b52..99c227c0 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -205,7 +205,6 @@ github.com/hashicorp/terraform-plugin-framework/provider/schema github.com/hashicorp/terraform-plugin-framework/providerserver github.com/hashicorp/terraform-plugin-framework/resource github.com/hashicorp/terraform-plugin-framework/resource/schema -github.com/hashicorp/terraform-plugin-framework/resource/schema/booldefault github.com/hashicorp/terraform-plugin-framework/resource/schema/boolplanmodifier github.com/hashicorp/terraform-plugin-framework/resource/schema/defaults github.com/hashicorp/terraform-plugin-framework/resource/schema/int64planmodifier