add default_ineligible configuration option

[wjordan]

Specifies if scheduling eligibility should be disabled by default for new client nodes.

Fixes #13081, see this issue for discussion of the use-case.

[jrasell]

Hi @wjordan and thanks for taking an initial pass at this. I have added some inline comments around the functionality.

I am also curious of how this approach would work. If the value is set via a config file value, would the operator be expected to modify this value and then restart the agent?

The Nomad API also includes a method for updating node eligibility which operators could use. If this was the case, and the agent restarted (crash or otherwise) would the config value override what the API call did?

[jrasell]

This nil check is a safety check to avoid a panic in the event the node config object is not correctly set. If we chase the function calls from this client.setupNode call we see:

• it is called by client.NewClient which comes from agent.setupClient
• the agent.setupClient function includes a call to agent.clientConfig
• agent.clientConfig calls convertClientConfig which performs a number of node struct setup items

Therefore during normal and hopefully every client setup this condition is never hit and therefore the eligibility toggle is not used.

[jrasell]

This test seems like it is attempting to match the exact conditions seen within the code which as previously noted is a bit of a red herring.

[wjordan]

Thanks @jrasell for quick feedback and pointing me in the right direction with the implementation. I've updated the code to pass DefaultIneligible as a NodeRegisterRequest argument to Node.Register RPC, so that the server can apply the configurable default appropriately: https://github.com/hashicorp/nomad/blob/cb2484b3f88585974fbd993e2976b2e96440ed53/nomad/node_endpoint.go#L124-L131

I've updated the test as well. Please take a look and let me know if this is headed in the right direction!

[wjordan]

To answer your questions:

I am also curious of how this approach would work. If the value is set via a config file value, would the operator be expected to modify this value and then restart the agent?

The intended behavior is to only apply the config value as an initial default for newly-registered nodes, so the operator would be expected to set this via a config file value before starting the agent the first time.

The Nomad API also includes a method for updating node eligibility which operators could use. If this was the case, and the agent restarted (crash or otherwise) would the config value override what the API call did?

No, the config value is only intended to allow configuring the default for newly-registered nodes, so it wouldn't override anything once the node has been registered.

[wjordan]

Third attempt at this in abc5188, just set the client node's SchedulingEligibility value based on DefaultIneligible in setupNode: https://github.com/hashicorp/nomad/blob/31e6a62381ba8e9b89f831bc570799c03affd118/client/client.go#L1475-L1479

This value will be sent to the server on the initial registration. Re-registering an existing node (e.g., on agent restart) should already correctly preserve the existing value, thanks to existing logic in upsertNodeTxn that retains SchedulingEligibility:

nomad/nomad/state/state_store.go

Line 896 in 5b5c853

node.SchedulingEligibility = exist.SchedulingEligibility // Retain the eligibility

[Tyrael]

any update on this? was surprised that this isn't supported yet.

[miguelwhite]

Are there any updates on this?

[luizrojo]

Any updates on this change?

[josegonzalez]

@wjordan would you be able to rebase the PR? If not, mind if I reissue it to get this feature merged by the hashicorp folks?

[4W5ZeZPMc]

@tgross @jrasell is it possible to have a look again at this PR, this feature would be awesome

[tehut]

@wjordan I did not intend to push to your fork (I assumed I wouldn't have permissions) but if you are still following this PR and would like to add these test fixes from the PR I opened. I'd be happy to merge your PR instead.