Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 62 additions & 2 deletions docs/testing/BINARY_S3_STORAGE.md
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,66 @@ per direct child record on every listing, and that includes the tombstone of eve
child, so it grows with the number of temporary names ever written directly in that folder until
tombstones can be reclaimed.

## Renditions and rendering

With the flag on, completed image-filter outputs are uploaded to the `generated-assets` group as
they are produced, at `generated-assets/{first}/{second}/{inode}/dotGenerated_...`, with the same
relative layout under the local `dotGenerated` root. A cold read restores a completed rendition
from S3 without running the filter again. A warm hit is served from local disk without contacting
S3, so an S3 outage does not break a cached response and a warm read cannot republish an
invalidated object. A cold read, which has to look in S3, fails while S3 is unavailable.
Incomplete filter outputs are never uploaded or evicted. Thumbnail invalidation and source
deletion target that inode's directory. With the flag off, renditions keep the existing
two-character layout. Existing rendition objects are not moved to the new layout.

The exporter finds an existing rendition by predicting the path each filter writes, so each
filter's prediction must match its output; the JPEG filter, for example, predicts `.jpg`. Some
requests leave the image unchanged, for instance a maximum width larger than the image. Such a
filter writes nothing, so the first request for it looks for its predicted output in S3 and finds
nothing. The node then remembers that this output is never produced, and later requests on that
node skip the lookup. Only an output written at its predicted path is uploaded.

Upload failures do not fail the request. If S3 rejects a rendition upload, the locally produced
rendition is served and a warning is logged. The rendition then stays local only: later requests
on that node are warm hits and do not retry the upload, eviction keeps the file because it has no
durable copy, and other nodes generate their own. Uploads run after the image-generation permit is
released.

Rendition keys include the original's revision key, so replacing bytes under the same inode and
filename never selects the previous revision's rendition. Crops read the focal point from the
content's metadata snapshot, including focal points saved on a temporary upload, and the Java and
native crop engines use the same pinned coordinates. When a focal point is read through the
content API instead, as `$content.image.fpx` does, content the current user cannot read, or that
does not exist, has no focal point, as with the flag off; only a failed lookup is an error.

Compiled Sass output also uses the `generated-assets` group. Its key covers the source and
dependency bytes, site, live or working mode, compiler options and application build, so a cold
node restores a matching CSS output without running Sass. Source maps keep their existing private,
uncached behavior. As with renditions, a compiled output whose upload fails is still served,
with a warning, and is kept locally only. Each change to the key, such as an edit to an imported
partial, a working-mode compile of an edit in progress, or an upgrade, stores a new CSS object
without removing the old one. Old objects are removed only when the main Sass file's generated
outputs are invalidated or its inode's binaries are deleted, so for a main file that rarely
changes they accumulate. Superseded compiled CSS objects are not reclaimed yet.

Markdown files are read through the protected FileAsset stream, and VTL and included files are
opened through the binary asset API, so a file whose local copy was evicted is restored from S3
before it is rendered. A missing VTL or included file outside the asset root cannot be restored,
so it is reported as not found, as with the flag off. When restoring a VTL, macro or included file
fails, the render fails with a storage error that is not cached as a missing template, so the next
render tries again. Templates, containers, pages and other database-built sources keep their
existing failure handling.

### Known limitation: long cache leases

Image-filter execution holds a cache lease for the whole export: the S3 lookups, the pixel work
and the uploads of every filter in the chain. Sass compilation holds a lease while it gathers and
copies the sources, which includes database lookups, and while it reads a stored output, but not
while Dart Sass runs. Eviction skips every file while any lease is held, so on a node with steady
image traffic a sweep can be deferred again and again, and the local cache can stay above its
budget. Narrowing the rendition lease to the moments a filter reads a local file needs the filters
to restore their inputs themselves, which is a larger change and is left as a follow-up.

## Local cache eviction

With the flag on, the local asset directory is a cache that `BinaryCacheEvictionJob` can trim.
Expand Down Expand Up @@ -485,7 +545,7 @@ docker run -d --rm --name binary-cleanup-postgres-test \
-e POSTGRES_DB=binary_storage_test postgres:16-alpine

