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

Feature/timelineid in clone section #1961

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+20 −44
Open

Feature/timelineid in clone section #1961

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8780827
modify apis to enable timeline_id parameter in clone section
Jul 11, 2022
2327062
modify cluster pkg to append timelineid
Jul 11, 2022
a2236c6
change type from string to int
Jul 17, 2022
ffbdb9d
reverting change f for int
Jul 18, 2022
641bb20
code formatting and change of variable name
Jul 18, 2022
baa5b04
update docs
Jul 18, 2022
15b693c
remove default from clone struct
Jul 19, 2022
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
3 changes: 3 additions & 0 deletions pkg/apis/acid.zalan.do/v1/crds.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ var PostgresCRDResourceValidation = apiextv1.CustomResourceValidation{
Type: "string",
Format: "uuid",
},
"clone_target_timeline": {
Type: "string",
Copy link
Member

Choose a reason for hiding this comment

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

type should be integer

Copy link
Author

Choose a reason for hiding this comment

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

Actually the timeline can be characters like A, B etc.. for eg C here: base_0000000C00000001000000A3

Copy link
Member

@FxKu FxKu Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

But this is not the timeline ID

Copy link
Contributor

Choose a reason for hiding this comment

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

@Sudeepta92 FxKu is right here. The actual ID can only be an integer. What you see in the WAL archive name is a hex value. So for instance 0000003400000000000000CD would be timeline_id 52. Nevertheless, one can also specify latest or current in the recovery.conf. Which is default if nothing is set.
So I think, you could use and int32 here.

Copy link
Author

Choose a reason for hiding this comment

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

Oh yeahh its a hex value in the end. I will change to int32.

Copy link
Author

Choose a reason for hiding this comment

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

@FxKu @thedatabaseme btw, on changing type to integer back from string it fails when no timeline id is passed as it can no longer receive "latest"/"current".

Copy link
Author

Choose a reason for hiding this comment

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

Ok, so have removed the possibility for string value "latest" and in such a scenario it will calcuate the latest timeline id as integer. You may ignore my previous comment.

},
},
},
"connectionPooler": {
Expand Down
3 changes: 2 additions & 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 @@ -197,7 +197,8 @@ type CloneDescription struct {
S3Endpoint string `json:"s3_endpoint,omitempty"`
S3AccessKeyId string `json:"s3_access_key_id,omitempty"`
S3SecretAccessKey string `json:"s3_secret_access_key,omitempty"`
S3ForcePathStyle *bool `json:"s3_force_path_style,omitempty" defaults:"false"`
S3ForcePathStyle *bool `json:"s3_force_path_style,omitempty" defaults:"false"`
TimelineID string `json:"clone_target_timeline" defaults:"latest"`
Copy link
Member

@FxKu FxKu Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please use go.fmt for right code formatting. This is misaligned.
And type should better be int32. I know it's used as an environment variable later but we can enforece users to only specify numbers here.

Copy link
Author

Choose a reason for hiding this comment

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

The type has been changed and the formatting is corrected.

}

// Sidecar defines a container to be run in the same pod as the Postgres container.
Expand Down
8 changes: 4 additions & 4 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 @@ -66,12 +66,12 @@ var cloneClusterDescriptions = []struct {
in *CloneDescription
err error
}{
{"cluster name invalid but EndTimeSet is not empty", &CloneDescription{"foo+bar", "", "NotEmpty", "", "", "", "", nil}, nil},
{"expect error as cluster name does not match DNS-1035", &CloneDescription{"foo+bar", "", "", "", "", "", "", nil},
{"cluster name invalid but EndTimeSet is not empty", &CloneDescription{"foo+bar", "", "NotEmpty", "", "", "", "", nil, 0}, nil},
{"expect error as cluster name does not match DNS-1035", &CloneDescription{"foo+bar", "", "", "", "", "", "", nil, 0},
errors.New(`clone cluster name must confirm to DNS-1035, regex used for validation is "^[a-z]([-a-z0-9]*[a-z0-9])?$"`)},
{"expect error as cluster name is too long", &CloneDescription{"foobar123456789012345678901234567890123456789012345678901234567890", "", "", "", "", "", "", nil},
{"expect error as cluster name is too long", &CloneDescription{"foobar123456789012345678901234567890123456789012345678901234567890", "", "", "", "", "", "", nil, 0},
errors.New("clone cluster name must be no longer than 63 characters")},
{"common cluster name", &CloneDescription{"foobar", "", "", "", "", "", "", nil}, nil},
{"common cluster name", &CloneDescription{"foobar", "", "", "", "", "", "", nil, 0}, nil},
}

var maintenanceWindows = []struct {
Expand Down
4 changes: 4 additions & 0 deletions pkg/cluster/k8sres.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1907,6 +1907,10 @@ func (c *Cluster) generateCloneEnvironment(description *acidv1.CloneDescription)

result = append(result, v1.EnvVar{Name: "CLONE_AWS_S3_FORCE_PATH_STYLE", Value: s3ForcePathStyle})
}

if description.TimelineID != "" {
result = append(result, v1.EnvVar{Name: "CLONE_TARGET_TIMELINE", Value: description.TimelineID})
}
}

return result
Expand Down