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

mon: load mgr commands at runtime #16028

Merged
merged 12 commits into from Jul 21, 2017

Conversation

Projects
None yet
3 participants
@jcsp
Contributor

jcsp commented Jun 29, 2017

No description provided.

@jcsp jcsp added mgr mon labels Jun 29, 2017

@jcsp jcsp requested review from tchaikov and liewegas Jun 29, 2017

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 29, 2017

This turned out to be much simpler when just persisting the commands like sage suggested.

// feed our pet MgrClient
mon->mgr_client.ms_dispatch(new MMgrMap(map));
if (map.get_available() && !old_available) {

This comment has been minimized.

@liewegas

liewegas Jun 29, 2017

Member

I think this could miss an update if, say,

  • mon.b is quorum, active mgr.x
  • mon.b drops out of quorum
  • other mons transition to mgr.y
  • mon.b rejoins quorum

This comment has been minimized.

@jcsp

jcsp Jun 29, 2017

Contributor

You're right, I guess this needs to compare active gid too.

This comment has been minimized.

@jcsp

jcsp Jun 30, 2017

Contributor

Pushed an update for this.

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 29, 2017

Yay! Should we update vstart and docs do to 'ceph restful create-...' instead of 'ceph tell mgr restful ...'?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 30, 2017

there's a build error

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jun 30, 2017

Hmm, unit test failures I suspect are due to commands running before mgr is up. Previously we would block until mgr was available, but now we immediately get an EINVAL.

Could sidestep this by priming MgrMonitor::command_descs from MgrCommands.h, and loading the real command set as soon as we can. That would go back to python commands being slightly second-class citizens, in that they wouldn't be safe to run immediately on startup (but they never can be unless we have the CLI block on a missing command until a mgr is up)

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 30, 2017

That seems reasonable. It is not really that different than the race between 'ceph mgr module enable' and being able to use the command.

#pragma once
struct MonCommand {

This comment has been minimized.

@tchaikov

tchaikov Jul 3, 2017

Contributor

shall we remove the definition of MonCommand in Monitor.h

This comment has been minimized.

@jcsp

jcsp Jul 4, 2017

Contributor

It's already gone, I think?

This comment has been minimized.

@tchaikov

tchaikov Jul 5, 2017

Contributor

right ! it's gone in the latest patch.

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jul 4, 2017

Rebased

@liewegas liewegas modified the milestone: luminous Jul 6, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 12, 2017

sorry, needs another rebase!

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jul 14, 2017

rebased

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 17, 2017

needs rebase

@jcsp

This comment has been minimized.

Contributor

jcsp commented Jul 17, 2017

rebased

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 18, 2017

The following tests FAILED:
          3 - ceph_objectstore_tool.py (Failed)
          5 - cephtool-test-mon.sh (Failed)
         14 - run-tox-ceph-disk (Failed)
         60 - unittest_mclock_priority_queue (Not Run)
        100 - test-erasure-code-plugins.sh (Failed)
        101 - test-erasure-code.sh (Failed)
        102 - test-erasure-eio.sh (Failed)
        126 - mon-bind.sh (Failed)
        153 - osd-reuse-id.sh (Failed)
        154 - osd-scrub-repair.sh (Failed)
        155 - osd-scrub-snaps.sh (Failed)
        158 - osd-dup.sh (Failed)

from teh cephtool test,

5: /home/sage/src/ceph2/qa/workunits/cephtool/test.sh:598: test_auth_profiles:  ceph -n client.xx-profile-ro -k client.xx.keyring pg dump
5: Error EINVAL: command not known
@liewegas

This comment has been minimized.

Member

liewegas commented Jul 18, 2017

see jcsp#4

John Spray and others added some commits Jun 28, 2017

John Spray
encoding: remove encode_array_nohead
This was just a for loop.  No longer needed for
MonCommands, and the usage in memstore/PageSet
was just iterating over char* and should never have
been there to begin with.

Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
mgr: use MonCommand for command descriptions
...and update the MonCommand encoding so that we
can readily send vectors of them around.

Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
mon: load mgr commands dynamically
So that the list of commands includes python modules,
thus allowing python-provided commands to be invoked
by the CLI with out a `tell mgr` prefix.

Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
mgr: transmit command descriptions to mgr in activating beacon
The mgr already sends a beacon to the mon immediately
after loading python modules in Mgr::init, to indicate
that it is now available.  Use that beacon to transmit
the command descriptions.

The monitor should handle this beacon by persisting
the command descriptions before persisting the updated
mgrmap that indicates that the mgr is now active.

Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
qa/doc: update for "mgr tell" no longer needed
Signed-off-by: John Spray <john.spray@redhat.com>
mon/MonCommands: std::
Signed-off-by: Sage Weil <sage@redhat.com>

liewegas added some commits Jul 18, 2017

mgr: move mgr_commands to separate compilation unit
Signed-off-by: Sage Weil <sage@redhat.com>
mon/MgrMonitor: mark mgr commands with FLAG_MGR
Signed-off-by: Sage Weil <sage@redhat.com>
mon: define static mgr_commands at mkfs time
This closes a window between mkfs and when the first mgr goes active
where *no* mgr commands are defined, and things like 'pg dump' fail.  We
do not get the default set of commands defined by modules, but we get
everything else.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas

This comment has been minimized.

Member

liewegas commented Jul 19, 2017

retest this please

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 19, 2017

the osd-scrub-repair.sh failure seems real, i can reproduce it locally:

154: /home/sage/src/ceph2/src/test/osd/osd-scrub-repair.sh:1664: corrupt_scrub_erasure:  rados list-inconsistent-pg ecpool
154: error 22: (22) Invalid argument
154: /home/sage/src/ceph2/src/test/osd/osd-scrub-repair.sh:1664: corrupt_scrub_erasure:  return 1

not sure where it's coming from tho?

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 19, 2017

jcsp#5 shoudl fix it

liewegas added some commits Jul 19, 2017

mon: constify _get_moncommand
Signed-off-by: Sage Weil <sage@redhat.com>
mon: restore mgr command lookup so that we can still proxy to mgr
Since mgr commands aren't in the main mon_commands array now, we need to
explicitly look up commands there too.  This restores the behavior
implemented below in which we forward misdirected mgr commands to the mon.

Signed-off-by: Sage Weil <sage@redhat.com>
librados: 'pg ls' is not a mgr command
Note that this breaks the command *during* the mon upgrade from jewel ->
luminous, which is slightly annoying, but means we avoid proxying via the
mon after upgrade is complete, which is good and IMO more important.

In the future we may want librados to cache the command descriptions so
that commands can be directed automatically.

Signed-off-by: Sage Weil <sage@redhat.com>
@jcsp

This comment has been minimized.

Contributor

jcsp commented Jul 20, 2017

pulled in commits from jcsp#5

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Jul 21, 2017

the failed tests are related to health checking and monitor, and they are addressed by #16477. so i don't think they are relevant.

@tchaikov tchaikov merged commit 0193e38 into ceph:master Jul 21, 2017

4 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment