Skip to content

Commit

Permalink
srm: allow sites to configure additional TExtraInfo for srmPing respo…
Browse files Browse the repository at this point in the history
…nses

An SRM server's response to a ping request allows the server to send
an arbitrary list of key-value pairs back to the client.  Currently,
dCache SRM is hard-coded to provide two such key-value pairs: the
implementation name ('dCache') and the dCache version.

This patch allows an admin to configure any number of additional
key-value pairs. This is achieved by the admin configuring dCache
properties that have a specific prefix.  This prefix is stripped and
the resulting key-value pairs are added to the SRM ping response.

The motivation is that we want to allow dCache to advertise
admin-configurable hints to SRM clients.  The most immediate one being
to allow dCache to suggest how many concurrent bring-online requests a
particular dCache instance can sustain.

As a consequence of this design, the patch introduces generic support
for prefix-based configuration.  This may be used in other places; for
example, when allowing admins to configure third-party libraries
(e.g., Hikari).

Known limitations of this patch:

  a. the "prefix" annotation (introduced to suppress info messages
     about non-system properties) and prefix used by
     ConfigurationMapFactoryBean are only "conincidentally" the same:
     any mismatch will not be detected.

  b. the prefix checking in ConfigurationProperties has terrible
     performance as the number of prefixes increases much beyond 1.
     This should be revisited if the prefix idea is reused elsewhere.

  c. the patch deliberately avoids using Java 8 constructs.  This is
     because we may want to back-port this to supported branches.

Target: master
Requires-book: yes
Requires-notes: yes
Patch: https://rb.dcache.org/r/7591/
Acked-by: Gerd Behrmann
Acked-by: Dmitry Litvintsev
  • Loading branch information
paulmillar committed Jan 29, 2015
1 parent e128bd2 commit a90c4dc
Show file tree
Hide file tree
Showing 11 changed files with 242 additions and 57 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,11 @@
value="${srm.persistence.reserve-space.enable.clean-pending-on-restart}"/>
<property name="databaseParametersForReserve.storeCompletedRequestsOnly"
value="#{ '${srm.persistence.reserve-space.enable.store-transient-state}'.equals('false') ? true : false }"/>
<property name="pingExtraInfo">
<bean class="org.dcache.util.ConfigurationMapFactoryBean">
<property name="prefix" value="srm.ping-extra-info"/>
</bean>
</property>
</bean>

<bean id="scheduler-get" class="diskCacheV111.srm.dcache.Scheduler"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ class DcacheConfigurationUsageChecker implements ConfigurationProperties.UsageCh
@Override
public boolean isStandardProperty(Properties defaults, String name)
{
return defaults.getProperty(name) != null || GENERATED_PROPERTIES.contains(name);
boolean hasDeclaredPrefix = false;
if (defaults instanceof ConfigurationProperties) {
hasDeclaredPrefix = ((ConfigurationProperties)defaults).hasDeclaredPrefix(name);
}
return hasDeclaredPrefix || defaults.getProperty(name) != null || GENERATED_PROPERTIES.contains(name);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import java.util.Set;

import org.dcache.util.ConfigurationProperties;
import org.dcache.util.Version;

import static com.google.common.collect.Iterables.transform;
import static org.dcache.boot.Properties.*;
Expand Down Expand Up @@ -48,6 +49,8 @@ private ConfigurationProperties loadSystemProperties()
localhost.getHostName().split("\\.")[0]);
config.setProperty(PROPERTY_HOST_FQDN,
localhost.getCanonicalHostName());
config.setProperty(PROPERTY_DCACHE_VERSION,
Version.of(this).getVersion());
return config;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ public class Properties
public static final String PROPERTY_DCACHE_CONFIG_CACHE = "dcache.config.cache";
public static final String PROPERTY_DCACHE_CONFIG_FILES = "dcache.config.files";
public static final String PROPERTY_DCACHE_CONFIG_DIRS = "dcache.config.dirs";
public static final String PROPERTY_DCACHE_VERSION = "dcache.version";
public static final String PROPERTY_HOST_NAME = "host.name";
public static final String PROPERTY_HOST_FQDN = "host.fqdn";
public static final String PROPERTY_DOMAIN_NAME = "dcache.domain.name";
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
package org.dcache.util;

import com.google.common.collect.ImmutableMap;
import org.springframework.beans.factory.FactoryBean;
import org.springframework.beans.factory.annotation.Required;

import javax.annotation.PostConstruct;

import java.util.Map;

import dmg.cells.nucleus.EnvironmentAware;
import dmg.util.Formats;
import dmg.util.Replaceable;

import static com.google.common.base.Preconditions.checkNotNull;

