Skip to content

Enable webhook leader election.#8386

Merged
knative-prow-robot merged 2 commits into
knative:masterfrom
mattmoor:allow-webhook-leaderelection
Jun 22, 2020
Merged

Enable webhook leader election.#8386
knative-prow-robot merged 2 commits into
knative:masterfrom
mattmoor:allow-webhook-leaderelection

Conversation

@mattmoor
Copy link
Copy Markdown
Member

This also directs e2e tests to enable this and use 10 buckets.

Fixes: #8139

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jun 19, 2020
@knative-prow-robot knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jun 19, 2020
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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

The pull request process is described here

Details 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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 19, 2020
@mattmoor mattmoor force-pushed the allow-webhook-leaderelection branch from b5b816e to 3655156 Compare June 19, 2020 21:17
@knative-prow-robot knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 19, 2020
Comment thread test/e2e-tests.sh
Comment on lines +94 to +95
min_replicas=$(kubectl get hpa activator -n "${SYSTEM_NAMESPACE}" -ojsonpath='{.spec.minReplicas}')
max_replicas=$(kubectl get hpa activator -n "${SYSTEM_NAMESPACE}" -ojsonpath='{.spec.maxReplicas}')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

kubectl get hpa activator -n "${SYSTEM_NAMESPACE}" -ojsonpath='"minReplicas": {.spec.minReplicas} "maxReplicas": {.spec.maxReplicas}'

will give you the innards directly

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am just moving this 🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah but why not improve things.

Comment thread test/e2e-tests.sh Outdated
add_trap "kubectl -n ${SYSTEM_NAMESPACE} patch configmap/config-autoscaler --type=merge --patch='{\"data\":{\"allow-zero-initial-scale\":\"false\"}}'" SIGKILL SIGTERM SIGQUIT

kubectl -n "${SYSTEM_NAMESPACE}" patch configmap/config-leader-election --type=merge \
--patch='{"data":{"enabledComponents":"controller,hpaautoscaler,webhook", "buckets": 10}}'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess controller & hpaautoscaler currently don't support buckets, since it hasn't been merged, but do we want to enable that here before they do?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I want to exercise the bucketing in the webhook, and it is harmless for the others until they switch to the new style of enabling LE.

@mattmoor
Copy link
Copy Markdown
Member Author

/retest

@mattmoor mattmoor force-pushed the allow-webhook-leaderelection branch 2 times, most recently from 7a15f18 to c168bfc Compare June 21, 2020 20:58
@mattmoor
Copy link
Copy Markdown
Member Author

Need to rerun codegen after rebasing. I also noticed that the failing test is due to buckets being an int instead of a string (it has to be a string for configmap). I added || failed=1 to a bunch more of the patch commands for good measure.

@mattmoor mattmoor force-pushed the allow-webhook-leaderelection branch from c168bfc to d0a71fc Compare June 21, 2020 21:24
Copy link
Copy Markdown
Contributor

@vagababov vagababov left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold
if you wish to improve things ;-)

Comment thread test/e2e-tests.sh
Comment on lines +94 to +95
min_replicas=$(kubectl get hpa activator -n "${SYSTEM_NAMESPACE}" -ojsonpath='{.spec.minReplicas}')
max_replicas=$(kubectl get hpa activator -n "${SYSTEM_NAMESPACE}" -ojsonpath='{.spec.maxReplicas}')
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yeah but why not improve things.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2020
@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@mattmoor
Copy link
Copy Markdown
Member Author

/hold cancel

I'll send a smaller PR to follow up that simplifies the bash.

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 22, 2020
This also directs e2e tests to enable this and use 10 buckets.

This also changes the e2e test configuration so that all of our tests are run with leaderelection enabled.

Fixes: knative#8139
@dprotaso
Copy link
Copy Markdown
Member

/test pull-knative-serving-unit-tests

@knative-test-reporter-robot
Copy link
Copy Markdown

The following jobs failed:

Test name Triggers Retries
pull-knative-serving-unit-tests 0/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-unit-tests:

_go_tests.GoTests

@mattmoor mattmoor force-pushed the allow-webhook-leaderelection branch from d0a71fc to 3ebd764 Compare June 22, 2020 18:59
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@dprotaso
Copy link
Copy Markdown
Member

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2020
@knative-prow-robot knative-prow-robot merged commit a5c4887 into knative:master Jun 22, 2020
@mattmoor mattmoor deleted the allow-webhook-leaderelection branch June 22, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/API API objects and controllers area/test-and-release It flags unit/e2e/conformance/perf test issues for product features cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

HA Webhook

6 participants