Support cron and timezone in manager task integration #1851

rzetelskik · 2024-03-19T23:08:34Z

Description of your changes: This PR adds missing cron and related fields, currently missing from out API, to manager task specs and extends the unit tests to cover them.

Which issue is resolved by this Pull Request:
Resolves #1739

/kind feature
/priority important-longterm
/cc

scylla-operator-bot · 2024-03-19T23:08:37Z

@rzetelskik: GitHub didn't allow me to request PR reviews from the following users: rzetelskik.

Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Description of your changes:

Which issue is resolved by this Pull Request:
Resolves #1739
Resolves #1796

/kind feature
/priority important-longterm
/cc

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

rzetelskik · 2024-03-20T09:08:03Z

/priority critical-urgent
since the bug fixes are a prerequisite for #1752

rzetelskik · 2024-03-20T09:39:19Z

/cc @Michal-Leszczynski
would you be able to verify if there's anything missing from manager task specs in our API?

pkg/api/scylla/v1/types_cluster.go

tnozicka

please fix the API to use pointers where needed

pkg/api/scylla/v1/types_cluster.go

Michal-Leszczynski

        // startDate specifies the task start date expressed in the RFC3339 format or now[+duration],
	// e.g. now+3d2h10m, valid units are d, h, m, s.
	// +kubebuilder:default:="now"
	// +optional
	StartDate string `json:"startDate,omitempty"`

The now syntax can cause a lot of pain since it is parsed on the operator or sctool side. Maybe it's worth to discourage users from using via appropriate field description?

	// interval represents a task schedule interval e.g. 3d2h10m, valid units are d, h, m, s.
	// +optional
	// +kubebuilder:default:="0"
	Interval string `json:"interval,omitempty"`

The interval field is (or should be) deprecated by SM. Is it also deprecated here?

        // For Scylla clusters that are row-level repair enabled, setting intensity below 1 has the same effect as setting intensity 1.
	// +kubebuilder:default:="1"
	// +optional
	Intensity string `json:"intensity,omitempty" mapstructure:"intensity,omitempty"`

Why is it string? SM supports float64 for backward compatibility, but all floats are converted to ints anyway, because they don't make sense anymore from Scylla POV.

pkg/api/scylla/v1/types_cluster.go

rzetelskik · 2024-04-18T12:44:13Z

@tnozicka I added cron and timezone validation. TZ and CRON_TZ are not accepted as part of cron for now. I'll raise an issue with the manager team.

rzetelskik · 2024-04-19T08:41:01Z

manager flake and upgrade flake - can't possibly be deteriorated by this PR
#1694 (comment)
#1233 (comment)

/test images
/retest

tnozicka

thanks, the api and overall logic looks sound to me, had some technical comments

tnozicka · 2024-04-19T08:48:09Z

pkg/api/scylla/validation/cluster_validation.go

@@ -34,6 +38,8 @@ var (
 		scyllav1.BroadcastAddressTypeServiceClusterIP,
 		scyllav1.BroadcastAddressTypeServiceLoadBalancerIngress,
 	}
+
+	schedulerTaskSpecCronParser = cron.NewParser(cron.SecondOptional | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow | cron.Descriptor)


I am not sure the parser should be a global var as it may be an issue with concurrent access. I'd globalize only the parsing options.

tnozicka · 2024-04-19T09:08:26Z

pkg/api/scylla/validation/cluster_validation.go

+
+	// We can only validate interval when cron is non-nil for backwards compatibility.
+	if schedulerTaskSpec.Interval != nil && schedulerTaskSpec.Cron != nil {
+		if intervalDuration, err := duration.ParseDuration(*schedulerTaskSpec.Interval); err != nil {


nit: please split the assignment to make it shorter

tnozicka · 2024-04-19T09:15:26Z

pkg/api/scylla/validation/cluster_validation_test.go

+			expectedErrorString: "",
+		},
+		{
+			name: "invalid timezone in manager backup task spec",


it's unclear whether the test should produce and error or not from it's name (in other places as well), especially given it says invalid and produces no error

my bad, it shouldn't say invalid here

…and cron

rzetelskik · 2024-04-19T11:30:12Z

@rzetelskik: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gke-release-script-latest 415c60b link true /test e2e-gke-release-script-latest
ci/prow/e2e-gke-parallel-clusterip 7ea90ba link true /test e2e-gke-parallel-clusterip
ci/prow/e2e-gke-parallel 7ea90ba link true /test e2e-gke-parallel
Full PR test history. Your PR dashboard.

manager flake
/retest

rzetelskik · 2024-04-19T12:23:55Z

same thing again
/retest

rzetelskik · 2024-04-19T12:41:13Z

/test images
/retest

rzetelskik · 2024-04-19T13:44:31Z

and again
/retest

tnozicka

thanks for the updates

/approve
high level lgtm but I only scanned the test
/assign @zimnx

rzetelskik · 2024-04-19T20:47:21Z

aaand again

/test images
/retest

scylla-operator-bot · 2024-04-19T21:39:15Z

@rzetelskik: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gke-release-script-latest	`415c60b`	link	true	`/test e2e-gke-release-script-latest`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

rzetelskik · 2024-04-19T22:23:56Z

/retest

zimnx

thanks
/lgtm

scylla-operator-bot · 2024-04-23T11:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [tnozicka,zimnx]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rzetelskik · 2024-04-23T11:48:07Z

/hold cancel

scylla-operator-bot bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 19, 2024

scylla-operator-bot bot added priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 19, 2024

rzetelskik force-pushed the manager-api-sync branch from ef1dc3e to d286501 Compare March 19, 2024 23:13

rzetelskik mentioned this pull request Mar 19, 2024

Fix manager task reconciliation and status synchronisation #1850

Merged

rzetelskik force-pushed the manager-api-sync branch 5 times, most recently from 777d05a to 0b41f8e Compare March 20, 2024 08:56

scylla-operator-bot bot added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 20, 2024

rzetelskik removed the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Mar 20, 2024

rzetelskik force-pushed the manager-api-sync branch from 0b41f8e to c21f5e5 Compare March 20, 2024 09:38

scylla-operator-bot bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 20, 2024

scylla-operator-bot bot requested a review from Michal-Leszczynski March 20, 2024 09:39

rzetelskik changed the title ~~[WIP] Support missing API fields in manager task spec and integration~~ Support missing API fields in manager task spec and integration Mar 20, 2024

scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2024

rzetelskik force-pushed the manager-api-sync branch from c21f5e5 to 8c58331 Compare March 20, 2024 09:46

tnozicka reviewed Mar 20, 2024

View reviewed changes

pkg/api/scylla/v1/types_cluster.go Outdated Show resolved Hide resolved

rzetelskik mentioned this pull request Mar 20, 2024

Deprecate interval in manager task schedule spec #1853

Merged

rzetelskik force-pushed the manager-api-sync branch from 8c58331 to 328d3a3 Compare March 20, 2024 10:36

tnozicka requested changes Mar 20, 2024

View reviewed changes

pkg/api/scylla/v1/types_cluster.go Outdated Show resolved Hide resolved

scylla-operator-bot bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Mar 20, 2024

Michal-Leszczynski reviewed Mar 20, 2024

View reviewed changes

pkg/api/scylla/v1/types_cluster.go Outdated Show resolved Hide resolved

pkg/api/scylla/v1/types_cluster.go Show resolved Hide resolved

pkg/api/scylla/v1/types_cluster.go Outdated Show resolved Hide resolved

pkg/api/scylla/v1/types_cluster.go Outdated Show resolved Hide resolved

scylla-operator-bot bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 18, 2024

rzetelskik requested a review from tnozicka April 18, 2024 12:23

scylla-operator-bot bot added the area/dependency Issues or PRs related to dependency changes label Apr 18, 2024

rzetelskik mentioned this pull request Apr 19, 2024

[Flake] Scylla Manager integration [It] should register cluster and sync repair tasks #1694

Closed

tnozicka reviewed Apr 19, 2024

View reviewed changes

rzetelskik added 3 commits April 19, 2024 12:23

Validate cron, timezone, and mutual exclusivity of non-zero interval …

d216bf2

…and cron

Update generated

fc81c74

Update dependencies

7ea90ba

rzetelskik force-pushed the manager-api-sync branch from c1517f7 to 7ea90ba Compare April 19, 2024 10:23

rzetelskik requested a review from tnozicka April 19, 2024 10:24

tnozicka approved these changes Apr 19, 2024

View reviewed changes

scylla-operator-bot bot assigned zimnx Apr 19, 2024

scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 19, 2024

zimnx approved these changes Apr 23, 2024

View reviewed changes

scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2024

scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 23, 2024

scylla-operator-bot bot merged commit 962fd4c into scylladb:master Apr 23, 2024
13 checks passed

rzetelskik deleted the manager-api-sync branch April 23, 2024 12:27

rzetelskik mentioned this pull request May 16, 2024

Support cron in backups and repairs. #1114

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support cron and timezone in manager task integration #1851

Support cron and timezone in manager task integration #1851

rzetelskik commented Mar 19, 2024 •

edited

Loading

scylla-operator-bot bot commented Mar 19, 2024

rzetelskik commented Mar 20, 2024

rzetelskik commented Mar 20, 2024

tnozicka left a comment

Michal-Leszczynski left a comment

rzetelskik commented Apr 18, 2024

rzetelskik commented Apr 19, 2024

tnozicka left a comment

tnozicka Apr 19, 2024

rzetelskik Apr 19, 2024

tnozicka Apr 19, 2024

rzetelskik Apr 19, 2024

tnozicka Apr 19, 2024

rzetelskik Apr 19, 2024

rzetelskik Apr 19, 2024

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

tnozicka left a comment

rzetelskik commented Apr 19, 2024

scylla-operator-bot bot commented Apr 19, 2024 •

edited

Loading

rzetelskik commented Apr 19, 2024

zimnx left a comment

scylla-operator-bot bot commented Apr 23, 2024

rzetelskik commented Apr 23, 2024

Support cron and timezone in manager task integration #1851

Support cron and timezone in manager task integration #1851

Conversation

rzetelskik commented Mar 19, 2024 • edited Loading

scylla-operator-bot bot commented Mar 19, 2024

rzetelskik commented Mar 20, 2024

rzetelskik commented Mar 20, 2024

tnozicka left a comment

Choose a reason for hiding this comment

Michal-Leszczynski left a comment

Choose a reason for hiding this comment

rzetelskik commented Apr 18, 2024

rzetelskik commented Apr 19, 2024

tnozicka left a comment

Choose a reason for hiding this comment

tnozicka Apr 19, 2024

Choose a reason for hiding this comment

rzetelskik Apr 19, 2024

Choose a reason for hiding this comment

tnozicka Apr 19, 2024

Choose a reason for hiding this comment

rzetelskik Apr 19, 2024

Choose a reason for hiding this comment

tnozicka Apr 19, 2024

Choose a reason for hiding this comment

rzetelskik Apr 19, 2024

Choose a reason for hiding this comment

rzetelskik Apr 19, 2024

Choose a reason for hiding this comment

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

rzetelskik commented Apr 19, 2024

tnozicka left a comment

Choose a reason for hiding this comment

rzetelskik commented Apr 19, 2024

scylla-operator-bot bot commented Apr 19, 2024 • edited Loading

rzetelskik commented Apr 19, 2024

zimnx left a comment

Choose a reason for hiding this comment

scylla-operator-bot bot commented Apr 23, 2024

rzetelskik commented Apr 23, 2024

rzetelskik commented Mar 19, 2024 •

edited

Loading

scylla-operator-bot bot commented Apr 19, 2024 •

edited

Loading