Disabling Maintenance mode does not trigger an event

[jakubgs]

Overview of the Issue

I was looking at disabling alerts in consul-alerts when host is in maintenance mode: AcalephStorage/consul-alerts#213

And I found out that while consul-alerts does create an entry in the KV store for _node_maintenance service check when maintenance mode is disabled, it does NOT update that entry once maintenance mode is disabled:

So I tried running consul watch myself same as the consul-alerts service does and I found out that disabling maintenance mode does not trigger an event:

 > consul watch -type checks cat | jq '.[] | select(.CheckID == "_node_maintenance")'
{
  "Node": "node-01.example.org",
  "CheckID": "_node_maintenance",
  "Name": "Node Maintenance Mode",
  "Status": "critical",
  "Notes": "TEST",
  "Output": "",
  "ServiceID": "",
  "ServiceName": "",
  "ServiceTags": [],
  "Type": "maintenance",
  "Definition": {
    "Interval": "0s",
    "Timeout": "0s",
    "DeregisterCriticalServiceAfter": "0s",
    "HTTP": "",
    "Header": null,
    "Method": "",
    "Body": "",
    "TLSServerName": "",
    "TLSSkipVerify": false,
    "TCP": ""
  },
  "CreateIndex": 67303625,
  "ModifyIndex": 67303625
}

I receive one event with Status set to critical, but no recovery to passing, since the _node_maintenance check is removed.

For this reason consul-alerts never updates its stage of _node_maintenance check.

Reproduction Steps

1. Use version 1.10.1
2. Run consul watch -type checks cat
3. See that no even is triggered for disabling of maintenance mode

Operating system and Environment details

Ubuntu 20.04.3, x86_64

[jakubgs]

And there we have it:

consul/agent/agent.go

Lines 3464 to 3483 in cc2abb7

	// DisableServiceMaintenance will deregister the fake maintenance mode check
	// if the service has been marked as in maintenance.
	func (a *Agent) DisableServiceMaintenance(serviceID structs.ServiceID) error {
	if a.State.Service(serviceID) == nil {
	return fmt.Errorf("No service registered with ID %q", serviceID.String())
	}
	// Check if maintenance mode is enabled
	checkID := serviceMaintCheckID(serviceID)
	if a.State.Check(checkID) == nil {
	// maintenance mode is not enabled
	return nil
	}
	// Deregister the maintenance check
	a.RemoveCheck(checkID, true)
	a.logger.Info("Service left maintenance mode", "service", serviceID.String())
	return nil
	}

consul/agent/agent.go

Lines 3510 to 3517 in cc2abb7

	// DisableNodeMaintenance removes a node from maintenance mode
	func (a *Agent) DisableNodeMaintenance() {
	if a.State.Check(structs.NodeMaintCheckID) == nil {
	return
	}
	a.RemoveCheck(structs.NodeMaintCheckID, true)
	a.logger.Info("Node left maintenance mode")
	}

We can clearly see that when disabling maintenance mode for a host RemoveCheck is called without changing the state.

[jakubgs]

I think just calling:

	a.State.UpdateCheck(checkID, api.HealthPassing, "")

Before RemoveCheck should do the trick. I'll make a PR.

[jkirschner-hashicorp]

It may be worth verifying that the event is still triggered even if UpdateCheck and RemoveCheck are called in quick succession. (I'm unfamiliar with this part of the code, so I can't comment on whether I'd expect the event to fire still or not.)

[jakubgs]

Though now that I look at it adding a test to verify the status was changed to passing before removal will be a bit more tricky:

consul/agent/agent_test.go

Lines 3358 to 3362 in cc2abb7

	// Leave maintenance mode
	a.DisableNodeMaintenance()
	// Ensure the check was deregistered
	requireCheckMissing(t, a, structs.NodeMaint)

I will have to do some kind of State dark magic to check that. In tests I changed before this was done using mock.NewNotify():

consul/agent/checks/check_test.go

Lines 894 to 899 in cc2abb7

	func TestStatusHandlerResetCountersOnNonIdenticalsConsecutiveChecks(t *testing.T) {
	t.Parallel()
	cid := structs.NewCheckID("foo", nil)
	notif := mock.NewNotify()
	logger := testutil.Logger(t)
	statusHandler := NewStatusHandler(notif, logger, 2, 2, 3)

But not sure how that would work here.

[jakubgs]

It may be worth verifying that the event is still triggered even if UpdateCheck and RemoveCheck are called in quick succession.

Yes, good point. I'll verify that on a some host from some test fleet in our infra.

[jakubgs]

You might be onto something. I tried with just UpdateCheck call and it didn't work:

 > consul watch -type checks cat | jq -c '.[] | select(.CheckID == "_node_maintenance") | {CheckID, Status}'   
{"CheckID":"_node_maintenance","Status":"critical"}

But with addition of a.State.SyncChanges() it does work:

 > consul watch -type checks cat | jq -c '.[] | select(.CheckID == "_node_maintenance") | {CheckID, Status}'
{"CheckID":"_node_maintenance","Status":"critical"}                                                                                         
{"CheckID":"_node_maintenance","Status":"passing"}

I also tried a.State.SyncFull() which also worked, but not sure which one is the correct choice in this case.

Thanks for the tip. Though I'm still not sure how to change the tests to take this into account.

[jakubgs]

As far as I can tell it's impossible to check what was the state of the check after it was removed.

I thought I could get it after it was removed from a.State.AllChecks() since:

consul/agent/local/state.go

Lines 78 to 80 in cc2abb7

	// Deleted is true when the service record has been marked as deleted
	// but has not been removed on the server yet.
	Deleted bool

consul/agent/local/state.go

Lines 334 to 338 in cc2abb7

	// To remove the service on the server we need the token.
	// Therefore, we mark the service as deleted and keep the
	// entry around until it is actually removed.
	s.InSync = false
	s.Deleted = true

But when I try to print checks after it has been removed:

	fmt.Printf("%v", a.State.AllChecks())

I get nothing:

map[]

[jakubgs]

Well, for now I made a PR without tests changed: #11332

Open to suggestions on how I can check for status change in a check that was removed.