Skip to content

Better counting v2 #807

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

Closed
wants to merge 20 commits into from
Closed

Conversation

dmnks
Copy link
Contributor

@dmnks dmnks commented Oct 3, 2019

Changelog:

  • Fix repeated countme flags in case of repo failures
  • Add support for PackageKit/microdnf code paths
  • Add support for longevity "buckets" sent with the countme flag
  • Drop libdnf version string from User-Agent for privacy reasons
  • Other minor fixes

Depends on: rpm-software-management/librepo#171
Covered by: rpm-software-management/ci-dnf-stack#640

@dmnks dmnks force-pushed the countme-v2 branch 8 times, most recently from 2541dcb to 9ddaa52 Compare October 6, 2019 17:07
@dmnks dmnks force-pushed the countme-v2 branch 3 times, most recently from eed25d5 to 4cf8eef Compare October 7, 2019 15:02
dmnks added a commit to dmnks/dnf that referenced this pull request Oct 7, 2019
This reflects the following changes in libdnf:

rpm-software-management/libdnf#807
dmnks added a commit to dmnks/ci-dnf-stack that referenced this pull request Oct 7, 2019
There's no need to call "makecache" beforehand anymore, see:
rpm-software-management/libdnf#807
@dmnks dmnks force-pushed the countme-v2 branch 2 times, most recently from 9abbe72 to 87d94f7 Compare October 8, 2019 07:57
@dmnks dmnks force-pushed the countme-v2 branch 2 times, most recently from dd5d4db to efc5694 Compare October 8, 2019 12:01
@packit-as-a-service
Copy link

There was an error while running a copr build: (0)
Reason: [Errno 113] No route to host

You can re-trigger copr build by adding a comment (/packit copr-build) into this pull request.

@dmnks dmnks force-pushed the countme-v2 branch 2 times, most recently from 11715f2 to 46bc0d0 Compare October 8, 2019 12:37
@dmnks
Copy link
Contributor Author

dmnks commented Oct 17, 2019

@jrohel Thanks for the feedback, I fixed the issues (as additional/fixup commits) and you can proceed now with the review (from commit bcf398f)

@jrohel
Copy link
Contributor

jrohel commented Oct 17, 2019

@rh-atomic-bot r+

@rh-atomic-bot
Copy link

📌 Commit 2e68870 has been approved by jrohel

@rh-atomic-bot
Copy link

⌛ Testing commit 2e68870 with merge 2b46699...

rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
This commit decouples the "countme" logic from the implementation
details of how librepo fetches files, metalinks in particular, to avoid
accidental overcount.

What we really need here is a way to address *individual* HTTP requests
(wrapped by CURL handles).  LrHandle just doesn't provide us with the
necessary resolution to accomplish that.  The reason is that librepo may
end up producing multiple HTTP requests without us being able to control
them, such as when it activates its own retry mechanism.  Since we
currently pass the final URL with the flag to our LrHandle, the flag
would also be repeated.  That would fool the server into thinking it got
multiple machines checking in, and skew the numbers.

Simply put, in the interest of accuracy, undercount is better than
overcount.

To achieve that, we should just let librepo take care of the details of
figuring out how to make the flag appear in one single HTTP request it
makes with the given LrHandle.  And that's exactly what the new
LRO_ONETIMEFLAG feature is for, so let's use it!

In this commit, we also move addCountmeFlag() to the top-level
lrHandlePerform() method so that it works whenever fetching metadata,
not just when running isMetalinkInSync().

Note: LRO_ONETIMEFLAG also applies to LRO_MIRRORLISTURL if configured.

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
This commit enables the countme feature for the PackageKit/microdnf code
path.  Note that this will no longer be needed once we finish the libdnf
consolidation process and both PK/microdnf adopt the new libdnf API.

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
Don't rely on the existence of any keys in the passed OS data when
constructing a User-Agent.  This involves a bit of refactoring.

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
In case of the User-Agent, there's no point in interrupting the program
flow if no os-release data is available or is invalid (in which case we
return the fallback value).

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
This makes sure both PackageKit and microdnf send the new User-Agent
string.

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
The choice of individual buckets is based on the following suggestion by
Matthew Miller:

https://bugzilla.redhat.com/show_bug.cgi?id=1672504#c13

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
There's a concern that the version number leaks too much information
about the system "patch-level", and is not useful for anything really,
so let's just remove it.

Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
Closes: #807
Approved by: jrohel
rh-atomic-bot pushed a commit that referenced this pull request Oct 17, 2019
Closes: #807
Approved by: jrohel
@rh-atomic-bot
Copy link

☀️ Test successful - status-papr
Approved by: jrohel
Pushing 2b46699 to master...

lukash pushed a commit to rpm-software-management/ci-dnf-stack that referenced this pull request Oct 18, 2019
There's no need to call "makecache" beforehand anymore, see:
rpm-software-management/libdnf#807
lukash pushed a commit to rpm-software-management/ci-dnf-stack that referenced this pull request Oct 18, 2019
The version is no longer included in the User-Agent field, see:
rpm-software-management/libdnf#807

Also, we can now remove the @xfail tags.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants