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.

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

Add informers, listers, full ScyllaCluster reconciliation, typed clients and much more #534

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

tnozicka
Copy link
Member

@tnozicka tnozicka commented Apr 1, 2021

Description of your changes:

  • Full ScyllaCluster CR reconciliation
  • Pruning old resources (like services on scale down)
  • Abides The Three Laws of Controllers
  • Using Informers and caches as much as possible
  • Using generated typed client for ScyllaCluster
  • Independent, cache based probes
  • Isolates flags from internal abstractions
  • All controllers can be run from outside of the cluster (for dev)
  • Includes partial seedless node support to avoid an old race (tests and restriction lift will go in Implement seedless mode #597)
  • Fixes scylla graceful termination
  • Splits Oprhaned PV controller and wires handlers for node deletion
  • Separates helper functions and validation out of the API package
  • Dedicated webhook server for separation of concerns and lifetime (better availability)
  • Graceful termination for webhook server, probes and operators
  • Fixes upgrade handlers that could be unintentionally skipped if a pod was evicted during the rolling upgrade.

Which issue is resolved by this Pull Request:
Fixes #469
Fixes #470
Fixes #429
Fixes #475
Fixes #469
Fixes #513
Fixes #514
Fixes #459
Fixes #607
Fixes #558
Fixes #526
Fixes #520

Followups

Audit results

  {
    "user": "system:serviceaccount:scylla-manager:scylla-manager-controller",
    "total": 17230,
    "verbs": {
      "create": 8,
      "get": 12174,
      "list": 6,
      "update": 5026,
      "watch": 16
    }
  },
  {
    "user": "system:serviceaccount:scylla-operator:scylla-operator",
    "total": 19855,
    "verbs": {
      "create": 578,
      "delete": 24,
      "get": 13100,
      "list": 18,
      "patch": 208,
      "update": 5798,
      "watch": 129
    }
  },

37085/48 = ~772 API calls per minute (doesn't include the side car and health checks)

  {
    "user": "system:serviceaccount:scylla-manager:scylla-manager-cluster-member",
    "total": 2466,
    "verbs": {
      "get": 2446,
      "list": 4,
      "watch": 16
    }
  },

the longest lived scylla cluster (health checks + side car) made 2466 API calls

  {
    "user": "system:serviceaccount:scylla-manager:scylla-manager-controller",
    "total": 1640,
    "verbs": {
      "create": 2,
      "get": 964,
      "list": 4,
      "update": 632,
      "watch": 38
    }
  },
  {
    "user": "system:serviceaccount:scylla-operator:scylla-operator",
    "total": 4929,
    "verbs": {
      "create": 1204,
      "delete": 530,
      "get": 1038,
      "list": 286,
      "patch": 440,
      "update": 1260,
      "watch": 171
    }
  },

6569/46= ~143 API calls per minute (doesn't include the side car and health checks)

  {
    "user": "system:serviceaccount:e2e-test-scyllacluster-92jtt-q6brr:e2e-user",
    "total": 155,
    "verbs": {
      "create": 6,
      "get": 124,
      "list": 14,
      "patch": 2,
      "watch": 9
    }
  },

the longest lived scylla cluster (health checks + side car) made 155 API calls

Final results

6569/37085=~18% of API calls per controllers
155/2466=~6% of API calls per scyllacluster

@tnozicka tnozicka marked this pull request as draft April 1, 2021 15:49
@tnozicka tnozicka force-pushed the informers branch 2 times, most recently from fd81ec5 to c245100 Compare April 1, 2021 15:55
@tnozicka tnozicka modified the milestones: 1.2, 1.3 Apr 7, 2021
@tnozicka tnozicka added kind/bug Categorizes issue or PR as related to a bug. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. kind/feature Categorizes issue or PR as related to a new feature. and removed kind/bug Categorizes issue or PR as related to a bug. labels Apr 12, 2021
@tnozicka tnozicka closed this Apr 26, 2021
@tnozicka tnozicka deleted the informers branch April 26, 2021 12:11
@tnozicka tnozicka restored the informers branch April 26, 2021 12:11
@tnozicka tnozicka reopened this Apr 26, 2021
@tnozicka tnozicka force-pushed the informers branch 3 times, most recently from 096e019 to 3ca32f5 Compare May 6, 2021 14:38
@tnozicka tnozicka force-pushed the informers branch 3 times, most recently from 312beeb to 58f1c49 Compare May 24, 2021 12:40
@tnozicka tnozicka modified the milestones: v1.3, v1.4 May 26, 2021
@tnozicka tnozicka force-pushed the informers branch 4 times, most recently from 5db8369 to 37ad2ca Compare May 28, 2021 10:18
@tnozicka tnozicka force-pushed the informers branch 3 times, most recently from 4b7e631 to 5eaf2cb Compare June 8, 2021 14:32
@tnozicka
Copy link
Member Author

@zimnx thanks for the first round, comments addressed

@tnozicka
Copy link
Member Author

fixed the new scale down test

Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

2nd batch

pkg/controller/scyllacluster/sync_services.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_services.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
pkg/controller/scyllacluster/sync_statefulsets.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

last batch for this iteration. I like changes very much, code looks very mature. Awesome work!

pkg/controller/sidecar/controller.go Outdated Show resolved Hide resolved
pkg/leaderelection/leaderelection.go Show resolved Hide resolved
pkg/resourceapply/apps.go Outdated Show resolved Hide resolved
@tnozicka
Copy link
Member Author

tnozicka commented Jun 23, 2021

Thanks for the review. I'v addressed most of it but there are still some comments I need to address tomorrow.

@tnozicka tnozicka force-pushed the informers branch 3 times, most recently from 1a2bef3 to 6ee0746 Compare June 25, 2021 10:45
pkg/util/parallel/parallel.go Outdated Show resolved Hide resolved
pkg/resourceapply/apps_test.go Show resolved Hide resolved
pkg/resourceapply/apps.go Outdated Show resolved Hide resolved
@tnozicka tnozicka force-pushed the informers branch 2 times, most recently from 16e40cc to 69660b0 Compare June 28, 2021 09:53
@zimnx zimnx mentioned this pull request Jun 28, 2021
Copy link
Collaborator

@zimnx zimnx left a comment

Choose a reason for hiding this comment

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

LGTM, good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment