fix/optimize the logic of finding cluster service/pod ip range #3285
Conversation
unilinu
requested review from
Oats87,
skitt,
sridhargaddam,
tpantelis and
vthapar
as code owners
|
🤖 Created branch: z_pr3285/unilinu/fix/optimize-finding-cluster-iprange |
pkg/discovery/network/generic.go
Outdated
| @@ -78,22 +77,31 @@ func discoverNetwork(ctx context.Context, client controllerClient.Client) (*Clus | |||
|
|
|||
| func findClusterIPRange(ctx context.Context, client controllerClient.Client) (string, error) { | |||
| clusterIPRange, err := findClusterIPRangeFromApiserver(ctx, client) | |||
| if err != nil || clusterIPRange != "" { | |||
| return clusterIPRange, err | |||
| if clusterIPRange != "" { | |||
There was a problem hiding this comment.
Why did you remove the error checking here and other places below? This hides the error which isn't best practice. An error here means it could not determine if the IP range could be obtained, most likely because the API server wasn't available, in which case we want to abort and report the error.
There was a problem hiding this comment.
Well, It seems I understand wrong the purpose of these error checking. I thought this function is to detect the available ClusterIP range, but it seems to detect the availability of component/service related to ClusterIP. I will cancel the related modifications later.
In addition, I think the point 1&2 should be retained, but for point 3, I wonder if add the redundant finding from kube-controller-manager.
There was a problem hiding this comment.
@tpantelis Another question, the original if conditions err != nil || clusterIPRange != "" is almost true, which will cover/skip the following judging!!
In my opinion, this function is used to find the service cluster ip range. Even if occurs error, which should be detected and handled somewhere, adding a waring/error log but continuing finding is a good idea here.
If you agree, I will modify it as above. If not, looking forward to your reply.
There was a problem hiding this comment.
In my opinion, this function is used to find the service cluster ip range. Even if occurs error, which should be detected and handled somewhere, adding a waring/error log but continuing finding is a good idea here.
If you agree, I will modify it as above. If not, looking forward to your reply.
I don't agree. Consider this scenario:
Let's say in a particular environment the first call, findClusterIPRangeFromApiserver, under normal circumstances with no error finds the cluster IP. But on a particular run a transient error occurs, eg the API server is busy and times out, in which case we would just log a warning and continue. For the rest of the code below, the API server responds and no error occurs but the cluster IP isn't found by any of the following methods. This function would return and report to the caller that no cluster IP was found and no error occurred. But this is incorrect as an error did occur but it was essentially ignored by just logging it. We must propagate errors to the caller to indicate that the result was indeterminate and to let the caller handle the error as it sees fit.
There was a problem hiding this comment.
I can understand it. So, each error should be checked and not be covered by assignments. However, there is no doubt that err != nil || clusterIPRange != "" condition will cover/skip the following statements. This point must be corrected. I'll modify the PR correspondingly.
There was a problem hiding this comment.
No - the error checking was fine as is. Given your recent changes and the scenario I outlined above, it would return a valid cluster IP and an error which is non-sensical and non-idiomatic.
Please just focus on the purpose of this PR as outlined in your summary and not modify the existing error handling.
There was a problem hiding this comment.
Thanks for your patience. I know what I misunderstood. err != nil || clusterIPRange != "" is not always true. There is a situation not finding cluster ip range from the args of apiserver/controller-manager.
I have modified my patch, removing the changes of error checking.
unilinu
force-pushed
the
fix/optimize-finding-cluster-iprange
branch
from
e588a15
to
128bf17
Compare
unilinu
force-pushed
the
fix/optimize-finding-cluster-iprange
branch
from
128bf17
to
66c3f51
Compare
| // the cluster is a single node deployment, we should rely on the node.Spec.PodCIDR as podCIDR of the cluster. | ||
| if len(nodes) == 1 { | ||
| return nodes[0].Spec.PodCIDR, nil | ||
| } |
There was a problem hiding this comment.
From the PR summary:
The node.spec.podCIDR field is not the cluster pod ip range, but the node-assigned.
But this is only considered if there's only one node as explained in the comments above and if the pod CIDR wasn't found from any prior methods. It seems valid to keep this.
|
@unilinu The DCO job failed - you need to add Signed-off-by your commit. |
What type of PR is this?
Fix or Optimize
What this PR does / why we need it:
According to the original logic, but fix some issues and optimize:
node.spec.podCIDRfield is not the cluster pod ip range, but the node-assigned.Correct the label selector of kube-proxy from
component=kube-proxytok8s-app=kube-proxySimilar to
findPodIPRangefunction, add the redundant finding from kube-controller-manager.