feat: Add an ability to measure clusters with dns only control plane …#3802
feat: Add an ability to measure clusters with dns only control plane …#3802k8s-ci-robot merged 1 commit intokubernetes:masterfrom
Conversation
|
Hi @matix522. Thanks for your PR. I'm waiting for a kubernetes 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. DetailsInstructions 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-sigs/prow repository. |
|
/ok-to-test |
| if clusterConfig.MasterDNSEndpoint != "" { | ||
| return nil, fmt.Errorf("cluster has no master IPs, but has master DNS endpoint %s", clusterConfig.MasterDNSEndpoint) | ||
| } |
There was a problem hiding this comment.
Two related questions here
1st: Is this check position intended? In other words, even when the master-endpoint is set the following combinations:
usePublicIPs(using--prometheus-scrape-masters-with-public-ips) and having > 0 master IPs- > 0 master internal IPs
Shouldn't error here? I'm a bit confused because I'd assume that we'd add this check at the beginning of the getMasterIps function and/or not invoke this function at all when MasterDNSEndpoint is present.
2nd: Should we invoke this function if we have clusterConfig.MasterDNSEndpoint? AFAIU currently it'd go like this:
NewControllerfunction invokes thegetMasterIpsalwaysclustersConfig.MasterDNSEndpointis specified, so we return error- We display the
Couldn't get master ip, will ignore manifests requiring it: %v
Probably not the end of the world, but should it really warn? It's a valid path after all?
There was a problem hiding this comment.
Good points, I moved the check to the top of the function (to disallow calling in future), as well as not calling this function in the first place when DNS endpoint is provided
ac76dea to
45769d6
Compare
…access This update allows ClusterLoader2 to target clusters using a DNS endpoint for the control plane. It includes support for the MASTER_DNS_ENDPOINT environment variable and relevant configuration changes. Changed logging to show all of the Node adresses to allow for dualstack clusters.
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matix522, mborsz, Qqkyu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…access
This update allows ClusterLoader2 to target clusters using a DNS endpoint for the control plane. It includes support for the MASTER_DNS_ENDPOINT environment variable and relevant configuration changes. Changed logging to show all of the Node adresses to allow for dualstack clusters.
What type of PR is this?
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR is needed by the GKE team to test the performance of clusters that use DNS address instead of IP address for cluster control plane
Special notes for your reviewer: