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

🐛 Start web hooks first #1690

Merged
merged 2 commits into from
Nov 10, 2021
Merged

🐛 Start web hooks first #1690

merged 2 commits into from
Nov 10, 2021

Conversation

fabriziopandini
Copy link
Member

This PR test a simpler and possible cleaner solution to #1685.
We should test how this impact start time

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 11, 2021
@fabriziopandini fabriziopandini changed the title [WIP] 🐛Start web hooks first [WIP] 🐛 Start web hooks first Oct 11, 2021
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Oct 11, 2021 •
edited
Loading

NOTE: starting the webhooks in CAPI (where we have about 15 to 20 webhooks) takes about 100ms; as a side effect this start cache faster in case they requires conversion

vincepri
vincepri reviewed Oct 11, 2021
View reviewed changes
pkg/manager/internal.go Outdated Show resolved Hide resolved
pkg/manager/internal.go Show resolved Hide resolved
@vincepri
Copy link
Member

This seems cleaner than the other PR, +1 on merging

@fabriziopandini fabriziopandini mentioned this pull request Oct 11, 2021
Closed
@alvaroaleman
Copy link
Member

I like the approach. Any chance we could test this?

@vincepri
Copy link
Member

@alvaroaleman When I first moved the code to run webhooks first there wasn't a nice way to do it from the outside.

@fabriziopandini
Copy link
Member Author

I will take a look about tests tomorrow morning, at least this is a chance to learn this part as well

@alvaroaleman
Copy link
Member

@alvaroaleman When I first moved the code to run webhooks first there wasn't a nice way to do it from the outside.

Maybe just set up a manager with a webhook and a runnable that does a http request to the webhook in its Start()? It is a bit annoying because if we race like before it will flake and not reliably fail, but I guess that is unavoidable when it comes to race conditions that are not data races?

@fabriziopandini
Copy link
Member Author

While investigating unit tests, we are testing this with CAPI and we just found some problems in https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-a[…]ager/capi-controller-manager-5d857f645f-wpv8v/manager.log

It seems we still have deadlock with the health probes. If the health probes are not started our conversion webhook won’t be accessible as the Pod doesn’t get ready.
@sbueringer is working to a fix for this

@sbueringer
Copy link
Member

Fix seems to work:

@fabriziopandini
Copy link
Member Author

I have a test ensuring webhook start before cache.
What is missing is adding to the test that health should start before cache too

@fabriziopandini
Copy link
Member Author

@vincepri should I close this one given the work on #1695 ?

@vincepri
Copy link
Member

Let's close it once/if the other one merges, this fix can probably be rebased on top of release-0.10

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 4, 2021
@k8s-ci-robot
Copy link
Contributor

@fabriziopandini: 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
pull-controller-runtime-test-master b267bb5 link true /test pull-controller-runtime-test-master
pull-controller-runtime-apidiff-master b267bb5 link false /test pull-controller-runtime-apidiff-master

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@fabriziopandini fabriziopandini changed the base branch from master to release-0.10 November 4, 2021 21:52
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 4, 2021
@fabriziopandini
Copy link
Member Author

/retest

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 4, 2021
@fabriziopandini
Copy link
Member Author

@vincepri rebased on to of the release 0.10 branch

@sbueringer
Copy link
Member

sbueringer commented Nov 5, 2021 •
edited
Loading

@fabriziopandini Can you please include the change in: fabriziopandini#1 ?

The problem is that otherwise we'll have the same deadlock just with the healthprobes.

I would then take this version and rebase my 2 test PRs in CAPI to run all our tests with and without leader election

@fabriziopandini
start healhprobes sync
708d539 
Co-authored-by: Stefan Büringer <[email protected]>
@fabriziopandini
Copy link
Member Author

@fabriziopandini Can you please include the change in: fabriziopandini#1 ?

Done!

This was referenced Nov 5, 2021
Closed
Closed
@sbueringer
Copy link
Member

sbueringer commented Nov 5, 2021 •
edited
Loading

Thx! Tests are running here:

P.S. I might need a few tries, just refactored those branches back to use CR release-0.10 instead of main.

UPDATE: tests are all green

@sbueringer
Copy link
Member

@fabriziopandini WDYT, ready for review/squash/un-wip? :)

@fabriziopandini fabriziopandini changed the title [WIP] 🐛 Start web hooks first 🐛 Start web hooks first Nov 10, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 10, 2021
@fabriziopandini
Copy link
Member Author

Removed WIP, given #1690 (comment)
two commits make sense for me but happy to squash if people prefer this way

@vincepri @alvaroaleman PTAL

vincepri
vincepri approved these changes Nov 10, 2021
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm
/milestone v0.10

@k8s-ci-robot
Copy link
Contributor

@vincepri: The provided milestone is not valid for this repository. Milestones in this repository: [1.0.0, Next, v0.10.x, v0.5.x, v0.6.x, v0.7.x, v0.8.x, v0.9.x]

Use /milestone clear to clear the milestone.

In response to this:

/lgtm
/milestone v0.10

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.

@vincepri vincepri added this to the v0.10.x milestone Nov 10, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 10, 2021
@k8s-ci-robot k8s-ci-robot merged commit f8472ce into kubernetes-sigs:release-0.10 Nov 10, 2021
This was referenced Nov 10, 2021
Closed
Closed
@fabriziopandini fabriziopandini deleted the start-webhooks-first branch November 15, 2021 12:57
@jmendesky jmendesky mentioned this pull request Aug 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@gerred gerred Awaiting requested review from gerred

Assignees

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v0.10.x
Development

Successfully merging this pull request may close these issues.
5 participants
@fabriziopandini @vincepri @alvaroaleman @sbueringer @k8s-ci-robot