Fixing endpoint status #339

aanm · 2017-03-18T02:38:26Z

@tgraf I can squash the last 2 commits if you want.

Fixes #207
Fixes #316

File: pkg/endpoint/endpoint.go:L229-236
What kind of Validation you were talking about?
The case where the value of state is not one of the model enum.
File: pkg/endpoint/endpoint.go:L255-279
Can you explain this priority system a bit. I assume the point is to not undo failure state unless we know that the actual failure is gone. How does this priority actual fix it?
Don't we need a bitmask which indicates the status of each failure mode and if any of them is set to failure the entire endpoint is marked as failed?
File: pkg/endpoint/endpoint.go:L249-305
@tgraf Tell me if by only reading this explanation it makes sense to you or if you had any doubts.
I'm not sure I fully understand the concept yet. The fundamental problem is that you have multiple causes for an endpoint to be considered in an failed state:
bpf generation or compilation
policy calculation
....
Each of these causes can be OK/warning/failure. You want to report an overall status. You also want to store a log of status changes for each. The priority seems to indicate that a BPF failure always overwrites a policy failure but that seems incomplete.
Let's assume:

bpf fails
policy fails
bpf succeeds
You still want the overall state to say failure.
I think instead of priority, what you want is this:
map[StatusComponent]statusLogMsg
BPF and priority are both StatusComponent
OverallStatus is Failure if any of the components failed, else warning if any component is warning, else OK.
The log shows the latest n status changes of the endpoint.

General Comment
TODO: Implement map for CurrentStatus to keep low priority non-OK messages.
File: pkg/endpoint/endpoint.go:L236-305
Looks great. What does priority mean in this context? I would rename this to componentStatus
Should I renamed the StatusPriority to StatusComponent?
It needs to have a sense of a priority in the name. Suggestions?
I think you want a struct ComponentStatus which is a member Latest in EndpointStatus. I know what you mean by priority but it's misleading. I think you really mean the latest status.
Or ComponentStatuses which is a slice of ComponentStatus
And the StatusPriority should I keep it as is?
How about EndpointComponentName?
Change it to StatusType, I think it makes sense.
File: pkg/endpoint/endpoint.go:L236-305
Minor as it is private but I would rename this as well to statusTypeSlice

aanm · 2017-03-18T02:38:52Z

pkg/endpoint/endpoint.go

@@ -224,7 +229,7 @@ func (e *Endpoint) GetModel() *models.Endpoint {
 		InterfaceName:    e.IfName,
 		Mac:              e.LXCMAC.String(),
 		HostMac:          e.NodeMAC.String(),
-		State:            models.EndpointState(e.State), // TODO: Validate
+		State:            currentState, // TODO: Validate


What kind of Validation you were talking about?

The case where the value of state is not one of the model enum.

tgraf

Separation into commits as-is OK, see question on priority.

tgraf · 2017-03-18T03:45:51Z

pkg/endpoint/endpoint.go

 	return idx
 }

