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

jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger #13450

Merged
merged 3 commits into from
May 17, 2017
Merged

jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger #13450

merged 3 commits into from
May 17, 2017

Conversation

robbat2
Copy link
Contributor

@robbat2 robbat2 commented Feb 16, 2017 •
edited by ghost

@robbat2
Copy link
Contributor Author

robbat2 commented Feb 16, 2017

Backport of #13370 for Jewel

@tchaikov tchaikov added this to the jewel milestone Feb 16, 2017
@ghost ghost self-assigned this Feb 20, 2017
@ghost ghost changed the title msg/simple/Pipe: support IPv6 QoS. jewel: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger Feb 20, 2017
@smithfarm smithfarm changed the title jewel: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger Feb 22, 2017
@smithfarm
Copy link
Contributor

@robbat2 Could you please amend the commit message and add a "Conflicts:" section describing the cherry-pick conflict and how you resolved it? If it was really minor or trivial, you can just state "minor context difference" or "trivial resolution".

@Yan-waller
msg/simple: cleanups
d22beca 
should save the `errno` which may be changed by `ldout` and/or `<<` operator

Signed-off-by: Yan Jun <yan.jun8@zte.com.cn>
(cherry picked from commit 91a29bc)
Signed-off-by: Robin H. Johnson <robin.johnson@dreamhost.com>
@robbat2
Copy link
Contributor Author

robbat2 commented Feb 22, 2017

@smithfarm I looked more closely, and also backporting cleanup 91a29bc makes it apply with no conflict.

@smithfarm
Copy link
Contributor

Jenkins retest this please

@robbat2
Copy link
Contributor Author

robbat2 commented Mar 2, 2017

what happened to this?

@smithfarm
Copy link
Contributor

@robbat2 This backport is on-track. It will go through integration testing before it is merged.

@robbat2
Copy link
Contributor Author

robbat2 commented Mar 2, 2017

I meant that Jenkins hasn't commented here like usual.

@smithfarm
Copy link
Contributor

@robbat2 I see "All checks have passed" . . . ?

@robbat2
Copy link
Contributor Author

robbat2 commented Mar 2, 2017

Those weren't showing up yesterday, but they are there today, and Jenkins did used to make a comment about passing as well.

@smithfarm
Copy link
Contributor

smithfarm commented Apr 10, 2017 •
edited

@robbat2 I just noticed that the (cherry picked from . . .) line is missing in the second commit; please re-do with git cherry-pick -x

Thanks!

@smithfarm
Copy link
Contributor

Note: this PR is targeting the 10.2.8 release - not to be merged until v10.2.7 tag is added!

This PR was included in the rados run at http://tracker.ceph.com/issues/19538#note-4 which had the following results:

Of 227 total jobs:

  • 222 passed on first try
  • 3 passed on second try
  • remaining 2 are problematic

