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

Fix for WFLY-16863, Express default standalone configurations with Galleon layers #15966

Merged

Conversation

jfdenise
Copy link
Contributor

@jfdenise jfdenise commented Aug 22, 2022

Issue: https://issues.redhat.com/browse/WFLY-16863
This change re-implement the default configurations with Galleon layers.
Added new tests to cover execution of default configs without inheriting all JBoss Module modules.
For full feature-pack, the gain in filesystem size is around 100MB. 275MB without layers, 165MB for std/std-ha , 175MB for full/full-ha.
For ee feature-pack, the gain in filesystem size is around 100MB. 261MB without layers, 161MB for std/std-ha , 173MB for full/full-ha.
For preview feature-pack, the gain in filesystem size is around 100MB. 275MB without layers, 165MB for std/std-ha ,177MB for full/full-ha.
@bstansberry I am thinking that we should remove the un-used feature_groups (not yet done in this PR). Although one could use them, they are not really part of any API. Keeping them will create maintenance issue (eg: doing a fix in an un-used feature-group).

Link to diff for standalone.xml: https://gist.github.com/jfdenise/ed53e08ef3439daa7807d3b6a5aebcd0

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

@jfdenise I haven't had time to look at this yet, but +1 to cleaning out and feature group cruft.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from cba4612 to c5bde7b Compare August 30, 2022 14:41
@jfdenise
Copy link
Contributor Author

@bstansberry , to be able to cleanup I had to express the example configs with layers + feature-groups.

We have one config that is not exactly the same with these changes. The standalone-minimalistic.xml that now contains access-control and audit-log.

I have also removed the example configs defined in ee-9, the example configs in ee-9 are now identical to the one used in wildfly and wildfly-ee feature-packs. I hope that it is OK.

We still have some standalone related feature-groups that are used by domain configurations. I was not able to remove them.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch 2 times, most recently from 54363ca to be8aec1 Compare September 5, 2022 13:35
@jfdenise
Copy link
Contributor Author

jfdenise commented Sep 5, 2022

@bstansberry ,

I am done with this refactoring. I feel that using Galleon layers the content of standalone configuration (config.xml files) is much easier to read, the content of each config is explicit. We have less duplication in the ee/full/preview feature-packs.

  • Un-used feature-groups have been removed.
  • Introduced 2 layers: standalone-profile and standalone-full-profile.
  • Default HA configs configure the standalone-profile and standalone-full-profile layers by excluding "local" layers and including "dist" ones.
  • WildFly Full now inherits standalone default configs from EE feature-pack and doesn't redefine them. That is possible thanks to the standalone-profile and standalone-full-profile layers defined by the ee feature-pack that full feature-pack extends to add some microprofile layers.
  • Example config standalone-minimalistic.xml now contains access-control and audit-log. If that is an issue, I can still express them with feature-groups (as they used to be), this will not bring maintenance issue. With the existence of Galleon layers this configuration is perhaps meaningless. Let me know.
  • EE-9, comply with the existing pattern (galleon-content vs galleon-microprofile-content). standalone-profile and standalone-full-profile are defined in the galleon-microprofile-content in order to add the microprofile Galleon layers.

The only missing Galleon layer I identified in the default configs could be a jaeger opentracing layer. That is not critical with respect to JBoss Modules provisioning, we are safe there. It would allow to exclude opentracing from the standalone configs, although internally (eg: example configs) we don't need to exclude it.

If we want to better cover the example configs, we could introduce an XTS and RTS Galleon layer, but I guess that it shouldn't block this refactoring. The example configs are not expected to be provisioned with passive+.

@bstansberry
Copy link
Contributor

Hi @jfdenise -- this is finally coming onto my radar screen...

What I'm going to do with this is build wildfly-dist with and without this and compare the output. Preview too.

I suspect https://issues.redhat.com/browse/WFLY-17004 will be a problem.

Then I'll dig into the changes.

@bstansberry
Copy link
Contributor

As we discussed earlier, the diffs look ok. Changes in element ordering in the xml, which if unavoidable are something to do in WF 27 (fyi re this @emmartins in case the server migration cares about element ordering in some way). Then the batch thing which we can chat about in zulip.

</feature>
</feature>
<feature-group name="private-interface"/>
<feature-group name="jgroups-all">
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we create feature groups instead of repeating this?

