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

jewel: rgw: curl* reuse and for debian, use openssl not gnutls. (jewel) #20623

Closed

Conversation

mdw-at-linuxbox
Copy link
Contributor

3 commits here; two are important, one is a minor build fix
0853bb0 rgw: reuse CURL* for keystone.
e550dfb build/link against curl w/ openssl not nss or gnutls. (debian)
b114d31 Fix tag handling for ceph_versioning.

0853bb0 rgw: reuse CURL* for keystone.
actual code change - queue to store recently used CURL*, thread to delete idle CURL*.
e550dfb build/link against curl w/ openssl not nss or gnutls. (debian)
this is just a configuration change; it's debian/ubuntu specific. centos doesn't have such a package (yet); I will do something for that too but almost certainly separately.
In order to build this I also wanted this fix,...
b114d31 Fix tag handling for ceph_versioning.

This is all for jewel; I'll post something shortly for master as well.

@tchaikov tchaikov added this to the jewel milestone Feb 28, 2018
@mdw-at-linuxbox mdw-at-linuxbox changed the title Wip jewel rgw openssl rgw: curl* reuse and for debian, use openssl not gnutls. (jewel) Feb 28, 2018
@ktdreyer
Copy link
Member

ktdreyer commented Mar 1, 2018

git tag has a "match" option. Wherever that runs, it should be matching against v* tags only

Copy link
Contributor

@smithfarm smithfarm left a comment

Choose a reason for hiding this comment

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

The PR summary says "In order to build this I also wanted this fix,... b114d31 Fix tag handling for ceph_versioning."

Can you give more details on (a) why this fix is needed and (b) how it relates to this PR?

I would prefer not to change jewel versioning this late in its release cycle.

Copy link
Contributor

@smithfarm smithfarm left a comment

Choose a reason for hiding this comment

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

I just read the commit message for b114d31 and clearly it's not compatible with our policy of making only minimal changes.

(I agree the versioning based on git tags is fragile. As @ktdreyer noted this has been mitigated in master but for jewel I don't want to touch it.)

@smithfarm smithfarm assigned smithfarm and unassigned mattbenjamin and cbodley Mar 2, 2018
@smithfarm smithfarm changed the title rgw: curl* reuse and for debian, use openssl not gnutls. (jewel) jewel: rgw: curl* reuse and for debian, use openssl not gnutls. (jewel) Mar 4, 2018
@smithfarm
Copy link
Contributor

smithfarm commented Mar 5, 2018

Maybe a little background is in order. A former dev lead (Sam Just) gave us the following principles for reviewing backport PRs:

Each backport PR (or backport tracker issue) should state:

  1. what bug is fixed
  2. why this fix is the minimal way to do it
  3. why does this need to be fixed in [release]

For more info, see: http://tracker.ceph.com/projects/ceph-releases/wiki/HOWTO_review_backport_PRs

N.B. there is a difference between releases that are early in their lifecycle, like luminous, and those that are mature, like jewel. As the release gets more advanced in age, we tend to get more and more serious about enforcing this.

@mattbenjamin
Copy link
Contributor

@smithfarm the changes are definitely defensible; I'm happy to take them only to our downstream if that's upstream's preference--discussion can happen here and in our standup meetings.

@smithfarm
Copy link
Contributor

@mattbenjamin To be clear, I'm only objecting to this change: b114d31

(We made it to 10.2.10 without touching this, and it's not related to rgw . . . why take the risk?)

@smithfarm
Copy link
Contributor

smithfarm commented Mar 5, 2018

Don't get me wrong, here, please - I was bitten by the same issue in our downstream (there's an easy workaround - just make sure the last tag is a version tag in the format "vX.Y.Z"). I would have welcomed the fix early in the jewel release cycle (though not as part of an rgw PR).

@smithfarm
Copy link
Contributor

So, #20635 has been merged. @mdw-at-linuxbox Could you redo this PR using git cherry-pick -x from the master PR?

@smithfarm
Copy link
Contributor

Also note that @theanalyst included this in his luminous backport PR here: #20722

Maybe the jewel backport could be done in a similar fashion (i.e. together with other related PRs, instead of separately). It's always easier that way, because it prevents merge conflicts when preparing the integration branch.

@theanalyst
Copy link
Member

theanalyst commented Mar 6, 2018

I included these changes in #20749 + my openssl locking fixes in master + the fix in radosgw-admin init

curl + gnutls has some big significant performance hits at least
when doing keystone validation.  nss has long-term memory growth
issues.

Signed-off-by: Marcus Watts <mwatts@redhat.com>
(cherry picked from commit cb12e1a)
When using keystone with https (the recommended setting),
it is inefficient to start up a new SSL connection for
each and every operation.  Keeping a CURL* structure around
should reduce the cost of doing this.  This logic tries
to do so, but it also tries to free them fairly aggressively
(5-10 seconds).  This should still greatly reduce load on
keystone at peak times while not tying up excess resources.

Signed-off-by: Marcus Watts <mwatts@redhat.com>
(cherry picked from commit 0c5cee1)
@mdw-at-linuxbox
Copy link
Contributor Author

I have pushed new commits here,
b114d31 dropped. I will be encouraging the folks with the bad tags to do something about it.
e550dfb added cherry pick line.
0853bb0 added cherry pick line.

@smithfarm
Copy link
Contributor

jenkins re-test this please

@smithfarm
Copy link
Contributor

timeout in unittest_throttle

jenkins re-test this please

Copy link
Contributor

@cbodley cbodley left a comment

Choose a reason for hiding this comment

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

on master, this introduced a regression in radosgw-admin (http://tracker.ceph.com/issues/23203), so the backport needs to include the fix for that as well. @theanalyst included that in #20749, so let's review/test that pr instead

@smithfarm
Copy link
Contributor

Closing in favor of #20749

@smithfarm smithfarm closed this Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants