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
Add sync_proxy_rules_no_endpoints_total metric #108930
Changes from all commits
f0dfac5
ba4f5c4
198367a
61b7e6c
6454248
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6050,3 +6050,157 @@ func TestEndpointCommentElision(t *testing.T) { | |
t.Errorf("numComments (%d) != 0 when numEndpoints (%d) > threshold (%d)", numComments, numEndpoints, endpointChainsNumberThreshold) | ||
} | ||
} | ||
|
||
func TestNoEndpointsMetric(t *testing.T) { | ||
type endpoint struct { | ||
ip string | ||
hostname string | ||
} | ||
|
||
internalTrafficPolicyLocal := v1.ServiceInternalTrafficPolicyLocal | ||
externalTrafficPolicyLocal := v1.ServiceExternalTrafficPolicyTypeLocal | ||
|
||
metrics.RegisterMetrics() | ||
testCases := []struct { | ||
name string | ||
internalTrafficPolicy *v1.ServiceInternalTrafficPolicyType | ||
externalTrafficPolicy v1.ServiceExternalTrafficPolicyType | ||
endpoints []endpoint | ||
expectedSyncProxyRulesNoLocalEndpointsTotalInternal int | ||
expectedSyncProxyRulesNoLocalEndpointsTotalExternal int | ||
}{ | ||
{ | ||
name: "internalTrafficPolicy is set and there is non-zero local endpoints", | ||
internalTrafficPolicy: &internalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", testHostname}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
}, | ||
{ | ||
name: "externalTrafficPolicy is set and there is non-zero local endpoints", | ||
externalTrafficPolicy: externalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", testHostname}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
}, | ||
{ | ||
name: "both policies are set and there is non-zero local endpoints", | ||
internalTrafficPolicy: &internalTrafficPolicyLocal, | ||
externalTrafficPolicy: externalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", testHostname}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
}, | ||
{ | ||
name: "internalTrafficPolicy is set and there is zero local endpoint", | ||
internalTrafficPolicy: &internalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", "host0"}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, | ||
}, | ||
{ | ||
name: "externalTrafficPolicy is set and there is zero local endpoint", | ||
externalTrafficPolicy: externalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", "host0"}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, | ||
}, | ||
{ | ||
name: "both policies are set and there is zero local endpoint", | ||
internalTrafficPolicy: &internalTrafficPolicyLocal, | ||
externalTrafficPolicy: externalTrafficPolicyLocal, | ||
endpoints: []endpoint{ | ||
{"10.0.1.1", "host0"}, | ||
{"10.0.1.2", "host1"}, | ||
{"10.0.1.3", "host2"}, | ||
}, | ||
expectedSyncProxyRulesNoLocalEndpointsTotalInternal: 1, | ||
expectedSyncProxyRulesNoLocalEndpointsTotalExternal: 1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would be good to have some test where the metric is more than 1 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we do this as a quick follow-up? It involves refactoring the test to include dynamically generating services. Since we're after the freeze, I'd like to get this one in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ACK |
||
}, | ||
} | ||
|
||
for _, tc := range testCases { | ||
t.Run(tc.name, func(t *testing.T) { | ||
defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.ServiceInternalTrafficPolicy, true)() | ||
ipt := iptablestest.NewFake() | ||
fp := NewFakeProxier(ipt) | ||
fp.OnServiceSynced() | ||
fp.OnEndpointSlicesSynced() | ||
|
||
serviceName := "svc1" | ||
namespaceName := "ns1" | ||
|
||
svc := &v1.Service{ | ||
ObjectMeta: metav1.ObjectMeta{Name: serviceName, Namespace: namespaceName}, | ||
Spec: v1.ServiceSpec{ | ||
ClusterIP: "172.30.1.1", | ||
Selector: map[string]string{"foo": "bar"}, | ||
Ports: []v1.ServicePort{{Name: "", Port: 80, Protocol: v1.ProtocolTCP, NodePort: 123}}, | ||
}, | ||
} | ||
if tc.internalTrafficPolicy != nil { | ||
svc.Spec.InternalTrafficPolicy = tc.internalTrafficPolicy | ||
} | ||
if tc.externalTrafficPolicy != "" { | ||
svc.Spec.Type = v1.ServiceTypeNodePort | ||
svc.Spec.ExternalTrafficPolicy = tc.externalTrafficPolicy | ||
} | ||
|
||
fp.OnServiceAdd(svc) | ||
|
||
tcpProtocol := v1.ProtocolTCP | ||
endpointSlice := &discovery.EndpointSlice{ | ||
ObjectMeta: metav1.ObjectMeta{ | ||
Name: fmt.Sprintf("%s-1", serviceName), | ||
Namespace: namespaceName, | ||
Labels: map[string]string{discovery.LabelServiceName: serviceName}, | ||
}, | ||
Ports: []discovery.EndpointPort{{ | ||
Name: utilpointer.StringPtr(""), | ||
Port: utilpointer.Int32Ptr(80), | ||
Protocol: &tcpProtocol, | ||
}}, | ||
AddressType: discovery.AddressTypeIPv4, | ||
} | ||
for _, ep := range tc.endpoints { | ||
endpointSlice.Endpoints = append(endpointSlice.Endpoints, discovery.Endpoint{ | ||
Addresses: []string{ep.ip}, | ||
Conditions: discovery.EndpointConditions{Ready: utilpointer.BoolPtr(true)}, | ||
NodeName: utilpointer.StringPtr(ep.hostname), | ||
}) | ||
} | ||
|
||
fp.OnEndpointSliceAdd(endpointSlice) | ||
fp.syncProxyRules() | ||
syncProxyRulesNoLocalEndpointsTotalInternal, err := testutil.GetGaugeMetricValue(metrics.SyncProxyRulesNoLocalEndpointsTotal.WithLabelValues("internal")) | ||
if err != nil { | ||
t.Errorf("failed to get %s value, err: %v", metrics.SyncProxyRulesNoLocalEndpointsTotal.Name, err) | ||
} | ||
|
||
if tc.expectedSyncProxyRulesNoLocalEndpointsTotalInternal != int(syncProxyRulesNoLocalEndpointsTotalInternal) { | ||
t.Errorf("sync_proxy_rules_no_endpoints_total metric mismatch(internal): got=%d, expected %d", int(syncProxyRulesNoLocalEndpointsTotalInternal), tc.expectedSyncProxyRulesNoLocalEndpointsTotalInternal) | ||
} | ||
|
||
syncProxyRulesNoLocalEndpointsTotalExternal, err := testutil.GetGaugeMetricValue(metrics.SyncProxyRulesNoLocalEndpointsTotal.WithLabelValues("external")) | ||
if err != nil { | ||
t.Errorf("failed to get %s value(external), err: %v", metrics.SyncProxyRulesNoLocalEndpointsTotal.Name, err) | ||
} | ||
|
||
if tc.expectedSyncProxyRulesNoLocalEndpointsTotalExternal != int(syncProxyRulesNoLocalEndpointsTotalExternal) { | ||
t.Errorf("sync_proxy_rules_no_endpoints_total metric mismatch(internal): got=%d, expected %d", int(syncProxyRulesNoLocalEndpointsTotalExternal), tc.expectedSyncProxyRulesNoLocalEndpointsTotalExternal) | ||
} | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -273,6 +273,20 @@ type Proxier struct { | |
// Inject for test purpose. | ||
networkInterfacer utilproxy.NetworkInterfacer | ||
gracefuldeleteManager *GracefulTerminationManager | ||
// serviceNoLocalEndpointsInternal represents the set of services that couldn't be applied | ||
// due to the absence of local endpoints when the internal traffic policy is "Local". | ||
// It is used to publish the sync_proxy_rules_no_endpoints_total | ||
// metric with the traffic_policy label set to "internal". | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this change? |
||
// sets.String is used here since we end up calculating endpoint topology multiple times for the same Service | ||
// if it has multiple ports but each Service should only be counted once. | ||
serviceNoLocalEndpointsInternal sets.String | ||
// serviceNoLocalEndpointsExternal irepresents the set of services that couldn't be applied | ||
// due to the absence of any endpoints when the external traffic policy is "Local". | ||
// It is used to publish the sync_proxy_rules_no_endpoints_total | ||
// metric with the traffic_policy label set to "external". | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should say There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this change? |
||
// sets.String is used here since we end up calculating endpoint topology multiple times for the same Service | ||
// if it has multiple ports but each Service should only be counted once. | ||
serviceNoLocalEndpointsExternal sets.String | ||
} | ||
|
||
// IPGetter helps get node network interface IP and IPs binded to the IPVS dummy interface | ||
|
@@ -1027,6 +1041,8 @@ func (proxier *Proxier) syncProxyRules() { | |
|
||
klog.V(3).InfoS("Syncing ipvs proxier rules") | ||
|
||
proxier.serviceNoLocalEndpointsInternal = sets.NewString() | ||
proxier.serviceNoLocalEndpointsExternal = sets.NewString() | ||
// Begin install iptables | ||
|
||
// Reset all buffers used later. | ||
|
@@ -1599,6 +1615,9 @@ func (proxier *Proxier) syncProxyRules() { | |
} | ||
} | ||
proxier.deleteEndpointConnections(endpointUpdateResult.StaleEndpoints) | ||
|
||
metrics.SyncProxyRulesNoLocalEndpointsTotal.WithLabelValues("internal").Set(float64(proxier.serviceNoLocalEndpointsInternal.Len())) | ||
metrics.SyncProxyRulesNoLocalEndpointsTotal.WithLabelValues("external").Set(float64(proxier.serviceNoLocalEndpointsExternal.Len())) | ||
} | ||
|
||
// writeIptablesRules write all iptables rules to proxier.natRules or proxier.FilterRules that ipvs proxier needed | ||
|
@@ -1980,6 +1999,14 @@ func (proxier *Proxier) syncEndpoint(svcPortName proxy.ServicePortName, onlyNode | |
// Traffic from an external source will be routed but the reply | ||
// will have the POD address and will be discarded. | ||
endpoints = clusterEndpoints | ||
|
||
if svcInfo.InternalPolicyLocal() && utilfeature.DefaultFeatureGate.Enabled(features.ServiceInternalTrafficPolicy) { | ||
proxier.serviceNoLocalEndpointsInternal.Insert(svcPortName.NamespacedName.String()) | ||
} | ||
|
||
if svcInfo.ExternalPolicyLocal() { | ||
proxier.serviceNoLocalEndpointsExternal.Insert(svcPortName.NamespacedName.String()) | ||
} | ||
} | ||
} else { | ||
endpoints = clusterEndpoints | ||
|
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.
It might be useful to add tests where we set both the internal and external traffic policy, or collapse the existing tests into two tests where both policies are set
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.
Added test