@bstansberry
Copy link
Contributor

@jfdenise I'm not sure of the objective here. This seems ok as a refactor, but doesn't the use of the value 'adjust-standalone-xxx' feature groups in the different config.xml files mean that we aren't expressing the default configurations using layers? We're closer than we were, but I'm unclear on what that gets us.

I'm not saying getting closer than where we were isn't something to do; I'd just like to understand better how this fits into the overall picture.

Some things I completely understand why we wouldn't express via layers; for example the 'welcome-content' stuff. Others I'm less clear.

@jfdenise
Copy link
Contributor Author

@bstansberry , the adjustments don't bring new subsystems or depend on some other JBoss Modules that the layer would not bring. Adjustments are layers "tuning" to comply with the existing default configs. We have something similar for the standalone-microprofile default configs.
The goal of this refactoring is to be able to provision a trimmed server for a given default standalone config with the right set of JBoss modules (knowing that we need to fix hibernate-search).
I agree that we could move these adjustments to some new Galleon layers. Having no feature-group at all would allow us to tweak a default config (by excluding some layers).

I could evolve this PR with this suggestion or log new issues to track the new layers.

@bstansberry
Copy link
Contributor

Thanks @jfdenise. Re evolution, I started writing this comment yesterday, but left off midway at end of my day...

This comment is kind of notes to myself or other who are interested re: what I'm seeing in the various 'adjust-standalone' feature groups.

  1. Add undertow-default-config
    a) Add welcome-content resources. I totally get why that's not in a layer.
    b) Sets 'default-security-domain=other'. It's feels a bit off, haven't dug in enough to have a sense of what to do about it.

  2. Unset the remoting subsystem endpoint worker attribute. I haven't looked but this feels like something that should be in one of the existing layers.

  3. The remote-naming, ejb3-db-pool and ejb-remote feature groups. My impression is this is because the 'ejb' layer also brings in the 'messaging-activemq' layer which is not wanted in standalone.xml. Perhaps we should make that dep excludable.

  4. Add ejb3-iiop. Initially adding a layer for that was part of the EJB layers plans. We didn't do it; I forget why not. Perhaps just to reduce scope.

  5. Adding infinispan-local-server or infinispan-dist-server. Those are part of the singleton-local and singleton-ha layers so I'm not sure what's the story with these two.

  6. Add the messaging-activemq-ha feature group to standalone-full-ha.

@jfdenise
Copy link
Contributor Author

  • Add undertow-default-config
    a) Add welcome-content resources. I totally get why that's not in a layer.
    b) Sets 'default-security-domain=other'. It's feels a bit off, haven't dug in enough to have a sense of what to do about it.

  • Unset the remoting subsystem endpoint worker attribute. I haven't looked but this feels like something that should be in one of the existing layers.

You mean updating the remoting layer? It appears that wildfy-ee-galleon-pack redefines the remoting feature_group that remoting layer uses. This could be done there.

  • The remote-naming, ejb3-db-pool and ejb-remote feature groups. My impression is this is because the 'ejb' layer also brings in the 'messaging-activemq' layer which is not wanted in standalone.xml. Perhaps we should make that dep excludable.

That would work except for the ejb3-mdb feature-group that ejb layer brings that is not in standalone configs.

  • Add ejb3-iiop. Initially adding a layer for that was part of the EJB layers plans. We didn't do it; I forget why not. Perhaps just to reduce scope.

Ok.

  • Adding infinispan-local-server or infinispan-dist-server. Those are part of the singleton-local and singleton-ha layers so I'm not sure what's the story with these two.

infinispan-dist-server is actually a left-over, using singleton-ha layer is enough. I can remove it.
We can't use the singleton-local layer in standalone config, singleton subsystem is not present in the current standalone configs.

  • Add the messaging-activemq-ha feature group to standalone-full-ha.

You mean to add the feature-group directly in the standalone-full-ha config and not transitively through the adjust-standalone-full-ha-config feature-group? Would you see a benefit to this change?

@bstansberry
Copy link
Contributor

