Skip to content

tentacle: Revert "Merge pull request #66958 from Hezko/wip-74413-tentacle"#67750

Merged
batrick merged 1 commit intoceph:tentaclefrom
batrick:revert-pr66958
Mar 18, 2026
Merged

tentacle: Revert "Merge pull request #66958 from Hezko/wip-74413-tentacle"#67750
batrick merged 1 commit intoceph:tentaclefrom
batrick:revert-pr66958

Conversation

@batrick
Copy link
Member

@batrick batrick commented Mar 12, 2026

Backport 6dddf54 introduced a new connection feature bit NVMEOF_BEACON_DIFF but there are plans (#66624) to make further enhancements on that feature bit. This would cause the mons to crash during upgrades.

However, this connection feature bit should not have been added to begin with. The correct way to do this is extend e55ad7b by @athanatos to require CEPH_MON_FEATURE_INCOMPAT_NVMEOF_BEACON_DIFF if all mons support it. This should be done by having mons add/update their supported features the MonMap via an update from MMonJoin (see for instance crush_loc which was recently added to mon_info_t). Once the supported features indicated for each mon in the MonMap show they understand the new NVMEOF_BEACON_DIFF, then it should be turned on globally in the MonMap as a required feature (added to the incompat set).

Contribution Guidelines

  • To sign and title your commits, please refer to Submitting Patches to Ceph.

  • If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

  • When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

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

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

@batrick batrick requested review from athanatos and rzarzynski March 12, 2026 07:54
@github-actions github-actions bot added this to the tentacle milestone Mar 12, 2026
@batrick batrick changed the title Revert "Merge pull request #66958 from Hezko/wip-74413-tentacle" tentacle: Revert "Merge pull request #66958 from Hezko/wip-74413-tentacle" Mar 12, 2026
@batrick batrick modified the milestones: tentacle, v20.2.1 Mar 12, 2026
@rzarzynski
Copy link
Contributor

rzarzynski commented Mar 13, 2026

There is an FTBFS. Looks there is a hidden dependency of the reverted commits.

[923/2775] Building CXX object src/mon/CMakeFiles/mon.dir/NVMeofGwMon.cc.o
FAILED: src/mon/CMakeFiles/mon.dir/NVMeofGwMon.cc.o 
/usr/local/bin/sccache /usr/bin/clang++-19 -DBOOST_ASIO_DISABLE_THREAD_KEYWORD_EXTENSION -DBOOST_ASIO_HAS_IO_URING -DBOOST_ASIO_NO_TS_EXECUTORS -DHAVE_CONFIG_H -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_REENTRANT -D_THREAD_SAFE -D__CEPH__ -D__STDC_FORMAT_MACROS -D__linux__ -I/ceph/build/src/include -I/ceph/src -isystem /opt/ceph/include -isystem /ceph/build/include -isystem /ceph/src/jaegertracing/opentelemetry-cpp/api/include -isystem /ceph/src/jaegertracing/opentelemetry-cpp/exporters/jaeger/include -isystem /ceph/src/jaegertracing/opentelemetry-cpp/ext/include -isystem /ceph/src/jaegertracing/opentelemetry-cpp/sdk/include -isystem /ceph/src/xxHash -isystem /ceph/src/fmt/include -isystem /ceph/src/rocksdb/include -isystem /ceph/build/src/liburing/src/include -g -Werror -fPIC -fno-builtin-malloc -fno-builtin-calloc -fno-builtin-realloc -fno-builtin-free -Wall -fno-strict-aliasing -fsigned-char -Wtype-limits -Wignored-qualifiers -Wpointer-arith -Werror=format-security -Winit-self -Wno-unknown-pragmas -Wnon-virtual-dtor -Wno-ignored-qualifiers -ftemplate-depth-1024 -DBOOST_ALLOW_DEPRECATED_HEADERS -Wpessimizing-move -Wredundant-move -Wno-inconsistent-missing-override -Wno-mismatched-tags -Wno-unused-private-field -Wno-address-of-packed-member -Wno-unused-function -Wno-unused-local-typedef -Wno-varargs -Wno-gnu-designator -Wno-missing-braces -Wno-parentheses -Wno-deprecated-register -Wno-vla-cxx-extension -DCEPH_DEBUG_MUTEX -D_GLIBCXX_ASSERTIONS -fdiagnostics-color=auto -std=c++20 -MD -MT src/mon/CMakeFiles/mon.dir/NVMeofGwMon.cc.o -MF src/mon/CMakeFiles/mon.dir/NVMeofGwMon.cc.o.d -o src/mon/CMakeFiles/mon.dir/NVMeofGwMon.cc.o -c /ceph/src/mon/NVMeofGwMon.cc
/ceph/src/mon/NVMeofGwMon.cc:558:7: error: use of undeclared identifier 'get_gw_listeners'
  558 |       get_gw_listeners(f.get(), group_key); 
      |       ^
/ceph/src/mon/NVMeofGwMon.cc:570:19: error: out-of-line definition of 'get_gw_listeners' does not match any declaration in 'NVMeofGwMon'
  570 | void NVMeofGwMon::get_gw_listeners(Formatter *f, std::pair<std::string, std::string>& group_key){
      |                   ^

@yuriw
Copy link
Contributor

yuriw commented Mar 13, 2026

I couldn't build it https://tracker.ceph.com/issues/75477

DEFINE_CEPH_FEATURE(49, 2, SERVER_SQUID);
DEFINE_CEPH_FEATURE_RETIRED(50, 1, MON_METADATA, MIMIC, OCTOPUS)
DEFINE_CEPH_FEATURE(50, 2, SERVER_TENTACLE);
DEFINE_CEPH_FEATURE(51, 2, NVMEOF_BEACON_DIFF)
Copy link
Contributor

Choose a reason for hiding this comment

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

Manual checking confirms we haven't released this.

$ git fetch origin 
$ git tag --contains 3b5d28bfa51e3bf024ee0d66acd5cf6eec78eb52
testing/wip-jcollin-testing-20260212.143545-tentacle
testing/wip-jcollin-testing-20260212.151113-tentacle
testing/wip-jcollin-testing-20260213.113451-tentacle
testing/wip-jcollin-testing-20260213.155635-tentacle
testing/wip-jcollin-testing-20260303.104211-tentacle
testing/wip-jcollin-testing-20260305.104944-tentacle
$ git log origin/tentacle  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"
    nvmeofgw: beacon diff implementation in the monitor and in the MonClient.
$ git log v20.0.0  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"
$ git log v20.1.0  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"
$ git log v20.1.1  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"
$ git log v20.2.0  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"
$ git log v20.3.0  | grep "nvmeofgw: beacon diff implementation in the monitor and in the MonClient"

@yuriw: I think this PR is a strong dependency for any further Tentacle release.

@batrick
Copy link
Member Author

batrick commented Mar 15, 2026

This reverts commit 6dddf54, reversing
changes made to 07ec509.

Backport 6dddf54 introduced a new connection feature bit
NVMEOF_BEACON_DIFF but there are plans (ceph#66624) to make further
enhancements on that feature bit. This would cause the mons to crash
during upgrades.

However, this connection feature bit should not have been added to
begin with. The correct way to do this is extend
e55ad7b by @athanatos to require
`CEPH_MON_FEATURE_INCOMPAT_NVMEOF_BEACON_DIFF` if all mons support it.
This should be done by having mons add/update their supported features
the MonMap via an update from `MMonJoin` (see for instance `crush_loc`
which was recently added to `mon_info_t`). Once the supported features
indicated for each mon in the `MonMap` show they understand the new
NVMEOF_BEACON_DIFF, then it should be turned on globally in the
`MonMap` as a required feature (added to the incompat set).

Conflicts:
	src/mon/NVMeofGwMon.h: conflicts with header change from 19c9be2
                               fix missing header change in ceph#66584

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
Copy link
Member Author

@batrick batrick left a comment

Choose a reason for hiding this comment

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

Tested-by: Patrick Donnelly <pdonnell@redhat.com>

@batrick batrick merged commit 147f7c6 into ceph:tentacle Mar 18, 2026
12 checks passed
@batrick batrick deleted the revert-pr66958 branch March 18, 2026 17:41
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.

3 participants