-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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(deployment): update and secure metacontroller #6537
Conversation
Hi @juliusvonkohout. Thanks for your PR. I'm waiting for a kubeflow 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 kubernetes/test-infra repository. |
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.
/lgtm
/assign @zijianjoy
For final approval
command: | ||
- /usr/bin/metacontroller | ||
- --zap-log-level=4 | ||
- '--discovery-interval=3600s' # less insane than 10 seconds |
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.
What does this arg do? If a new namespace is created can required resources get created right away?
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.
Yes the resources are created right away in my installation.
From the documentation https://metacontroller.github.io/metacontroller/guide/configuration.html?highlight=discovery-interval#command-line-flags
--discovery-interval | How often to refresh discovery cache to pick up newly-installed resources (e.g. --discovery-interval=10s). I think that means CRDs. A low value creates too much log spam and on start the cache is refreshed anyway. I am also fine with 10 minutes if that is what you want.
--zap-log-level | ZapGws Level to configure the verbosity of logging. Can be one of ‘debug’, ‘info’, ‘error’, or any integer value > 0 which corresponds to custom debug levels of increasing verbosity(e.g. --zap-log-level=5). Level 4 logs Metacontroller's interaction with the API server. Levels 5 and up additionally log details of Metacontroller's invocation of lambda hooks. See the troubleshooting guide for more.
What you are worrying about is https://metacontroller.github.io/metacontroller/api/compositecontroller.html?highlight=discovery%20cache#resync-period but even that can be set to a high value in the compositecontroller "Sometimes you may want to sync periodically even if nothing has changed in the Kubernetes API objects"
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.
Is this detailed enough, or should i test something for you ?
/approve Thank you @juliusvonkohout |
Thank you too. "To complete the pull request process, please ask for approval from bobgy after the PR has been reviewed." So i think @zijianjoy approval is not enough @Bobgy |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy, zijianjoy 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 |
@juliusvonkohout are you sure about the way "restricted the namespaces to kubeflow profile namespaces" is done? It seems that here is no labelSelector field in parentResource field of CompositeController. |
I am not sure anymore. According to https://metacontroller.github.io/metacontroller/api/compositecontroller.html#label-selector you might have to alter sync.py to to add the labels to the child resources "Children you create must have labels that satisfy the parent's selector, or else they will be immediately orphaned and you'll never see them again." Please also test profile/namespace deletion and creation. |
@juliusvonkohout the error I got with this commit is with K8S itself which does'nt want to create the CompositeController resource because As I'm not using a standard environment, can you just confirm it works on your side? |
Correction: it seems to be fine with the label selector. It is the same on my clusters. Please delete your cluster, use a proper kubernetes 1.19-1.21 and try again. |
@juliusvonkohout found the problem: kubeflow-pipelines is not using the official CRD for CompositeController and I'm using the official one which has not the |
@sebastien-prudhomme then please create a new issue and pull request such that it is fixed long term for every one. |
@Bobgy @zijianjoy @orugantichetan
Fixes #5578
I
Sadly it still needs cluster-admin rights, but maybe someone has an idea on how to restrict it even further.