search

scylladb / scylla-operator

The Kubernetes Operator for ScyllaDB
https://operator.docs.scylladb.com/
Apache License 2.0
332 stars 162 forks source link

Support cron and timezone in manager task integration #1851

Closed rzetelskik closed 5 months ago

rzetelskik commented 6 months ago

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[bot] commented 6 months ago

@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](https://github.com/scylladb/scylla-operator/pull/1851): > > >**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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
rzetelskik commented 6 months ago

/priority critical-urgent since the bug fixes are a prerequisite for https://github.com/scylladb/scylla-operator/issues/1752

rzetelskik commented 6 months ago

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

Michal-Leszczynski commented 6 months ago

Feel free to ignore my comments if they are not related to this PR. I just wanted to highlight some fields that perhaps require additional attention and give context for some of them.

rzetelskik commented 6 months ago

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?

Agreed. Is that mentioned in sctool/sm?

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

There's a separate PR in queue with interval deprecation. Best we can do is a field description though.

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.

To be honest - no idea. I don't think we can change this in this API version though.

rzetelskik commented 6 months ago

split mapstructure fix into a separate PR: https://github.com/scylladb/scylla-operator/pull/1854 /hold for https://github.com/scylladb/scylla-operator/pull/1854

rzetelskik commented 6 months ago

/hold cancel

rzetelskik commented 6 months ago

ping @tnozicka

rzetelskik commented 6 months ago

/hold for https://github.com/scylladb/scylla-operator/pull/1868

rzetelskik commented 5 months ago

@zimnx @tnozicka I believe I have addressed all your comments - please have another go.

rzetelskik commented 5 months ago

@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 commented 5 months ago

manager flake and upgrade flake - can't possibly be deteriorated by this PR https://github.com/scylladb/scylla-operator/issues/1694#issuecomment-2066105109 https://github.com/scylladb/scylla-operator/issues/1233#issuecomment-2066106629

/test images /retest

rzetelskik commented 5 months ago

@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 commented 5 months ago

same thing again /retest

rzetelskik commented 5 months ago

/test images /retest

rzetelskik commented 5 months ago

and again /retest

rzetelskik commented 5 months ago

aaand again

/test images /retest

scylla-operator-bot[bot] commented 5 months ago

@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 415c60b1b0dcf5fe7a85ec399e8ce47c4fd79bf2 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
rzetelskik commented 5 months ago

/retest

scylla-operator-bot[bot] commented 5 months ago

[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](https://github.com/scylladb/scylla-operator/blob/master/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 commented 5 months ago

/hold cancel