-
Notifications
You must be signed in to change notification settings - Fork 752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
support tcpCheck in podProbe #1474
Conversation
almost LGTM plz fix the ut. |
ef6b8e4
to
900f3cd
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1474 +/- ##
==========================================
+ Coverage 48.55% 49.09% +0.53%
==========================================
Files 157 157
Lines 22480 22604 +124
==========================================
+ Hits 10916 11098 +182
+ Misses 10360 10297 -63
- Partials 1204 1209 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -95,8 +96,8 @@ func (p *enqueueRequestForPod) Update(evt event.UpdateEvent, q workqueue.RateLim | |||
if newInitialCondition == nil { | |||
return | |||
} | |||
if kubecontroller.IsPodActive(new) && (oldInitialCondition == nil || oldInitialCondition.Status == corev1.ConditionFalse) && | |||
newInitialCondition.Status == corev1.ConditionTrue { | |||
if kubecontroller.IsPodActive(new) && (((oldInitialCondition == nil || oldInitialCondition.Status == corev1.ConditionFalse) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the if clause is too complex , consider move IsPodActive to a separate clause, e.g.
if !kubecontroller.IsPodActive(new) { return }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using two if clause to support the condition judgment
`
if !kubecontroller.IsPodActive(new) {
return
}
if ((oldInitialCondition == nil || oldInitialCondition.Status == corev1.ConditionFalse) &&
newInitialCondition.Status == corev1.ConditionTrue) || old.Status.PodIP != new.Status.PodIP {...}
`
if kubecontroller.IsPodActive(new) && (oldInitialCondition == nil || oldInitialCondition.Status == corev1.ConditionFalse) && | ||
newInitialCondition.Status == corev1.ConditionTrue { | ||
if kubecontroller.IsPodActive(new) && (((oldInitialCondition == nil || oldInitialCondition.Status == corev1.ConditionFalse) && | ||
newInitialCondition.Status == corev1.ConditionTrue) || (old.Status.PodIP != new.Status.PodIP)) { | ||
ppms, err := p.getPodProbeMarkerForPod(new) | ||
if err != nil { | ||
klog.Errorf("List PodProbeMarker fialed: %s", err.Error()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz fix typo here ""List PodProbeMarker fialed" -> ""List PodProbeMarker fail"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
plz fix dco. |
900f3cd
to
5e4d1c2
Compare
// look up a port in a container by name & convert container name port | ||
if probe.Probe.TCPSocket != nil { | ||
probe, err = convertTcpSocketProbeCheckPort(probe, pod) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plz add ut of Reconcile for tcpsocket and httpget
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
5e4d1c2
to
55dcfa1
Compare
02f62f6
to
a92a23d
Compare
2d0faee
to
53eb747
Compare
Signed-off-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
53eb747
to
5667ad3
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zmberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: jicheng.sk <jicheng.sk@alibaba-inc.com>
Ⅰ. Describe what this PR does
support tcp check in PodProber. This is the second pr submission(total: 5).
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how to verify it
using the tcp check in podprobemarker.
Ⅳ. Special notes for reviews