/**
* The ConfigurationMapFactoryBean builds a Map from some (possibly empty)
* subset of dCache configuration. The Bean takes a String prefix as an
* argument. All configuration properties with a key that starts with this
* prefix are used to build the map, all others are ignored. The map entries
* are created by removing the prefix from matching property keys to form the
* map-entry's key. The corresponding map-entry's value is the property value.
*/
public class ConfigurationMapFactoryBean implements EnvironmentAware,
FactoryBean<ImmutableMap<String,String>>
{
private String _prefix;
private Map<String,Object> _environment;
private ImmutableMap<String,String> _object;

@Override
public void setEnvironment(Map<String, Object> environment)
{
_environment = environment;
}

@Required
public void setPrefix(String value)
{
_prefix = checkNotNull(value) + ConfigurationProperties.PREFIX_SEPARATOR;
}

@PostConstruct
private void buildMap()
{
ImmutableMap.Builder<String,String> builder = ImmutableMap.builder();

Replaceable replaceable = new Replaceable() {
@Override
public String getReplacement(String name)
{
Object value = _environment.get(name);
return (value == null) ? null : value.toString().trim();
}
};

int prefixLength = _prefix.length();
for (Map.Entry<String,Object> item : _environment.entrySet()) {
String name = item.getKey();
if (item.getValue() instanceof String && name.startsWith(_prefix)) {
String value = (String) item.getValue();
String key = name.substring(prefixLength);
if (!key.isEmpty()) {
builder.put(key, Formats.replaceKeywords(value, replaceable));
}
}
}

_object = builder.build();
}

@Override
public ImmutableMap<String, String> getObject() throws Exception
{
return _object;
}

@Override
public Class<?> getObjectType()
{
return ImmutableMap.class;
}

@Override
public boolean isSingleton()
{
return true;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,15 @@
import java.io.InputStream;
import java.io.LineNumberReader;
import java.io.Reader;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumMap;
import java.util.EnumSet;
import java.util.Enumeration;
import java.util.HashMap;
import java.util.InvalidPropertiesFormatException;
import java.util.List;
import java.util.Map;
import java.util.Properties;
import java.util.Set;
Expand Down Expand Up @@ -94,6 +96,12 @@ public class ConfigurationProperties
{
private static final long serialVersionUID = -5684848160314570455L;

/**
* The character that separates the prefix from the key for PREFIX-annotated
* properties.
*/
public static final String PREFIX_SEPARATOR = "!";

private static final Set<Annotation> OBSOLETE_FORBIDDEN =
EnumSet.of(Annotation.OBSOLETE, Annotation.FORBIDDEN);

Expand All @@ -106,6 +114,7 @@ public class ConfigurationProperties
private final Map<String,AnnotatedKey> _annotatedKeys =
new HashMap<>();
private final UsageChecker _usageChecker;
private final List<String> _prefixes = new ArrayList<>();

private boolean _loading;
private boolean _isService;
Expand All @@ -127,7 +136,9 @@ public ConfigurationProperties(Properties defaults, UsageChecker usageChecker)
super(defaults);

if( defaults instanceof ConfigurationProperties) {
_problemConsumer = ((ConfigurationProperties) defaults)._problemConsumer;
ConfigurationProperties defaultConfig = (ConfigurationProperties) defaults;
_problemConsumer = defaultConfig._problemConsumer;
_prefixes.addAll(defaultConfig._prefixes);
}
_usageChecker = usageChecker;
}
Expand All @@ -147,6 +158,17 @@ public void setIsService(boolean isService)
_isService = isService;
}

public boolean hasDeclaredPrefix(String name)
{
for (String prefix : _prefixes) {
if (name.startsWith(prefix)) {
return true;
}
}

return false;
}

/**
* @throws IllegalArgumentException during loading if a property
* is defined multiple times.
Expand Down Expand Up @@ -252,6 +274,10 @@ public synchronized Object put(Object rawKey, Object value)
return null;
}

if (key.hasAnnotation(Annotation.PREFIX)) {
_prefixes.add(key.getPropertyName() + PREFIX_SEPARATOR);
}

checkIsAllowed(key, (String) value);

if (key.hasAnnotations()) {
Expand Down Expand Up @@ -286,33 +312,20 @@ private void checkKeyValid(AnnotatedKey existingKey, AnnotatedKey key)
{
String name = key.getPropertyName();

if(existingKey.hasAnnotations() && key.hasAnnotations()) {
if (existingKey.hasAnnotations() && key.hasAnnotations()) {
_problemConsumer.error("Property " + name + ": " +
"remove \"" + key.getAnnotationDeclaration() + "\"; " +
"annotated assignments are not allowed");
}

if(existingKey.hasAnnotation(Annotation.IMMUTABLE)) {
_problemConsumer.error(immutableErrorMessageFor(existingKey));
}

if(existingKey.hasAnnotation(Annotation.FORBIDDEN)) {
_problemConsumer.error(forbiddenErrorMessageFor(existingKey));
}

if(existingKey.hasAnnotation(Annotation.OBSOLETE)) {
_problemConsumer.warning(obsoleteErrorMessageFor(existingKey));
if (existingKey.hasAnyOf(EnumSet.of(Annotation.IMMUTABLE,
Annotation.PREFIX, Annotation.FORBIDDEN))) {
_problemConsumer.error(messageFor(existingKey));
}

if(existingKey.hasAnnotation(Annotation.DEPRECATED)) {
_problemConsumer.warning("Property " + name + ": " +
deprecatedWarningInstructionsFor(name) + "; support for " +
name + " will be removed in the future");
}

if(_isService && existingKey.hasAnnotation(Annotation.NOT_FOR_SERVICES)) {
_problemConsumer.warning("Property " + name
+ ": consider moving to a domain scope; it has no effect here");
if ((_isService && existingKey.hasAnnotation(Annotation.NOT_FOR_SERVICES)) ||
existingKey.hasAnyOf(EnumSet.of(Annotation.OBSOLETE, Annotation.DEPRECATED))) {
_problemConsumer.warning(messageFor(existingKey));
}
}

Expand Down Expand Up @@ -348,17 +361,6 @@ private void checkDataValid(AnnotatedKey key, String value)
}
}

private String deprecatedWarningInstructionsFor(String propertyName)
{
String synonym = findSynonymOf(propertyName);

if(synonym != null) {
return "use \"" + synonym + "\" instead";
} else {
return "please review configuration";
}
}

/**
* Define the binary relationship property A hasSynonym property B
* as true iff either:
Expand Down Expand Up @@ -404,28 +406,38 @@ private String findSynonymOf(String propertyName)
return synonym;
}

private String forbiddenErrorMessageFor(AnnotatedKey key)
{
String customError = key.getError();

String suffix = customError.isEmpty() ? "this property no longer affects dCache" :
customError;

return "Property " + key.getPropertyName() + ": may not be adjusted; " + suffix;
}

private String immutableErrorMessageFor(AnnotatedKey key)
private String messageFor(AnnotatedKey key)
{
return "Property " + key.getPropertyName() + ": may not be adjusted as it is marked 'immutable'";
}

private String obsoleteErrorMessageFor(AnnotatedKey key)
{
String customError = key.getError();
String name = key.getPropertyName();

String suffix = customError.isEmpty() ? "it has no effect" : customError;
StringBuilder sb = new StringBuilder();
sb.append("Property ").append(name).append(": ");

if (key.hasAnnotation(Annotation.IMMUTABLE)) {
sb.append("may not be adjusted as it is marked 'immutable'");
} else if (key.hasAnnotation(Annotation.PREFIX)) {
sb.append("may not be adjusted as it is marked 'prefix'");
} else if (key.hasAnnotation(Annotation.FORBIDDEN)) {
sb.append("may not be adjusted; ");
sb.append(key.hasError() ? key.getError() : "this property no longer affects dCache");
} else if (key.hasAnnotation(Annotation.OBSOLETE)) {
sb.append("please remove this assignment; ");
sb.append(key.hasError() ? key.getError() : "it has no effect");
} else if(key.hasAnnotation(Annotation.DEPRECATED)) {
String synonym = findSynonymOf(name);
if (synonym != null) {
sb.append("use \"").append(synonym).append("\" instead");
} else {
sb.append("please review configuration");
}
sb.append("; support for ").append(name).append(" will be removed in the future");
} else if (key.hasAnnotation(Annotation.NOT_FOR_SERVICES)) {
sb.append("consider moving to a domain scope; it has no effect here");
} else {
sb.append("has an unknown problem");
}

return "Property " + key.getPropertyName() + ": please remove this assignment; " + suffix;
return sb.toString();
}

@Override
Expand Down Expand Up @@ -577,6 +589,10 @@ public String getError() {
return _error;
}

public boolean hasError() {
return !_error.isEmpty();
}

public String getParameter(Annotation annotation) {
String parameter = _annotations.get(annotation);

Expand All @@ -602,7 +618,8 @@ public enum Annotation
DEPRECATED("deprecated"),
NOT_FOR_SERVICES("not-for-services"),
IMMUTABLE("immutable"),
ANY_OF("any-of", true);
ANY_OF("any-of", true),
PREFIX("prefix");

private static final Map<String,Annotation> ANNOTATION_LABELS =
new HashMap<>();
Expand Down
Loading

0 comments on commit a90c4dc

Please sign in to comment.