Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config: add native service discovery admin boolean parameter. #12190

Merged
merged 1 commit into from
Mar 14, 2022

Conversation

jrasell
Copy link
Member

@jrasell jrasell commented Mar 4, 2022

This adds a boolean configuration parameter giving operators the ability to enable/disable clients from utilising the Nomad native service discovery feature.

closes https://github.com/hashicorp/team-nomad/issues/261

@jrasell jrasell added this to the 1.3.0 milestone Mar 4, 2022
@jrasell jrasell requested a review from schmichael March 4, 2022 10:17
@jrasell jrasell self-assigned this Mar 4, 2022
// enable/disable to Nomad native service discovery feature on the client.
// This parameter is exposed via the Nomad fingerprinter and used to ensure
// correct scheduling decisions on allocations which require this.
NativeServiceDiscovery *bool `hcl:"native_service_discovery"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I guess this is our last chance to bikeshed so I'll bite:

Should we call this this NomadServiceDiscovery / nomad_service_discovery?

I know having a self-referential configuration name is weird, but if we ever added an option to disable the Consul provider it would have slightly nicer symmetry: {nomad,consul}_service_discovery = {true,false}

Feel free to ignore... just thought I'd put thoughts down in case anybody has strong feelings one way or the other.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense and lines up more with the fingerprint value. The self reference is a little odd, but in the larger context as you mention it sounds reasonable.

This only targets the feature branch, so we can always change it if we want again before release.

@@ -966,6 +973,7 @@ func DefaultConfig() *Config {
BindWildcardDefaultHostNetwork: true,
CNIPath: "/opt/cni/bin",
CNIConfigDir: "/opt/cni/config",
NativeServiceDiscovery: helper.BoolToPtr(true),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 12, 2022

CLA assistant check
All committers have signed the CLA.

@jrasell jrasell force-pushed the f-gh-264 branch 2 times, most recently from ab21a49 to 9215d0c Compare March 14, 2022 09:01
@vercel vercel bot temporarily deployed to Preview – nomad March 14, 2022 10:48 Inactive
Base automatically changed from f-gh-264 to f-1.3-boogie-nights March 14, 2022 12:18
@jrasell jrasell merged commit b8ecce6 into f-1.3-boogie-nights Mar 14, 2022
@jrasell jrasell deleted the f-gh-261 branch March 14, 2022 12:19
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants