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

rgw, mon: normalize delete/remove in admin console (cleanup 22444) #19439

Merged
merged 3 commits into from Mar 26, 2018

Conversation

@chardan
Copy link
Contributor

commented Dec 11, 2017

Makes "delete", "del", "remove", and "rm" synonyms.

http://tracker.ceph.com/issues/22444
See also: http://tracker.ceph.com/issues/14363

Cleanup: 22444
Signed-off-by: Jesse Williamson jwilliamson@suse.de

@chardan chardan self-assigned this Dec 11, 2017

@chardan chardan requested a review from dmick Dec 11, 2017

@liewegas liewegas requested a review from yehudasa Dec 12, 2017

@yehudasa
Copy link
Member

left a comment

lgtm, we may want to modify the usage dump also to be more consistent

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch from c23af09 to c17159e Dec 12, 2017

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 13, 2017

@yehudasa That seems like a good idea. Do you know if there is a preference between delete/del and remove/rm that we should advertise? Or should there be a documentation note saying "these mean the same thing"?

@dmick

This comment has been minimized.

Copy link
Member

commented Dec 13, 2017

btw, the bug (as I noted there) was about the ceph CLI, not the rgw admin console; you should probably file a new different issue for this fix

@chardan chardan changed the title rgw: normalize delete/remove in admin console (cleanup 14363) rgw: normalize delete/remove in admin console (cleanup 22444) Dec 14, 2017

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch 2 times, most recently from 05374c6 to fe6bb18 Dec 14, 2017

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 14, 2017

@dmick Noted, and I see the confusion. I'll have you covered and am starting in on addressing your original concerns! Meanwhile, new tracker URL:
http://tracker.ceph.com/issues/22444

Next question: is there a GitHub option somewhere to change the name of this branch, but keep the discussion?

@dmick

This comment has been minimized.

Copy link
Member

commented Dec 14, 2017

Nothing I know of to rename branches on GH, but that doesn't mean it's not there somewhere. I suspect not, because (of course) that would make your local copy disconnected from this branch.

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

Yeah, that's what I was hoping to avoid. I'm ok with either way of doing this:

  1. since everything seems to look ok here, let it go in with the slightly misleading branch name;
  2. I can close this PR, post again with the right ticket number in the new branch, but lose the history.
    Anyone have feelings either way?
@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

@yehudasa I went ahead with "rm" as that appeared to be the most common name.

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch 5 times, most recently from aff9cca to 66c59f4 Dec 18, 2017

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

jenkins test docs

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2017

@yehudasa Looking ok? What do I need to do to get the docs check to pass? :-)

@cbodley

This comment has been minimized.

Copy link
Contributor

commented Dec 19, 2017

jenkins test docs

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2017

Jenkins test docs

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

Hmm... so, what needs to happen here to get the doc check to pass? :-)

@liewegas

This comment has been minimized.

Copy link
Member

commented Dec 29, 2017

@yehudasa

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

@chardan lgtm, re docs, maybe @alfredodeza knows

@theanalyst

This comment has been minimized.

Copy link
Member

commented Jan 2, 2018

jenkins test docs

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Thanks! I think I was a bit confused about the goal of the change, I'll make revisions.

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch from 73b6099 to 6b06493 Feb 21, 2018

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

I think I see my confusion... the original bug:
http://tracker.ceph.com/issues/22444
...requests that there should be "less annoying" behavior that as I read it suggests making synonyms.
The other territory that I've mixed in here deals with deprecating old commands not of the "rm" form.
I've gone ahead and restored the "rm" commands in OSDMonitor.cc.

So basically, the original ticket is NaB? And not something we actually /want/ to do?

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 21, 2018

Jenkins retest this please.

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch 5 times, most recently from e4f0e6d to dedda8c Feb 23, 2018

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Feb 28, 2018

@liewegas Should be normalized now.

@theanalyst

This comment has been minimized.

Copy link
Member

commented Feb 28, 2018

Can you add a PendingReleaseNotes entry so that we keep a track in release announcements

@liewegas
Copy link
Member

left a comment

looks okay to me. @yehudasa are the radosgw-admin changes okay?

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch from dedda8c to cbfbd39 Mar 6, 2018

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

@theanalyst Updated, thanks for pointing that out!

@chardan chardan force-pushed the chardan:jfw-wip-14363-normalize-rm branch from cbfbd39 to b658941 Mar 6, 2018

Jesse Williamson added some commits Dec 8, 2017

Jesse Williamson
rgw: normalize delete/remove in admin console
Normalizes "rm", deprecates any other (eg. "delete", del",
or "remove"). Provides backward compatibility with
existing commands, deprecates non-"rm".

http://tracker.ceph.com/issues/14363

Cleanup: http://tracker.ceph.com/issues/22444
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Jesse Williamson
mon: update MonCommands
Regardless of the "inner" implementation, at this level we officially
make all remove commands use "rm" and not "remove", "del", or "delete".

Deprecates "remove", "del", and "delete".
Adjusts code so that "rm" preceeds "remove".

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Jesse Williamson
restore original OSDMonitor.cc
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2018

How are things looking to everyone? Any further thoughts? Thanks to everyone for the constructive comments so far!

@jecluis
Copy link
Member

left a comment

lgtm

@@ -348,10 +349,10 @@ COMMAND("mds compat rm_incompat " \
COMMAND_WITH_FLAG("mds add_data_pool " \
"name=pool,type=CephString", \
"add data pool <pool>", "mds", "rw", "cli,rest", FLAG(OBSOLETE))
COMMAND_WITH_FLAG("mds remove_data_pool " \
COMMAND_WITH_FLAG("mds rm_data_pool " \

This comment has been minimized.

Copy link
@jecluis

jecluis Mar 14, 2018

Member

this sequence of removal/addition seems weird. Am I reading this wrong, or are we effectively removing and adding lines that are essentially the same, only at different points of the code?

This comment has been minimized.

Copy link
@chardan

chardan Mar 15, 2018

Author Contributor

The reason that one looks a little funny is that I tried to make things show up in a consistent order, with "rm" being first before the now-deprecated "remove". The intent, though, is that both commands should be supported, and the "remove" (or "delete" or "del") forms be flagged deprecated.

@chardan chardan added the needs-qa label Mar 23, 2018

@chardan

This comment has been minimized.

Copy link
Contributor Author

commented Mar 23, 2018

How is everyone feeling about this?

@liewegas

This comment has been minimized.

Copy link
Member

commented Mar 23, 2018

👍

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 23, 2018

This underwent a rados run at http://pulpito.ceph.com/smithfarm-2018-03-23_16:16:12-rados-wip-smithfarm-testing-distro-basic-smithi/ (with 2 unrelated failures and 1 job still running after 5 hours)

I guess it also needs an rgw suite for the radosgw-admin changes.

@smithfarm smithfarm requested a review from cbodley Mar 26, 2018

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

The rgw suite ran here: http://pulpito.ceph.com/smithfarm-2018-03-25_08:09:24-rgw-wip-smithfarm-testing-distro-basic-smithi/

It has 9 failed and 1 dead job out of a total of 147. That failure rate seems a bit high to me, but I'm not sure what's expected currently from an rgw suite on master.

@chardan Maybe you could go through those failed and dead jobs and check if any seem like they might be related to the changes in this PR?

@cbodley
Copy link
Contributor

left a comment

thanks @smithfarm, that looks like a normal rgw suite run. the s3a and ragweed tests are broken and being worked on

@smithfarm smithfarm merged commit b71eb01 into ceph:master Mar 26, 2018

5 checks passed

Docs: build check OK - docs built
Details
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
You can’t perform that action at this time.