[MAGNOLIA-6139] WebappVersionHandler should bootstrap even if initalized workspaces exist in a cluster Created: 20/Mar/15  Updated: 11/Nov/16  Resolved: 26/Oct/15

Status: Closed
Project: Magnolia
Component/s: None
Affects Version/s: 5.3.7
Fix Version/s: 5.3.12, 5.4.3

Type: Bug Priority: Neutral
Reporter: Richard Gange Assignee: Ilgun Ilgun
Resolution: Fixed Votes: 0
Labels: bootstrap, clustering, initialization, support
Remaining Estimate: 0d
Time Spent: 13d 7h
Original Estimate: 1d

Attachments: Zip Archive config.zip     PNG File errorwhenbootstrap.png     Text File stacktrace.txt    
Issue Links:
causality
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Release notes required:
Yes
Date of First Response:
Sprint: Sprint 2 (Basel), Basel 11, Kromeriz 17
Story Points: 8

 Description   

If the WebappVersionHandler detects initialized workspaces in a cluster then it will not bootstrap the webapp. Just because an initialized workspace exists in the cluster doesn't mean that the webapp has been installed. For example if author, public1, and public2 share the forum workspace, author may have created and initialized it before public1 installs.



 Comments   
Comment by Trang Truong [ 05/Jun/15 ]

This is fixed by ignoring trigger bootstrap web-app in cluster workspaces.

Based on configFile param in RepositoryDefinition to check the repository is exist cluster config matched to key <code>magnolia.repositories.jackrabbit.cluster.config</code> in magnolia.properties

Comment by Roman Kovařík [ 07/Jul/15 ]

Reopened:

  • info.magnolia.module.webapp.WebappVersionHandler#checkIfInitialized
    • redundant throws AccessDeniedException
    • _sharedWorkspace any special reason to start with underscore?
  • info.magnolia.repository.RepositoryManager#isClusterWorkspace
    • maybe it should be named isClusteredWorkspace?
    • adding method to an interface is API change which should be done as part of a major release. We could:
    • A] make an exception
    • B] add this method only to DefaultRepositoryManager and check for instance of DefaultRepositoryManager in info.magnolia.module.webapp.WebappVersionHandler#checkIfInitialized
Comment by Trang Truong [ 08/Jul/15 ]

Fixed reopened by removing exposed of interface API info.magnolia.repository.RepositoryManager#isClusterWorkspace

Comment by Philip Mundt [ 13/Jul/15 ]
  • Please rename method isClusterWorkspace(String) to isClusteredWorkspace(String) in info.magnolia.repository.DefaultRepositoryManager
  • I fixed some typos in the javadoc. Please make sure when you add @param workspace to also provide some information about the parameter – or alternatively, leave it out completely.
  • Use info.magnolia.init.MagnoliaConfigurationProperties (@Inject in ctor) instead of SystemProperty.getProperty(String) (new code should not introduce usage of deprected APIs).
  • Please add a test
Comment by Trang Truong [ 15/Jul/15 ]

Provide unit test

Comment by Jan Haderka [ 15/Jul/15 ]

trang.truong did you try to time whole build of core before and after your changes to the test? IIUIC, not only yours, but all tests will now use 2 repos (normal/default plus clustered) so I was wondering what is the penalty for doing that.

Comment by Philip Mundt [ 15/Jul/15 ]

While you're at it trang.truong:
In info.magnolia.repository.DefaultRepositoryManager the default ctor has to inititalize MagnoliaConfigurationProperties too:

public DefaultRepositoryManager() {
    this.magnoliaConfigurationProperties = Components.getComponent(MagnoliaConfigurationProperties.class);
}

Also make private MagnoliaConfigurationProperties magnoliaConfigurationProperties; final.

Comment by Philip Mundt [ 16/Jul/15 ]

Resolved on branch MAGNOLIA-6139-prm

Comment by Trang Truong [ 24/Jul/15 ]

sang.ngo please see attached config file, put it into WEB-INF to run cluster env.

Comment by Sang Ngo Huu [ 24/Jul/15 ]

There are steps I started QA:

  • Get configuration from attached file
  • Add into eclipse and run webapp project in first time under clustered enviroment
  • The result: the message in logs
    _- First time: _
    2015-07-24 11:05:15,953 INFO  info.magnolia.module.webapp.WebappVersionHandler  : Content was not found in the repository, will bootstrap web-app.
    _- Second time: _
    2015-07-24 11:20:12,021 INFO info.magnolia.module.webapp.WebappVersionHandler : Content was found in the repository, will not bootstrap web-app.
  • Delete repo magnolia, keep clustered repo and start again
  • Result:
    2015-07-24 11:35:15,953 INFO  info.magnolia.module.webapp.WebappVersionHandler  : Content was not found in the repository, will bootstrap web-app.
    => Ticket is resovled.
    But when I start installing from web, it faced out error, and the installation is hang. I'm not sure it is related this issue or not. See stacktrace stacktrace.txt and screenshot
