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

WIP: Save _schedule.conf under <proxy ID> dir #39776

Merged
merged 1 commit into from Mar 2, 2017

Conversation

mirceaulinic
Copy link
Contributor

@mirceaulinic mirceaulinic commented Mar 2, 2017

Save _schedule.conf under <proxy ID> dir.
So each proxy can have its own scheduled functions, independently.

35b8b8f only saves the config file under the right path, but I din't find yet the way to tell Salt to load it from there. Ping @cro: any ideas how to?

What issues does this PR fix or reference?

#39775

Previous Behavior

All proxies sharing the same schedule config file.

New Behavior

Each proxy has its config file

Tests written?

No

So each proxy can have its own scheduled functions, independently.
@mirceaulinic mirceaulinic changed the title WIP: Save _schedule.conf under <minion ID> dir WIP: Save _schedule.conf under <proxy ID> dir Mar 2, 2017
@cachedout cachedout requested a review from cro March 2, 2017 16:24
@cro cro merged commit 965f474 into saltstack:2016.11 Mar 2, 2017
@cro
Copy link
Contributor

cro commented Mar 2, 2017

Thank you @mirceaulinic. Good catch.

@mirceaulinic
Copy link
Contributor Author

mirceaulinic commented Mar 2, 2017

Hi @cro - thanks for looking into this. I think, but please correct me if I'm wrong, the changes from 35b8b8f are only half of what's required to solve #39775 completely. While 35b8b8f saves the _schedule.conf file per proxy, I think we need to specify also to load it from there? But I didn't know where to apply the necessary changes. Thanks!

@cro
Copy link
Contributor

cro commented Mar 2, 2017 via email

@mirceaulinic
Copy link
Contributor Author

@cro @cachedout would you be able to provide more input on this please? I'd like to get this sorted in 2016.11.4. Thanks!

@cro
Copy link
Contributor

cro commented Mar 9, 2017

Oh, I remember now. When we persist the data to the _schedule.conf file, it's in the same format as the regular minion configuration file, and it resides in /etc/salt/proxy.d/. Thus, it gets loaded as part of the regular configuration file processing when the minion starts up.

I think that means that it might be a little harder to handle this since ALL files in /etc/salt/proxy.d get parsed by all proxy processes that start up. I wonder if a better solution is to modify the config parser so that append_minionid_config_dirs includes /etc/salt/proxy.d by default. Then we would have paths like /etc/salt/proxy.d/proxy01/_scheduler.conf. What do you think?

@mirceaulinic
Copy link
Contributor Author

Hi @cro - thanks for replying!

Then we would have paths like /etc/salt/proxy.d/proxy01/_scheduler.conf

The changes from this PR do this: save the schedule config file under paths such as etc/salt/proxy.d/proxy01/_scheduler.conf, etc/salt/proxy.d/proxy02/_scheduler.conf etc, corresponding to proxy01, proxy02 etc. Would you mind giving a try to this branch and confirm?

My question is: where do we need to make the changes to load them from there? Currently it is still looking under /etc/salt/proxy.d/_schedule.conf.

I hope I understood your point, otherwise, please let me know!

@cro
Copy link
Contributor

cro commented Mar 9, 2017

Yes, I was reiterating your suggestion but didn't word it very well.

We would need to modify the actual configuration file parser to get it to look in subdirectories named after the proxy ids. That's kind of a risky change for a point release, but I will talk to the rest of the core team about it.

@mirceaulinic
Copy link
Contributor Author

Thanks for confirming!

While I understand the risk, I consider solving a pretty serious bug, unnoticed during several releases, more important. Basically all proxies have been sharing the same configuration, while minions are designed to have individual settings. So I believe we should fix this in a dot release though.

@cro
Copy link
Contributor

cro commented Mar 9, 2017

OK, I'm willing to do this work, let me see if I can get a PR together today before I have to leave.

@mirceaulinic
Copy link
Contributor Author

Fantastic! Thank you @cro!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants