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

minion_id_remove_domain could contain string and bool values #49378

Merged

Conversation

marbx
Copy link
Contributor

@marbx marbx commented Aug 28, 2018

This PR replaces PR #49213

What does this PR do?

When a minion lacks its minion_id at startup, salt.config.__init__.py requests one from utils.network.generate_minion_id(), which returns the fully qualified domain name (FQDN) of the host.

When all minions are within one domain and named after their hostnames, generated a "FQDN minion id" would be an inconsistent naming.

salt.config.__init__.py has the option to lowercase the minion_id.
salt.config.__init__.py also has the option to append a domain to a generated "hostname minion id".

I hereby propose a new option, to generate a "hostname minion id", by removing a single domain from a FQDN.

What issues does this PR fix or reference?

Issue #49212

Previous Behavior (example)

The generated minion_id is king_bob.foo.org

New Behavior (example)

With minion_id_remove_domain: False, as previously.
With minion_id_remove_domain: bar.org, as previously.
With minion_id_remove_domain: foo.org, the generated minion_id becomes king_bob
With minion_id_remove_domain: True, the generated minion_id becomes king_bob

Tests written?

Yes

@marbx marbx requested a review from as a code owner Aug 28, 2018
@ghost ghost self-requested a review Aug 28, 2018
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko left a comment

LGTM, I'd just like to see some minor changes in there.

salt/config/__init__.py Outdated Show resolved Hide resolved
salt/config/__init__.py Outdated Show resolved Hide resolved
salt/config/__init__.py Outdated Show resolved Hide resolved
salt/config/__init__.py Outdated Show resolved Hide resolved
isbm
isbm approved these changes Aug 29, 2018
Copy link
Contributor

@isbm isbm left a comment

I am approving this, thanks @markuskramerIgitt ! However, I have just one suggestion to rephrase your table-list schematic example into something generic, typically readable. I include my suggestion.

doc/ref/configuration/minion.rst Show resolved Hide resolved
@marbx
Copy link
Contributor Author

@marbx marbx commented Aug 29, 2018

Hello @isbm, the page states you approved my changes. Why does the page still shows "Changes requested"?

Copy link
Collaborator

@terminalmage terminalmage left a comment

We need test cases for the True and False values for this config option. Also, is this intended to replace #49213? If so, we should probably close #49213.

doc/ref/configuration/minion.rst Outdated Show resolved Hide resolved
salt/config/__init__.py Outdated Show resolved Hide resolved
@isbm
Copy link
Contributor

@isbm isbm commented Aug 29, 2018

@markuskramerIgitt because @terminalmage needs unit tests (+1, BTW) and some minor fixes. 😉

@terminalmage
Copy link
Collaborator

@terminalmage terminalmage commented Aug 29, 2018

Actually, I think it is because @DmitryKuzmenko had requested changes and has not re-reviewed with an Approved outcome, because @markuskramerIgitt's question pre-dated my review.

@DmitryKuzmenko
Copy link
Contributor

@DmitryKuzmenko DmitryKuzmenko commented Aug 29, 2018

who? me? sorry, done. =-/

@marbx
Copy link
Contributor Author

@marbx marbx commented Aug 30, 2018

Of the two PR...

  • #49213 is string type and complete
  • #49378 is (string,bool) type, needs refactoring due to cyclomatic complexity 22 in get_id(), boolean test cases and a more serene description for boolean True.

… only one is necessary (they are mutually exclusive).
Either one is fine for me

@terminalmage, which do you like? Can you decide alone?

@isbm, if #49378 is favored, could you please change/add the Boolean part? I see that you have more experience in Python.

@isbm
Copy link
Contributor

@isbm isbm commented Aug 30, 2018

[...] Can you decide alone?

So everyone can blame @terminalmage for this? 😄 In case #49378 is there, I'd like to be blamed too!

This boils down that while any domain cut would be indeed better user experience for small deployments, question is should we introduce some new tweaks and knobs to existing pile, or just reuse what already is there. My personal opinion is to reuse. In Salt we already doing this all over the place, so there is no reason that this time it needs to be any different. Also if the directive is to how to remove domain that refers to the same action, I see no reason to have two separate options doing conceptually the same thing.

But I also think that @terminalmage still possibly can find me wrong here, so objections to the above are welcome.

@markuskramerIgitt what exactly should I change?

@terminalmage
Copy link
Collaborator

@terminalmage terminalmage commented Aug 30, 2018

I like this approach better than the other PR, but the changes I requested still need to be made before this PR is ready for merge.

@marbx
Copy link
Contributor Author

