reflect readiness probe in health check for NEG enabled ClusterIP service backend #582
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
approved
freehan
force-pushed
the
neg-readiness-probe
branch
2 times, most recently
from
3b32456 to
053f57c
Compare
k8s-ci-robot
added
size/S
freehan
force-pushed
the
neg-readiness-probe
branch
2 times, most recently
from
d696c01 to
7129d62
Compare
…abled ClusterIP service backend
freehan
force-pushed
the
neg-readiness-probe
branch
from
7129d62 to
5bc2a43
Compare
| // only one Service can match this nodePort, try and look up | ||
| // the readiness probe of the pods behind it | ||
| if int32(port.NodePort) == sp.NodePort { | ||
| if port.NodePort != 0 && int32(port.NodePort) == sp.NodePort { |
There was a problem hiding this comment.
Can you explain this check?
There was a problem hiding this comment.
For ClusterIP type service, NodePort will be 0.
For NEG, ingress can point to ClusterIP type service.
Then before this PR, when it gets here, it may pick up a random ClusterIP type service because its NodePort is 0 and use the corresponding pod's readiness probe which can be totally irrelevant.
This has not break anything because ingress requires NodePort type service before.
For NEG case, it will captured above.
Adding this check just to provide additional safety.
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: freehan, rramkumar1 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 |
Cherrypick #582 into release-1.4 branch
fixes: #541