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

Jump to bottom

change Clone attribute of PostgresSpec to *CloneDescription #1020

Merged
merged 7 commits into from
Jul 30, 2020
Merged

change Clone attribute of PostgresSpec to *CloneDescription #1020

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7e364bd
change Clone attribute of PostgresSpec to *ConnectionPooler
hlihhovac Jun 16, 2020
38b0a05
Merge pull request #1 from zalando/master
pashagolub Jun 16, 2020
cf399ed
Merge remote-tracking branch 'cybertec/master' into change-pgspec.clo…
pashagolub Jun 17, 2020
c9b96d4
update go.mod from master
pashagolub Jun 17, 2020
f0da1b5
fix TestConnectionPoolerSynchronization()
pashagolub Jun 17, 2020
75b2d37
Merge branch 'master' into change-pgspec.clone-to-ptr
hlihhovac Jun 17, 2020
ca37369
Update pkg/apis/acid.zalan.do/v1/postgresql_type.go
pashagolub Jul 17, 2020
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ _testmain.go
/docker/build/
/github.com/
.idea
.vscode

scm-source.json

Expand Down
5 changes: 3 additions & 2 deletions pkg/apis/acid.zalan.do/v1/marshal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,9 @@ func (p *Postgresql) UnmarshalJSON(data []byte) error {

if clusterName, err := extractClusterName(tmp2.ObjectMeta.Name, tmp2.Spec.TeamID); err != nil {
tmp2.Error = err.Error()
tmp2.Status.PostgresClusterStatus = ClusterStatusInvalid
} else if err := validateCloneClusterDescription(&tmp2.Spec.Clone); err != nil {
tmp2.Status = PostgresStatus{PostgresClusterStatus: ClusterStatusInvalid}
} else if err := validateCloneClusterDescription(tmp2.Spec.Clone); err != nil {

tmp2.Error = err.Error()
tmp2.Status.PostgresClusterStatus = ClusterStatusInvalid
} else {
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/acid.zalan.do/v1/postgresql_type.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ type PostgresSpec struct {
NumberOfInstances int32 `json:"numberOfInstances"`
Users map[string]UserFlags `json:"users"`
MaintenanceWindows []MaintenanceWindow `json:"maintenanceWindows,omitempty"`
Clone CloneDescription `json:"clone"`
Clone *CloneDescription `json:"clone"`
ClusterName string `json:"-"`
Databases map[string]string `json:"databases,omitempty"`
PreparedDatabases map[string]PreparedDatabase `json:"preparedDatabases,omitempty"`
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/acid.zalan.do/v1/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func extractClusterName(clusterName string, teamName string) (string, error) {

func validateCloneClusterDescription(clone *CloneDescription) error {
// when cloning from the basebackup (no end timestamp) check that the cluster name is a valid service name
if clone.ClusterName != "" && clone.EndTimestamp == "" {
if clone != nil && clone.ClusterName != "" && clone.EndTimestamp == "" {
if !serviceNameRegex.MatchString(clone.ClusterName) {
return fmt.Errorf("clone cluster name must confirm to DNS-1035, regex used for validation is %q",
serviceNameRegexString)
Expand Down
12 changes: 6 additions & 6 deletions pkg/apis/acid.zalan.do/v1/util_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ var unmarshalCluster = []struct {
"kind": "Postgresql","apiVersion": "acid.zalan.do/v1",
"metadata": {"name": "acid-testcluster1"}, "spec": {"teamId": 100}}`), &tmp).Error(),
},
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{}},"status":"Invalid"}`),
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":"Invalid"}`),
err: nil},
{
about: "example with /status subresource",
Expand All @@ -184,7 +184,7 @@ var unmarshalCluster = []struct {
"kind": "Postgresql","apiVersion": "acid.zalan.do/v1",
"metadata": {"name": "acid-testcluster1"}, "spec": {"teamId": 100}}`), &tmp).Error(),
},
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{}},"status":{"PostgresClusterStatus":"Invalid"}}`),
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`),
err: nil},
{
about: "example with detailed input manifest and deprecated pod_priority_class_name -> podPriorityClassName",
Expand Down Expand Up @@ -327,7 +327,7 @@ var unmarshalCluster = []struct {
EndTime: mustParseTime("05:15"),
},
},
Clone: CloneDescription{
Clone: &CloneDescription{
ClusterName: "acid-batman",
},
ClusterName: "testcluster1",
Expand All @@ -351,7 +351,7 @@ var unmarshalCluster = []struct {
Status: PostgresStatus{PostgresClusterStatus: ClusterStatusInvalid},
Error: errors.New("name must match {TEAM}-{NAME} format").Error(),
},
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"teapot-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null} ,"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{}},"status":{"PostgresClusterStatus":"Invalid"}}`),
marshal: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"teapot-testcluster1","creationTimestamp":null},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null} ,"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`),
err: nil},
{
about: "example with clone",
Expand All @@ -366,7 +366,7 @@ var unmarshalCluster = []struct {
},
Spec: PostgresSpec{
TeamID: "acid",
Clone: CloneDescription{
Clone: &CloneDescription{
ClusterName: "team-batman",
},
ClusterName: "testcluster1",
Expand Down Expand Up @@ -405,7 +405,7 @@ var unmarshalCluster = []struct {
err: errors.New("unexpected end of JSON input")},
{
about: "expect error on JSON with field's value malformatted",
in: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster","creationTimestamp":qaz},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":{}},"status":{"PostgresClusterStatus":"Invalid"}}`),
in: []byte(`{"kind":"Postgresql","apiVersion":"acid.zalan.do/v1","metadata":{"name":"acid-testcluster","creationTimestamp":qaz},"spec":{"postgresql":{"version":"","parameters":null},"volume":{"size":"","storageClass":""},"patroni":{"initdb":null,"pg_hba":null,"ttl":0,"loop_wait":0,"retry_timeout":0,"maximum_lag_on_failover":0,"slots":null},"resources":{"requests":{"cpu":"","memory":""},"limits":{"cpu":"","memory":""}},"teamId":"acid","allowedSourceRanges":null,"numberOfInstances":0,"users":null,"clone":null},"status":{"PostgresClusterStatus":"Invalid"}}`),
out: Postgresql{},
marshal: []byte{},
err: errors.New("invalid character 'q' looking for beginning of value"),
Expand Down
6 changes: 5 additions & 1 deletion pkg/apis/acid.zalan.do/v1/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.

4 changes: 2 additions & 2 deletions pkg/cluster/k8sres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,7 +747,7 @@ func (c *Cluster) generateSpiloPodEnvVars(uid types.UID, spiloConfiguration stri
envVars = append(envVars, v1.EnvVar{Name: "KUBERNETES_USE_CONFIGMAPS", Value: "true"})
}

if cloneDescription.ClusterName != "" {
if cloneDescription != nil && cloneDescription.ClusterName != "" {
envVars = append(envVars, c.generateCloneEnvironment(cloneDescription)...)
}

Expand Down Expand Up @@ -1004,7 +1004,7 @@ func (c *Cluster) generateStatefulSet(spec *acidv1.PostgresSpec) (*appsv1.Statef
spiloEnvVars := c.generateSpiloPodEnvVars(
c.Postgresql.GetUID(),
spiloConfiguration,
&spec.Clone,
spec.Clone,
spec.StandbyCluster,
customPodEnvVarsList,
)
Expand Down
59 changes: 31 additions & 28 deletions pkg/cluster/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,44 +63,47 @@ func noEmptySync(cluster *Cluster, err error, reason SyncReason) error {

func TestConnectionPoolerSynchronization(t *testing.T) {
testName := "Test connection pooler synchronization"
var cluster = New(
Config{
OpConfig: config.Config{
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
ConnectionPooler: config.ConnectionPooler{
ConnectionPoolerDefaultCPURequest: "100m",
ConnectionPoolerDefaultCPULimit: "100m",
ConnectionPoolerDefaultMemoryRequest: "100Mi",
ConnectionPoolerDefaultMemoryLimit: "100Mi",
NumberOfInstances: int32ToPointer(1),
newCluster := func() *Cluster {
Copy link
Contributor

@erthalion erthalion Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@hlihhovac @pashagolub Why these changes are necessary for the CloneDescription?

Copy link
Member

Choose a reason for hiding this comment

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

Ping, should this be removed for the scope of having a smaller PR=

Copy link
Contributor

Choose a reason for hiding this comment

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

@Jan-M do you want to remove test completely?

Copy link
Contributor

Choose a reason for hiding this comment

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

@erthalion these changes fix error during manual creation of postgresqls.acid.zalan.do CRD.

Copy link
Contributor

Choose a reason for hiding this comment

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

@erthalion these changes fix error during manual creation of postgresqls.acid.zalan.do CRD.

That's the goal of the whole PR, isn't it? I'm just trying to figure out if there is any mysterious value in changing unit test that seems to be completely unrelated (connection pooler test).

Copy link
Contributor

Choose a reason for hiding this comment

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

That unit test was broken with new functionality:
--- FAIL: TestConnectionPoolerSynchronization (0.00s)

Objects for testing before changes were static objects, after change introduced they became pointers... So before changes, the test just copies static objects and takes the address of it. And now copying pointer will still point to the same object! So tests always ruined the same underlying object with mock copies.

That's why all new mocked objects now created using New(), but not copying.

I hope it's clear enough :)

Copy link
Contributor

Choose a reason for hiding this comment

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

But only CloneDescription became a point, right? And it's not involved in this test, so still sounds strange for me. But ok, at least it's not included by mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

The thing is the test itself was written for static properties. As soon as we've added pointers the simple copying became a problem. That's why we do not copy mock objects but create them separately so they do not interfere

return New(
Config{
OpConfig: config.Config{
ProtectedRoles: []string{"admin"},
Auth: config.Auth{
SuperUsername: superUserName,
ReplicationUsername: replicationUserName,
},
ConnectionPooler: config.ConnectionPooler{
ConnectionPoolerDefaultCPURequest: "100m",
ConnectionPoolerDefaultCPULimit: "100m",
ConnectionPoolerDefaultMemoryRequest: "100Mi",
ConnectionPoolerDefaultMemoryLimit: "100Mi",
NumberOfInstances: int32ToPointer(1),
},
},
},
}, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder)
}, k8sutil.KubernetesClient{}, acidv1.Postgresql{}, logger, eventRecorder)
}
cluster := newCluster()

cluster.Statefulset = &appsv1.StatefulSet{
ObjectMeta: metav1.ObjectMeta{
Name: "test-sts",
},
}

clusterMissingObjects := *cluster
clusterMissingObjects := newCluster()
clusterMissingObjects.KubeClient = k8sutil.ClientMissingObjects()

clusterMock := *cluster
clusterMock := newCluster()
clusterMock.KubeClient = k8sutil.NewMockKubernetesClient()

clusterDirtyMock := *cluster
clusterDirtyMock := newCluster()
clusterDirtyMock.KubeClient = k8sutil.NewMockKubernetesClient()
clusterDirtyMock.ConnectionPooler = &ConnectionPoolerObjects{
Deployment: &appsv1.Deployment{},
Service: &v1.Service{},
}

clusterNewDefaultsMock := *cluster
clusterNewDefaultsMock := newCluster()
clusterNewDefaultsMock.KubeClient = k8sutil.NewMockKubernetesClient()

tests := []struct {
Expand All @@ -124,7 +127,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
ConnectionPooler: &acidv1.ConnectionPooler{},
},
},
cluster: &clusterMissingObjects,
cluster: clusterMissingObjects,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: objectsAreSaved,
Expand All @@ -139,7 +142,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
EnableConnectionPooler: boolToPointer(true),
},
},
cluster: &clusterMissingObjects,
cluster: clusterMissingObjects,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: objectsAreSaved,
Expand All @@ -154,7 +157,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
ConnectionPooler: &acidv1.ConnectionPooler{},
},
},
cluster: &clusterMissingObjects,
cluster: clusterMissingObjects,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: objectsAreSaved,
Expand All @@ -169,7 +172,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
newSpec: &acidv1.Postgresql{
Spec: acidv1.PostgresSpec{},
},
cluster: &clusterMock,
cluster: clusterMock,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: objectsAreDeleted,
Expand All @@ -182,7 +185,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
newSpec: &acidv1.Postgresql{
Spec: acidv1.PostgresSpec{},
},
cluster: &clusterDirtyMock,
cluster: clusterDirtyMock,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: objectsAreDeleted,
Expand All @@ -203,7 +206,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
},
},
},
cluster: &clusterMock,
cluster: clusterMock,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: deploymentUpdated,
Expand All @@ -220,7 +223,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
ConnectionPooler: &acidv1.ConnectionPooler{},
},
},
cluster: &clusterNewDefaultsMock,
cluster: clusterNewDefaultsMock,
defaultImage: "pooler:2.0",
defaultInstances: 2,
check: deploymentUpdated,
Expand All @@ -239,7 +242,7 @@ func TestConnectionPoolerSynchronization(t *testing.T) {
ConnectionPooler: &acidv1.ConnectionPooler{},
},
},
cluster: &clusterMock,
cluster: clusterMock,
defaultImage: "pooler:1.0",
defaultInstances: 1,
check: noEmptySync,
Expand Down