@jfdenise Note that my previous comment was basically just a list of what I saw, as a way to organize discussion. So if what I said wasn't clear about what to do, it's because I didn't know. ;)

  • Add undertow-default-config
    a) Add welcome-content resources. I totally get why that's not in a layer.
    b) Sets 'default-security-domain=other'. It's feels a bit off, haven't dug in enough to have a sense of what to do about it.
  • Unset the remoting subsystem endpoint worker attribute. I haven't looked but this feels like something that should be in one of the existing layers.

You mean updating the remoting layer? It appears that wildfy-ee-galleon-pack redefines the remoting feature_group that remoting layer uses. This could be done there.

That sounds likely. I haven't looked into why the remoting feature_group does that. But my instinct is either it doesn't need to, or the layer itself should already do it. I suspect there's some no longer important historical reason for this one.

  • The remote-naming, ejb3-mdb-pool and ejb-remote feature groups. My impression is this is because the 'ejb' layer also brings in the 'messaging-activemq' layer which is not wanted in standalone.xml. Perhaps we should make that dep excludable.

That would work except for the ejb3-mdb feature-group that ejb layer brings that is not in standalone configs.

Let's see what @chengfang and @tadamski say about that. This is what that feature group brings:

<feature spec="subsystem.ejb3">
        <param name="default-resource-adapter-name" value="${ejb.resource-adapter-name:activemq-ra.rar}"/>
        <param name="default-mdb-instance-pool" value="mdb-strict-max-pool"/>
    </feature>

I'm not an expert but the 'default-mdb-instance-pool' feels harmless, given the config does have the pool resource. The 'default-resource-adapter-name' sounds more problematic.

  • Add ejb3-iiop. Initially adding a layer for that was part of the EJB layers plans. We didn't do it; I forget why not. Perhaps just to reduce scope.

Ok.

  • Adding infinispan-local-server or infinispan-dist-server. Those are part of the singleton-local and singleton-ha layers so I'm not sure what's the story with these two.

infinispan-dist-server is actually a left-over, using singleton-ha layer is enough. I can remove it. We can't use the singleton-local layer in standalone config, singleton subsystem is not present in the current standalone configs.

@pferraro Does the 'infinispan-local-server' feature group bring any value to standalone.xml if the singleton subsystem isn't present?

  • Add the messaging-activemq-ha feature group to standalone-full-ha.

You mean to add the feature-group directly in the standalone-full-ha config and not transitively through the adjust-standalone-full-ha-config feature-group? Would you see a benefit to this change?

For this one I didn't mean anything; I was just listing what the 'adjust-xxx' feature groups were doing and didn't have any vague thoughts to add on to this item. :-)

@pferraro
Copy link
Contributor

@bstansberry

Does the 'infinispan-local-server' feature group bring any value to standalone.xml if the singleton subsystem isn't present?

It is only required by capability references (e.g. the singleton subsystem).
The only remaining cache-container without any capability references that should exist by default is "hibernate".

@bstansberry
Copy link
Contributor

@pferraro I suppose a benefit of having it in the OOTB config even though the singleton subsystem isn't is it makes it easier to add the subsystem via CLI, xml editing etc.

@emmartins
Copy link
Contributor

emmartins commented Sep 19, 2022 via email

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from be8aec1 to 4efca43 Compare September 28, 2022 10:50
@jfdenise
Copy link
Contributor Author

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 4efca43 to 5c31bd8 Compare October 19, 2022 12:16
@jfdenise
Copy link
Contributor Author

Resynced with main branch:

  • Removed embedded broker from preview feature-pack configs.
  • Removed metrics from preview feature-pack configs.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 5c31bd8 to 127bc70 Compare November 7, 2022 12:19
@jfdenise
Copy link
Contributor Author

jfdenise commented Nov 7, 2022

Resynced with main branch. Impact on ec2 example configs.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 127bc70 to 214899a Compare November 14, 2022 16:22
@jfdenise
Copy link
Contributor Author

Resynced with main branch. Impact on all example configs containing jgroups subsystem.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 214899a to e190c94 Compare December 14, 2022 10:23
@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch 2 times, most recently from c2f7ab0 to 209cb72 Compare January 20, 2023 13:28
@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 209cb72 to f308e8f Compare January 30, 2023 15:19
@luck3y
Copy link
Contributor

luck3y commented Jan 30, 2023

/retest

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch 2 times, most recently from 2a42d60 to 8f9a69b Compare February 3, 2023 13:01
@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 8f9a69b to 9bf88fe Compare February 16, 2023 14:57
@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch 2 times, most recently from 9427d99 to bf622a5 Compare March 14, 2023 17:56
@jfdenise
Copy link
Contributor Author