./mvnw test -pl :dotcms-core -Dmaven.build.cache.enabled=false \
-Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest,BinaryAssetReferenceTest,BinaryCacheEvictionJobTest,BinaryFileSystemStorageTest,BinaryAssetStorageAPIImplTest,MetadataLocalCacheTest,BinaryAssetCleanupTransactionTest,BinaryAssetCleanupProcessorTest,ContentletBackupStorageGateTest,BinaryFieldCleanupProcessorTest,AssetJobEventSerializationTest,BinaryAssetBackfillCheckpointTest,ImportStarterWorkflowCleanupTest,BinaryAssetBackfillProcessorTest,BinaryAssetBackfillTest,ExportStarterFailureTest,BundleArchiveStorageTest,FileAssetBundlerTest,TemporaryAssetStorageTest,WebdavAssetStorageTest,TemporaryMetadataStorageTest,WebdavTemporaryStorageTest \
-Dtest=AssetStorageFeatureTest,AssetStorageFeatureLatchTest,S3StorageConfigurationTest,NoWebIdentityCredentialsProviderChainTest,BinaryS3StorageTest,BinaryAssetReferenceTest,BinaryCacheEvictionJobTest,BinaryFileSystemStorageTest,BinaryAssetStorageAPIImplTest,MetadataLocalCacheTest,BinaryAssetCleanupTransactionTest,BinaryAssetCleanupProcessorTest,ContentletBackupStorageGateTest,BinaryFieldCleanupProcessorTest,AssetJobEventSerializationTest,BinaryAssetBackfillCheckpointTest,ImportStarterWorkflowCleanupTest,BinaryAssetBackfillProcessorTest,BinaryAssetBackfillTest,ExportStarterFailureTest,BundleArchiveStorageTest,FileAssetBundlerTest,TemporaryAssetStorageTest,WebdavAssetStorageTest,TemporaryMetadataStorageTest,WebdavTemporaryStorageTest,ImageFilterExporterFocalPointTest,ImageFilterExporterTest,ImageFilterExporterSharedStoreTest,ImageFilterExporterEngineSelectionTest,ImageFilterExporterStorageTest,FocalPointAPIImplTest,StoredVelocityFileLoaderTest \
-Ds3.test.endpoint=http://127.0.0.1:19002 \
-Ds3.test.jdbc=jdbc:postgresql://127.0.0.1:19003/binary_storage_test

Expand All @@ -498,7 +558,7 @@ stack:
```sh
./mvnw install -pl :dotcms-core --am -DskipTests -Ddocker.skip
./mvnw verify -pl :dotcms-integration -Dmaven.build.cache.enabled=false -Dcoreit.test.skip=false \
-Dit.test=BinaryAssetStorageIntegrationTest,ContentletBackupStorageTest,SharedAssetStorageIntegrationTest,BinaryAssetStarterRestoreTest,PublishingArchiveStorageTest,DotWebdavHelperTest
-Dit.test=BinaryAssetStorageIntegrationTest,ContentletBackupStorageTest,SharedAssetStorageIntegrationTest,BinaryAssetStarterRestoreTest,PublishingArchiveStorageTest,DotWebdavHelperTest,AssetTemplateStorageTest,CSSAssetStorageTest
```

