Fix discovery service creation #5
Conversation
| for i := 0; i < 20; i++ { | ||
| service, err := kubecli.CoreV1().Services(ns).Get(ctx, svcName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| status <- err.Error() |
There was a problem hiding this comment.
If we receive an error fetching the service we could return the error probably, one thing is to have the status != created and other is having a problem IMO, in which case the kubecli.CoreV1().Services(ns).Get(ctx, svcName, metav1.GetOptions{}) could return an error?
There was a problem hiding this comment.
If the status != created then the call succeed and we have a service object, therefore error == nil, which won't be caught by the if in line 105.
Diving a bit deeper on this, possible errors are either if the service is not found or some kubernetes error (like unreachable or failed state) since it returns the straight call response: https://github.com/kubernetes/client-go/blob/master/kubernetes/typed/core/v1/service.go#L71
pkg/util/k8sutil/k8sutil.go
Outdated
| } else { | ||
| err = createService(ctx, kubecli, serviceName, clusterName, ns, defaultPort, owner, false, nil) | ||
| annotations := map[string]string{} |
There was a problem hiding this comment.
I usually prefer instead of adding many if else, to extract the code in a function that for example do this:
if cluster mode = disc:
service = set service
annotations = set annotations
return service, annotations
return nil, nil
There was a problem hiding this comment.
Don't know if I agree since its just plain value creation using straight typing or struct assignment
pkg/util/k8sutil/k8sutil.go
Outdated
| ClusterIP: v1.ClusterIPNone, | ||
|
|
||
| service := &api.ServicePolicy{} | ||
| annotations := map[string]string{} |
There was a problem hiding this comment.
Same here, probably you could re-use the function adding a couple of parameters
There was a problem hiding this comment.
this one is actually unnecessary, fixing
| @@ -81,6 +81,40 @@ const ( | |||
|
|
|||
| var ErrDiscoveryTokenNotProvided = errors.New("cluster token not provided, you must provide a token when clustering mode is discovery") | |||
|
|
|||
| var CreateSvc CreateService = func(ctx context.Context, kubecli kubernetes.Interface, svcName string, clusterName string, ns string, ports []v1.ServicePort, owner metav1.OwnerReference, publishNotReadyAddresses bool, service *api.ServicePolicy, annotations map[string]string) error { | |||
There was a problem hiding this comment.
Do you need to assign this function in a variable here? Can't we just declare the function and pass it as a parameter in the CreateClientService call?
There was a problem hiding this comment.
I thought about setting the type to make sure that in the future we don't disrupt the tests, that's why I created the type in line 284
This corrects the creation of services in the discovery mode. If that mode is set we create load balancers that can be reached from other clusters so that the etcd members can communicate.