Comment by Jan Haderka [ 24/Jul/15 ]

The error is due to fact we are trying to rebootstrap content that exists in clustered workspace from all nodes in the cluster.
To avoid this problem, we would need to ensure we bootstrap (or change via update tasks) content in clustered workspaces only once. This might proof to be difficult to achieve since installation process have not been designed with such use case in mind back in Magnolia 3.0.
Currently the biggest obstacle is deciding when content should be bootstrapped (or modified) in clustered workspace. It can be moderately easy to figure out upon installation, but would be very difficult to dynamically resolve such situation on upgrades, therefore as most viable solution we should try to:

  • introduce new property in magnolia.properties denoting master/bootstrap-owner cluster-node
  • modify bootstrap utilities to bootstrap content in clustered workspaces only when the flag in properties file is set
  • modify module installer to call Session.save() after module installation on clustered workspaces only on the cluster-node where flag is set.
  • add automated test for both (bootstrap conflict and update via update tasks) to check for this condition explicitly
Comment by Philip Mundt [ 08/Sep/15 ]

After a short discussion with ilgun and had we identified potential remaining issues with tasks that are executed twice (by two instances, even though only master cluster node does the session.save()) if they are not written in such way that they can be actually be executed multiple times. Whenever running into such an issue we'll try to fix them but we are aware of the fact that we might not be able to cover all cases. Clients using this feature should also be aware of this.

Comment by Ilgun Ilgun [ 11/Sep/15 ]

Changes are present in the following branches - >
MAGNOLIA-6139-5.4.x-ilgun and MAGNOLIA-6139-5.3.x-ilgun

Comment by Espen Jervidalo [ 16/Sep/15 ]

info.magnolia.module.webapp.WebappVersionHandler#checkIfInitialized

  • if (isClusteredWorkspace && !isMasterCluster) -> should continue IMO and not return false

info.magnolia.module.ModuleManagerImpl#saveChanges

  • remove ‘don't call save or refresh if useless’
  • move cluster information where needed -> inside if (persist)
    • OR before, why rollback on slave if we don’t persist?

info.magnolia.repository.DefaultRepositoryManager#DefaultRepositoryManager(info.magnolia.init.MagnoliaConfigurationProperties, info.magnolia.repository.WorkspaceMapping)
-> I would vote for removing the Ctor. It’s only used in test cases.

Comment by Ilgun Ilgun [ 18/Sep/15 ]

Pushed the changes to the same branches.

Comment by Espen Jervidalo [ 18/Sep/15 ]

info.magnolia.module.webapp.WebappVersionHandler#checkIfInitialized

  • I still think we should continue in this check, instead of returning 'false':
    if (isClusteredWorkspace && !isMasterCluster) {
        log.info("Skipped clustered workspace '{}' initializing check because it is not cluster master", workspace);
        return false;
    }
    

info.magnolia.module.ModuleManagerImpl#isClustered(RepositoryManager repositoryManager, ..)
info.magnolia.module.ModuleManagerImpl#isMasterCluster(RepositoryManager repositoryManager)

  • remove the RepositoryManager parameter, it's already defined as field in the class.

info.magnolia.module.ModuleManagerImpl#saveChanges

  • line #548: while at it, use RepositoryManager#getWorkspaceNames() instead of ContentRepository.getAllRepositoryNames()
Comment by Magnolia International [ 08/Oct/15 ]

In the following snippet, `persist` means "tasks were executed successfully". In previous versions that translated into "yes, save all changes made by tasks", or "roll it back". Now, it looks to me like we're still rolling back on failure, but saving only on master or non-clustered workspaces. Which leads me to think that on a clustered workspace, that session will be "dirty" forever; I'm not even sure how clustering will work with that: will it override the dirty stuff, or not apply changes from master until session is cleaned/refreshed ? Shouldn't we simply also rollback in those cases ? (and let master node apply the changes whenever)

As a side note: should the title of this ticket still be specific to WebappVersionHandler and bootstrapping, while the fix isn't ?

if (session.hasPendingChanges()) {
                    if (persist) {
                        boolean isMasterCluster = isMasterCluster();
                        boolean isClustered = isClustered(repoName);

                        if (!isClustered || isMasterCluster) {
                            session.save();
                        } else {
                            log.info("Skipping bootstrapping of repository '{}' because it is clustered and node is not cluster master ", repoName);
                        }
                    } else {
                        session.refresh(false);
                    }
                }
Comment by Jan Haderka [ 09/Oct/15 ]

Indeed. The inner else clause should have not only log.info but also session.refresh(false) to drop pending changes. Good catch. Did you run into issue with it or did you just spotted in the code?

AFAIK as long as we kept dirty changes, upon cluster node sync interval, those changes will be synced w/ changes coming from the cluster and in case of conflict InvalidItemStateException would be thrown upon attempt to save such session.

Generated at Mon Feb 12 04:11:40 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.