Clean logging #220
Clean logging #220
Conversation
| @@ -224,7 +226,8 @@ func newControlPlaneEndpointManager(eipTag, projectID string, deviceIPSrv packng | |||
| // the `default/kubernetes` service | |||
| func (m *controlPlaneEndpointManager) reconcileServices(ctx context.Context, svcs []*v1.Service, mode UpdateMode) error { | |||
| if m.eipTag == "" { | |||
| return errors.New("elastic ip tag is empty. Nothing to do") | |||
There was a problem hiding this comment.
I'm hitting this error in a cluster I just brought up under k8s 1.23, haven't looked into why yet.
Can you explain the change in behavior here?
There was a problem hiding this comment.
Yeah. The way it is defined, if you set an EIP tag, then you want CPEM to manage it; if you do not set an EIP tag, then you are telling CPEM, "there is nothing for you to manage at the control plane EIP level." Lack of EIP tag = "nothing to do here". That definitely is not an error, and never should have been treated as such.
There was a problem hiding this comment.
Don't ask how many people that led astray. 🤦♂️
There was a problem hiding this comment.
I sort of did "Nothing to do." as an indication that this is not an error, just nothing that needs to be done here. The correct answer to that is, "so don't make it an error!" 😬
Signed-off-by: Avi Deitcher <avi@deitcher.net>
…ate before returning Signed-off-by: Avi Deitcher <avi@deitcher.net>
deitch
force-pushed
the
clean-logging
branch
from
f548ab7
to
3ebe81c
Compare
|
Rebased. |
There was a problem hiding this comment.
Looks good to me - the errors that are returned vs logged make sense.
The watcher movement to a different file looks fine - perhaps we could move this to an internal package in the future or implement LoadBalancers (see my comment).
| ) | ||
|
|
||
| // createNodesWatcher create a cache.SharedIndexInformation for node handlers | ||
| func createNodesWatcher(ctx context.Context, informer informers.SharedInformerFactory, cloudServices []cloudService) (cache.SharedIndexInformer, error) { |
There was a problem hiding this comment.
If we were to implement LoadBalancer, we would get these node/service watchers/informers for free.
I don't know if kube-vip or metallb register themselves as cloud-provider LoadBalancers (what does that really mean? I don't know.), I presume they do not. If they do I would be curious to know what users would provide for Service loadBalancerClass https://kubernetes.io/docs/concepts/services-networking/service/#load-balancer-class (would it be cloud-provider-equinix-metal, kube-vip, metallb).
Why do we not implement LoadBalancer?
There was a problem hiding this comment.
We should. It is on my list of todos, to which I haven't gotten yet. Your work on InstancesV2() also inspired me to think about getting moving on that. If it simplifies our work, even better.
There was a problem hiding this comment.
But, like moving watchers.go into an internal package, subject for another PR.
Co-authored-by: Marques Johansson <mjohansson@equinix.com>
Co-authored-by: Marques Johansson <mjohansson@equinix.com> Signed-off-by: Avi Deitcher <avi@deitcher.net>
deitch
force-pushed
the
clean-logging
branch
from
78e2b2a
to
4bdc51a
Compare
|
CI failed because of |
|
CI green, ready for you @displague |
This has two commits.
The first restructures some of the files and funcs without changing any functionality. It just reduces duplicate code, makes it easier to understand, separates some things into their own file, etc.
The second cleans up the logging:
nilI have wanted to do this for a long time.