Improve logging statements in CES usage and reduce code reuse

[yanggangtony]

Signed-off-by: yanggang gang.yang@daocloud.io

/kind bug
fix panic when map return nil.
fix: #22417

[joestringer]

@Weil0ng , could you take a look at this or pull in someone with CES background?

[joestringer]

@yanggangtony do you have an example stack trace for the failure? That would be helpful also to include in the commit message and PR description.

[yanggangtony]

@joestringer
thanks for review.
the stack is displayd in #22417.
Is that can explain it?

[joestringer]

Ah yes, I missed it since it wasn't in the commit message or PR description. That one should do 👍

[nebril]

Thanks for the PR, I think it's a good catch. Please consider expanding it a bit as explained in the comment.

[yanggangtony]

@nebril
thanks for review.
Just updated the code .

[hongkunyoo]

Hello, thanks for handling my issue.
I'm just curious, what is the meaning of CES not found in map desiredCESs?
Is this a bug or an acceptable situation? (I understand not handling a missing CES is a bug which leads to nil pointer dereference. On the other hand, I'm wondering whether missing CES in the map is a bug)
I'm just wondering why CES is missing in the desiredCES map in the first place.
Please understand that I am very new to cilium and have limited knowledge.

[yanggangtony]

@hongkunyoo
Hi, your question is good..But i have no idea about that..
I am also freshman to cilium..
That fix code just avoiding the nil pointer dereference..
Maybe we can continue deep into the code to found the answer

[joestringer]

I agree, it would be very useful to investigate how it got into this state in the first place. If there's a way to reproduce the original bug, that would be very helpful.

Either way, I think that if we catch the error to avoid a nil pointer exception and log the error, that is better than the current state. Then over time hopefully we can find out more about the underlying bug that causes this to happen.

[joestringer]

@yanggangtony the code doesn't compile, please investigate.

[yanggangtony]

@joestringer
thanks for reminding.
Sorry for push code in a hurry.
Had updated and compiled local.

[nebril]

/test

[yanggangtony]

rebase master code .

[pchaigno]

Several reviewers have asked for the commit description to be expanded. Adding the stack trace there would probably be good enough.

[yanggangtony]

Several reviewers have asked for the commit description to be expanded. Adding the stack trace there would probably be good enough.

Ah, thanks for reminder.
Had updated the commit message.

[joestringer]

I didn't take too deep of a look but I noticed some aspects of the logging that deviates from the usual approach to logging in Cilium. I'm also not sure I understand whether the new logging situations are expected or unexpected conditions. Typically log.Info() should be limited to mostly once-off messages that do not recur frequently. If we're hiding an error condition, then maybe we should be reflecting those errors somewhere but I'm not exactly sure where. Probably someone with better CES and/or k8s knowledge could provide more concrete feedback in this regard. cc @Weil0ng @cilium/sig-k8s

[yanggangtony]

@varunmar
thanks for the suggestion.
rebased .

[varunmar]

Thanks for the changes. Looks good to me now.

[varunmar]

Weilong is out of office for several weeks. I'm fine with this change though - can we merge it?

[yanggangtony]

thanks for review..

[pchaigno]

/test

[Weil0ng]

Sorry I was OOO, thank you for spotting the issue and sending in the mitigation! Change LGTM. Just curious, is the cluster under heavy pod churn when you encountered the panic?

[Weil0ng]

/cc @dlapcevic

[dlapcevic]

Hi, I already fixed the nil access panic bug in getCESQueueDelayInSeconds() - #22884
The explanation of how this happens is in the description.

I was unaware of this PR and the issue, we should’ve closed #22417 when my PR was merged.

I think there is no way it will try to access nil pointer right now, because all parts of the code that use getCESTrackerOnly() have checks for it, either by checking if it’s nil or using getCESName() to see if it exists.

getCESName() is also reliable because createCES() and insertCEP() are used together, so if cepNametoCESName exists, a cesTracker for that CEP will also exist.
Code:
getCESTrackerOnly - https://github.com/cilium/cilium/blob/master/operator/pkg/ciliumendpointslice/ceptocesmap.go#L89
getCESName - https://github.com/cilium/cilium/blob/master/operator/pkg/ciliumendpointslice/ceptocesmap.go#L47
createCES - https://github.com/cilium/cilium/blob/master/operator/pkg/ciliumendpointslice/manager.go#L200
insertCEP - https://github.com/cilium/cilium/blob/master/operator/pkg/ciliumendpointslice/ceptocesmap.go#L34

Anyway, it’s good to remove getCESTrackerOnly() to avoid future issues with unsafe access.

LGTM

[pchaigno]

@sayboras I believe all team review requests are already covered, that is cilium/cli and cilium/operator.

[joestringer]

Thanks for your review @dlapcevic!

I took a closer look and I couldn't see any bug that this PR was fixing. I've updated the PR title accordingly.

Additionally, I did the following:

• Split the PR into refactors vs. logging changes,
• Removed extraneous whitespace changes,
• Improved the duplicate logging statements to make the logs unique,
• Rebased against the tip of the main branch,
• Removed references to the panic that was already fixed by another change in-tree.

No functional changes should have been made since the prior test run, so if this passes the basic checks then I will be confident to merge it into the tree.