(see http://tracker.ceph.com/issues/19538#note-4 for details)

@smithfarm
Copy link
Contributor

marking DNM until the commit message is fixed

@smithfarm smithfarm changed the title jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger [DNM] jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger Apr 10, 2017
@smithfarm smithfarm changed the title [DNM] jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger jewel: msg: IPv6 Heartbeat packets are not marked with DSCP QoS - simple messenger Apr 12, 2017
@smithfarm
Copy link
Contributor

Removing DNM to include this in next integration testing round; still hoping @robbat2 will fix the commit message. . .

msg/simple/Pipe: support IPv6 QoS.
d311eea 
Extend DSCP marking for heartbeat packets to IPv6, as commit
9b9a682 only implemented
support for IPv4.

Conflicts: Cherry-picked 91a29bc from master to avoid conflict.
Backport: jewel, luminious
Fixes: http://tracker.ceph.com/issues/18887
Signed-off-by: Robin H. Johnson <robin.johnson@dreamhost.com>
(cherry picked from commit 2d6021f)
@robbat2
Copy link
Contributor Author

robbat2 commented Apr 12, 2017

Commit message fixed; I had it locally but it hadn't pushed out for some reason; Did amend & push again and it worked.

@smithfarm smithfarm mentioned this pull request Apr 13, 2017
Closed
@smithfarm
Copy link
Contributor

@robbat2 Do you have a jewel backport for #13418 as well?

@smithfarm smithfarm mentioned this pull request Apr 13, 2017
Closed
@smithfarm
Copy link
Contributor

Confirmed that git cherry-pick -x 91a29bc 2d6021f applies cleanly.

@smithfarm smithfarm requested a review from tchaikov April 25, 2017 05:17
@smithfarm
Copy link
Contributor

@tchaikov This passed another rados run at http://tracker.ceph.com/issues/19538#note-39

@smithfarm smithfarm requested a review from jdurgin April 25, 2017 05:57
@smithfarm
Copy link
Contributor

@jdurgin This has passed rados twice (see above). @robbat2 would like to get it into 10.2.8

tchaikov
tchaikov reviewed Apr 26, 2017
View reviewed changes
src/msg/simple/Pipe.cc Outdated

if (peer_addr.get_family() == AF_INET) {
r = ::setsockopt(sd, IPPROTO_IP, IP_TOS, &iptos, sizeof(iptos));
r = -errno;
Copy link
Contributor

Choose a reason for hiding this comment

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

this is buggy.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd recommend backport #14795 along with this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tchaikov Do you mean that the bugginess of that code can be fixed by cherry-picking the commits from #14795 into this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@smithfarm yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tchaikov #14795 touches src/msg/async/net_handler.cc, not this file (src/msg/simple/Pipe.cc), also requires backporting the set_priority prototype which otherwise isn't in jewel.

Copy link
Contributor

@tchaikov tchaikov May 4, 2017 •
edited

Choose a reason for hiding this comment

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

@robbat2, you can also fix the issue by assigning -errno to r only if r is less than 0 without backporting #14795 in a wholesale.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done now instead, since #14795 actually introduces the same change in async, and doesn't fix it in the simple.

@smithfarm
Copy link
Contributor

Passed another rados run at http://tracker.ceph.com/issues/19538#note-49

@smithfarm
Copy link
Contributor

@robbat2 Could you please cherry-pick the commit from #14795 into this PR

@robbat2
Copy link
Contributor Author

robbat2 commented May 3, 2017

@smithfarm done, with minor conflict fix in the method signature

@smithfarm
Copy link
Contributor

@robbat2 Could you move the conflict resolution description to under the "(cherry picked from ...)" line?

(By convention, the part above "(cherry picked from . . .)" is the commit message of the original (verbatim, without any edits) and the part below "(cherry picked from . . .)" is appended by the person who does the cherry-pick.)

Also, there's a compilation error:

msg/async/net_handler.cc:121:59: error: no ‘void ceph::NetHandler::set_priority(int, int, int)’ member function declared in class ‘ceph::NetHandler’
 void NetHandler::set_priority(int sd, int prio, int domain)
                                                           ^
Makefile:22991: recipe for target 'msg/async/net_handler.lo' failed

msg/simple/Pipe: manual backport of fix in PR#14795
4ceaa7c 
Manual backport of errno fixup from PR#14795
(6f1037e), as noted by
ceph#13450 (comment).

Signed-off-by: Robin H. Johnson <robin.johnson@dreamhost.com>
@robbat2
Copy link
Contributor Author

robbat2 commented May 4, 2017

Extra commit per #13450 (comment)

jdurgin
jdurgin approved these changes May 16, 2017
View reviewed changes
Copy link
Member

@jdurgin jdurgin left a comment

Choose a reason for hiding this comment

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

lgtm, sorry I missed this one before

@smithfarm smithfarm merged commit 966f222 into ceph:jewel May 17, 2017
asheplyakov pushed a commit to asheplyakov/ceph that referenced this pull request May 18, 2017
@smithfarm
Merge pull request ceph#13450: jewel: msg: IPv6 Heartbeat packets are…
572fb34 
… not marked with DSCP QoS - simple messenger

Reviewed-by: Nathan Cutler <ncutler@suse.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov left review comments

@smithfarm smithfarm smithfarm left review comments

@jdurgin jdurgin jdurgin approved these changes

Assignees
No one assigned
Labels
common feature
Projects
None yet
Milestone
jewel
6 participants
@robbat2 @smithfarm @tchaikov @jdurgin @liewegas @Yan-waller