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

[DRAFT] standalone default configs expressed with layers #15609

Closed
wants to merge 5 commits into from

Conversation

jfdenise
Copy link
Contributor

@jfdenise jfdenise commented Jun 3, 2022

This would allow to provision trimmed default configs.
Main use case being Bootable JAR for wildfly-ee-galleon-pack bare metal (standalone.xml) and cloud (standalone-ha.xml).

ISSUES

These issues are blocking the merge of this PR:

We have some subsystem included with no layers defined:

  • singleton WFLY-16453
  • modcluster: WFLY-16452
  • embedded broker: WFLY-13798

@github-actions github-actions bot added the deps-ok Dependencies have been checked, and there are no significant changes label Jun 3, 2022
@bstansberry
Copy link
Contributor

ISSUES

We have some subsystem included with no layers defined, should we have layers for those? :

  • singleton
  • modcluster

@rhusar @pferraro Yes, we need layers for these.

Subsystem extended that could need a galleon layer:

  • microprofile-opentracing-jaeger

@jasondlee @ehsavoie Let's discuss. I hope we don't need such a layer, as we already have two layers related to MP OT. (Can we get rid of one now?) I've forgotten the reasoning around how we use the microprofile-opentracing-jaeger feature group. I've noticed that standalone-microprofile.xml does not include the jaeger resource, while the other standalone.xml variants all do. It seems like one or the other of those is wrong.

WARNING: If we keep remote-naming layer, must add it to documentation. New dependency of ejb layer.

@ehsavoie I figure some messaging layers must need remote naming. Perhaps we're not detecting that in tests?

If we can just have higher level layers (in ejb and messaging and ???) that require remote naming declare use of the remote naming feature-spec that would be nice. Remote naming is kind of an implementation detail. In theory someone could want remote naming without remote ejb or remote messaging but that doesn't seem like a use case we should go out of our way to support. Such people could write their own layer easily enough and not even need to depend on a feature group we provide.

@rhusar
Copy link
Member

rhusar commented Jun 6, 2022

ISSUES
We have some subsystem included with no layers defined, should we have layers for those? :

  • singleton
  • modcluster

@rhusar @pferraro Yes, we need layers for these.

Agreed – I have created https://issues.redhat.com/browse/WFLY-16452 and https://issues.redhat.com/browse/WFLY-16453 for these.

@jfdenise
Copy link
Contributor Author

jfdenise commented Jun 7, 2022

@bstansberry , I have added the standalone.xml and standalone-ha.xml default configs to the preview FP and removed un-used feature-groups. I added new tests to the layers module to cover the 2 default configs. @rhusar the tests for ha config have a workaround for https://issues.redhat.com/browse/WFLY-16452 and https://issues.redhat.com/browse/WFLY-16453
It appears that we already have a remote-naming layer in the test feature-pack. This one would have to be removed.

@jfdenise
Copy link
Contributor Author

jfdenise commented Jun 7, 2022

@darranl @bstansberry , a diff exists with standalone and standalone-ha files prior to this change, jberet has now a security-domain name="ApplicationDomain" resource that the batch-jberet layer brings.

@ehsavoie
Copy link
Contributor

ehsavoie commented Jun 7, 2022

@bstansberry @jasondlee I think that both layers (microprofile-opentracingand open-tracing) are basically the same so we should have an alias.
For jaeger it was an implementation detail at the beginning since the MP opentracing could support any compliant implementation (in theory). I remember writting a PoC with ZipKin.

@jasondlee
Copy link
Contributor

@bstansberry @jasondlee I think that both layers (_microprofile-opentracing_and open-tracing) are basically the same so we should have an alias.

I don't think they're quite identical:

<layer-spec xmlns="urn:jboss:galleon:layer-spec:1.0" name="microprofile-opentracing">
    <dependencies>
        <layer name="open-tracing"/>
        <layer name="microprofile-config"/>
    </dependencies>
</layer-spec>

and

<layer-spec xmlns="urn:jboss:galleon:layer-spec:1.0" name="open-tracing">
    <dependencies>
        <layer name="cdi"/>
        <layer name="microprofile-config"/>
    </dependencies>
    <feature spec="subsystem.microprofile-opentracing-smallrye"/>
</layer-spec>

The first depends on the second, which is also a dependency here: microprofile/galleon-common/src/main/resources/layers/standalone/observability/layer-spec.xml

It does seem, though, that we should be able to collapse those into one (which will probably be removed in WF 28 (?) once we move to MP 6)

@ehsavoie
Copy link
Contributor

ehsavoie commented Jun 8, 2022

@jasondlee I don't really see the value of microprofile-opentracing. It seems to me that it should be aliased

@jasondlee
Copy link
Contributor

You're probably right. I think I was reading that wrong yesterday. Rather than an alias, I wouldn't mind just removing it altogether and changing any references to it. Not sure if that would break anything/anyone, but it does seem like noise.

@jasondlee
Copy link
Contributor

I've opened a ticket for the microprofile-opentracing layer: https://issues.redhat.com/browse/WFLY-16468

@jfdenise jfdenise changed the title [DRAFT] standalone.xml and standalone-ha.xml expressed with layers [DRAFT] standalone default configs expressed with layers Jun 22, 2022
@jfdenise
Copy link
Contributor Author

@bstansberry , I have updated this PR:

  • Removed the remote-naming layer, introduced a feature-group
  • Covered the 4 kind of standalone default configs (standalone, standalone-ha, standalone-full and standalone-full-ha) in the 3 feature-packs (ee, full, preview).
  • The example configs have not been updated, they still rely on feature-groups.
  • Evolved layers test to cover the standalone configs.
  • The PR depends on missing layers: embedded-broker (WFLY-13798), singleton (WFLY-16453) and modcluster (WFLY-16452).

@darranl
Copy link
Contributor

darranl commented Jun 30, 2022

@jfdenise should this still be considered draft?

@jfdenise
Copy link
Contributor Author

@darranl , I am still blocked by missing layers, this can't be merged until we have those layers. I could change it to not being a draft but it has to be hold.

@darranl
Copy link
Contributor

darranl commented Jun 30, 2022

@jfdenise if you don't mind could you please convert to a real draft - I am running through the non-draft PRs at the moment and this one popped up.

@jfdenise
Copy link
Contributor Author

Closing it, the correct PR is #15966

@jfdenise jfdenise closed this Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deps-ok Dependencies have been checked, and there are no significant changes
Projects
None yet
6 participants