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

Issue #3968: Allow hostNetwork=true for seldon operator #3971

Merged
merged 5 commits into from
Mar 17, 2022
Merged

Issue #3968: Allow hostNetwork=true for seldon operator #3971

merged 5 commits into from
Mar 17, 2022

Conversation

marianobilli
Copy link
Contributor

What this PR does / why we need it:
It allows to set hostNetwork for seldon core operator.
It is needed for EKS clusters with CNI plugin such as calico in order to allow communication from the control plane to the operator webhook

Which issue(s) this PR fixes:
Fixes #3968

Special notes for your reviewer:

@seldondev
Copy link
Collaborator

Hi @marianobilli. Thanks for your PR.

I'm waiting for a SeldonIO or todo member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@marianobilli
Copy link
Contributor Author

/assign @ivan-valkov

@axsaucedo
Copy link
Contributor

Thank you @marianobilli - only thing to bare in mind is that the changes for helm charts are actually generated automatically from the raw operator manifest files, so you would actually have to add these entries under operator/helm/split_resources.py. Conscious that this file is qutie fiddly it would be useful to see if you think you'd be able to add it there, otherwise we'd have to look at adding it as one of the issues for us to pick up.

@marianobilli
Copy link
Contributor Author

got it, no problem I will make the change there

@seldondev seldondev added size/M and removed size/S labels Mar 7, 2022
@marianobilli
Copy link
Contributor Author

@axsaucedo done, please check it, also please note I have a VSCode plugin to delete spaces at the end of lines, so I hope you dont mind that.

@axsaucedo
Copy link
Contributor

@marianobilli this looks great - thank you very much, I will test locally to validate and we can merge. Thanks for the prompt iterations 👍

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@axsaucedo
Copy link
Contributor

@marianobilli it seems we're getting this issue:

  Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: ValidationError(Deployment.spec.template.spec.hostNetwork): invalid type for io.k8s.api.core.v1.PodSpec.hostNetwork: got "string", expected "boolean"

Based on this it seems it may just require removing the quotes from the resulting helm chart/template(s)

Mariano Billinghurst added 2 commits March 8, 2022 10:32
Remove quotes from hostNetwork when generating helm chart
Remove quotes from hostNetwork when generating helm chart
@marianobilli
Copy link
Contributor Author

@marianobilli it seems we're getting this issue:

  Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: ValidationError(Deployment.spec.template.spec.hostNetwork): invalid type for io.k8s.api.core.v1.PodSpec.hostNetwork: got "string", expected "boolean"
Based on this it seems it may just require removing the quotes from the resulting helm chart/template(s)

sorry for that @axsaucedo! I forgot to test it, you were correct, now it is fixed and I tested the chart.

Note: when I run make create there is also a change in templates\webhook.yaml but as this is not part of my change I thought better not to include it in my change.

@axsaucedo
Copy link
Contributor

Thanks for the fast turnaround @marianobilli - yes I think the webhook often changes removing a cert if statement so definitely good call on not including it. Ok perfect, will rerun the tests

@axsaucedo
Copy link
Contributor

/test integration

@axsaucedo
Copy link
Contributor

/test notebooks

@marianobilli
Copy link
Contributor Author

Hi @axsaucedo will the failure on the docs-lint be a problem to move forward with the merge? Thanks.

@axsaucedo
Copy link
Contributor

/approve

@seldondev
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@axsaucedo axsaucedo merged commit c515a00 into SeldonIO:master Mar 17, 2022
@axsaucedo
Copy link
Contributor

Yes - nice one, thank you for the contribution

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

Successfully merging this pull request may close these issues.

Duplicate metrics generated for GRPC requests hitting seldon predict_raw
4 participants