endpoint: Implement new Invalid endpoint state #11884

christarazi · 2020-06-04T02:28:35Z

See commit msgs.

Endpoint (EP) creation can fail for many reasons due to invalid data, such as IP conflict or if it's created with reserved labels. When EP creation failed, EPs can be stuck in the "waiting-for-identity" state, and left to be garbage collected. However, even though the object would be garbage collected, the metric representing which state the EP is in, is never decremented, thus a leak.

This PR fixes this by introducing a new EP state representing an "invalid" EP. When EP creations fails, the EP will transition from its current state to "invalid", thus decrementing the current state metric.

Fix leaking endpoint state metric

christarazi · 2020-06-04T02:40:16Z

test-me-please

coveralls · 2020-06-04T03:01:08Z

Coverage increased (+0.06%) to 37.0% when pulling 8d3ddda on christarazi:pr/christarazi/fix-missed-decrement-metric-for-endpoints into 7316fa6 on cilium:master.

aanm · 2020-06-04T08:38:24Z

pkg/metrics/metrics.go

@@ -1147,6 +1147,16 @@ func GetCounterValue(m prometheus.Counter) float64 {
 	return 0
 }

+// GetGaugeValue returns the current value stored for the gauge


Write in the comment that this function is only used in tests.

aanm · 2020-06-04T08:42:07Z

daemon/cmd/endpoint.go

+func (d *Daemon) invalidDataError(ep *endpoint.Endpoint, deleteEP bool, err error) (*endpoint.Endpoint, int, error) {
+	if deleteEP {
+		d.deleteEndpointQuiet(ep, endpoint.DeleteConfig{
+			// The IP has been provided by the caller and must be released
+			// by the caller
+			NoIPRelease: true,
+		})
+	}
+


I would suggest to create another function instead of reusing this one.

aanm · 2020-06-04T08:42:20Z

daemon/cmd/endpoint.go

 	}

 	oldEp := d.endpointManager.LookupCiliumID(ep.ID)
 	if oldEp != nil {
-		return invalidDataError(ep, fmt.Errorf("endpoint ID %d already exists", ep.ID))
+		return d.invalidDataError(ep, true, fmt.Errorf("endpoint ID %d already exists", ep.ID))


this should be false, not true. If an endpoint already exists aren't we potentially deleting that same endpoint?

Good catch, will fix

On second thought, this would suffer from the same thing I described below. Not deleting this endpoint object would leak the metric. Are you saying that because in this specific case, both endpoint have the same ID, that would result in the original endpoint object being deleted?

Disregard my comments--was reading your comments from in order and didn't see your proposal at the end 👍

aanm · 2020-06-04T08:42:48Z

daemon/cmd/endpoint.go

 	}

 	oldEp = d.endpointManager.LookupContainerID(ep.GetContainerID())
 	if oldEp != nil {
-		return invalidDataError(ep, fmt.Errorf("endpoint for container %s already exists", ep.GetContainerID()))
+		return d.invalidDataError(ep, true, fmt.Errorf("endpoint for container %s already exists", ep.GetContainerID()))


this should be false, not true. If an endpoint already exists aren't we potentially deleting that same endpoint?

Good catch, will fix

aanm · 2020-06-04T08:52:19Z

daemon/cmd/endpoint.go

@@ -240,27 +248,27 @@ func (d *Daemon) createEndpoint(ctx context.Context, epTemplate *models.Endpoint
 	for _, id := range checkIDs {
 		oldEp, err := d.endpointManager.Lookup(id)
 		if err != nil {
-			return invalidDataError(ep, err)
+			return d.invalidDataError(ep, true, err)


this should be false, not true. same reasons above

This would also be a very similar situation that I described below.

aanm · 2020-06-04T08:52:31Z

daemon/cmd/endpoint.go

 		} else if oldEp != nil {
-			return invalidDataError(ep, fmt.Errorf("IP %s is already in use", id))
+			return d.invalidDataError(ep, true, fmt.Errorf("IP %s is already in use", id))


this should be false, not true. same reasons above

This was the exact error case that a customer ran into. This specific case was causing the errant metric.

aanm · 2020-06-04T08:52:38Z

daemon/cmd/endpoint.go

 		}
 	}

 	if err = endpoint.APICanModify(ep); err != nil {
-		return invalidDataError(ep, err)
+		return d.invalidDataError(ep, true, err)


this should be false, not true. same reasons above

Why would this be problematic? I'm not clear on what this check is validating.

aanm · 2020-06-04T09:07:50Z

daemon/cmd/endpoint.go

@@ -174,7 +174,15 @@ func (d *Daemon) fetchK8sLabelsAndAnnotations(nsName, podName string) (*slim_cor
 	return p, containerPorts, identityLabels, infoLabels, annotations, nil


In this file, instead of the proposed changes I suggest to do the following (on all error conditions):

ep.SetState(endpoint.StateInvalidEndpoint, "Invalid endpoint")

and in pkg/endpoint/endpoint.go

diff --git a/pkg/endpoint/endpoint.go b/pkg/endpoint/endpoint.go index 8bd438963..bd504c97a 100644 --- a/pkg/endpoint/endpoint.go +++ b/pkg/endpoint/endpoint.go @@ -96,6 +96,8 @@ const ( // StateRestoring is used to set the endpoint is being restored. StateRestoring = string(models.EndpointStateRestoring) + // TODO Needs an new state in the api/v1/models` + StateInvalidEndpoint = string(models.StateInvalidEndpoint) + // IpvlanMapName specifies the tail call map for EP on egress used with ipvlan. IpvlanMapName = "cilium_lxc_ipve_" ) @@ -1319,7 +1321,7 @@ func (e *Endpoint) setState(toState, reason string) bool { } case StateWaitingForIdentity: switch toState { - case StateReady, StateDisconnecting: + case StateReady, StateDisconnecting, StateInvalidEndpoint: goto OKState } case StateReady: @@ -1389,7 +1391,7 @@ OKState: // Since StateDisconnected is the final state, after which the // endpoint is gone, we should not increment metrics for this state. - if toState != "" && toState != StateDisconnected { + if toState != "" && (toState != StateDisconnected || toState != StateInvalidEndpoint) { metrics.EndpointStateCount. WithLabelValues(toState).Inc() } @@ -1403,7 +1405,7 @@ func (e *Endpoint) BuilderSetStateLocked(toState, reason string) bool { // Validate the state transition. fromState := e.state switch fromState { // From state - case StateWaitingForIdentity, StateReady, StateDisconnecting, StateDisconnected: + case StateWaitingForIdentity, StateReady, StateDisconnecting, StateDisconnected, StateInvalidEndpoint: // No valid transitions for the builder case StateWaitingToRegenerate, StateRestoring: switch toState { @@ -1456,7 +1458,7 @@ OKState: // Since StateDisconnected is the final state, after which the // endpoint is gone, we should not increment metrics for this state. - if toState != "" && toState != StateDisconnected { + if toState != "" && (toState != StateDisconnected || toState != StateInvalidEndpoint) { metrics.EndpointStateCount. WithLabelValues(toState).Inc() }

joestringer

Endpoint creation strikes again.

joestringer · 2020-06-05T17:33:28Z

pkg/endpoint/endpoint_test.go

+func isFinalState(state string) bool {
+	return (state == StateDisconnected || state == StateInvalid)
+}


FWIW I think a function like this would be nice to have in the actual code as well, to reference from the core state transition function. But I don't think it matters enough to further change this before merging.

joestringer · 2020-06-05T18:02:56Z

daemon/cmd/endpoint.go

@@ -176,6 +176,7 @@ func (d *Daemon) fetchK8sLabelsAndAnnotations(nsName, podName string) (*slim_cor

 func invalidDataError(ep *endpoint.Endpoint, err error) (*endpoint.Endpoint, int, error) {
 	ep.Logger(daemonSubsys).WithError(err).Warning("Creation of endpoint failed due to invalid data")
+	ep.SetState(endpoint.StateInvalid, "Invalid endpoint")


I checked through as well to see that we don't need this anywhere else such as errorDuringCreation(), but that path is fine because it invokes deleteEndpointQuiet() which would transition the endpoint into StateDisconnecting.

christarazi requested a review from a team June 4, 2020 02:28

christarazi requested review from a team as code owners June 4, 2020 02:28

This comment has been minimized.

Sign in to view

christarazi added area/daemon Impacts operation of the Cilium daemon. area/metrics Impacts statistics / metrics gathering, eg via Prometheus. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Jun 4, 2020

maintainer-s-little-helper bot added dont-merge/needs-release-note and removed dont-merge/needs-release-note labels Jun 4, 2020

maintainer-s-little-helper bot added this to In progress in 1.8.0 Jun 4, 2020

christarazi marked this pull request as draft June 4, 2020 02:28

christarazi requested review from joestringer and jrajahalme June 4, 2020 02:29

christarazi added the needs-backport/1.7 label Jun 4, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.7.5 Jun 4, 2020

christarazi added the needs-backport/1.8 label Jun 4, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.8.0 Jun 4, 2020

christarazi added the needs-backport/1.6 label Jun 4, 2020

maintainer-s-little-helper bot added this to Needs backport from master in 1.6.9 Jun 4, 2020

aanm requested changes Jun 4, 2020

View reviewed changes

christarazi force-pushed the pr/christarazi/fix-missed-decrement-metric-for-endpoints branch from fd86c73 to 5418fa1 Compare June 4, 2020 21:55

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 4, 2020

christarazi force-pushed the pr/christarazi/fix-missed-decrement-metric-for-endpoints branch from 5418fa1 to 7876367 Compare June 4, 2020 21:56

This comment has been minimized.

Sign in to view

christarazi force-pushed the pr/christarazi/fix-missed-decrement-metric-for-endpoints branch from 7876367 to 28e6c57 Compare June 4, 2020 21:57

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 4, 2020

cilium deleted a comment from maintainer-s-little-helper bot Jun 4, 2020

christarazi requested a review from a team June 5, 2020 06:42

aanm approved these changes Jun 5, 2020

View reviewed changes

ianvernon approved these changes Jun 5, 2020

View reviewed changes

borkmann merged commit 4a4a59b into cilium:master Jun 5, 2020

1.8.0 automation moved this from In progress to Merged Jun 5, 2020

christarazi deleted the pr/christarazi/fix-missed-decrement-metric-for-endpoints branch June 5, 2020 17:30

christarazi mentioned this pull request Jun 5, 2020

v1.7 backports for 11884 #11930

Merged

christarazi added backport-pending/1.7 and removed needs-backport/1.7 labels Jun 5, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.5 Jun 5, 2020

joestringer approved these changes Jun 5, 2020

View reviewed changes

christarazi mentioned this pull request Jun 5, 2020

v1.6 backports 2020-06-05 #11933

Merged

christarazi added backport-pending/1.6 and removed needs-backport/1.6 labels Jun 5, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.6 in 1.6.9 Jun 5, 2020

joestringer added backport-done/1.6 and removed backport-pending/1.6 labels Jun 5, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.6 to Backport done to v1.6 in 1.6.9 Jun 5, 2020

christarazi mentioned this pull request Jun 5, 2020

v1.8 backports 2020-06-05 (2) #11937

Merged

christarazi added backport-pending/1.8 and removed needs-backport/1.8 labels Jun 5, 2020

maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.0 Jun 5, 2020

joestringer added backport-done/1.7 and removed backport-pending/1.7 labels Jun 6, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.5 Jun 6, 2020

christarazi added backport-done/1.8 and removed backport-pending/1.8 labels Jun 8, 2020

maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.0 Jun 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

endpoint: Implement new Invalid endpoint state #11884

endpoint: Implement new Invalid endpoint state #11884

christarazi commented Jun 4, 2020 •

edited

This comment has been minimized.

christarazi commented Jun 4, 2020

coveralls commented Jun 4, 2020 •

edited

aanm Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

aanm Jun 4, 2020

christarazi Jun 4, 2020

christarazi Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

christarazi Jun 4, 2020

aanm Jun 4, 2020

This comment has been minimized.

joestringer left a comment

joestringer Jun 5, 2020

joestringer Jun 5, 2020

		@@ -174,7 +174,15 @@ func (d Daemon) fetchK8sLabelsAndAnnotations(nsName, podName string) (slim_cor
		return p, containerPorts, identityLabels, infoLabels, annotations, nil

endpoint: Implement new Invalid endpoint state #11884

endpoint: Implement new Invalid endpoint state #11884

Conversation

christarazi commented Jun 4, 2020 • edited

This comment has been minimized.

christarazi commented Jun 4, 2020

coveralls commented Jun 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

joestringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christarazi commented Jun 4, 2020 •

edited

coveralls commented Jun 4, 2020 •

edited