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

RFC: Makes salt minion's master_port and publish_port options depend on each master's ip #50412

Closed
kstreee opened this issue Nov 7, 2018 · 4 comments
Labels
Feature new functionality including changes to functionality and code refactors, etc.
Milestone

Comments

@kstreee
Copy link
Contributor

kstreee commented Nov 7, 2018

  • Feature Name: Makes salt minion's master_port and publish_port options depend on each master's ip.
  • Start Date: 2018-11-07
  • RFC PR:
  • Salt Issue:

Summary

For example, current salt minion configuration can't cover the following setting of multi-master.

master1's config

 interface: 1.1.1.1
 ret_port: 30001 
 publish_port: 30002

master2's config

 interface: 1.1.1.2
 ret_port: 30008
 publish_port: 30009

minion's config

master:
  1.1.1.1:30001
  1.1.1.2:30001
publish_port: ?

Motivation

We are trying to deploy salt master using kubernetes, and kubernetes-like systems allocate network resources highly dynamically to use the limited resources efficiently. To leverage the system's network environment (dynamically allocate ports to expose container's ports to the outer network), publish_port and ret_port of salt masters of a multi-master environment can be assigned as different ports like the above setting.

Design

Current salt minion's master config

master:
  1.1.1.1:RET_PORT (MASTER_PORT)
  1.1.1.2:RET_PORT (MASTER_PORT)
publish_port: PUBLISH_PORT

Desired salt minion's master config

master:
  - host: 1.1.1.1
    master_port: 30001
    publish_port: 30002
  - host: 1.1.1.2
    master_port: 30008
    publish_port: 30009

Alternatives

Giving up multi-master setting which uses different ports for the same purpose..?

Unresolved questions

Not sure... how to find all being affected points in the minion code?

Drawbacks

  • Backward compatibility, how to support both formats (old format, and new format) of master connection information of salt minion config?
  • Implementation cost, we should find all being affected points in the minion code, and patch it to accept the new form of salt master connection config.
  • Documentation
@dwoz dwoz added the Feature new functionality including changes to functionality and code refactors, etc. label Nov 7, 2018
@dwoz dwoz added this to the Approved milestone Nov 7, 2018
@dwoz
Copy link
Contributor

dwoz commented Nov 7, 2018

@kstreee Thanks for the feature request. It makes sense to me and I like it.. @core-team comments?

@isbm
Copy link
Contributor

isbm commented Nov 17, 2018

What's wrong would be just adding the publish_port instead?

master:
  - [2001:0db8:85a3:0000:0000:8a2e:0370:7334]:3001
    publish_port: 3002
  - 123.123.123.123:3001
    publish_port: 3002
  - foo.bar.blah.com:3001
    publish_port: 3002
  - publish_port: 3002  # Default port

@isbm
Copy link
Contributor

isbm commented Nov 17, 2018

Second, this have to be an RFC pull request, not an issue. Please close the issue and rewrite this as an RFC, and make a PR so we can discuss it there.

@kstreee
Copy link
Contributor Author

kstreee commented Nov 20, 2018

@isbm Sure, I would rather change the pr what I sent as a RFC. When I opened this issue, I thought to add this feature might be tricky so I opened this issue to discuss before providing sample code of this RFC. But, I realized it isn't tricky at all after dived into the code, so to show an example and modification points of this RFC, sent the pr. As your advice, closed this issue. Please move your comment to the pr here #50521 to discuss about that (and I think it is a good point and we should talk about that more). Thanks for the guiding me.

@kstreee kstreee closed this as completed Nov 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature new functionality including changes to functionality and code refactors, etc.
Projects
None yet
Development

No branches or pull requests

3 participants