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

Refactor watcher and kyma reconcilers to improve load test performance #277

Merged
merged 66 commits into from
Nov 11, 2022
Merged

Refactor watcher and kyma reconcilers to improve load test performance #277

merged 66 commits into from
Nov 11, 2022

Conversation

fourthisle
Copy link
Contributor

@fourthisle fourthisle commented Oct 18, 2022 •
edited
Loading

AC:

  • Dedicate controller for Watcher CR
    • In the reconcile, do virtualservice update
    • Config 1 worker to avoid concurrent write
  • Migrate skrwebhook installation logic into Kyma controller
    • Skrwebhook installation state as kyma condition
  • Make sure ca cert between Secret and ValidationWebhookConfigurations consistent:

    Done by removing secret lookup from SKR web-hook chart here
    Verification steps:

    k get secret skr-webhook-tls -o json | jq -r '.data["ca.crt"]' | base64 --decode > skr-ca-cert.pem && \
k get validatingwebhookconfigurations.admissionregistration.k8s.io skr-webhook -o json | jq -r '.webhooks[0].clientConfig.caBundle' | base64 --decode > skr-ca-bundle.pem && \
diff -u skr-ca-cert.pem skr-ca-bundle.pem
  • Make sure web-hook deployment pods are restarted when chart is updated

@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2022
@kyma-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kyma-bot kyma-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 18, 2022
@fourthisle fourthisle linked an issue Oct 18, 2022 that may be closed by this pull request
Refactoring watcher reconcile logic #266
Closed
6 tasks
@fourthisle
Copy link
Contributor Author

/test all

@kyma-bot kyma-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 Oct 20, 2022
@fourthisle fourthisle changed the title Watcher Reconciler reconciles Kymas WIP--Watcher Reconciler reconciles Kymas Oct 20, 2022
@fourthisle fourthisle marked this pull request as ready for review October 20, 2022 07:29
fourthisle and others added 19 commits October 20, 2022 10:00
@fourthisle
Merge remote-tracking branch 'upstream/main' into watcher-reconciles-…
3397e6a 
…kymas
@fourthisle
go mod tidy
63f4f77
@fourthisle
fix nil httpRoute bug
8f1dbaa
@fourthisle
disable watcher cr state handling
f16c728
@fourthisle
fix first bug in ACs
fe89be7
@fourthisle
add pre-install check for skr webhook chart
24476ad
@ruanxin
add condition for skrwebhook installation
7223aee
@ruanxin
Merge remote-tracking branch 'khlifi411/watcher-reconciles-kymas' int…
1c5b9bf 
…o watcher-reconciles-kymas

# Conflicts:
#	operator/internal/deploy/chart_manager.go
@fourthisle
refactor skr chart config out from kyma reconciler
dcc29ea
@fourthisle
Merge remote-tracking branch 'origin/watcher-reconciles-kymas' into w…
94f66d9 
…atcher-reconciles-kymas
@fourthisle
fix tests for deploy pkg
9c88194
@fourthisle
fix failing tests for watcher ctrl
6f8f208
@ruanxin
create a dedicate test suite for watcher
935f6fa
@fourthisle
refactor watcher controller tests to run with kyma controller
a168513
@fourthisle
remove commented code
b61bf95
@fourthisle
move creating istio resources to suite setup for with_watcher tests
6f3398b
@fourthisle
remove duplicate tests and fix kyma reconciler _with_watcher tests
00df998
@fourthisle
set MaxConcurrentReconciles to 1 for WatcherReconciler
5faf3ef
@fourthisle
reuse duplicate methods in kyma reconciler tests
7c7fa74
jakobmoellerdev
jakobmoellerdev reviewed Nov 3, 2022
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

I noticed that in the watcher kustomize target, there is a patch saying

- op: add
  path: /spec/template/spec/containers/0/args/-
  value: --enable-kcp-watcher=false

this is probably wrong since the watcher kustomization should enable the watcher. Not sure how you currently test it but this is not working out of the box I think...

operator/main.go Show resolved Hide resolved
operator/main.go Outdated Show resolved Hide resolved
This was referenced Nov 7, 2022
Closed
Closed
Closed
jakobmoellerdev
jakobmoellerdev suggested changes Nov 8, 2022
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Please Rebase

@fourthisle fourthisle changed the title Refactor watcher reconciler and kyma reconciler to improve load test performance WIP-Refactor watcher and kyma reconcilers to improve load test performance Nov 9, 2022
@kyma-bot kyma-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 9, 2022
jakobmoellerdev
jakobmoellerdev suggested changes Nov 9, 2022
View reviewed changes
Copy link
Contributor

@jakobmoellerdev jakobmoellerdev left a comment

Choose a reason for hiding this comment

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

Please Rebase and fix tests

@kyma-bot
Copy link
Contributor

kyma-bot commented Nov 11, 2022 •
edited
Loading

@khlifi411: 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
pull-lifecycle-manager-pjtester 7f86a7a link true /test pull-lifecycle-manager-pjtester

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.

@fourthisle fourthisle changed the title WIP-Refactor watcher and kyma reconcilers to improve load test performance Refactor watcher and kyma reconcilers to improve load test performance Nov 11, 2022
@kyma-bot kyma-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 11, 2022
@kyma-bot kyma-bot added the lgtm Looks good to me! label Nov 11, 2022
@kyma-bot kyma-bot merged commit 791816f into kyma-project:main Nov 11, 2022
@fourthisle fourthisle mentioned this pull request Nov 16, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanxin ruanxin ruanxin left review comments

@jakobmoellerdev jakobmoellerdev jakobmoellerdev approved these changes

Assignees

@adityabhatia adityabhatia

@jakobmoellerdev jakobmoellerdev

Labels
lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactoring watcher reconcile logic
5 participants
@fourthisle @kyma-bot @ruanxin @jakobmoellerdev @adityabhatia