-func (e *EndpointStatus) addStatusLog(s *statusLog) {
+func (e *EndpointStatus) addStatusLog(s *statusLogMsg) {
+	if e.CurrentStatus.Status.Code == OK {


Can you explain this priority system a bit. I assume the point is to not undo failure state unless we know that the actual failure is gone. How does this priority actual fix it?

Don't we need a bitmask which indicates the status of each failure mode and if any of them is set to failure the entire endpoint is marked as failed?

aanm · 2017-03-18T04:15:23Z

pkg/endpoint/endpoint.go

 	return idx
 }

-func (e *EndpointStatus) addStatusLog(s *statusLog) {
+// addStatusLog adds statusLogMsg to endpoint log.


@tgraf Tell me if by only reading this explanation it makes sense to you or if you had any doubts.

I'm not sure I fully understand the concept yet. The fundamental problem is that you have multiple causes for an endpoint to be considered in an failed state:

bpf generation or compilation

policy calculation

....

Each of these causes can be OK/warning/failure. You want to report an overall status. You also want to store a log of status changes for each. The priority seems to indicate that a BPF failure always overwrites a policy failure but that seems incomplete.

Let's assume:

bpf fails

policy fails

bpf succeeds

You still want the overall state to say failure.

I think instead of priority, what you want is this:
map[StatusComponent]statusLogMsg

BPF and priority are both StatusComponent

OverallStatus is Failure if any of the components failed, else warning if any component is warning, else OK.

The log shows the latest n status changes of the endpoint.

aanm · 2017-03-18T05:02:10Z

TODO: Implement map for CurrentStatus to keep low priority non-OK messages.

tgraf · 2017-03-18T21:15:28Z

pkg/endpoint/endpoint.go

+type statusLog []*statusLogMsg
+
+// priorityStatus represents a map of a single statusLogMsg by StatusPriority.
+type priorityStatus map[StatusPriority]*statusLogMsg


Looks great. What does priority mean in this context? I would rename this to componentStatus

Should I renamed the StatusPriority to StatusComponent?

It needs to have a sense of a priority in the name. Suggestions?

I think you want a struct ComponentStatus which is a member Latest in EndpointStatus. I know what you mean by priority but it's misleading. I think you really mean the latest status.

Or ComponentStatuses which is a slice of ComponentStatus

And the StatusPriority should I keep it as is?

How about EndpointComponentType?

I change it to StatusType, I think it makes sense.

tgraf · 2017-03-19T23:11:10Z

pkg/endpoint/endpoint.go

-	e.indexMU.RLock()
-	defer e.indexMU.RUnlock()
+// sPriority represents a slice of StatusType, is used for sorting purposes.
+type sPriority []StatusType


Minor as it is private but I would rename this as well to statusTypeSlice

tgraf

Despite the minor comment, this looks fine now.

Signed-off-by: André Martins <andre@cilium.io>

aanm added the kind/bug This is a bug in the Cilium logic. label Mar 18, 2017

aanm requested a review from tgraf March 18, 2017 02:38

aanm commented Mar 18, 2017

View reviewed changes

aanm force-pushed the fixing-endpoint-status branch from 7c7bc3c to 0a211bf Compare March 18, 2017 02:39

tgraf reviewed Mar 18, 2017

View reviewed changes

aanm force-pushed the fixing-endpoint-status branch 2 times, most recently from 21bd7f8 to 6286b30 Compare March 18, 2017 04:14

aanm commented Mar 18, 2017

View reviewed changes

aanm force-pushed the fixing-endpoint-status branch from 6286b30 to de36dbb Compare March 18, 2017 04:41

aanm added the wip label Mar 18, 2017

tgraf added this to the 0.8 release milestone Mar 18, 2017

tgraf added the release blocker label Mar 18, 2017

aanm force-pushed the fixing-endpoint-status branch from de36dbb to 9cf63bd Compare March 18, 2017 21:01

aanm requested a review from tgraf March 18, 2017 21:01

aanm added pending-review and removed wip labels Mar 18, 2017

aanm force-pushed the fixing-endpoint-status branch from 9cf63bd to 7ab72cc Compare March 18, 2017 21:02

tgraf reviewed Mar 18, 2017

View reviewed changes

aanm force-pushed the fixing-endpoint-status branch from 7ab72cc to 0e25076 Compare March 19, 2017 23:06

aanm requested a review from tgraf March 19, 2017 23:07

tgraf reviewed Mar 19, 2017

View reviewed changes

tgraf approved these changes Mar 19, 2017

View reviewed changes

aanm force-pushed the fixing-endpoint-status branch from 0e25076 to 8a9eabb Compare March 19, 2017 23:31

aanm added 3 commits March 20, 2017 00:12

endpoint status: new status log with priority awareness

1f26214

Signed-off-by: André Martins <andre@cilium.io>

api/v1: add not-ready for endpoint state

0fb1853

Signed-off-by: André Martins <andre@cilium.io>

endpoint: add not-ready state if endpoint status not OK

3502379

Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the fixing-endpoint-status branch from 8a9eabb to 3502379 Compare March 20, 2017 00:12

aanm merged commit 8361b36 into master Mar 20, 2017

aanm deleted the fixing-endpoint-status branch March 20, 2017 00:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing endpoint status #339

Fixing endpoint status #339

aanm commented Mar 18, 2017 •

edited

aanm Mar 18, 2017

tgraf Mar 18, 2017

tgraf left a comment

tgraf Mar 18, 2017

aanm Mar 18, 2017

tgraf Mar 18, 2017

aanm commented Mar 18, 2017

tgraf Mar 18, 2017

aanm Mar 18, 2017

aanm Mar 18, 2017

tgraf Mar 18, 2017

tgraf Mar 18, 2017

aanm Mar 18, 2017

tgraf Mar 18, 2017 •

edited

aanm Mar 19, 2017 •

edited

tgraf Mar 19, 2017

tgraf left a comment

Fixing endpoint status #339

Fixing endpoint status #339

Conversation

aanm commented Mar 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgraf left a 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

aanm commented Mar 18, 2017

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

tgraf Mar 18, 2017 • edited

Choose a reason for hiding this comment

aanm Mar 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgraf left a comment

Choose a reason for hiding this comment

aanm commented Mar 18, 2017 •

edited

tgraf Mar 18, 2017 •

edited

aanm Mar 19, 2017 •

edited