-
Notifications
You must be signed in to change notification settings - Fork 831
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
feat(pdb): Add support for managing PDBs #2515
Conversation
Hi @groszewn. Thanks for your PR. I'm waiting for a SeldonIO member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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 jenkins-x/lighthouse repository. |
@@ -288,7 +289,8 @@ type SeldonPodSpec struct { | |||
Metadata metav1.ObjectMeta `json:"metadata,omitempty" protobuf:"bytes,1,opt,name=metadata"` | |||
Spec v1.PodSpec `json:"spec,omitempty" protobuf:"bytes,2,opt,name=spec"` | |||
HpaSpec *SeldonHpaSpec `json:"hpaSpec,omitempty" protobuf:"bytes,3,opt,name=hpaSpec"` | |||
Replicas *int32 `json:"replicas,omitempty" protobuf:"bytes,4,opt,name=replicas"` | |||
PdbSpec *SeldonPdbSpec `json:"pdbSpec,omitempty" protobuf:"bytes,4,opt,name=pdbSpec"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to keep the order of elements to keep backward compatibility for gRPC protobufs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
df82a53
to
4ff9a07
Compare
@cliveseldon reordered the elements to keep backwards compatibility and generated the helm chart updates to the CRD. |
/ok-to-test |
Mon Oct 5 12:50:42 UTC 2020 impatient try |
Mon Oct 5 12:50:58 UTC 2020 impatient try |
@cliveseldon do the tests need to be rekicked? |
/test integration |
Tue Oct 6 13:00:50 UTC 2020 impatient try |
@groszewn is there anyway to add an end-to-end test either via a notebook example and then calling it from https://github.com/SeldonIO/seldon-core/blob/master/testing/scripts/test_notebooks.py |
@cliveseldon e2e test added and documentation updated to link the notebook. |
Wed Oct 7 02:33:37 UTC 2020 impatient try |
Wed Oct 7 02:33:42 UTC 2020 impatient try |
Wed Oct 7 02:52:28 UTC 2020 impatient try |
Wed Oct 7 02:52:37 UTC 2020 impatient try |
Wed Oct 7 12:27:53 UTC 2020 impatient try |
Wed Oct 7 12:28:03 UTC 2020 impatient try |
/test notebooks |
Wed Oct 7 14:23:37 UTC 2020 impatient try |
@cliveseldon rebased off of master to get those fixes |
/retest |
Sat Oct 10 22:17:37 UTC 2020 impatient try |
/retest |
Tue Oct 13 16:28:28 UTC 2020 impatient try |
@axsaucedo Haven't rebased since the fix was merged to master, will do so now. |
Ok sounds perfect, it seems all should be passing now (except the known issues with #2541) |
/test notebooks |
Wed Oct 14 12:49:59 UTC 2020 impatient try |
Wed Oct 14 12:49:57 UTC 2020 impatient try |
Wed Oct 14 12:50:10 UTC 2020 impatient try |
It seems the explainer test is failing, as well as another which seems to be flaky, re-testing to confirm and we'll investigate. /test notebooks |
Hmm.... @axsaucedo #2541 is about
|
Wed Oct 14 15:39:27 UTC 2020 impatient try |
/retest |
Wed Oct 14 19:09:09 UTC 2020 impatient try |
We have confirmed that the tests failing in the notebooks job are flaky, so we're looking to merge this PR - it seems there are some merge conflicts, if you can resolve them we can rerun and merge @groszewn |
@axsaucedo sounds good to me, rerunning some validations locally but I'll have the PR updated shortly. |
Add support for managing PDBs through the SeldonSpec. Contributes to SeldonIO#2508 Signed-off-by: Nick Groszewski <groszewn@gmail.com>
Thu Oct 15 12:52:51 UTC 2020 impatient try |
Thu Oct 15 12:53:14 UTC 2020 impatient try |
/test integration |
Thu Oct 15 13:13:03 UTC 2020 impatient try |
@groszewn: The following tests failed, say
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 jenkins-x/lighthouse repository. I understand the commands that are listed here. |
Thanks @groszewn - current integration test pass, only failure is a flaky one |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: axsaucedo 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 |
Add support for managing PDBs through the SeldonSpec.
Contributes to #2508
Signed-off-by: Nick Groszewski groszewn@gmail.com
What this PR does / why we need it:
Adds support for specifying and managing PDBs as part of a SeldonDeployment so that disruptions can be properly budgeted.
Which issue(s) this PR fixes:
Fixes #2508
Special notes for your reviewer:
These changes were tested on a local
kind
cluster, but I'd appreciate it if we could double check that all necessary generated files actually exist.I haven't updated the helm chart roles/CRDs. Is there a
make
target that would appropriately copy the roles and CRDs from theoperator
folder to the chart, or is that done manually?Does this PR introduce a user-facing change?: