Allow more flexible way to config the api server addr in persistence agent #867
Conversation
|
Hi @ywskycn. 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. |
1 similar comment
|
Hi @ywskycn. 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. |
|
/ok-to-test |
|
@ywskycn Have you verified on your local cluster this would work?
I'd be curious what cluster those are. Could you provide more detail. |
| @@ -55,8 +55,8 @@ func NewPipelineClient( | |||
| mlPipelineServiceName string, | |||
| mlPipelineServiceHttpPort string, | |||
| mlPipelineServiceGRPCPort string) (*PipelineClient, error) { | |||
| httpAddress := fmt.Sprintf(addressTemp, mlPipelineServiceName, namespace, mlPipelineServiceHttpPort) | |||
There was a problem hiding this comment.
instead, can you parameterize the domain name, and give default value "local"?
%s.%s.svc.cluster.%s:%s
It would be easier for customizations in future. we don't assume api and persistentagent in the same namespace.
There was a problem hiding this comment.
We need to put "cluster" together with "local", as this field represents , https://github.com/kubernetes/dns/blob/master/docs/specification.md#21---definitions. If we go that way, we'll have three configs to generate the full service name: ..svc.. Another way (just as the diff) would be, just using one config to config the full path, which includes the three configs aforementioned.
There was a problem hiding this comment.
i think these deserves flexible configurations so having a "zone" parameter with default value cluster.local make sense.
With that it's easy to change namespace without changing anything else.
WDYT?
There was a problem hiding this comment.
sounds good. do you want to do it as part of this PR?
There was a problem hiding this comment.
Updated the diff to remove "namespace" which is not needed anymore.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: IronPan 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 |
In some clusters, service addr may not be formatted as "%s.%s.svc.cluster.local:%s". We need to a more flexible way to handle it. I checked approach in ml-pipeline-ui, which just make the entire FQDN configurable. But this change would bring incompatibility, as previously users may only config the service name while now need full name to make it work.
@IronPan thoughts?
This change is