#5674 closed enhancement (complete)

Implement hold state in HA

Reported by: tomek Owned by: marcin
Priority: medium Milestone: Kea1.5
Component: high-availability Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The details of this feature will be laid out in #5673.

This ticket covers the implementation of parameters and the mechanism for HA to stop in a given state. For now, the only possible state will be "waiting".

A separate ticket will cover the ability to resume (move out of the waiting state).

Subtickets

Change History (7)

comment:1 Changed 16 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:2 Changed 16 months ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

Implemented HA state machine pausing in all states, although for certain states it has no real effect (backup, terminated).

Proposed ChangeLog entry:

14XX.	[func]		marcin
	Implemented state HA state machine pausing in the high
	availability hooks library.
	(Trac #5674, git cafe)

comment:3 Changed 16 months ago by tmark

  • Owner changed from UnAssigned to tmark

comment:4 follow-up: Changed 16 months ago by tmark

  • Owner changed from tmark to marcin

Seems odd that we can only command it to un-pause and not to pause/


Why would you configure a state to "never" pause? Is this just the default value


I think there is general naming issue. You are adding a a new element "state-machine", which is a map of states. Normally a map of elements is plural thing like "peers", so it really ought to be

    "states" : [
        {
            "state" : "waiting";
            :
        },
        {
            "state" : "ready";
            :
        }
    ]

Which means you need to rename HAConfig::state_machine_ and follow what you did with peers_.

As far as I know our list elements are always a plural form of what they contain. If we've been inconsistent with this, shame on us.

OR

"state-machine" becomes a map element that contains a list element, "states", and has room for other things, if we imagine ever needing them:

    "state-machine": {
            "somevalue": ,
            "someothervalue":,
            "states":[
                {
                },
                {
                },
            ] 
    }

This would imply that you need a StateMachineConfig? class which contains a map of StateConfigs?, and provides methods for adding, finding StateConfigs?:

ex: StateMachineConfig::createStateConfig(), etc...


General issue, I am thinking that pausing and resuming is something that should be supported inside StateModel?. HAstateMachineControl is sort of an overlay and is tracking the current state (something HAService as a StateModel? already knows). This call to notify becomes unnecessary:

    if (doOnEntry()) {
        : 
        state_machine_control_.notify(getCurrState());
        :
    }

As we would move the pause logic into StateModel::transition().
And this :

    if (state_machine_control_.amPaused()) {
        postNextEvent(NOP_EVT);
        return;
    }

Becomes this:

    if (amPaused() {
        postNextEvent(NOP_EVT);
        return;
    }

I think this at least merits a ticket to explore refactoring it.


Can you a devise tests to verify that heart beats continue when paused when appropriate?


It compiles and unit tests pass under Centos 7.

comment:5 in reply to: ↑ 4 Changed 16 months ago by marcin

  • Owner changed from marcin to tmark

Replying to tmark:

Seems odd that we can only command it to un-pause and not to pause/

That's not really a technical point. We don't have a use case for the command to pause. Plus, this really belongs to the ticket #5675 as it implements a command to unpause.


Why would you configure a state to "never" pause? Is this just the default value

This is the default value but the default value needs its explicit representation. For example: someone typical pauses in a certain state, but now he wants to temporarily bring it to the default but without removing the whole configuration section.


I think there is general naming issue. You are adding a a new element "state-machine", which is a map of states. Normally a map of elements is plural thing like "peers", so it really ought to be

    "states" : [
        {
            "state" : "waiting";
            :
        },
        {
            "state" : "ready";
            :
        }
    ]

Which means you need to rename HAConfig::state_machine_ and follow what you did with peers_.

As far as I know our list elements are always a plural form of what they contain. If we've been inconsistent with this, shame on us.

I don't like the idea of turning state-machine to states because states seems too ambiguous. It doesn't really relate to the states of the state machine.

OR

"state-machine" becomes a map element that contains a list element, "states", and has room for other things, if we imagine ever needing them:

    "state-machine": {
            "somevalue": ,
            "someothervalue":,
            "states":[
                {
                },
                {
                },
            ] 
    }

This would imply that you need a StateMachineConfig? class which contains a map of StateConfigs?, and provides methods for adding, finding StateConfigs?:

ex: StateMachineConfig::createStateConfig(), etc...

I like more the idea of nesting "states" list within "state-machine" map. This leaves room for additional/global parameters when we need them. Obviously, it is at the expense of more complicated JSON structure, but I am ok with this.


General issue, I am thinking that pausing and resuming is something that should be supported inside StateModel?. HAstateMachineControl is sort of an overlay and is tracking the current state (something HAService as a StateModel? already knows). This call to notify becomes unnecessary:

    if (doOnEntry()) {
        : 
        state_machine_control_.notify(getCurrState());
        :
    }

As we would move the pause logic into StateModel::transition().
And this :

    if (state_machine_control_.amPaused()) {
        postNextEvent(NOP_EVT);
        return;
    }

Becomes this:

    if (amPaused() {
        postNextEvent(NOP_EVT);
        return;
    }

I think this at least merits a ticket to explore refactoring it.

I didn't like the option of a ticket to refactor, so I applied the changes to the StateModel and removed the !HAStateMachineControl class altogether.


Can you a devise tests to verify that heart beats continue when paused when appropriate?

Added checks for those.


It compiles and unit tests pass under Centos 7.

comment:6 Changed 16 months ago by tmark

  • Owner changed from tmark to marcin

Changes look good! Still builds, unit tests still pass. Merge ahoy.

comment:7 Changed 16 months ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit b9f3f082c7a88fe98fa4545b9649193ceb5e3ef5

Note: See TracTickets for help on using tickets.