Skip to content

Revert "common/tracer: fix decoding when jaeger tracing is disabled"#51331

Merged
idryomov merged 1 commit intoceph:mainfrom
Matan-B:wip-matanb-revert-51043
May 3, 2023
Merged

Revert "common/tracer: fix decoding when jaeger tracing is disabled"#51331
idryomov merged 1 commit intoceph:mainfrom
Matan-B:wip-matanb-revert-51043

Conversation

@Matan-B
Copy link
Contributor

@Matan-B Matan-B commented May 3, 2023 •
edited
Loading

This reverts commit 3701ffa.

This commit currently breaks all centos8 builds (with !HAVE_JAEGER).

/home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3687-g9dd84573/rpm/el8/BUILD/ceph-18.0.0-3687-g9dd84573/src/common/tracer.h: In function ‘void tracing::encode(const jspan_context&, ceph::bufferlist&, uint64_t)’:
/home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3687-g9dd84573/rpm/el8/BUILD/ceph-18.0.0-3687-g9dd84573/src/common/tracer.h:105:3: error: ‘ENCODE_START’ was not declared in this scope
  105 |   ENCODE_START(1, 1, bl);
      |   ^~~~~~~~~~~~
/home/jenkins-build/build/workspace/ceph-dev-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3687-g9dd84573/rpm/el8/BUILD/ceph-18.0.0-3687-g9dd84573/src/common/tracer.h:108:10: error: invalid initialization of reference of type ‘const jspan_context&’ from expression of type ‘bool’
  108 |   encode(is_valid, bl);
      |          ^~~~~~~~

https://shaman.ceph.com/builds/ceph/wip-adk-testing-2023-05-02-1727/24d8b207c00b7180a761adae28d1821aba627d6e/crimson/341501/

#51043 was introduced because of:

message decoding failures after a recent change [1]
[1] https://github.com/ceph/ceph/pull/47457

#47457 was reverted here #51293 so we can also revert #51043.

Signed-off-by: Matan Breizman mbreizma@redhat.com

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@Matan-B
Revert "common/tracer: fix decoding when jaeger tracing is disabled"
fa02c68 
This reverts commit 3701ffa.

Signed-off-by: Matan Breizman <mbreizma@redhat.com>
@github-actions github-actions bot added the common label May 3, 2023
@Matan-B Matan-B requested a review from gregsfortytwo May 3, 2023 08:14
@Matan-B Matan-B mentioned this pull request May 3, 2023
Merged
14 tasks
@petrutlucian94
Copy link
Contributor

petrutlucian94 commented May 3, 2023

Makes sense, this one will have to be reverted as well. To be honest, I'm surprised that the Windows CI isn't failing, now that #include "include/encoding.h" was removed.

https://jenkins.ceph.com/view/Windows/job/ceph-windows-pull-requests/

@Matan-B
Copy link
Contributor Author

Matan-B commented May 3, 2023

@idryomov, Can we merge this revert without additional qa?

@idryomov
Copy link
Contributor

idryomov commented May 3, 2023

To be honest, I'm surprised that the Windows CI isn't failing, now that #include "include/encoding.h" was removed.

It's likely included somewhere else. The build fails in RGW -- totally out of reach of Windows CI:

[ 86%] Building CXX object src/rgw/CMakeFiles/rgw_common.dir/rgw_tracer.cc.o
In file included from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/rgw/rgw_tracer.h:4,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/rgw/rgw_tracer.cc:5:
/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/common/tracer.h: In function ‘void tracing::encode(const jspan_context&, ceph::bufferlist&, uint64_t)’:
/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/common/tracer.h:105:3: error: ‘ENCODE_START’ was not declared in this scope
  105 |   ENCODE_START(1, 1, bl);
      |   ^~~~~~~~~~~~
/home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/common/tracer.h:105:3: note: the macro ‘ENCODE_START’ had not yet been defined
In file included from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/include/uuid.h:9,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/include/types.h:21,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/common/ceph_crypto.h:10,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/rgw/rgw_common.h:26,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/rgw/rgw_tracer.h:6,
                 from /home/jenkins-build/build/workspace/ceph-dev-new-build/ARCH/x86_64/AVAILABLE_ARCH/x86_64/AVAILABLE_DIST/centos8/DIST/centos8/MACHINE_SIZE/gigantic/release/18.0.0-3733-g24d8b207/rpm/el8/BUILD/ceph-18.0.0-3733-g24d8b207/src/rgw/rgw_tracer.cc:5:

@idryomov
Copy link
Contributor

idryomov commented May 3, 2023

@idryomov, Can we merge this revert without additional qa?

Yup, I'd say so!

@idryomov idryomov merged commit ac7bba5 into ceph:main May 3, 2023
@petrutlucian94 petrutlucian94 mentioned this pull request Feb 17, 2024
Merged
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@idryomov idryomov idryomov approved these changes

@petrutlucian94 petrutlucian94 petrutlucian94 approved these changes

@yuvalif yuvalif Awaiting requested review from yuvalif

@alimaredia alimaredia Awaiting requested review from alimaredia

@gregsfortytwo gregsfortytwo Awaiting requested review from gregsfortytwo

Assignees

No one assigned

Labels

common

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Matan-B @petrutlucian94 @idryomov