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

(PUP-8730) Yumrepo Parameters: report_instanceid and fastestmirror_enabled #6834

Conversation

Zordrak
Copy link

@Zordrak Zordrak commented May 15, 2018

Update the Yumrepo type to include support for the parameters report_instanceid and fastestmirror_enabled.
Both parameters are defined by default on Amazon Linux AMIs and docker images.
report_instanceid is a valid requirement.
fastestmirror_enabled is defined in Amazon Linux, and supported by Chef already, but I am not convinced it does anything. I haven't yet found any applicable yum or fastestmirror plugin source code that consumes this parameter.

https://tickets.puppetlabs.com/browse/PUP-8730

@puppetlabs-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

…abled

Update the Yumrepo type to include support for the parameters report_instanceid and fastestmirror_enabled.
Both parameters are defined by default on Amazon Linux AMIs and docker images.
report_instanceid is a valid requirement.
fastestmirror_enabled is defined in Amazon Linux, and supported by Chef already, but I am not convinced it does anything. I haven't yet found any applicable yum or fastestmirror plugin source code that consumes this parameter.
@Zordrak Zordrak force-pushed the fix/master/yum-parameters-instanceid-fastestmirror branch from 7d47a4c to c61e623 Compare May 15, 2018 13:58
@melissa melissa self-assigned this May 16, 2018
@melissa
Copy link
Contributor

melissa commented May 23, 2018

Hello! Sorry for the delay on this. I have a little bit more testing before I can move this forward, but I wanted to make sure you know I haven't forgotten!

munge(&munge_yum_bool)
end

newproperty(:fastestmirror_enabled) do
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a setting that actually does anything, at least not that I can tell. I see that amazon linux adds this in their repo configs, but it looks like it doesn't actually impact how the fastestmirror plugin functions.

When installed, the fastestmirror plugin has a conf, a la below

$ cat /etc/yum/pluginconf.d/fastestmirror.conf
[main]
enabled=1
verbose=0
always_print_best_host = true
socket_timeout=3
#  Relative paths are relative to the cachedir (and so works for users as well
# as root).
hostfilepath=timedhosts.txt
maxhostfileage=10
maxthreads=15
#exclude=.gov, facebook
#include_only=.nl,.de,.uk,.ie

When this is enabled in the config, it looks like all calls to a yum repo will load this plugin, regardless of how fastestmirror_enabled is set for that particular repo. I can't find any other documentation that indicates exactly how to configure fastestmirror though. Is there some documentation you're working from that details fastestmirror_enabled other than the default amazon linux repo files and chef?

Copy link
Author

Choose a reason for hiding this comment

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

No, not at all. I am in fact reacting to a contribution to a puppetlabs-approved module that causes the module to fail because the module does the correct thing of replicating the upstream configuration in Amazon Linux (voxpupuli/puppet-yum#100).

I've done all I can to validate whether Amazon run a custom version of yum or some other reason why they might have this parameter, but I cannot find any. The correct solution here is probably to approach Amazon to request they remove the aberrant parameter from their default repository source configuration; however I wouldn't expect that to get much traction, especially with Amazon Linux v1 no longer getting full releases.

Part of the issue is also that this is effectively puppet trying to validate what are in fact arbitrary parameters. If someone writes their own custom yum module that accepts additional repo parameters, it is not possible for them to configure their OS to use that module's parameters using the puppet Yumrepo type.

While it is a valid question to ask as to whether yum will use this particular parameter or not, perhaps the implementation should be such that any user may define any parameter they wish as puppet may not have knowledge enough to validate all possible parameters. This could be done, for example, with a no-validate flag for existing parameters, or an additional hash of custom_parameters. This would allow a user to choose whether or not to add additional lines to the repo ini format as required.

I welcome other thoughts on the subject, but my initial approach was to presume that if Amazon upstream wish to define that setting as one of their defaults, then I would expect the default position of puppet to be to support that setting rather than question its validity, so long as yum functions correctly with the setting in place which it evidently does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm.

I'd much rather see this PR implementing something like your description ofcustom_parameters. I don't like the idea of presenting a parameters that may or may not actually do anything. It seems a lot more useful to allow users to specify their own parameters and not worry on our end if they're valid or not. That will answer this question for parameters like this in the future as well.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not personally in a position to deliver such a rewrite. I agree that the rewrite would be the best approach - but unless your team wishes to deliver such a change in the near future, it would seem pragmatic to continue to add desirable parameters until such time as a rewrite is prioritised.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's totally reasonable! I've opened https://tickets.puppetlabs.com/browse/PUP-8904 to add the custom_parameters hash. We'll prioritize it and hopefully get to it in the near future! In the meantime, I don't want to add new parameters just to deprecate them soon. As such, I'm going to close this pull request. Thank you for the work you put into this!

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

3 participants