@marbx marbx commented Sep 1, 2018

@terminalmage, thank you for your vote, I see there are more examples of "mixed type" in the same module. Have I now completed this PR?

I think both PR #49213 and #49378 are ready for merge.
Please accept one and close the other.

@marbx
Copy link
Contributor Author

@marbx marbx commented Sep 4, 2018

Hi @terminalmage, belowit says that you "request changes", but I don't find which ones.
Which changes do you request?

I also don't understand which checks exactly are "not successful".

@marbx
Copy link
Contributor Author

@marbx marbx commented Oct 4, 2018

Hi @isbm,
the above exception is thrown by salt/_compat.py#L194

        try:
            packed = bool(int(str(bytearray(data)).encode('hex'), 16))
        except ValueError:

This is intended as a test if data is a packed binay.
I would add TypeError to the known exception list.

10 days ago you altered this line - what do you think?

Thank you,
Markus

@isbm
Copy link
Contributor

@isbm isbm commented Oct 5, 2018

@markuskramerIgitt hmm, why do we have there TypeError? What you're getting into data variable then?

@isbm
Copy link
Contributor

@isbm isbm commented Oct 5, 2018

@markuskramerIgitt oh OK, I know why. You found a bug, congrats. You also found a correct solution (I've debugged it and think the same). 🍿

PR to fix this: #49908

@rallytime
Copy link
Contributor

@rallytime rallytime commented Oct 5, 2018

Nice - thanks for helping out here @isbm!

@rallytime rallytime reopened this Oct 5, 2018
@marbx
Copy link
Contributor Author

@marbx marbx commented Oct 5, 2018

Hi @isbm, thank you for picking it up.

@marbx
Copy link
Contributor Author

@marbx marbx commented Oct 5, 2018

Jenkins still fails, but for no (apparent) reason?

Is there something I can do here?

@rallytime
Copy link
Contributor

@rallytime rallytime commented Oct 5, 2018

@markuskramerIgitt We're working through an issue with our pip installer right now, so no need to worry here right now. We'll get these going again when the issue is resolved. Thanks!

@rallytime rallytime merged commit fe2713e into saltstack:develop Oct 9, 2018
7 of 11 checks passed
@marbx marbx deleted the minion_id_remove_domain_as_mixed_type branch Oct 25, 2018
@marbx
Copy link
Contributor Author

@marbx marbx commented Oct 25, 2018

@rallytime, thank you for merging.
How will this pull request make its way from 'develop' into a release?

@gtmanfred
Copy link
Contributor

@gtmanfred gtmanfred commented Oct 25, 2018

This will be in the Neon feature release.

@marbx
Copy link
Contributor Author

@marbx marbx commented Oct 26, 2018

Thank you, now I unterstand versionadded:: Neon.
Neon comes after Fluorine

@marbx
Copy link
Contributor Author

@marbx marbx commented May 24, 2019

@gtmanfred, could you please consider to merge this change into a Fluorine release?
Otherwise I would need to create a local modification.

@marbx
Copy link
Contributor Author

@marbx marbx commented May 24, 2019

@gtmanfred, please forget to consider merging at an earlier time.
I just found that I can help myself with id_function, no local modification needed.

@marbx
Copy link
Contributor Author

@marbx marbx commented Sep 18, 2019

As of today, Neon still has no release version.

garethgreenaway added a commit to garethgreenaway/salt that referenced this issue Sep 19, 2019
@marbx
Copy link
Contributor Author

@marbx marbx commented Sep 19, 2019

@gtmanfred, please forget to consider merging at an earlier time.
I just found that I can help myself with id_function, no local modification needed.

Unfortunately: id_function, requires configuration of the salt master and of the (to be modified) salt-minion installer:

The installers must configure id_function before the minion service starts, and the salt-master must not remove this function, later.

@waynew waynew added this to PR needs port to master in PRs to port to master Oct 24, 2019
dwoz added a commit that referenced this issue Dec 6, 2019
@marbx
Copy link
Contributor Author

@marbx marbx commented Mar 11, 2020

minion_id_remove_domain is not in git checkout v2019.2.1 and git checkout v2019.2.3 but in

git checkout v2019.8
git checkout v3000 

@garethgreenaway garethgreenaway moved this from PR needs port to master to PR merged in PRs to port to master Mar 24, 2020
@garethgreenaway garethgreenaway moved this from PR merged to PR has port to master in PRs to port to master Mar 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
PRs to port to master
  
PR has port to master
Development

Successfully merging this pull request may close these issues.

None yet

6 participants