Looks good.
Do we want migration-warnings=-[] in the success case, or just omit it?
I don't have a strong preference either way.
I have changed it so this will not be displayed.
Seeing all the details in the failure case made me nervous about RBAC.
But I think the key there is in the read of the subsystem that goes to
produce the ops. We should test that that fails if the caller is not
able to address some child resource or if the caller is not able to read
some attribute value.
Should this hold up the PR, or is this a task for later?
Stuart
If we know the caller can read the entire subsystem then it is safe to
provide details of any failure on the writes, since it is the same data.
Once everything is clear we should work out a way to have shared code so
this stuff has a higher guarantee of consistency across subsystems.
On 8/14/15 1:32 AM, Stuart Douglas wrote:
> I have a PR that builds on Brian's work for the web migrate op:
> https://github.com/wildfly/wildfly/pull/7931
>
> Output is:
>
> Standard EAP config (no warnings or errors):
>
> [standalone@localhost:9999 /] /subsystem=web:migrate
> {
> "outcome" => "success",
> "result" => {"migration-warnings" => []}
> }
>
> Success with a warning (in this case due to an invalid protocol
> attribute on a connector):
>
> [standalone@localhost:9999 /] /subsystem=web:migrate
> {
> "outcome" => "success",
> "result" => {"migration-warnings" => ["WFLYWEB0002: Could not
> migrate resource {
> \"protocol\" => \"INVALID\",
> \"scheme\" => \"http\",
> \"socket-binding\" => \"ajp\",
> \"enable-lookups\" => undefined,
> \"proxy-binding\" => undefined,
> \"proxy-name\" => undefined,
> \"proxy-port\" => undefined,
> \"redirect-binding\" => undefined,
> \"redirect-port\" => undefined,
> \"secure\" => undefined,
> \"max-post-size\" => undefined,
> \"max-save-post-size\" => undefined,
> \"enabled\" => undefined,
> \"executor\" => undefined,
> \"max-connections\" => undefined,
> \"virtual-server\" => undefined,
> \"name\" => \"ajp\",
> \"operation\" => \"add\",
> \"address\" => [
> (\"subsystem\" => \"web\"),
> (\"connector\" => \"ajp\")
> ]
> }"]}
> }
>
> Failed OP:
>
> [standalone@localhost:9999 /] /subsystem=web:migrate
> {
> "outcome" => "failed",
> "result" => {
> "migration-warnings" => [],
> "migration-error" => {
> "operation" => {
> "operation" => "add",
> "address" => [
> ("subsystem" => "undertow"),
> ("server" => "default-server"),
> ("ajp-listener" => "ajp")
> ],
> "socket-binding" => "ajp",
> "secure" => undefined,
> "redirect-socket" => undefined,
> "enabled" => undefined,
> "resolve-peer-address" => undefined,
> "max-post-size" => undefined,
> "max-connections" => "sadfadsfasdgasdfg",
> "operation-headers" => {
> "caller-type" => "user",
> "access-mechanism" => "NATIVE"
> },
> "worker" => undefined,
> "buffer-pool" => undefined,
> "disallowed-methods" => undefined,
> "max-header-size" => undefined,
> "buffer-pipelined-data" => undefined,
> "max-parameters" => undefined,
> "max-headers" => undefined,
> "max-cookies" => undefined,
> "allow-encoded-slash" => undefined,
> "decode-url" => undefined,
> "url-charset" => undefined,
> "always-set-keep-alive" => undefined,
> "max-buffered-request-size" => undefined,
> "record-request-start-time" => undefined,
> "allow-equals-in-cookie-value" => undefined,
> "no-request-timeout" => undefined,
> "request-parse-timeout" => undefined,
> "tcp-backlog" => undefined,
> "receive-buffer" => undefined,
> "send-buffer" => undefined,
> "tcp-keep-alive" => undefined,
> "read-timeout" => undefined,
> "write-timeout" => undefined
> },
> "result" => {
> "outcome" => "failed",
> "failure-description" => "WFLYCTL0097: Wrong type for
> max-connections. Expected [INT] but was STRING",
> "rolled-back" => true
> }
> }
> },
> "rolled-back" => true
> }
>
> Stuart
>
>
>
>
> On Fri, 14 Aug 2015 at 11:03 Brian Stansberry
> <brian.stansberry@redhat.com <mailto:brian.stansberry@redhat.com>> wrote:
>
> On 8/13/15 10:03 AM, Brian Stansberry wrote:
> > On 8/13/15 3:11 AM, Stuart Douglas wrote:
> >>
> >>
> >> On Thu, 13 Aug 2015 at 16:27 Ladislav Thon <lthon@redhat.com
> <mailto:lthon@redhat.com>
> >> <mailto:lthon@redhat.com <mailto:lthon@redhat.com>>> wrote:
> >>
> >>
> >> > The approach I took was to add the warnings to the
> describe-migration
> >> > op, and when running the migrate op make the
> describe-migration
> >> > operation the first step in the composite, so the output
> looks like
> >> > this: http://pastebin.com/B01KNAHX
> >>
> >> The fact that it's a composite operation makes the output a
> bit cryptic
> >> (step-X?), but otherwise I think it's actually good.
> >>
> >> I guess that only handling the warnings in
> :describe-migration makes the
> >> implementation easier (not sure?) and printing
> :describe-migration as a
> >> part of :migrate makes sense too, precisely because it
> shows the steps
> >> that are performed.
> >>
> >> So my only objection would be those cryptic "step-X" names,
> but I guess
> >> that we can live with that (if all :migrate operations took
> the same
> >> approach; inconsistency wouldn't really help here).
> >>
> >>
> >> That is because this is implemented as a composite operation, AFAIK
> >> there is lots of special handing for composite ops, so I don't
> think we
> >> want to be implementing our own one with different output just
> for this
> >> case.
> >>
> >> I could be wrong though, this is not really my area, Brian will
> know more.
> >>
> >
> > In the success case, I don't see the point of outputting "step-2" =>
> > {"outcome" => "success"}, "step-3" => {"outcome" => "success"}, etc.
> >
> > In the failure case though, having an intelligible
> failure-description
> > is important. And the composite op handler may be getting in your
> way there
> >
> > Instead of writing more text, I'll write some code to illustrate some
> > thoughts and post back later.
> >
>
> Here's what I was thinking.
>
> [1] is a change to break out the step registration stuff
> CompositeOperationHandler does into a utility method, with a couple
> other variants added that are better suited for use by other OSHs.
>
> Basically, it lets you pass in a Map of ops and a holder map of response
> nodes, and it will add the steps and populate the response node map.
> (You can optionally pre-populate the response node map. There's also a
> variant that uses lists instead of maps in case that's more convenient.)
>
> Using that, the response nodes for the added ops don't have to directly
> appear in the overall operation response. Instead the migrate op handler
> can hold a ref to them and in its ResultHandler use them to format an
> understandable response.
>
> The top two commits in [2] are an example tweak to the current
> WebMigrationOp in master to illustrate use of this.
>
> [1] https://github.com/wildfly/wildfly-core/pull/956
>
> [2] https://github.com/bstansberry/wildfly/tree/help-migrate-multistep
>
> >> Stuart
> >>
> >>
> >> LT
> >> _______________________________________________
> >> wildfly-dev mailing list
> >> wildfly-dev@lists.jboss.org <mailto:wildfly-dev@lists.jboss.org>
> <mailto:wildfly-dev@lists.jboss.org
> <mailto:wildfly-dev@lists.jboss.org>>
> >> https://lists.jboss.org/mailman/listinfo/wildfly-dev
> >>
> >>
> >>
> >> _______________________________________________
> >> wildfly-dev mailing list
> >> wildfly-dev@lists.jboss.org <mailto:wildfly-dev@lists.jboss.org>
> >> https://lists.jboss.org/mailman/listinfo/wildfly-dev
> >>
> >
> >
>
>
> --
> Brian Stansberry
> Senior Principal Software Engineer
> JBoss by Red Hat
> _______________________________________________
> wildfly-dev mailing list
> wildfly-dev@lists.jboss.org <mailto:wildfly-dev@lists.jboss.org>
> https://lists.jboss.org/mailman/listinfo/wildfly-dev
>
--
Brian Stansberry
Senior Principal Software Engineer
JBoss by Red Hat