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.

Sign up for GitHub

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

Azure SQL FailoverGroup improvements #1361

Merged
merged 2 commits into from
Jan 20, 2021
Merged

Azure SQL FailoverGroup improvements #1361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8bb337
Azure SQL FailoverGroup improvements
matthchr Jan 14, 2021
2c418cc
Add unit tests to CI
matthchr Jan 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@ generate-test-certs:
.PHONY: test-integration-controllers
test-integration-controllers: generate fmt vet manifests
TEST_RESOURCE_PREFIX=$(TEST_RESOURCE_PREFIX) TEST_USE_EXISTING_CLUSTER=false REQUEUE_AFTER=20 \
go test -v -tags "$(BUILD_TAGS)" -coverprofile=reports/integration-controllers-coverage-output.txt -coverpkg=./... -covermode count -parallel 4 -timeout 45m \
./controllers/...
go test -v -tags "$(BUILD_TAGS)" -coverprofile=reports/integration-controllers-coverage-output.txt -coverpkg=./... -covermode count -parallel 4 -timeout 45m ./controllers/...
#2>&1 | tee reports/integration-controllers-output.txt
#go-junit-report < reports/integration-controllers-output.txt > reports/integration-controllers-report.xml

Expand All @@ -81,13 +80,13 @@ test-integration-managers: generate fmt vet manifests
.PHONY: test-unit
test-unit: generate fmt vet manifests
TEST_USE_EXISTING_CLUSTER=false REQUEUE_AFTER=20 \
go test -v -tags "$(BUILD_TAGS)" -coverprofile=coverage-unit.txt -covermode count -parallel 4 -timeout 10m \
./api/... \
./pkg/secrets/...
go test -v -tags "$(BUILD_TAGS)" -coverprofile=reports/unittest-coverage-ouput.txt -covermode count -parallel 4 -timeout 10m \
./pkg/resourcemanager/keyvaults/unittest/ \
./pkg/resourcemanager/azuresql/azuresqlfailovergroup
# The below folders are commented out because the tests in them fail...
# ./api/... \
Copy link
Member Author

Choose a reason for hiding this comment

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

Leave comment here explaining why these are commented out

Copy link
Member

Choose a reason for hiding this comment

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

Was the problem the missing line-continuation on the secrets line? I don't get why the api tests weren't running in that case though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I just got to the azure pipeline change - there was nothing running it before, right.

# ./pkg/secrets/... \
#2>&1 | tee testlogs.txt
#go-junit-report < testlogs.txt > report-unit.xml
go tool cover -html=coverage/coverage.txt -o cover-unit.html

# Merge all the available test coverage results and publish a single report
.PHONY: test-process-coverage
Expand Down
11 changes: 11 additions & 0 deletions api/v1beta1/azuresqlfailovergroup_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,17 @@ import (
)

// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized.

// ReadWriteEndpointFailoverPolicy - wraps https://godoc.org/github.com/Azure/azure-sdk-for-go/services/preview/sql/mgmt/v3.0/sql#ReadWriteEndpointFailoverPolicy
type ReadWriteEndpointFailoverPolicy string

const (
// Automatic ...
FailoverPolicyAutomatic ReadWriteEndpointFailoverPolicy = "Automatic"
// Manual ...
FailoverPolicyManual ReadWriteEndpointFailoverPolicy = "Manual"
)

// AzureSqlFailoverGroupSpec defines the desired state of AzureSqlFailoverGroup
type AzureSqlFailoverGroupSpec struct {
// Important: Run "make" to regenerate code after modifying this file
Expand All @@ -24,6 +33,8 @@ type AzureSqlFailoverGroupSpec struct {
// +kubebuilder:validation:Required
Server string `json:"server"`
FailoverPolicy ReadWriteEndpointFailoverPolicy `json:"failoverPolicy"`
// TODO: This field should be a ptr as it must not be specified if the failover policy is Manual,
// TODO: but is required when the policy is Automatic
FailoverGracePeriod int32 `json:"failoverGracePeriod"`
SecondaryServer string `json:"secondaryServer"`
SecondaryServerResourceGroup string `json:"secondaryServerResourceGroup"`
Expand Down
20 changes: 20 additions & 0 deletions api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 12 additions & 0 deletions azure-pipelines.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,18 @@ steps:
make generate-test-certs
workingDirectory: '$(System.DefaultWorkingDirectory)'

- script: |
set -e
export PATH=$PATH:$(go env GOPATH)/bin
make test-unit
displayName: Run unit tests
condition: eq(variables['check_changes.SOURCE_CODE_CHANGED'], 'true')
continueOnError: 'false'
env:
GO111MODULE: on
BUILD_ID: $(Build.BuildId)
workingDirectory: '$(System.DefaultWorkingDirectory)'

- script: |
set -e
export PATH=$PATH:$(go env GOPATH)/bin:$(go env GOPATH)/kubebuilder/bin
Expand Down
63 changes: 35 additions & 28 deletions controllers/azuresql_combined_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"strings"
"testing"

sql "github.com/Azure/azure-sdk-for-go/services/preview/sql/mgmt/v3.0/sql"
"github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
Expand All @@ -21,8 +22,10 @@ import (
"github.com/Azure/azure-service-operator/api/v1beta1"
"github.com/Azure/azure-service-operator/pkg/errhelp"
helpers "github.com/Azure/azure-service-operator/pkg/helpers"
"github.com/Azure/azure-service-operator/pkg/resourcemanager/azuresql/azuresqlshared"
"github.com/Azure/azure-service-operator/pkg/resourcemanager/config"
kvsecrets "github.com/Azure/azure-service-operator/pkg/secrets/keyvault"

)

func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
Expand Down Expand Up @@ -50,6 +53,7 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {

// create and wait
RequireInstance(ctx, t, tc, sqlServerInstance)
RequireInstance(ctx, t, tc, sqlServerInstance2)

//verify secret exists in k8s for server 1 ---------------------------------
secret := &v1.Secret{}
Expand Down Expand Up @@ -83,40 +87,29 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
var sqlFirewallRuleInstanceLocal *v1beta1.AzureSqlFirewallRule
var sqlFirewallRuleInstanceRemote *v1beta1.AzureSqlFirewallRule

// Create the SqlDatabase object and expect the Reconcile to be created
sqlDatabaseInstance1 = &v1beta1.AzureSqlDatabase{
ObjectMeta: metav1.ObjectMeta{
Name: sqlDatabaseName1,
Namespace: "default",
},
Spec: v1beta1.AzureSqlDatabaseSpec{
Location: rgLocation,
ResourceGroup: rgName,
Server: sqlServerName,
Edition: 0,
},
}

EnsureInstance(ctx, t, tc, sqlDatabaseInstance1)

// run sub tests that require 1 sql server ----------------------------------
t.Run("group1", func(t *testing.T) {
t.Run("sub test for actions", func(t *testing.T) {
t.Parallel()
RunSQLActionHappy(t, sqlServerName)
})

// Wait for 2nd sql server to resolve
t.Run("set up secondary server", func(t *testing.T) {
t.Parallel()
EnsureInstance(ctx, t, tc, sqlServerInstance2)
})

// Create a database in the new server
t.Run("set up database in primary server using edition", func(t *testing.T) {
t.Parallel()

// Create the SqlDatabase object and expect the Reconcile to be created
sqlDatabaseInstance1 = &v1beta1.AzureSqlDatabase{
ObjectMeta: metav1.ObjectMeta{
Name: sqlDatabaseName1,
Namespace: "default",
},
Spec: v1beta1.AzureSqlDatabaseSpec{
Location: rgLocation,
ResourceGroup: rgName,
Server: sqlServerName,
Edition: 0,
},
}

EnsureInstance(ctx, t, tc, sqlDatabaseInstance1)
})

t.Run("set up second database in primary server using sku with maxsizebytes, then update it to use a different SKU", func(t *testing.T) {
t.Parallel()

Expand Down Expand Up @@ -507,7 +500,7 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
Location: rgLocation,
ResourceGroup: rgName,
Server: sqlServerName,
FailoverPolicy: "automatic",
FailoverPolicy: v1beta1.FailoverPolicyAutomatic,
FailoverGracePeriod: 30,
SecondaryServer: sqlServerTwoName,
SecondaryServerResourceGroup: rgName,
Expand All @@ -524,6 +517,20 @@ func TestAzureSqlServerCombinedHappyPath(t *testing.T) {
return strings.Contains(string(secrets["azureSqlPrimaryServer"]), sqlServerName)
}, tc.timeout, tc.retry, "wait for secret store to show failovergroup server names ")

sqlFailoverGroupInstance.Spec.FailoverPolicy = v1beta1.FailoverPolicyManual
sqlFailoverGroupInstance.Spec.FailoverGracePeriod = 0 // GracePeriod cannot be set when policy is manual

err = tc.k8sClient.Update(ctx, sqlFailoverGroupInstance)
assert.Equal(nil, err, "updating sql failover group in k8s")

failoverGroupsClient, err := azuresqlshared.GetGoFailoverGroupsClient(config.GlobalCredentials())
assert.Equal(nil, err, "getting failovergroup client")

assert.Eventually(func() bool {
fog, err := failoverGroupsClient.Get(ctx, rgName, sqlServerName, sqlFailoverGroupName)
assert.Equal(nil, err, "err getting failover group from Azure")
return fog.ReadWriteEndpoint.FailoverPolicy == sql.Manual
}, tc.timeout, tc.retry, "wait for sql failover group failover policy to be updated in Azure")
})

})
Expand Down
6 changes: 3 additions & 3 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,9 +153,9 @@ func setup() error {

// +kubebuilder:scaffold:scheme
k8sManager, err = ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme.Scheme,
CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir,
Port: testEnv.WebhookInstallOptions.LocalServingPort,
Scheme: scheme.Scheme,
CertDir: testEnv.WebhookInstallOptions.LocalServingCertDir,
Port: testEnv.WebhookInstallOptions.LocalServingPort,
})
if err != nil {
return err
Expand Down
17 changes: 17 additions & 0 deletions pkg/errhelp/errhelp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"fmt"
"regexp"
"strings"

"github.com/Azure/azure-service-operator/pkg/helpers"
)

// ErrIdsRegex is used to find and remove uuids from errors
Expand Down Expand Up @@ -40,3 +42,18 @@ func StripErrorTimes(err string) string {
return ErrTimesRegex.ReplaceAllString(err, "")

}

func HandleEnsureError(err error, allowedErrorTypes []string, unrecoverableErrorTypes []string) (bool, error) {
azerr := NewAzureError(err)
if helpers.ContainsString(allowedErrorTypes, azerr.Type) {
return false, nil // false means the resource is not in a terminal state yet, keep trying to reconcile.
}
if helpers.ContainsString(unrecoverableErrorTypes, azerr.Type) {
// Unrecoverable error, so stop reconcilation
return true, nil
}

// We don't know how to classify this error, so bubble it up in the operator logs but don't assume it's
// unrecoverable/terminal
return false, err
}
6 changes: 3 additions & 3 deletions pkg/resourcemanager/azuresql/azuresqldb/azuresqldb_reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,10 @@ func (db *AzureSqlDbManager) Ensure(ctx context.Context, obj runtime.Object, opt
server,
dbName,
azuresqlshared.SQLDatabaseBackupLongTermRetentionPolicy{
WeeklyRetention: instance.Spec.WeeklyRetention,
WeeklyRetention: instance.Spec.WeeklyRetention,
MonthlyRetention: instance.Spec.MonthlyRetention,
YearlyRetention: instance.Spec.YearlyRetention,
WeekOfYear: instance.Spec.WeekOfYear,
YearlyRetention: instance.Spec.YearlyRetention,
WeekOfYear: instance.Spec.WeekOfYear,
})
if err != nil {
failureErrors := []string{
Expand Down
Loading