@pferraro , we are wandering if removing the infinispan-local-server feature-group from the standalone.xml and standalone-full.xml configuration would be ok with you. There is no singleton subsystem in these 2. @bstansberry , in relation to our discussion.

@jfdenise
Copy link
Contributor Author

@chengfang , this PR introduces the usage of the jberet Galleon layer to define the standalone default configurations (standalone, standalone-full, standalone-ha and standalone-full-ha). This introduces a difference with today configurations, the jberet subsystem is now secure using the "ApplicationDomain" elytron security domain. This is what the jberet Galleon layer is doing.
Is it OK with you to introduce this change in our default configurations? Thank-you. @bstansberry @fjuma FYI.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from bf622a5 to 5c1249a Compare March 15, 2023 15:42
@pferraro
Copy link
Contributor

@pferraro , we are wandering if removing the infinispan-local-server feature-group from the standalone.xml and standalone-full.xml configuration would be ok with you. There is no singleton subsystem in these 2. @bstansberry , in relation to our discussion.

The capabilities provided by that feature-group are only required by the singleton subsystem - so this should be ok.

@chengfang
Copy link
Contributor

@jfdenise batch-jberet subsystem has a security-domain element that defines the default security domain to use. Does your change interfere with this existing configuration?

@bstansberry
Copy link
Contributor

Hi @chengfang

The does not interfere with the ability to configure that attribute. It changes its value in the OOTB standalone.xml and domain.xml files.

If you look in our current standard OOTB configs, the batch-jberet subsystem's security-domain attribute is not set. (The xml element is not present.)

If you provision a slimmed server and specify the batch-jberet Galleon layer, then the xml has

<security-domain name="ApplicationDomain"/>

This PR has the effect of aligning the standard OOTB configs with what you would get if a user created configs using the Galleon layers. So the effect is all the OOTB standalone.xml and domain.xml variants will have set security-domain="ApplicationDomain". @jfdenise and I are looking for your approval of that change.

@chengfang
Copy link
Contributor

@bstansberry @jfdenise Adding jberet-batch security-domain to OOTB configuration looks good to me.

@jfdenise
Copy link
Contributor Author

jfdenise commented Mar 17, 2023

@pferraro , it appears that we have 6 tests that expect the server cache to be present in the standalone and standalonefull configs:

  • org.jboss.as.test.integration.ee.injection.resource.infinispan.InfinispanCdiTestCase
  • org.jboss.as.test.integration.ee.injection.resource.infinispan.InfinispanInjectionTestCase
  • org.jboss.as.test.integration.ee.injection.resource.infinispan.InfinispanResourceRefTestCase
  • org.jboss.as.test.clustering.single.singleton.SingletonServiceTestCase
  • org.jboss.as.test.clustering.single.registry.RegistryTestCase
  • org.jboss.as.test.clustering.single.provider.ServiceProviderRegistrationTestCase

@pferraro we could update the injection tests to use the web container (hope that is fine) and introduce a clustering.cli script to add the server container to the standalone and standalone-full configurations. I am working on a fix that I will push as an additional commit.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from a3d7ad9 to 5db4f9e Compare March 22, 2023 21:40
@bstansberry
Copy link
Contributor

@jfdenise FYI re https://issues.redhat.com/browse/WFLY-17791, which I filed to keep track of things to look into that shouldn't block this. Feel free to edit that with others you may want to track.

Copy link
Contributor

@bstansberry bstansberry left a comment

Choose a reason for hiding this comment

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

@jfdenise LGTM. If you can squash down the commits I'll aim to get this merged into wf-28-beta-dev during my Friday morning.

@jfdenise jfdenise force-pushed the all-default-configs-with-layers-final branch from 5db4f9e to 58c1837 Compare March 24, 2023 09:25
@jfdenise
Copy link
Contributor Author

@bstansberry I squashed part of the commits, kept the commit for WFLY-17778 fix.

@bstansberry bstansberry merged commit 8a65f38 into wildfly:main Mar 27, 2023
13 of 14 checks passed
@bstansberry
Copy link
Contributor

Thanks @jfdenise, @pferraro and @chengfang!

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