These default to flag-off mode, where the S3 cases are skipped. To run the S3 cases, create a
Expand Down
23 changes: 23 additions & 0 deletions dotCMS/src/main/java/com/dotcms/csspreproc/DotCSSCompiler.java
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@ abstract class DotCSSCompiler {
protected final String inputURI;
protected final Host inputHost;
protected final boolean inputLive;
protected final Map<String, String> sourceDigests = new java.util.TreeMap<>();

public Set<String> getAllImportedURI() {
return allImportedURI;
Expand Down Expand Up @@ -165,6 +166,18 @@ protected List<String> filterImportedUris(String baseUri, List<String> uris) thr
protected File createCompileDir(Host currentHost, String uri, boolean live)
throws IOException, DotStateException, DotDataException, DotSecurityException {

if (com.dotcms.storage.AssetStorageFeature.isEnabled()) {
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
sourceDigests.clear();
return createCompileDirInternal(currentHost, uri, live);
}
}
return createCompileDirInternal(currentHost, uri, live);
}

private File createCompileDirInternal(Host currentHost, String uri, boolean live)
throws IOException, DotStateException, DotDataException, DotSecurityException {

File compDir = new File(APILocator.getFileAssetAPI().getRealAssetPathTmpBinary() + File.separator + "css_compile_space" + File.separator
+ UUIDGenerator.generateUuid());
compDir.mkdirs();
Expand Down Expand Up @@ -195,6 +208,7 @@ protected File createCompileDir(Host currentHost, String uri, boolean live)
if (file != null && InodeUtils.isSet(file.getInode())) {
File ff = file.getBinary(FileAssetAPI.BINARY_FIELD);
if (ff != null && ff.isFile()) {
recordSource(host.getHostname() + fileuri, ff);

// destfile: /../../../tmpdir/host/path/to/asset.extension

Expand Down Expand Up @@ -288,13 +302,22 @@ protected File createCompileDir(Host currentHost, String uri, boolean live)
getAllImportedURI().add(assetUri);
f.getParentFile().mkdirs();
FileUtil.copyFile(asset.getFileAsset(), f);
recordSource(inputHost.getHostname() + assetUri, f);
}

}

return compDir;
}

private void recordSource(final String path, final File file) throws IOException {
if (com.dotcms.storage.AssetStorageFeature.isEnabled()) {
try (var input = java.nio.file.Files.newInputStream(file.toPath())) {
sourceDigests.put(path, org.apache.commons.codec.digest.DigestUtils.sha256Hex(input));
}
}
}

private String addImportUnderscore(String url) {
return url.substring(0, url.lastIndexOf('/')) + "/_" + url.substring(url.lastIndexOf('/') + 1, url.length());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,20 @@ public DotLibSassCompiler(final Host host, final String uri, final boolean live,
this.sourceMap = sourceMap;
}

/**
* Compiles the SCSS or Sass file into {@link #getOutput()}.
*
* <p>With S3 asset storage on, a compiled CSS output stored under a key that covers the source
* bytes, site, mode, options and build is reused, locally or restored from S3, without running
* Dart Sass. A new output is written locally and uploaded. If storing it fails, the compiled
* CSS is still returned and a warning is logged. A cache lease is held only while the sources
* are copied and while a stored output is read, not while Dart Sass runs.
*
* @throws DotSecurityException if a source cannot be read by the system user
* @throws DotStateException if the dotCMS state is inconsistent
* @throws DotDataException if a source or its version information cannot be loaded
* @throws IOException if the compile directory cannot be prepared
*/
@Override
public void compile() throws DotSecurityException, DotStateException, DotDataException, IOException {

Expand All @@ -88,7 +102,9 @@ public void compile() throws DotSecurityException, DotStateException, DotDataExc
}

final FileAsset mainFile = APILocator.getFileAssetAPI()
.fromContentlet(APILocator.getContentletAPI().find(info.get().getWorkingInode(),
.fromContentlet(APILocator.getContentletAPI().find(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚪ [P3] DotLibSassCompiler.java:106 null live inode breaks live compile when live version is absent

Current code:

.fromContentlet(APILocator.getContentletAPI().find(
        com.dotcms.storage.AssetStorageFeature.isEnabled() && live
                ? info.get().getLiveInode() : info.get().getWorkingInode(),

Problem: With the flag on and live == true, a null getLiveInode() makes find(null, …) throw, failing a request that compiled from the working inode before.

Fix:

final String inode = com.dotcms.storage.AssetStorageFeature.isEnabled() && live
        && info.get().getLiveInode() != null
        ? info.get().getLiveInode() : info.get().getWorkingInode();

Assumption: CSSPreProcessServlet confirms a live asset for the identifier before compiling, so getLiveInode() should be non-null in practice. What to verify: that a published-then-unpublished SCSS file cannot reach this line in live mode; the null-guard removes the risk regardless.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚪ [P3] DotLibSassCompiler.java:107 null live inode breaks live compile when live version is absent

Current code:

.fromContentlet(APILocator.getContentletAPI().find(
        com.dotcms.storage.AssetStorageFeature.isEnabled() && live
                ? info.get().getLiveInode() : info.get().getWorkingInode(),

Problem: With flag on and live, a null getLiveInode() makes find(null, …) throw, failing requests that previously compiled from the working inode.

Fix:

.fromContentlet(APILocator.getContentletAPI().find(
        com.dotcms.storage.AssetStorageFeature.isEnabled() && live
                && info.get().getLiveInode() != null
                ? info.get().getLiveInode() : info.get().getWorkingInode(),

Assumption: a live asset is normally confirmed before compiling, so this is defensive. What to verify: that a published-then-unpublished SCSS file cannot reach this line in live mode.

com.dotcms.storage.AssetStorageFeature.isEnabled() && live
? info.get().getLiveInode() : info.get().getWorkingInode(),
APILocator.systemUser(), true));

// build directories to build scss
Expand All @@ -102,10 +118,31 @@ public void compile() throws DotSecurityException, DotStateException, DotDataExc
}
final File compileDestinationFile = new File(compileTargetFile.getAbsoluteFile() + ".css");
final CompilerOptions options = new CompilerOptions.Builder().sourceMap(this.sourceMap).build();
// Source maps remain request-private and uncached, as in the servlet's existing contract.
final File storedCSS = com.dotcms.storage.AssetStorageFeature.isEnabled() && !sourceMap
? compiledCacheFile(mainFile.getInode(), options) : null;
if (storedCSS != null) {
try (var lease = APILocator.getBinaryAssetStorageAPI().acquireCacheLease()) {
final File cached = APILocator.getBinaryAssetStorageAPI().getGeneratedFile(storedCSS);
if (cached != null && (req == null || req.getParameter("recompile") == null)) {
this.output = java.nio.file.Files.readAllBytes(cached.toPath());
return;
}
}
}
final DartSassCompiler compiler = new DartSassCompiler(options, compileTargetFile, compileDestinationFile);
final Optional<String> out = compiler.compile();
handleOutput(compiler.terminalOutput());
this.output = out.isPresent() ? out.get().getBytes() : null;
if (storedCSS != null && this.output != null && compiler.terminalOutput().exitValue() == 0) {
try {
storeCompiledOutput(storedCSS);
} catch (final Exception e) {
// Availability first: the CSS is compiled, so a storage failure must not fail the request.
Logger.warnAndDebug(DotLibSassCompiler.class, "Unable to store compiled CSS for " + inputHost
+ ":" + inputURI + "; serving the compiled output: " + e.getMessage(), e);
}
}
} catch (final Exception ex) {
final String errorMsg = String.format("Unable to compile SASS code in %s:%s [ live:%s ]: %s", inputHost,
inputURI, inputLive, ex.getMessage());
Expand All @@ -119,6 +156,46 @@ public void compile() throws DotSecurityException, DotStateException, DotDataExc
}
}

private File compiledCacheFile(final String inode, final CompilerOptions options) {
final StringBuilder identity = new StringBuilder("css-v1:")
.append(com.liferay.portal.util.ReleaseInfo.getVersion()).append(':')
.append(com.liferay.portal.util.ReleaseInfo.getBuildNumber()).append(':')
.append(inputHost.getIdentifier()).append(':').append(inputURI).append(':')
.append(live).append(':').append(options.generate());
sourceDigests.forEach((path, digest) -> identity.append(':').append(path.length())
.append(':').append(path).append(':').append(digest));
final String hash = org.apache.commons.codec.digest.DigestUtils.md5Hex(identity.toString());
return new File(com.dotmarketing.util.ConfigUtils.getDotGeneratedPath(),
inode.charAt(0) + "/" + inode.charAt(1) + "/" + inode + "/dotGenerated_css_" + hash + ".css");
}

/**
* Writes the compiled CSS atomically to its local {@code dotGenerated} path and uploads it.
* If the upload fails, the local file is kept: later requests on this node serve it, and
* eviction never removes it because it has no durable copy.
*
* @param target the local path of the compiled CSS
* @throws IOException if the local file cannot be written
* @throws DotDataException if the upload fails
*/
private void storeCompiledOutput(final File target) throws IOException, DotDataException {
final java.nio.file.Path path = target.toPath();
java.nio.file.Files.createDirectories(path.getParent());
final java.nio.file.Path temporary = java.nio.file.Files.createTempFile(path.getParent(), "css-", ".tmp");
try {
java.nio.file.Files.write(temporary, this.output);
try {
java.nio.file.Files.move(temporary, path, java.nio.file.StandardCopyOption.ATOMIC_MOVE,
java.nio.file.StandardCopyOption.REPLACE_EXISTING);
} catch (java.nio.file.AtomicMoveNotSupportedException e) {
java.nio.file.Files.move(temporary, path, java.nio.file.StandardCopyOption.REPLACE_EXISTING);
}
APILocator.getBinaryAssetStorageAPI().storeGeneratedFile(target);
} finally {
java.nio.file.Files.deleteIfExists(temporary);
}
}

/**
* Handles special situations related to how Dart SASS notifies Users when errors or warnings were generated during
* the compilation. The {@link RuntimeUtils.TerminalOutput} contains important information generated by the CLI
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@


import com.dotmarketing.business.CacheLocator;
import com.dotmarketing.exception.DotDataException;
import com.dotmarketing.util.Config;
import com.dotmarketing.util.Logger;
import com.dotmarketing.util.UtilMethods;
import java.io.InputStream;
import org.apache.commons.collections.ExtendedProperties;
import org.apache.velocity.exception.ResourceNotFoundException;
import org.apache.velocity.exception.VelocityException;
import org.apache.velocity.runtime.resource.Resource;
import org.apache.velocity.runtime.resource.loader.ResourceLoader;

Expand All @@ -22,6 +24,19 @@ public DotResourceLoader() {
}


/**
* Opens the Velocity source for a resource path, choosing the loader by the path's type.
*
* <p>A failure normally records a cache miss and is reported as a
* {@link ResourceNotFoundException}. With S3 asset storage on, a {@link DotDataException}
* from a loader that reads a stored file (VTL, macro, legacy VL and include files) means the
* file could not be restored from storage. It is reported as a plain {@link VelocityException}
* and no miss is recorded, so the next render tries the storage again.
*
* @param filePath the Velocity resource path
* @return the resource source
* @throws ResourceNotFoundException if the path is empty or the resource cannot be loaded
*/
@Override
public InputStream getResourceStream(final String filePath) throws ResourceNotFoundException {
if (!UtilMethods.isSet(filePath)) {
Expand Down Expand Up @@ -75,13 +90,40 @@ public InputStream getResourceStream(final String filePath) throws ResourceNotFo

} catch (Exception e) {
Logger.warn(this, "filePath: " + filePath + ", msg:" + e.getMessage(), e);
if (com.dotcms.storage.AssetStorageFeature.isEnabled()
&& e instanceof DotDataException && readsStoredFile(key.type)) {
throw new VelocityException("Unable to load stored resource: " + key.path, e);
}
CacheLocator.getVeloctyResourceCache().addMiss(key.path);
throw new ResourceNotFoundException("Cannot parse velocity file : " + key.path, e);
}
}

}

/**
* Tells whether a resource type is loaded from a file that S3 asset storage may have to
* restore. These are the types {@link #getResourceStream} hands to {@link VTLLoader} and
* {@link IncludeLoader}; the other loaders build their source from the database.
*
* @param type the resource type
* @return {@code true} for VTL, macro, legacy VL and include files
*/
private static boolean readsStoredFile(final VelocityType type) {
switch (type) {
case CONTAINER:
case TEMPLATE:
case CONTENT:
case FIELD:
case CONTENT_TYPE:
case SITE:
case HTMLPAGE:
return false;
default:
return true;
}
}

/*
* (non-Javadoc)
*
Expand Down
Loading
Loading