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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions lib/puppet/type/yumrepo.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -427,4 +427,22 @@
newvalues(/.*/, :absent)
sensitive true
end

newproperty(:report_instanceid) do
desc "Determines if yum reports an AWS instanceid and region as a URL parameter when downloading packages.
#{YUM_BOOLEAN_DOC}
#{ABSENT_DOC}"

newvalues(YUM_BOOLEAN, :absent)
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!

desc "Determines if the fastestmirror plugin is enabled for this repository.
#{YUM_BOOLEAN_DOC}
#{ABSENT_DOC}"

newvalues(YUM_BOOLEAN, :absent)
munge(&munge_yum_bool)
end
end
11 changes: 10 additions & 1 deletion spec/unit/type/yumrepo_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,6 @@ def self.instances; []; end
it_behaves_like "a yumrepo parameter that can be absent", :assumeyes
end


describe "cost" do
it_behaves_like "a yumrepo parameter that can be absent", :cost
it_behaves_like "a yumrepo parameter that can be an integer", :cost
Expand Down Expand Up @@ -472,5 +471,15 @@ def self.instances; []; end
expect(sync_event.message).to eq 'changed [redacted] to [redacted]'
end
end

describe "report_instanceid" do
it_behaves_like "a yumrepo parameter that expects a boolean parameter", :report_instanceid
it_behaves_like "a yumrepo parameter that can be absent", :report_instanceid
end

describe "fastestmirror_enabled" do
it_behaves_like "a yumrepo parameter that expects a boolean parameter", :fastestmirror_enabled
it_behaves_like "a yumrepo parameter that can be absent", :fastestmirror_enabled
end
end
end