diff options
70 files changed, 930 insertions, 692 deletions
diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/Search.java b/config-model/src/main/java/com/yahoo/searchdefinition/Search.java index a988da9664e..d7c4c27b2b0 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/Search.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/Search.java @@ -3,6 +3,7 @@ package com.yahoo.searchdefinition; import com.yahoo.config.application.api.ApplicationPackage; import com.yahoo.document.Field; +import com.yahoo.searchdefinition.derived.SummaryClass; import com.yahoo.searchdefinition.document.Attribute; import com.yahoo.searchdefinition.document.ImmutableImportedSDField; import com.yahoo.searchdefinition.document.ImmutableSDField; @@ -46,12 +47,10 @@ public class Search implements Serializable, ImmutableSearch { private static final Logger log = Logger.getLogger(Search.class.getName()); private static final String SD_DOC_FIELD_NAME = "sddocname"; private static final List<String> RESERVED_NAMES = Arrays.asList( - "index", "index_url", "summary", "attribute", "select_input", "host", "documentid", + "index", "index_url", "summary", "attribute", "select_input", "host", SummaryClass.DOCUMENT_ID_FIELD, "position", "split_foreach", "tokenize", "if", "else", "switch", "case", SD_DOC_FIELD_NAME, "relevancy"); - /** - * @return True if the given field name is a reserved name. - */ + /** Returns true if the given field name is a reserved name */ public static boolean isReservedName(String name) { return RESERVED_NAMES.contains(name); } @@ -99,6 +98,7 @@ public class Search implements Serializable, ImmutableSearch { /** * Creates a proper search definition + * * @param name of the the searchdefinition * @param applicationPackage the application containing this */ diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java b/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java index 151ad02a3fa..d8ec0b053ad 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/SearchBuilder.java @@ -332,7 +332,11 @@ public class SearchBuilder { * @throws ParseException if there was a problem parsing the file content. */ public static SearchBuilder createFromFile(String fileName) throws IOException, ParseException { - return createFromFile(fileName, new BaseDeployLogger(), new RankProfileRegistry(), new QueryProfileRegistry()); + return createFromFile(fileName, new BaseDeployLogger()); + } + + public static SearchBuilder createFromFile(String fileName, DeployLogger logger) throws IOException, ParseException { + return createFromFile(fileName, logger, new RankProfileRegistry(), new QueryProfileRegistry()); } /** diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/SummaryClass.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/SummaryClass.java index dd5826e716f..56cfb2a595c 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/SummaryClass.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/SummaryClass.java @@ -24,6 +24,8 @@ import java.util.logging.Level; */ public class SummaryClass extends Derived { + public static final String DOCUMENT_ID_FIELD = "documentid"; + /** True if this summary class needs to access summary information on disk */ private boolean accessingDiskSummary=false; @@ -53,7 +55,7 @@ public class SummaryClass extends Derived { /** MUST be called after all other fields are added */ private void deriveImplicitFields(DocumentSummary summary) { if (summary.getName().equals("default")) { - addField("documentid", DataType.STRING); + addField(SummaryClass.DOCUMENT_ID_FIELD, DataType.STRING); } } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmSummary.java b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmSummary.java index 471234928b4..17210452b3f 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmSummary.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/derived/VsmSummary.java @@ -17,6 +17,7 @@ import java.util.*; * @author bratseth */ public class VsmSummary extends Derived implements VsmsummaryConfig.Producer { + private Map<SummaryField, List<String>> summaryMap = new java.util.LinkedHashMap<>(1); public VsmSummary(Search search) { @@ -80,10 +81,10 @@ public class VsmSummary extends Derived implements VsmsummaryConfig.Producer { return true; } - private List<String> toStringList(Iterator i) { + private List<String> toStringList(Iterator<SummaryField.Source> i) { List<String> ret = new ArrayList<>(); while (i.hasNext()) { - ret.add(i.next().toString()); + ret.add(i.next().getName()); } return ret; } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java index ff11c2fdf22..dc2a1a2bbae 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableImportedSDField.java @@ -5,6 +5,7 @@ import com.yahoo.document.DataType; import com.yahoo.document.Field; import com.yahoo.searchdefinition.Index; import com.yahoo.searchdefinition.Search; +import com.yahoo.vespa.documentmodel.SummaryField; import com.yahoo.vespa.indexinglanguage.expressions.Expression; import com.yahoo.vespa.indexinglanguage.expressions.ScriptExpression; @@ -150,6 +151,11 @@ public class ImmutableImportedSDField implements ImmutableSDField { } @Override + public Map<String, SummaryField> getSummaryFields() { + throw createUnsupportedException("summary fields"); + } + + @Override public String getName() { return importedField.fieldName(); // Name of the imported field, not the target field } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java index 6fe8a4da92b..4ae7561a7bc 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/ImmutableSDField.java @@ -5,10 +5,12 @@ import com.yahoo.document.DataType; import com.yahoo.document.Field; import com.yahoo.searchdefinition.Index; import com.yahoo.searchdefinition.Search; +import com.yahoo.vespa.documentmodel.SummaryField; import com.yahoo.vespa.indexinglanguage.expressions.Expression; import com.yahoo.vespa.indexinglanguage.expressions.ScriptExpression; import java.util.Collection; +import java.util.Collections; import java.util.List; import java.util.Map; @@ -76,9 +78,9 @@ public interface ImmutableSDField { String getName(); - /** - * @return a {@link Field} representation (which is sadly not immutable). - */ + Map<String, SummaryField> getSummaryFields(); + + /** Returns a {@link Field} representation (which is sadly not immutable) */ Field asField(); boolean hasFullIndexingDocprocRights(); diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java b/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java index 6fc896b90c6..55260b6e68f 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/document/SDField.java @@ -78,10 +78,10 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer, private List<String> queryCommands=new java.util.ArrayList<>(0); /** Summary fields defined in this field */ - private Map<String,SummaryField> summaryFields = new java.util.LinkedHashMap<>(0); + private Map<String, SummaryField> summaryFields = new java.util.LinkedHashMap<>(0); /** The explicitly index settings on this field */ - private Map<String, Index> indices=new java.util.LinkedHashMap<>(); + private Map<String, Index> indices = new java.util.LinkedHashMap<>(); /** True if body or header is set explicitly for this field */ private boolean headerOrBodyDefined = false; @@ -254,7 +254,7 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer, * to the main summary-field for the struct field. */ for (SDField structField : getStructFields()) { - for (SummaryField sumF : structField.getSummaryFields()) { + for (SummaryField sumF : structField.getSummaryFields().values()) { for (String dest : sumF.getDestinations()) { summaryField.addDestination(dest); } @@ -672,15 +672,17 @@ public class SDField extends Field implements TypedKey, FieldOperationContainer, this.stemming=stemming; } - /** - * List of static summary fields - * @return list of static summary fields - */ - public Collection<SummaryField> getSummaryFields() { return summaryFields.values(); } + /** Returns an unmodifiable map of the summary fields defined in this */ + @Override + public Map<String, SummaryField> getSummaryFields() { + return Collections.unmodifiableMap(summaryFields); + } - /** - * Add summary field - */ + public void removeSummaryFields() { + summaryFields.clear(); + } + + /** Adds a summary field */ public void addSummaryField(SummaryField summaryField) { summaryFields.put(summaryField.getName(),summaryField); } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldLongOperation.java b/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldLongOperation.java index 4d6e077aaaa..f05873c5c6b 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldLongOperation.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldLongOperation.java @@ -11,9 +11,10 @@ import java.util.List; import java.util.Set; /** - * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a> + * @author Einar M R Rosenvinge */ public class SummaryInFieldLongOperation extends SummaryInFieldOperation { + private DataType type; private Boolean bold; private Set<String> destinations = new java.util.LinkedHashSet<>(); diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldOperation.java b/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldOperation.java index 6abbc2e9ac8..b36a1cb0cbc 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldOperation.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/fieldoperation/SummaryInFieldOperation.java @@ -7,9 +7,10 @@ import com.yahoo.vespa.documentmodel.SummaryTransform; import java.util.Set; /** - * @author <a href="mailto:einarmr@yahoo-inc.com">Einar M R Rosenvinge</a> + * @author Einar M R Rosenvinge */ public abstract class SummaryInFieldOperation implements FieldOperation { + protected String name; protected SummaryTransform transform; protected Set<SummaryField.Source> sources = new java.util.LinkedHashSet<>(); @@ -41,4 +42,5 @@ public abstract class SummaryInFieldOperation implements FieldOperation { public void addSource(SummaryField.Source source) { sources.add(source); } + } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Bolding.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Bolding.java index b9be30e8485..b59d3527e87 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Bolding.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Bolding.java @@ -25,7 +25,7 @@ public class Bolding extends Processor { public void process(boolean validate, boolean documentsOnly) { if ( ! validate) return; for (SDField field : search.allConcreteFields()) { - for (SummaryField summary : field.getSummaryFields()) { + for (SummaryField summary : field.getSummaryFields().values()) { if (summary.getTransform().isBolded() && !((summary.getDataType() == DataType.STRING) || (summary.getDataType() == DataType.URI))) { diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/CreatePositionZCurve.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/CreatePositionZCurve.java index ad862ef767f..c56c4f6b056 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/CreatePositionZCurve.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/CreatePositionZCurve.java @@ -133,11 +133,11 @@ public class CreatePositionZCurve extends Processor { private Set<String> removeSummaryTo(SDField field) { Set<String> summaryTo = new HashSet<>(); - Collection<SummaryField> summaryFields = field.getSummaryFields(); + Collection<SummaryField> summaryFields = field.getSummaryFields().values(); for (SummaryField summary : summaryFields) { summaryTo.addAll(summary.getDestinations()); } - summaryFields.clear(); + field.removeSummaryFields(); return summaryTo; } diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImplicitSummaries.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImplicitSummaries.java index 1f795458875..724fb060dc2 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImplicitSummaries.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/ImplicitSummaries.java @@ -98,7 +98,7 @@ public class ImplicitSummaries extends Processor { } // Explicits - for (SummaryField summaryField : field.getSummaryFields()) { + for (SummaryField summaryField : field.getSummaryFields().values()) { // Make sure we fetch from attribute here too Attribute attribute = field.getAttributes().get(fieldName); if (attribute != null && summaryField.getTransform() == SummaryTransform.NONE) { diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingOutputs.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingOutputs.java index 11d69bf6c75..4feb5b6103b 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingOutputs.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/IndexingOutputs.java @@ -82,7 +82,7 @@ public class IndexingOutputs extends Processor { } private static void fillSummaryToFromField(SDField field, Set<String> dynamicSummary, Set<String> staticSummary) { - for (SummaryField summaryField : field.getSummaryFields()) { + for (SummaryField summaryField : field.getSummaryFields().values()) { String summaryName = summaryField.getName(); if (summaryField.getTransform().isDynamic()) { dynamicSummary.add(summaryName); diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java index 1af2a979cb4..f744eeb82e1 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/Processing.java @@ -62,6 +62,7 @@ public class Processing { FilterFieldNames::new, MatchConsistency::new, ValidateFieldTypes::new, + SummaryDiskAccessValidator::new, DisallowComplexMapAndWsetKeyTypes::new, SortingSettings::new, FieldSetValidity::new, @@ -80,7 +81,7 @@ public class Processing { IndexingValues::new); } - /** Processors of rank profiles only (those who tolerate and so something useful when the search field is null) */ + /** Processors of rank profiles only (those who tolerate and do something useful when the search field is null) */ private Collection<ProcessorFactory> rankProfileProcessors() { return Arrays.asList( RankProfileTypeSettingsProcessor::new, diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDiskAccessValidator.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDiskAccessValidator.java new file mode 100644 index 00000000000..d30006a753e --- /dev/null +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDiskAccessValidator.java @@ -0,0 +1,65 @@ +package com.yahoo.searchdefinition.processing; + +import com.yahoo.config.application.api.DeployLogger; +import com.yahoo.searchdefinition.RankProfileRegistry; +import com.yahoo.searchdefinition.Search; +import com.yahoo.searchdefinition.derived.SummaryClass; +import com.yahoo.searchdefinition.document.ImmutableSDField; +import com.yahoo.vespa.documentmodel.DocumentSummary; +import com.yahoo.vespa.documentmodel.SummaryField; +import com.yahoo.vespa.model.container.search.QueryProfiles; + +import java.util.Optional; +import java.util.logging.Level; + +/** + * Emits a warning for summaries which accesses disk. + * + * @author bratseth + */ +public class SummaryDiskAccessValidator extends Processor { + + public SummaryDiskAccessValidator(Search search, + DeployLogger deployLogger, + RankProfileRegistry rankProfileRegistry, + QueryProfiles queryProfiles) { + super(search, deployLogger, rankProfileRegistry, queryProfiles); + } + + + @Override + public void process(boolean validate, boolean documentsOnly) { + if ( ! validate) return; + if (documentsOnly) return; + + for (DocumentSummary summary : search.getSummaries().values()) { + for (SummaryField summaryField : summary.getSummaryFields()) { + for (SummaryField.Source source : summaryField.getSources()) { + ImmutableSDField field = search.getField(source.getName()); + if (field == null) + field = findFieldProducingSummaryField(source.getName(), search).orElse(null); + if (field == null && ! source.getName().equals(SummaryClass.DOCUMENT_ID_FIELD)) + throw new IllegalArgumentException(summaryField + " in " + summary + " references " + + source + ", but this field does not exist"); + if ( ! isInMemory(field) && ! summary.isFromDisk()) { + // TODO: Set to warning on vespa 7 + deployLogger.log(Level.FINE, summaryField + " in " + summary + " references " + + source + ", which is not an attribute: Using this " + + "summary will cause disk accesses. " + + "Set 'from-disk' on this summary class to silence this warning."); + } + } + } + } + } + + private boolean isInMemory(ImmutableSDField field) { + if (field == null) return false; // For DOCUMENT_ID_FIELD, which may be implicit, but is then not in memory + return field.doesAttributing(); + } + + private Optional<ImmutableSDField> findFieldProducingSummaryField(String name, Search search) { + return search.allFields().filter(field -> field.getSummaryFields().get(name) != null).findAny(); + } + +} diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDynamicStructsArrays.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDynamicStructsArrays.java index b8d170c07f6..255e95e9667 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDynamicStructsArrays.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryDynamicStructsArrays.java @@ -32,7 +32,7 @@ public class SummaryDynamicStructsArrays extends Processor { for (SDField field : search.allConcreteFields()) { DataType type = field.getDataType(); if (type instanceof ArrayDataType || type instanceof WeightedSetDataType || type instanceof StructDataType) { - for (SummaryField sField : field.getSummaryFields()) { + for (SummaryField sField : field.getSummaryFields().values()) { if (sField.getTransform().equals(SummaryTransform.DYNAMICTEASER)) { throw new IllegalArgumentException("For field '"+field.getName()+"': dynamic summary is illegal " + "for fields of type struct, array or weighted set. Use an " + diff --git a/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryFieldsMustHaveValidSource.java b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryFieldsMustHaveValidSource.java index 9b51c7c473e..c87801685bb 100644 --- a/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryFieldsMustHaveValidSource.java +++ b/config-model/src/main/java/com/yahoo/searchdefinition/processing/SummaryFieldsMustHaveValidSource.java @@ -4,6 +4,7 @@ package com.yahoo.searchdefinition.processing; import com.yahoo.config.application.api.DeployLogger; import com.yahoo.searchdefinition.RankProfileRegistry; import com.yahoo.searchdefinition.Search; +import com.yahoo.searchdefinition.derived.SummaryClass; import com.yahoo.vespa.documentmodel.DocumentSummary; import com.yahoo.vespa.documentmodel.SummaryField; import com.yahoo.vespa.documentmodel.SummaryTransform; @@ -13,7 +14,6 @@ import com.yahoo.vespa.model.container.search.QueryProfiles; * Verifies that the source fields actually refers to a valid field. * * @author baldersheim - * */ public class SummaryFieldsMustHaveValidSource extends Processor { @@ -51,7 +51,7 @@ public class SummaryFieldsMustHaveValidSource extends Processor { return isDocumentField(source) || (isNotInThisSummaryClass(summary, source) && isSummaryField(source)) || (isInThisSummaryClass(summary, source) && !source.equals(summaryField.getName())) || - ("documentid".equals(source)); + (SummaryClass.DOCUMENT_ID_FIELD.equals(source)); } private void verifySource(String source, SummaryField summaryField, DocumentSummary summary) { diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java index 3860a1b278c..8dc5356026b 100644 --- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java +++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/DocumentSummary.java @@ -15,6 +15,8 @@ import java.util.List; */ public class DocumentSummary extends FieldView { + private boolean fromDisk = false; + /** * Creates a DocumentSummary with the given name. * @@ -24,6 +26,11 @@ public class DocumentSummary extends FieldView { super(name); } + public void setFromDisk(boolean fromDisk) { this.fromDisk = fromDisk; } + + /** Returns whether the user has noted explicitly that this summary accesses disk */ + public boolean isFromDisk() { return fromDisk; } + /** * The model is constrained to ensure that summary fields of the same name * in different classes have the same summary transform, because this is diff --git a/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java b/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java index 049c1f1318b..0f162ee9d51 100644 --- a/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java +++ b/config-model/src/main/java/com/yahoo/vespa/documentmodel/SummaryField.java @@ -18,9 +18,10 @@ import static com.yahoo.text.Lowercase.toLowerCase; public class SummaryField extends Field implements Cloneable, TypedKey { /** - * This class represents a source (field name). + * A source (field name). */ public static class Source implements Serializable { + private String name; private boolean override = false; public Source(String name) { @@ -29,9 +30,13 @@ public class SummaryField extends Field implements Cloneable, TypedKey { public String getName() { return name; } public void setOverride(boolean override) { this.override = override; } public boolean getOverride() { return override; } + + @Override public int hashCode() { return name.hashCode() + Boolean.valueOf(override).hashCode(); } + + @Override public boolean equals(Object obj) { if (!(obj instanceof Source)) { return false; @@ -40,9 +45,12 @@ public class SummaryField extends Field implements Cloneable, TypedKey { return name.equals(other.name) && override == other.override; } + + @Override public String toString() { - return name; + return "source field '" + name + "'"; } + } /** A name-value property (used for smart summary) */ @@ -267,12 +275,7 @@ public class SummaryField extends Field implements Cloneable, TypedKey { } public String toString() { - return - "summary field '" + getName() + ' ' + getDestinationString() + - "' [type: '" + getDataType().getName() + - "' transform: '" + transform + - "', source: '" + toString(sources) + - "', to '" + toString(destinations) + "']"; + return "summary field '" + getName() + "'"; } /** Returns a string which aids locating this field in the source search definition */ diff --git a/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java b/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java index 2ecbd891fe0..2919812e8e4 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/admin/monitoring/VespaMetricSet.java @@ -334,6 +334,7 @@ public class VespaMetricSet { metrics.add(new Metric("content.proton.documentdb.matching.rank_profile.query_collateral_time.average")); metrics.add(new Metric("content.proton.documentdb.matching.rank_profile.query_latency.average")); metrics.add(new Metric("content.proton.documentdb.matching.rank_profile.docs_matched.rate")); + metrics.add(new Metric("content.proton.documentdb.matching.rank_profile.limited_queries.rate")); return metrics; } diff --git a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeMessageBuilder.java b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeMessageBuilder.java index ce430c27013..d680f6bd37c 100644 --- a/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeMessageBuilder.java +++ b/config-model/src/main/java/com/yahoo/vespa/model/application/validation/change/search/IndexingScriptChangeMessageBuilder.java @@ -71,7 +71,7 @@ public class IndexingScriptChangeMessageBuilder { } private void checkSummaryTransform(ChangeMessageBuilder builder) { - for (SummaryField nextSummaryField : nextField.getSummaryFields()) { + for (SummaryField nextSummaryField : nextField.getSummaryFields().values()) { String fieldName = nextSummaryField.getName(); SummaryField currentSummaryField = currentField.getSummaryField(fieldName); if (currentSummaryField != null) { diff --git a/config-model/src/main/javacc/SDParser.jj b/config-model/src/main/javacc/SDParser.jj index 40da779dd20..d1c67a6d425 100644 --- a/config-model/src/main/javacc/SDParser.jj +++ b/config-model/src/main/javacc/SDParser.jj @@ -286,6 +286,7 @@ TOKEN : | < SOURCE: "source" > | < TO: "to" > | < DIRECT: "direct" > +| < FROMDISK: "from-disk" > | < ALWAYS: "always" > | < ONDEMAND: "on-demand" > | < NEVER: "never" > @@ -1715,9 +1716,16 @@ Object documentSummary(Search search) : } { ( ( <DOCUMENTSUMMARY> | - <SUMMARY> { deployLogger.log(Level.WARNING, "Directive 'summary' is deprecated, use 'document-summary' instead."); } ) + <SUMMARY> { deployLogger.log(Level.WARNING, "Directive 'summary' is deprecated, use 'document-summary' instead."); } ) // TODO: Remove on Vespa 7 name = identifier() { search.addSummary(summary = new DocumentSummary(name)); } - lbrace() (documentSummaryItem(summary) (<NL>)*)* <RBRACE> ) + lbrace() + ( + <FROMDISK> { summary.setFromDisk(true); } | + documentSummaryItem(summary) | + <NL> + )* + <RBRACE> + ) { return null; } } diff --git a/config-model/src/test/examples/disksummary.sd b/config-model/src/test/examples/disksummary.sd new file mode 100644 index 00000000000..766f0e0d2e1 --- /dev/null +++ b/config-model/src/test/examples/disksummary.sd @@ -0,0 +1,14 @@ +search disksummary { + + document disksummary { + + field inmemory type string { + indexing: attribute | summary + } + field ondisk type string { + indexing: index | summary + } + + } + +}
\ No newline at end of file diff --git a/config-model/src/test/examples/disksummaryexplicit.sd b/config-model/src/test/examples/disksummaryexplicit.sd new file mode 100644 index 00000000000..b0179e04801 --- /dev/null +++ b/config-model/src/test/examples/disksummaryexplicit.sd @@ -0,0 +1,18 @@ +search disksummary { + + document disksummary { + + field inmemory type string { + indexing: attribute | summary + } + field ondisk type string { + indexing: index | summary + } + + } + + document-summary default { + from-disk + } + +}
\ No newline at end of file diff --git a/config-model/src/test/examples/memorysummary.sd b/config-model/src/test/examples/memorysummary.sd new file mode 100644 index 00000000000..79be30bc0fd --- /dev/null +++ b/config-model/src/test/examples/memorysummary.sd @@ -0,0 +1,14 @@ +search memorysummary { + + document memorysummary { + + field inmemory type string { + indexing: attribute | summary + } + field ondisk type string { + indexing: index # no summary, so ignored + } + + } + +}
\ No newline at end of file diff --git a/config-model/src/test/java/com/yahoo/searchdefinition/SummaryTestCase.java b/config-model/src/test/java/com/yahoo/searchdefinition/SummaryTestCase.java new file mode 100644 index 00000000000..6fa716d9b76 --- /dev/null +++ b/config-model/src/test/java/com/yahoo/searchdefinition/SummaryTestCase.java @@ -0,0 +1,47 @@ +package com.yahoo.searchdefinition; + +import com.yahoo.config.application.api.DeployLogger; +import com.yahoo.searchdefinition.parser.ParseException; +import com.yahoo.vespa.model.test.utils.DeployLoggerStub; +import org.junit.Test; + +import java.io.IOException; +import java.util.logging.Level; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * Tests summary validation + * + * @author bratseth + */ +public class SummaryTestCase { + + @Test + public void testMemorySummary() throws IOException, ParseException { + DeployLoggerStub logger = new DeployLoggerStub(); + SearchBuilder.createFromFile("src/test/examples/memorysummary.sd", logger); + assertTrue(logger.entries.isEmpty()); + } + + @Test + public void testDiskSummary() throws IOException, ParseException { + DeployLoggerStub logger = new DeployLoggerStub(); + SearchBuilder.createFromFile("src/test/examples/disksummary.sd", logger); + assertEquals(1, logger.entries.size()); + assertEquals(Level.FINE, logger.entries.get(0).level); + assertEquals("summary field 'ondisk' in document summary 'default' references source field 'ondisk', " + + "which is not an attribute: Using this summary will cause disk accesses. " + + "Set 'from-disk' on this summary class to silence this warning.", + logger.entries.get(0).message); + } + + @Test + public void testDiskSummaryExplicit() throws IOException, ParseException { + DeployLoggerStub logger = new DeployLoggerStub(); + SearchBuilder.createFromFile("src/test/examples/disksummaryexplicit.sd", logger); + assertTrue(logger.entries.isEmpty()); + } + +} diff --git a/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java b/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java index a6607092d8e..f438b8ee76f 100644 --- a/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java +++ b/config-provisioning/src/main/java/com/yahoo/config/provision/HostSpec.java @@ -33,11 +33,6 @@ public class HostSpec implements Comparable<HostSpec> { this(hostname, new ArrayList<>(), Optional.empty(), membership); } - // TODO: Remove after May 2018 - public HostSpec(String hostname, ClusterMembership membership, Flavor flavor) { - this(hostname, new ArrayList<>(), Optional.of(flavor), Optional.of(membership)); - } - public HostSpec(String hostname, ClusterMembership membership, Flavor flavor, Optional<com.yahoo.component.Version> version) { this(hostname, new ArrayList<>(), Optional.of(flavor), Optional.of(membership), version); } diff --git a/container-accesslogging/src/main/java/com/yahoo/container/logging/LogFileHandler.java b/container-accesslogging/src/main/java/com/yahoo/container/logging/LogFileHandler.java index 33896c870a5..8bb072b5a64 100644 --- a/container-accesslogging/src/main/java/com/yahoo/container/logging/LogFileHandler.java +++ b/container-accesslogging/src/main/java/com/yahoo/container/logging/LogFileHandler.java @@ -5,6 +5,7 @@ import com.yahoo.concurrent.ThreadFactoryFactory; import com.yahoo.container.core.AccessLogConfig; import com.yahoo.io.NativeIO; import com.yahoo.log.LogFileDb; +import com.yahoo.system.ProcessExecuter; import java.io.File; import java.io.FileInputStream; @@ -25,7 +26,6 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.zip.GZIPOutputStream; - /** * <p>Implements log file naming/rotating logic for container logs.</p> * @@ -308,11 +308,9 @@ public class LogFileHandler extends StreamHandler { } String [] cmd = new String[]{"/bin/ln", "-sf", canonicalPath, f2.getPath()}; try { - Runtime r = Runtime.getRuntime(); - Process p = r.exec(cmd); + int retval = new ProcessExecuter().exec(cmd).getFirst(); // Detonator pattern: Think of all the fun we can have if ln isn't what we // think it is, if it doesn't return, etc, etc - int retval = p.waitFor(); if (retval != 0) { logger.warning("Command '" + Arrays.toString(cmd) + "' + failed with exitcode=" + retval); } diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java index eeb5a3af79a..08dcbe17db2 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4InvokerFactory.java @@ -43,7 +43,7 @@ public class FS4InvokerFactory { } public SearchInvoker getSearchInvoker(Query query, SearchCluster.Node node) { - return new FS4SearchInvoker(searcher, query, fs4ResourcePool, node.hostname(), node.fs4port(), node.key()); + return new FS4SearchInvoker(searcher, query, fs4ResourcePool, node); } public Optional<SearchInvoker> getSearchInvoker(Query query, List<SearchCluster.Node> nodes) { diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java index 82f87fcac19..ac48aef7063 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FS4SearchInvoker.java @@ -11,10 +11,13 @@ import com.yahoo.fs4.mplex.FS4Channel; import com.yahoo.fs4.mplex.InvalidChannelException; import com.yahoo.search.Query; import com.yahoo.search.Result; +import com.yahoo.search.dispatch.SearchCluster; import com.yahoo.search.dispatch.SearchInvoker; +import com.yahoo.search.result.Coverage; import com.yahoo.search.result.ErrorMessage; import java.io.IOException; +import java.util.Arrays; import java.util.List; import java.util.Optional; import java.util.logging.Level; @@ -30,18 +33,17 @@ import static java.util.Arrays.asList; public class FS4SearchInvoker extends SearchInvoker { private final VespaBackEndSearcher searcher; private FS4Channel channel; - private final Optional<Integer> distributionKey; + private final Optional<SearchCluster.Node> node; private ErrorMessage pendingSearchError = null; private Query query = null; private QueryPacket queryPacket = null; - public FS4SearchInvoker(VespaBackEndSearcher searcher, Query query, FS4ResourcePool fs4ResourcePool, String hostname, int port, - int distributionKey) { + public FS4SearchInvoker(VespaBackEndSearcher searcher, Query query, FS4ResourcePool fs4ResourcePool, SearchCluster.Node node) { this.searcher = searcher; - this.distributionKey = Optional.of(distributionKey); + this.node = Optional.of(node); - Backend backend = fs4ResourcePool.getBackend(hostname, port); + Backend backend = fs4ResourcePool.getBackend(node.hostname(), node.fs4port()); this.channel = backend.openChannel(); channel.setQuery(query); } @@ -49,7 +51,7 @@ public class FS4SearchInvoker extends SearchInvoker { // fdispatch code path public FS4SearchInvoker(VespaBackEndSearcher searcher, Query query, Backend backend) { this.searcher = searcher; - this.distributionKey = Optional.empty(); + this.node = Optional.empty(); this.channel = backend.openChannel(); channel.setQuery(query); } @@ -82,20 +84,20 @@ public class FS4SearchInvoker extends SearchInvoker { @Override protected List<Result> getSearchResults(CacheKey cacheKey) throws IOException { if(pendingSearchError != null) { - return asList(new Result(query, pendingSearchError)); + return errorResult(pendingSearchError); } BasicPacket[] basicPackets; try { basicPackets = channel.receivePackets(query.getTimeLeft(), 1); } catch (ChannelTimeoutException e) { - return asList(new Result(query, ErrorMessage.createTimeout("Timeout while waiting for " + getName()))); + return errorResult(ErrorMessage.createTimeout("Timeout while waiting for " + getName())); } catch (InvalidChannelException e) { - return asList(new Result(query, ErrorMessage.createBackendCommunicationError("Invalid channel for " + getName()))); + return errorResult(ErrorMessage.createBackendCommunicationError("Invalid channel for " + getName())); } if (basicPackets.length == 0) { - return asList(new Result(query, ErrorMessage.createBackendCommunicationError(getName() + " got no packets back"))); + return errorResult(ErrorMessage.createBackendCommunicationError(getName() + " got no packets back")); } if (isLoggingFine()) @@ -114,7 +116,7 @@ public class FS4SearchInvoker extends SearchInvoker { searcher.addMetaInfo(query, queryPacket.getQueryPacketData(), resultPacket, result); - searcher.addUnfilledHits(result, resultPacket.getDocuments(), false, queryPacket.getQueryPacketData(), cacheKey, distributionKey); + searcher.addUnfilledHits(result, resultPacket.getDocuments(), false, queryPacket.getQueryPacketData(), cacheKey, node.map(SearchCluster.Node::key)); Packet[] packets; CacheControl cacheControl = searcher.getCacheControl(); PacketWrapper packetWrapper = cacheControl.lookup(cacheKey, query); @@ -129,12 +131,21 @@ public class FS4SearchInvoker extends SearchInvoker { } else { packets = new Packet[1]; packets[0] = resultPacket; - cacheControl.cache(cacheKey, query, new DocsumPacketKey[0], packets, distributionKey); + cacheControl.cache(cacheKey, query, new DocsumPacketKey[0], packets, node.map(SearchCluster.Node::key)); } } return asList(result); } + private List<Result> errorResult(ErrorMessage errorMessage) { + Result error = new Result(query, errorMessage); + node.ifPresent(n -> { + Coverage coverage = new Coverage(0, n.getActiveDocuments(), 0); + error.setCoverage(coverage); + }); + return Arrays.asList(error); + } + @Override public void release() { if (channel != null) { diff --git a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java index b429995460d..36d283040a2 100644 --- a/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java +++ b/container-search/src/main/java/com/yahoo/prelude/fastsearch/FastSearcher.java @@ -22,6 +22,7 @@ import com.yahoo.search.dispatch.SearchInvoker; import com.yahoo.search.grouping.GroupingRequest; import com.yahoo.search.grouping.request.GroupingOperation; import com.yahoo.search.query.Ranking; +import com.yahoo.search.result.Coverage; import com.yahoo.search.result.ErrorMessage; import com.yahoo.search.searchchain.Execution; import edu.umd.cs.findbugs.annotations.NonNull; @@ -264,16 +265,21 @@ public class FastSearcher extends VespaBackEndSearcher { } private Result mergeResults(List<Result> results, Query query, Execution execution) { - if(results.size() == 1) { + if (results.size() == 1) { return results.get(0); } Result result = new Result(query); + // keep a separate tally of coverage as the normal merge counts using + // federated query rules + Coverage finalCoverage = new Coverage(0, 0); for (Result partialResult : results) { + finalCoverage.mergeWithPartition(partialResult.getCoverage(true)); result.mergeWith(partialResult); result.hits().addAll(partialResult.hits().asUnorderedHits()); } + result.setCoverage(finalCoverage); if (query.getOffset() != 0 || result.hits().size() > query.getHits()) { // with multiple results, each partial result is expected to have diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java index 455696c16b1..9eac9b9b63d 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/LoadBalancer.java @@ -1,7 +1,6 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.search.dispatch; -import com.yahoo.processing.request.CompoundName; import com.yahoo.search.Query; import com.yahoo.search.dispatch.SearchCluster.Group; @@ -24,8 +23,6 @@ public class LoadBalancer { private static final Logger log = Logger.getLogger(LoadBalancer.class.getName()); - private static final CompoundName QUERY_NODE_GROUP_AFFINITY = new CompoundName("dispatch.group.affinity"); - private final List<GroupSchedule> scoreboard; private int needle = 0; @@ -54,16 +51,7 @@ public class LoadBalancer { return Optional.empty(); } - Integer groupAffinity = query.properties().getInteger(QUERY_NODE_GROUP_AFFINITY); - if (groupAffinity != null) { - Optional<Group> previouslyChosen = allocateFromGroup(groupAffinity); - if (previouslyChosen.isPresent()) { - return previouslyChosen; - } - } - Optional<Group> allocatedGroup = allocateNextGroup(); - allocatedGroup.ifPresent(group -> query.properties().set(QUERY_NODE_GROUP_AFFINITY, group.id())); - return allocatedGroup; + return allocateNextGroup(); } /** @@ -83,18 +71,6 @@ public class LoadBalancer { } } - private Optional<Group> allocateFromGroup(int groupId) { - synchronized (this) { - for (GroupSchedule schedule : scoreboard) { - if (schedule.group.id() == groupId) { - schedule.adjustScore(1); - return Optional.of(schedule.group); - } - } - } - return Optional.empty(); - } - private Optional<Group> allocateNextGroup() { synchronized (this) { GroupSchedule bestSchedule = null; @@ -139,12 +115,12 @@ public class LoadBalancer { } public boolean isPreferredOver(GroupSchedule other) { - if (! group.hasSufficientCoverage()) { - return false; - } if (other == null) { return true; } + if (! group.hasSufficientCoverage()) { + return false; + } return this.score < other.score; } diff --git a/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java b/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java index a0c7447fd3e..0d50702acfd 100644 --- a/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java +++ b/container-search/src/main/java/com/yahoo/search/dispatch/SearchCluster.java @@ -155,7 +155,7 @@ public class SearchCluster implements NodeManager<SearchCluster.Node> { /** Returns the n'th (zero-indexed) group in the cluster if possible */ public Optional<Group> group(int n) { - if (orderedGroups.size() < n) { + if (orderedGroups.size() > n) { return Optional.of(orderedGroups.get(n)); } else { return Optional.empty(); diff --git a/container-search/src/main/java/com/yahoo/search/result/Coverage.java b/container-search/src/main/java/com/yahoo/search/result/Coverage.java index 81aa4a08f18..e340132a507 100644 --- a/container-search/src/main/java/com/yahoo/search/result/Coverage.java +++ b/container-search/src/main/java/com/yahoo/search/result/Coverage.java @@ -46,4 +46,16 @@ public class Coverage extends com.yahoo.container.handler.Coverage { public Coverage setNodesTried(int nodesTried) { super.setNodesTried(nodesTried); return this; } + public void mergeWithPartition(Coverage other) { + if (other == null) { + return; + } + int newResultSets = Integer.max(this.resultSets, other.resultSets); + int newFullResultSets = Integer.min(this.fullResultSets, other.fullResultSets); + + merge(other); + + this.resultSets = newResultSets; + this.fullResultSets = newFullResultSets; + } } diff --git a/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java b/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java index 21650d531be..e64e0bc8f8d 100644 --- a/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java +++ b/container-search/src/main/java/com/yahoo/search/result/FieldComparator.java @@ -57,11 +57,6 @@ public class FieldComparator extends ChainableComparator { Object a = getField(first,fieldName); Object b = getField(second,fieldName); - // If either of the values are null, don't touch the ordering - // This is to avoid problems if the sorting is called before the - // result is filled. - if ((a == null) || (b == null)) return 0; - int x = compareValues(a, b, fieldOrder.getSorter()); if (x != 0) { if (fieldOrder.getSortOrder() == Sorting.Order.DESCENDING) @@ -81,6 +76,12 @@ public class FieldComparator extends ChainableComparator { @SuppressWarnings("rawtypes") private int compareValues(Object first, Object second, Sorting.AttributeSorter s) { + if (first == null) { + if (second == null) return 0; + return -1; + } else if (second == null) { + return 1; + } if (first.getClass().isInstance(second) && first instanceof Comparable) { // We now know: diff --git a/container-search/src/main/java/com/yahoo/search/result/HitGroup.java b/container-search/src/main/java/com/yahoo/search/result/HitGroup.java index cc0aea74d70..c008b133595 100644 --- a/container-search/src/main/java/com/yahoo/search/result/HitGroup.java +++ b/container-search/src/main/java/com/yahoo/search/result/HitGroup.java @@ -844,62 +844,18 @@ public class HitGroup extends Hit implements DataList<Hit>, Cloneable, Iterable< return fillableHits().iterator().hasNext(); } + /** Returns the set of summaries for which all concrete hits recursively below this is filled. */ @Override public Set<String> getFilled() { - Iterator<Hit> hitIterator = hits.iterator(); - Set<String> firstSummaryNames = getSummaryNamesNextFilledHit(hitIterator); - if (firstSummaryNames == null || firstSummaryNames.isEmpty()) - return firstSummaryNames; - - Set<String> intersection = firstSummaryNames; - while (true) { - Set<String> summaryNames = getSummaryNamesNextFilledHit(hitIterator); - if (summaryNames == null) - break; - - if (intersection.size() == 1) - return getFilledSingle(first(intersection), hitIterator); - - - boolean notInSet = false; - if (intersection == firstSummaryNames) { - if (intersection.size() == summaryNames.size()) { - for(String s : summaryNames) { - if ( ! intersection.contains(s)) { - intersection = new HashSet<>(firstSummaryNames); - notInSet = true; - break; - } - } - } - } - if (notInSet) { - intersection.retainAll(summaryNames); - } - - } - - return intersection; - } - - private Set<String> getSummaryNamesNextFilledHit(Iterator<Hit> hitIterator) { - while (hitIterator.hasNext()) { - Set<String> filled = hitIterator.next().getFilled(); - if (filled != null) - return filled; - } - return null; - } - - private Set<String> getFilledSingle(String summaryName, Iterator<Hit> hitIterator) { - while (true) { - Set<String> summaryNames = getSummaryNamesNextFilledHit(hitIterator); - if (summaryNames == null) { - return Collections.singleton(summaryName); - } else if (!summaryNames.contains(summaryName)) { - return Collections.emptySet(); - } + Set<String> filled = null; + for (Hit hit : hits) { + if (hit.getFilled() == null) continue; + if (filled == null) + filled = new HashSet<>(hit.getFilled()); + else + filled.retainAll(hit.getFilled()); } + return filled; } private Iterable<Hit> fillableHits() { diff --git a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java index 5fa9dee8370..b08a3a73a01 100644 --- a/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java +++ b/container-search/src/test/java/com/yahoo/search/dispatch/LoadBalancerTest.java @@ -80,27 +80,6 @@ public class LoadBalancerTest { } @Test - public void requreThatLoadBalancerReturnsSameGroupForSameQuery() { - Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); - Node n2 = new SearchCluster.Node(1, "test-node2", 1, 1); - SearchCluster cluster = new SearchCluster(88.0, Arrays.asList(n1, n2), null, 1, null); - LoadBalancer lb = new LoadBalancer(cluster); - - Query q = new Query(); - // get first group - Optional<Group> grp = lb.takeGroupForQuery(q); - Group group = grp.get(); - int id1 = group.id(); - // release allocation - lb.releaseGroup(group); - - // continue with same query - grp = lb.takeGroupForQuery(q); - group = grp.get(); - assertThat(group.id(), equalTo(id1)); - } - - @Test public void requreThatLoadBalancerReturnsGroupWithShortestQueue() { Node n1 = new SearchCluster.Node(0, "test-node1", 0, 0); Node n2 = new SearchCluster.Node(1, "test-node2", 1, 1); diff --git a/container-search/src/test/java/com/yahoo/search/result/test/FillingTestCase.java b/container-search/src/test/java/com/yahoo/search/result/test/FillingTestCase.java index 01383ba29f2..8a068b7cb3b 100644 --- a/container-search/src/test/java/com/yahoo/search/result/test/FillingTestCase.java +++ b/container-search/src/test/java/com/yahoo/search/result/test/FillingTestCase.java @@ -5,6 +5,9 @@ import com.yahoo.search.result.Hit; import com.yahoo.search.result.HitGroup; import org.junit.Test; +import java.util.Collections; + +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; @@ -50,6 +53,40 @@ public class FillingTestCase { assertTrue(group.isFilled("otherSummary")); } + @Test + public void testPartiallyFilledWith2Hits() { + Hit hit1 = new Hit("id1"); + Hit hit2 = new Hit("id2"); + + hit1.setFilled("summary"); + hit2.setFillable(); + + HitGroup hits = new HitGroup(); + hits.add(hit1); + hits.add(hit2); + + assertEquals(Collections.emptySet(), hits.getFilled()); + } + + @Test + public void testPartiallyFilledDiverse() { + Hit hit1 = new Hit("id1"); + Hit hit2 = new Hit("id2"); + Hit hit3 = new Hit("id3"); + + hit1.setFilled("summary1"); + hit1.setFilled("summary2"); + hit2.setFilled("summary1"); + hit3.setFilled("summary1"); + + HitGroup hits = new HitGroup(); + hits.add(hit1); + hits.add(hit2); + hits.add(hit3); + + assertEquals(Collections.singleton("summary1"), hits.getFilled()); + } + private Hit createNonFilled(String id) { Hit hit=new Hit(id); hit.setFillable(); diff --git a/fbench/src/fbench/client.cpp b/fbench/src/fbench/client.cpp index 0200ea2d067..754fc809511 100644 --- a/fbench/src/fbench/client.cpp +++ b/fbench/src/fbench/client.cpp @@ -8,13 +8,13 @@ #include <cassert> #include <cstring> -Client::Client(ClientArguments *args) +Client::Client(vespalib::CryptoEngine::SP engine, ClientArguments *args) : _args(args), _status(new ClientStatus()), _reqTimer(new Timer()), _cycleTimer(new Timer()), _masterTimer(new Timer()), - _http(new HTTPClient(_args->_hostname, _args->_port, + _http(new HTTPClient(std::move(engine), _args->_hostname, _args->_port, _args->_keepAlive, _args->_headerBenchmarkdataCoverage, _args->_extraHeaders, _args->_authority)), _reader(new FileReader()), diff --git a/fbench/src/fbench/client.h b/fbench/src/fbench/client.h index 1e0b750dbb2..ce7c13fc982 100644 --- a/fbench/src/fbench/client.h +++ b/fbench/src/fbench/client.h @@ -4,6 +4,7 @@ #include <fstream> #include <atomic> #include <thread> +#include <vespa/vespalib/net/crypto_engine.h> #define FBENCH_DELIMITER "\n[--xxyyzz--FBENCH_MAGIC_DELIMITER--zzyyxx--]\n" @@ -188,7 +189,7 @@ public: * The client arguments given to this method becomes the * responsibility of the client. **/ - Client(ClientArguments *args); + Client(vespalib::CryptoEngine::SP engine, ClientArguments *args); /** * Delete objects owned by this client, including the client arguments. diff --git a/fbench/src/fbench/fbench.cpp b/fbench/src/fbench/fbench.cpp index c98c3ead4fe..36c60e08ef7 100644 --- a/fbench/src/fbench/fbench.cpp +++ b/fbench/src/fbench/fbench.cpp @@ -3,16 +3,40 @@ #include <httpclient/httpclient.h> #include <util/filereader.h> #include <util/clientstatus.h> +#include <vespa/vespalib/net/crypto_engine.h> +#include <vespa/vespalib/net/tls/transport_security_options.h> +#include <vespa/vespalib/net/tls/tls_crypto_engine.h> +#include <vespa/vespalib/net/tls/crypto_exception.h> +#include <vespa/vespalib/io/mapped_file_input.h> #include "client.h" #include "fbench.h" #include <cstring> #include <cmath> #include <csignal> +namespace { + +std::string maybe_load(const std::string &file_name, bool &failed) { + std::string content; + if (!file_name.empty()) { + vespalib::MappedFileInput file(file_name); + if (file.valid()) { + content = std::string(file.get().data, file.get().size); + } else { + fprintf(stderr, "could not load file: '%s'\n", file_name.c_str()); + failed = true; + } + } + return content; +} + +} + sig_atomic_t exitSignal = 0; FBench::FBench() - : _clients(), + : _crypto_engine(), + _clients(), _ignoreCount(0), _cycle(0), _filenamePattern(NULL), @@ -35,6 +59,44 @@ FBench::~FBench() free(_outputPattern); } +bool +FBench::init_crypto_engine(const std::string &ca_certs_file_name, + const std::string &cert_chain_file_name, + const std::string &private_key_file_name) +{ + if (ca_certs_file_name.empty() && + cert_chain_file_name.empty() && + private_key_file_name.empty()) + { + _crypto_engine = std::make_shared<vespalib::NullCryptoEngine>(); + return true; + } + if (ca_certs_file_name.empty()) { + fprintf(stderr, "CA certificate required; specify with -T\n"); + return false; + } + if (cert_chain_file_name.empty() != private_key_file_name.empty()) { + fprintf(stderr, "both client certificate AND client private key required; specify with -C and -K\n"); + return false; + } + bool load_failed = false; + vespalib::net::tls::TransportSecurityOptions + tls_opts(maybe_load(ca_certs_file_name, load_failed), + maybe_load(cert_chain_file_name, load_failed), + maybe_load(private_key_file_name, load_failed)); + if (load_failed) { + fprintf(stderr, "failed to load transport security options\n"); + return false; + } + try { + _crypto_engine = std::make_shared<vespalib::TlsCryptoEngine>(tls_opts); + } catch (vespalib::net::tls::CryptoException &e) { + fprintf(stderr, "%s\n", e.what()); + return false; + } + return true; +} + void FBench::InitBenchmark(int numClients, int ignoreCount, int cycle, const char *filenamePattern, const char *outputPattern, @@ -78,7 +140,7 @@ FBench::CreateClients() off_beg = _queryfileOffset[i]; off_end = _queryfileOffset[i+1]; } - client = std::make_unique<Client>( + client = std::make_unique<Client>(_crypto_engine, new ClientArguments(i, _clients.size(), _filenamePattern, _outputPattern, _hostnames[i % _hostnames.size()].c_str(), _ports[i % _ports.size()], _cycle, @@ -226,12 +288,15 @@ FBench::Usage() printf(" -o <str> : save query results to output files with the given pattern\n"); printf(" (default is not saving.)\n"); printf(" -r <num> : number of times to re-use each query file. -1 means no limit [-1]\n"); - printf(" -m <num> : max line size in input query files [8192].\n"); + printf(" -m <num> : max line size in input query files [131072].\n"); printf(" Can not be less than the minimum [1024].\n"); printf(" -p <num> : print summary every <num> seconds.\n"); printf(" -k : disable HTTP keep-alive.\n"); - printf(" -y : write data on coverage to output file (must used with -x).\n"); - printf(" -z : use single query file to be distributed between clients.\n\n"); + printf(" -y : write data on coverage to output file.\n"); + printf(" -z : use single query file to be distributed between clients.\n"); + printf(" -T <str> : CA certificate file to verify peer against.\n"); + printf(" -C <str> : client certificate file name.\n"); + printf(" -K <str> : client private key file name.\n\n"); printf(" <hostname> : the host you want to benchmark.\n"); printf(" <port> : the port to use when contacting the host.\n\n"); printf("Several hostnames and ports can be listed\n"); @@ -263,6 +328,9 @@ FBench::Main(int argc, char *argv[]) const char *outputFilePattern = NULL; std::string queryStringToAppend; std::string extraHeaders; + std::string ca_certs_file_name; // -T + std::string cert_chain_file_name; // -C + std::string private_key_file_name; // -K int restartLimit = -1; bool keepAlive = true; @@ -282,7 +350,7 @@ FBench::Main(int argc, char *argv[]) idx = 1; optError = false; - while((opt = GetOpt(argc, argv, "H:A:a:n:c:l:i:s:q:o:r:m:p:kxyzP", arg, idx)) != -1) { + while((opt = GetOpt(argc, argv, "H:A:T:C:K:a:n:c:l:i:s:q:o:r:m:p:kxyzP", arg, idx)) != -1) { switch(opt) { case 'A': authority = arg; @@ -294,6 +362,15 @@ FBench::Main(int argc, char *argv[]) return -1; } break; + case 'T': + ca_certs_file_name = std::string(arg); + break; + case 'C': + cert_chain_file_name = std::string(arg); + break; + case 'K': + private_key_file_name = std::string(arg); + break; case 'a': queryStringToAppend = std::string(arg); break; @@ -365,6 +442,11 @@ FBench::Main(int argc, char *argv[]) return -1; } + if (!init_crypto_engine(ca_certs_file_name, cert_chain_file_name, private_key_file_name)) { + fprintf(stderr, "failed to initialize crypto engine\n"); + return -1; + } + short hosts = args / 2; for (int i=0; i<hosts; ++i) diff --git a/fbench/src/fbench/fbench.h b/fbench/src/fbench/fbench.h index 4c5885d8988..8cbab2e6d6c 100644 --- a/fbench/src/fbench/fbench.h +++ b/fbench/src/fbench/fbench.h @@ -10,6 +10,7 @@ class FBench { private: + vespalib::CryptoEngine::SP _crypto_engine; std::vector<Client::UP> _clients; int _numClients; int _ignoreCount; @@ -32,6 +33,10 @@ private: std::string _extraHeaders; std::string _authority; + bool init_crypto_engine(const std::string &ca_certs_file_name, + const std::string &cert_chain_file_name, + const std::string &private_key_file_name); + void InitBenchmark(int numClients, int ignoreCount, int cycle, const char *filenamePattern, const char *outputPattern, int byteLimit, int restartLimit, int maxLineSize, diff --git a/fbench/src/geturl/geturl.cpp b/fbench/src/geturl/geturl.cpp index f279b0b55c8..868d33f30a9 100644 --- a/fbench/src/geturl/geturl.cpp +++ b/fbench/src/geturl/geturl.cpp @@ -1,4 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/net/crypto_engine.h> #include <httpclient/httpclient.h> #include <iostream> @@ -10,7 +12,8 @@ main(int argc, char** argv) return -1; } - HTTPClient client(argv[1], atoi(argv[2]), false, false); + auto engine = std::make_shared<vespalib::NullCryptoEngine>(); + HTTPClient client(engine, argv[1], atoi(argv[2]), false, false); if (!client.Fetch(argv[3], &std::cout).Ok()) { fprintf(stderr, "geturl: could not fetch 'http://%s:%d%s'\n", argv[1], atoi(argv[2]), argv[3]); diff --git a/fbench/src/httpclient/httpclient.cpp b/fbench/src/httpclient/httpclient.cpp index c49ef5da12c..002d2770dcd 100644 --- a/fbench/src/httpclient/httpclient.cpp +++ b/fbench/src/httpclient/httpclient.cpp @@ -17,10 +17,12 @@ HTTPClient::ChunkedReader HTTPClient::ChunkedReader::_instance; -HTTPClient::HTTPClient(const char *hostname, int port, +HTTPClient::HTTPClient(vespalib::CryptoEngine::SP engine, const char *hostname, int port, bool keepAlive, bool headerBenchmarkdataCoverage, const std::string & extraHeaders, const std::string &authority) - : _socket(new FastOS_Socket()), + : _engine(std::move(engine)), + _address(vespalib::SocketAddress::select_remote(port, hostname)), + _socket(), _hostname(hostname), _port(port), _keepAlive(keepAlive), @@ -48,7 +50,6 @@ HTTPClient::HTTPClient(const char *hostname, int port, _dataDone(false), _reader(NULL) { - _socket->SetAddressByHostName(port, hostname); if (_authority == "") { char tmp[1024]; snprintf(tmp, 1024, "%s:%d", hostname, port); @@ -56,17 +57,31 @@ HTTPClient::HTTPClient(const char *hostname, int port, } } +bool +HTTPClient::connect_socket() +{ + _socket.reset(); + auto handle = _address.connect([](auto &h) + { + return (h.set_nodelay(true) && + h.set_linger(false, 0)); + }); + if (!handle.valid()) { + return false; + } + _socket = vespalib::SyncCryptoSocket::create(*_engine, std::move(handle), false); + return bool(_socket); +} + ssize_t HTTPClient::FillBuffer() { - _bufused = _socket->Read(_buf, _bufsize); // may be -1 + _bufused = _socket->read(_buf, _bufsize); // may be -1 _bufpos = 0; return _bufused; } HTTPClient::~HTTPClient() { - if (_socket) - _socket->Close(); delete [] _buf; } @@ -148,9 +163,9 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen // try to reuse connection if keep-alive is enabled if (_keepAlive - && _socket->IsOpened() - && _socket->Write(req, strlen(req)) == (ssize_t)strlen(req) - && (!usePost || _socket->Write(content, cLen) == (ssize_t)cLen) + && _socket + && _socket->write(req, strlen(req)) == (ssize_t)strlen(req) + && (!usePost || _socket->write(content, cLen) == (ssize_t)cLen) && FillBuffer() > 0) { // DEBUG @@ -161,17 +176,14 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen } return true; } else { - _socket->Close(); + _socket.reset(); ResetBuffer(); } // try to open new connection to server - if (_socket->SetSoBlocking(true) - && _socket->Connect() - && _socket->SetNoDelay(true) - && _socket->SetSoLinger(false, 0) - && _socket->Write(req, strlen(req)) == (ssize_t)strlen(req) - && (!usePost || _socket->Write(content, cLen) == (ssize_t)cLen)) + if (connect_socket() + && _socket->write(req, strlen(req)) == (ssize_t)strlen(req) + && (!usePost || _socket->write(content, cLen) == (ssize_t)cLen)) { // DEBUG @@ -181,7 +193,7 @@ HTTPClient::Connect(const char *url, bool usePost, const char *content, int cLen } return true; } else { - _socket->Close(); + _socket.reset(); } // DEBUG @@ -395,7 +407,7 @@ HTTPClient::ConnCloseReader::Read(HTTPClient &client, res = fromBuffer; } if ((len - fromBuffer) > (len >> 1)) { - readRes = client._socket->Read(static_cast<char *>(buf) + readRes = client._socket->read(static_cast<char *>(buf) + fromBuffer, len - fromBuffer); if (readRes < 0) { client.Close(); @@ -434,7 +446,7 @@ HTTPClient::ContentLengthReader::Read(HTTPClient &client, readLen = (len - fromBuffer < client._contentLength - client._dataRead) ? len - fromBuffer : client._contentLength - client._dataRead; - readRes = client._socket->Read(static_cast<char *>(buf) + readRes = client._socket->read(static_cast<char *>(buf) + fromBuffer, readLen); if (readRes < 0) { client.Close(); @@ -510,7 +522,7 @@ HTTPClient::Close() || _connectionCloseGiven || !_dataDone || (_httpVersion == 0 && !_keepAliveGiven)) ? - _socket->Close() : true; + (_socket.reset(), true) : true; } HTTPClient::FetchStatus diff --git a/fbench/src/httpclient/httpclient.h b/fbench/src/httpclient/httpclient.h index 783545744d5..9c3ccd437d1 100644 --- a/fbench/src/httpclient/httpclient.h +++ b/fbench/src/httpclient/httpclient.h @@ -3,7 +3,9 @@ #include <ostream> #include <memory> -#include <vespa/fastos/socket.h> +#include <vespa/vespalib/net/sync_crypto_socket.h> +#include <vespa/vespalib/net/crypto_engine.h> +#include <vespa/vespalib/net/socket_address.h> /** * This class implements a HTTP client that may be used to fetch @@ -88,7 +90,10 @@ protected: }; friend class HTTPClient::ChunkedReader; - std::unique_ptr<FastOS_Socket> _socket; + vespalib::CryptoEngine::SP _engine; + vespalib::SocketAddress _address; + vespalib::SyncCryptoSocket::UP _socket; + std::string _hostname; int _port; bool _keepAlive; @@ -131,6 +136,17 @@ protected: _bufused = 0; } + /** + * (re)connects the socket to the host/port specified in the + * constructor. The hostname is not resolved again; the resolve + * result is cached by the constructor. Also sets tcp nodelay flag + * and disables lingering. Note to servers: This is a no-nonsense + * socket that will be closed in your face in very ungraceful + * ways. Do not expect half-close niceties or tls session + * termination packets. + **/ + bool connect_socket(); + /** * Fill the internal buffer with data from the url we are connected * to. @@ -215,8 +231,8 @@ public: * @param port the TCP port to use when contacting the host. * @param keepAlive flag indicating if keep-alive should be enabled. **/ - HTTPClient(const char *hostname, int port, bool keepAlive, - bool headerBenchmarkdataCoverage, const std::string & extraHeaders="", const std::string &authority = ""); + HTTPClient(vespalib::CryptoEngine::SP engine, const char *hostname, int port, bool keepAlive, + bool headerBenchmarkdataCoverage, const std::string & extraHeaders="", const std::string &authority = ""); /** * Disconnect from server and free memory. diff --git a/fbench/src/test/httpclient.cpp b/fbench/src/test/httpclient.cpp index 4201da68b97..0d350c393c1 100644 --- a/fbench/src/test/httpclient.cpp +++ b/fbench/src/test/httpclient.cpp @@ -1,4 +1,6 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. + +#include <vespa/vespalib/net/crypto_engine.h> #include <httpclient/httpclient.h> #include <iostream> #include <thread> @@ -11,13 +13,14 @@ main(int argc, char **argv) return 1; } + auto engine = std::make_shared<vespalib::NullCryptoEngine>(); HTTPClient *client; ssize_t len; if(argc == 4) { - client = new HTTPClient(argv[1], atoi(argv[2]), false, true); + client = new HTTPClient(engine, argv[1], atoi(argv[2]), false, true); } else { - client = new HTTPClient(argv[1], atoi(argv[2]), true, true); + client = new HTTPClient(engine, argv[1], atoi(argv[2]), true, true); } std::ostream * output = & std::cout; diff --git a/fbench/src/test/httpclient_splitstring.cpp b/fbench/src/test/httpclient_splitstring.cpp index d766b0f8f4b..4f3e0027db0 100644 --- a/fbench/src/test/httpclient_splitstring.cpp +++ b/fbench/src/test/httpclient_splitstring.cpp @@ -1,13 +1,15 @@ // Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. +#include <vespa/vespalib/net/crypto_engine.h> #include <httpclient/httpclient.h> #include <cstring> class DebugHTTPClient : public HTTPClient { public: - DebugHTTPClient(const char* server, int port, bool keepAlive) - : HTTPClient(server, port, keepAlive, true) {} + DebugHTTPClient() + : HTTPClient(std::make_shared<vespalib::NullCryptoEngine>(), + "localhost", 80, true, true) {} static void SplitLineTest(const char *input); static void DebugSplitLine(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java index fd75cf6f2b4..13ebf84228c 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/docker/DockerOperationsImpl.java @@ -299,6 +299,7 @@ public class DockerOperationsImpl implements DockerOperations { directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/daemontools_y"), false); directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/jdisc_core"), false); directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/langdetect/"), false); + directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/nginx"), false); directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/vespa"), false); directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/yca"), true); directoriesToMount.put(environment.pathInNodeUnderVespaHome("logs/yck"), false); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java index ecdf871a5ff..3e43ed63aea 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/StorageMaintainer.java @@ -6,6 +6,7 @@ import com.yahoo.config.provision.NodeType; import com.yahoo.io.IOUtils; import com.yahoo.log.LogLevel; import com.yahoo.system.ProcessExecuter; +import com.yahoo.vespa.hosted.dockerapi.Container; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerNetworking; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; @@ -19,7 +20,6 @@ import com.yahoo.vespa.hosted.node.admin.maintenance.coredump.CoredumpHandler; import java.io.IOException; import java.io.InputStreamReader; -import java.io.UncheckedIOException; import java.nio.file.Files; import java.nio.file.Path; import java.time.Duration; @@ -240,17 +240,12 @@ public class StorageMaintainer { } /** Checks if container has any new coredumps, reports and archives them if so */ - public void handleCoreDumpsForContainer(NodeAgentContext context, NodeSpec node) { - final Path coredumpsPath = context.pathOnHostFromPathInNode(context.pathInNodeUnderVespaHome("var/crash")); - final Map<String, Object> nodeAttributes = getCoredumpNodeAttributes(node); - try { - coredumpHandler.processAll(coredumpsPath, nodeAttributes); - } catch (IOException e) { - throw new UncheckedIOException("Failed to process coredumps", e); - } + public void handleCoreDumpsForContainer(NodeAgentContext context, NodeSpec node, Optional<Container> container) { + final Map<String, Object> nodeAttributes = getCoredumpNodeAttributes(node, container); + coredumpHandler.converge(context, nodeAttributes); } - private Map<String, Object> getCoredumpNodeAttributes(NodeSpec node) { + private Map<String, Object> getCoredumpNodeAttributes(NodeSpec node, Optional<Container> container) { Map<String, Object> attributes = new HashMap<>(); attributes.put("hostname", node.getHostname()); attributes.put("parent_hostname", environment.getParentHostHostname()); @@ -259,7 +254,7 @@ public class StorageMaintainer { attributes.put("flavor", node.getFlavor()); attributes.put("kernel_version", System.getProperty("os.version")); - node.getCurrentDockerImage().ifPresent(image -> attributes.put("docker_image", image.asString())); + container.map(c -> c.image).ifPresent(image -> attributes.put("docker_image", image.asString())); node.getVespaVersion().ifPresent(version -> attributes.put("vespa_version", version)); node.getOwner().ifPresent(owner -> { attributes.put("tenant", owner.getTenant()); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java index 9830d03240a..d1c44a55c1b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollector.java @@ -1,15 +1,14 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.maintenance.coredump; -import com.yahoo.collections.Pair; -import com.yahoo.system.ProcessExecuter; +import com.yahoo.vespa.hosted.dockerapi.ProcessResult; +import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; -import java.io.IOException; -import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Arrays; -import java.util.LinkedHashMap; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.logging.Level; @@ -18,31 +17,31 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; /** - * Takes in a compressed (lz4) or uncompressed core dump and collects relevant metadata. + * Takes in an uncompressed core dump and collects relevant metadata. * * @author freva */ -class CoreCollector { - static final String GDB_PATH = "/usr/bin/gdb"; - private static final String LZ4_PATH = "/usr/bin/lz4"; +public class CoreCollector { + private static final Logger logger = Logger.getLogger(CoreCollector.class.getName()); + private static final Pattern CORE_GENERATOR_PATH_PATTERN = Pattern.compile("^Core was generated by `(?<path>.*?)'.$"); private static final Pattern EXECFN_PATH_PATTERN = Pattern.compile("^.* execfn: '(?<path>.*?)'"); private static final Pattern FROM_PATH_PATTERN = Pattern.compile("^.* from '(?<path>.*?)'"); - private static final Pattern TOTAL_MEMORY_PATTERN = Pattern.compile("^MemTotal:\\s*(?<totalMem>\\d+) kB$", Pattern.MULTILINE); - private static final Logger logger = Logger.getLogger(CoreCollector.class.getName()); - private final ProcessExecuter processExecuter; + private final DockerOperations docker; + private final Path gdb; - CoreCollector(ProcessExecuter processExecuter) { - this.processExecuter = processExecuter; + public CoreCollector(DockerOperations docker, Path pathToGdbInContainer) { + this.docker = docker; + this.gdb = pathToGdbInContainer; } - Path readBinPathFallback(Path coredumpPath) throws IOException { - String command = GDB_PATH + " -n -batch -core " + coredumpPath + " | grep \'^Core was generated by\'"; + Path readBinPathFallback(NodeAgentContext context, Path coredumpPath) { + String command = gdb + " -n -batch -core " + coredumpPath + " | grep \'^Core was generated by\'"; String[] wrappedCommand = {"/bin/sh", "-c", command}; - Pair<Integer, String> result = processExecuter.exec(wrappedCommand); + ProcessResult result = docker.executeCommandInContainerAsRoot(context.containerName(), wrappedCommand); - Matcher matcher = CORE_GENERATOR_PATH_PATTERN.matcher(result.getSecond()); + Matcher matcher = CORE_GENERATOR_PATH_PATTERN.matcher(result.getOutput()); if (! matcher.find()) { throw new RuntimeException(String.format("Failed to extract binary path from GDB, result: %s, command: %s", result, Arrays.toString(wrappedCommand))); @@ -50,115 +49,58 @@ class CoreCollector { return Paths.get(matcher.group("path").split(" ")[0]); } - Path readBinPath(Path coredumpPath) throws IOException { + Path readBinPath(NodeAgentContext context, Path coredumpPath) { String[] command = {"file", coredumpPath.toString()}; try { - Pair<Integer, String> result = processExecuter.exec(command); - - if (result.getFirst() != 0) { + ProcessResult result = docker.executeCommandInContainerAsRoot(context.containerName(), command); + if (result.getExitStatus() != 0) { throw new RuntimeException("file command failed with " + result); } - Matcher execfnMatcher = EXECFN_PATH_PATTERN.matcher(result.getSecond()); + Matcher execfnMatcher = EXECFN_PATH_PATTERN.matcher(result.getOutput()); if (execfnMatcher.find()) { return Paths.get(execfnMatcher.group("path").split(" ")[0]); } - Matcher fromMatcher = FROM_PATH_PATTERN.matcher(result.getSecond()); + Matcher fromMatcher = FROM_PATH_PATTERN.matcher(result.getOutput()); if (fromMatcher.find()) { return Paths.get(fromMatcher.group("path").split(" ")[0]); } - } catch (Throwable e) { - logger.log(Level.WARNING, String.format("Failed getting bin path, command: %s. " + + } catch (RuntimeException e) { + context.log(logger, Level.WARNING, String.format("Failed getting bin path, command: %s. " + "Trying fallback instead", Arrays.toString(command)), e); } - return readBinPathFallback(coredumpPath); + return readBinPathFallback(context, coredumpPath); } - List<String> readBacktrace(Path coredumpPath, Path binPath, boolean allThreads) throws IOException { + List<String> readBacktrace(NodeAgentContext context, Path coredumpPath, Path binPath, boolean allThreads) { String threads = allThreads ? "thread apply all bt" : "bt"; - String[] command = {GDB_PATH, "-n", "-ex", threads, "-batch", binPath.toString(), coredumpPath.toString()}; - Pair<Integer, String> result = processExecuter.exec(command); - if (result.getFirst() != 0) { + String[] command = {gdb.toString(), "-n", "-ex", threads, "-batch", binPath.toString(), coredumpPath.toString()}; + ProcessResult result = docker.executeCommandInContainerAsRoot(context.containerName(), command); + if (result.getExitStatus() != 0) { throw new RuntimeException("Failed to read backtrace " + result + ", Command: " + Arrays.toString(command)); } - return Arrays.asList(result.getSecond().split("\n")); + return Arrays.asList(result.getOutput().split("\n")); } - Map<String, Object> collect(Path coredumpPath) { - Map<String, Object> data = new LinkedHashMap<>(); - try { - coredumpPath = compressCoredump(coredumpPath); - } catch (IOException e) { - logger.log(Level.WARNING, "Failed compressing/decompressing core dump", e); - } - + /** + * Collects metadata about a given core dump + * @param context context of the NodeAgent that owns the core dump + * @param coredumpPath path to core dump file inside the container + * @return map of relevant metadata about the core dump + */ + Map<String, Object> collect(NodeAgentContext context, Path coredumpPath) { + Map<String, Object> data = new HashMap<>(); try { - Path binPath = readBinPath(coredumpPath); + Path binPath = readBinPath(context, coredumpPath); data.put("bin_path", binPath.toString()); - data.put("backtrace", readBacktrace(coredumpPath, binPath, false)); - data.put("backtrace_all_threads", readBacktrace(coredumpPath, binPath, true)); - } catch (Throwable e) { - logger.log(Level.WARNING, "Failed to extract backtrace", e); - } - - try { - deleteDecompressedCoredump(coredumpPath); - } catch (IOException e) { - logger.log(Level.WARNING, "Failed to delete decompressed core dump", e); + data.put("backtrace", readBacktrace(context, coredumpPath, binPath, false)); + data.put("backtrace_all_threads", readBacktrace(context, coredumpPath, binPath, true)); + } catch (RuntimeException e) { + context.log(logger, Level.WARNING, "Failed to extract backtrace", e); } return data; } - - - /** - * This method will either compress or decompress the core dump if the input path is to a decompressed or - * compressed core dump, respectively. - * - * @return Path to the decompressed core dump - */ - private Path compressCoredump(Path coredumpPath) throws IOException { - if (! coredumpPath.toString().endsWith(".lz4")) { - processExecuter.exec( - new String[]{LZ4_PATH, "-f", coredumpPath.toString(), coredumpPath.toString() + ".lz4"}); - return coredumpPath; - - } else { - if (!diskSpaceAvailable(coredumpPath)) { - throw new RuntimeException("Not decompressing " + coredumpPath + " due to not enough disk space available"); - } - - Path decompressedPath = Paths.get(coredumpPath.toString().replaceFirst("\\.lz4$", "")); - Pair<Integer, String> result = processExecuter.exec( - new String[] {LZ4_PATH, "-f", "-d", coredumpPath.toString(), decompressedPath.toString()}); - if (result.getFirst() != 0) { - throw new RuntimeException("Failed to decompress file " + coredumpPath + ": " + result); - } - return decompressedPath; - } - } - - /** - * Delete the core dump unless: - * - The file is compressed - * - There is no compressed file (i.e. it was not decompressed in the first place) - */ - void deleteDecompressedCoredump(Path coredumpPath) throws IOException { - if (! coredumpPath.toString().endsWith(".lz4") && Paths.get(coredumpPath.toString() + ".lz4").toFile().exists()) { - Files.delete(coredumpPath); - } - } - - private boolean diskSpaceAvailable(Path path) throws IOException { - String memInfo = new String(Files.readAllBytes(Paths.get("/proc/meminfo"))); - return path.toFile().getFreeSpace() > parseTotalMemorySize(memInfo); - } - - int parseTotalMemorySize(String memInfo) { - Matcher matcher = TOTAL_MEMORY_PATTERN.matcher(memInfo); - if (!matcher.find()) throw new RuntimeException("Could not parse meminfo: " + memInfo); - return Integer.valueOf(matcher.group("totalMem")); - } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java index eb48086eb0f..16037f3233b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandler.java @@ -2,20 +2,26 @@ package com.yahoo.vespa.hosted.node.admin.maintenance.coredump; import com.fasterxml.jackson.databind.ObjectMapper; -import com.yahoo.system.ProcessExecuter; -import com.yahoo.vespa.hosted.node.admin.maintenance.FileHelper; +import com.google.common.collect.ImmutableMap; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder; +import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; +import com.yahoo.vespa.hosted.node.admin.task.util.process.Terminal; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; -import java.time.Duration; import java.util.Comparator; -import java.util.HashMap; import java.util.Map; import java.util.Optional; import java.util.UUID; -import java.util.logging.Level; +import java.util.function.Supplier; import java.util.logging.Logger; +import java.util.regex.Pattern; + +import static com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder.nameMatches; +import static com.yahoo.vespa.hosted.node.admin.task.util.file.FileFinder.nameStartsWith; +import static com.yahoo.vespa.hosted.node.admin.task.util.file.IOExceptionUtil.uncheck; /** * Finds coredumps, collects metadata and reports them @@ -24,127 +30,145 @@ import java.util.logging.Logger; */ public class CoredumpHandler { + private static final Pattern JAVA_CORE_PATTERN = Pattern.compile("java_pid.*\\.hprof"); + private static final String LZ4_PATH = "/usr/bin/lz4"; private static final String PROCESSING_DIRECTORY_NAME = "processing"; - static final String METADATA_FILE_NAME = "metadata.json"; + private static final String METADATA_FILE_NAME = "metadata.json"; + private static final String COREDUMP_FILENAME_PREFIX = "dump_"; private final Logger logger = Logger.getLogger(CoredumpHandler.class.getName()); private final ObjectMapper objectMapper = new ObjectMapper(); + private final Terminal terminal; private final CoreCollector coreCollector; - private final Path doneCoredumpsPath; private final CoredumpReporter coredumpReporter; + private final Path crashPatchInContainer; + private final Path doneCoredumpsPath; + private final Supplier<String> coredumpIdSupplier; - public CoredumpHandler(CoredumpReporter coredumpReporter, Path doneCoredumpsPath) { - this(new CoreCollector(new ProcessExecuter()), coredumpReporter, doneCoredumpsPath); + /** + * @param crashPathInContainer path inside the container where core dump are dumped + * @param doneCoredumpsPath path on host where processed core dumps are stored + */ + public CoredumpHandler(Terminal terminal, CoreCollector coreCollector, CoredumpReporter coredumpReporter, + Path crashPathInContainer, Path doneCoredumpsPath) { + this(terminal, coreCollector, coredumpReporter, crashPathInContainer, doneCoredumpsPath, () -> UUID.randomUUID().toString()); } - CoredumpHandler(CoreCollector coreCollector, CoredumpReporter coredumpReporter, Path doneCoredumpsPath) { + CoredumpHandler(Terminal terminal, CoreCollector coreCollector, CoredumpReporter coredumpReporter, + Path crashPathInContainer, Path doneCoredumpsPath, Supplier<String> coredumpIdSupplier) { + this.terminal = terminal; this.coreCollector = coreCollector; this.coredumpReporter = coredumpReporter; + this.crashPatchInContainer = crashPathInContainer; this.doneCoredumpsPath = doneCoredumpsPath; + this.coredumpIdSupplier = coredumpIdSupplier; } - public void processAll(Path coredumpsPath, Map<String, Object> nodeAttributes) throws IOException { - removeJavaCoredumps(coredumpsPath); - handleNewCoredumps(coredumpsPath, nodeAttributes); - removeOldCoredumps(); - } - private void removeJavaCoredumps(Path coredumpsPath) throws IOException { - if (! coredumpsPath.toFile().isDirectory()) return; - FileHelper.deleteFiles(coredumpsPath, Duration.ZERO, Optional.of("^java_pid.*\\.hprof$"), false); - } + public void converge(NodeAgentContext context, Map<String, Object> nodeAttributes) { + Path containerCrashPathOnHost = context.pathOnHostFromPathInNode(crashPatchInContainer); + Path containerProcessingPathOnHost = containerCrashPathOnHost.resolve(PROCESSING_DIRECTORY_NAME); - private void removeOldCoredumps() throws IOException { - if (! doneCoredumpsPath.toFile().isDirectory()) return; - FileHelper.deleteDirectories(doneCoredumpsPath, Duration.ofDays(10), Optional.empty()); - } + // Remove java core dumps + FileFinder.files(containerCrashPathOnHost) + .match(nameMatches(JAVA_CORE_PATTERN)) + .maxDepth(1) + .deleteRecursively(); - private void handleNewCoredumps(Path coredumpsPath, Map<String, Object> nodeAttributes) { - enqueueCoredumps(coredumpsPath); - processAndReportCoredumps(coredumpsPath, nodeAttributes); + // Check if we have already started to process a core dump or we can enqueue a new core one + getCoredumpToProcess(containerCrashPathOnHost, containerProcessingPathOnHost) + .ifPresent(path -> processAndReportSingleCoredump(context, path, nodeAttributes)); } + /** @return path to directory inside processing directory that contains a core dump file to process */ + Optional<Path> getCoredumpToProcess(Path containerCrashPathOnHost, Path containerProcessingPathOnHost) { + return FileFinder.directories(containerProcessingPathOnHost).stream() + .map(FileFinder.FileAttributes::path) + .findAny() + .map(Optional::of) + .orElseGet(() -> enqueueCoredump(containerCrashPathOnHost, containerProcessingPathOnHost)); + } /** * Moves a coredump to a new directory under the processing/ directory. Limit to only processing * one coredump at the time, starting with the oldest. + * + * @return path to directory inside processing directory which contains the enqueued core dump file */ - void enqueueCoredumps(Path coredumpsPath) { - Path processingCoredumpsPath = getProcessingCoredumpsPath(coredumpsPath); - processingCoredumpsPath.toFile().mkdirs(); - if (!FileHelper.listContentsOfDirectory(processingCoredumpsPath).isEmpty()) return; - - FileHelper.listContentsOfDirectory(coredumpsPath).stream() - .filter(path -> path.toFile().isFile() && ! path.getFileName().toString().startsWith(".")) - .min((Comparator.comparingLong(o -> o.toFile().lastModified()))) - .ifPresent(coredumpPath -> { - try { - enqueueCoredumpForProcessing(coredumpPath, processingCoredumpsPath); - } catch (Throwable e) { - logger.log(Level.WARNING, "Failed to process coredump " + coredumpPath, e); - } + Optional<Path> enqueueCoredump(Path containerCrashPathOnHost, Path containerProcessingPathOnHost) { + return FileFinder.files(containerCrashPathOnHost) + .match(nameStartsWith(".").negate()) + .maxDepth(1) + .stream() + .min(Comparator.comparing(FileFinder.FileAttributes::lastModifiedTime)) + .map(FileFinder.FileAttributes::path) + .map(coredumpPath -> { + UnixPath coredumpInProcessingDirectory = new UnixPath( + containerProcessingPathOnHost + .resolve(coredumpIdSupplier.get()) + .resolve(COREDUMP_FILENAME_PREFIX + coredumpPath.getFileName())); + coredumpInProcessingDirectory.createParents(); + return uncheck(() -> Files.move(coredumpPath, coredumpInProcessingDirectory.toPath())).getParent(); }); } - void processAndReportCoredumps(Path coredumpsPath, Map<String, Object> nodeAttributes) { - Path processingCoredumpsPath = getProcessingCoredumpsPath(coredumpsPath); - doneCoredumpsPath.toFile().mkdirs(); - - FileHelper.listContentsOfDirectory(processingCoredumpsPath).stream() - .filter(path -> path.toFile().isDirectory()) - .forEach(coredumpDirectory -> processAndReportSingleCoredump(coredumpDirectory, nodeAttributes)); - } - - private void processAndReportSingleCoredump(Path coredumpDirectory, Map<String, Object> nodeAttributes) { + void processAndReportSingleCoredump(NodeAgentContext context, Path coredumpDirectory, Map<String, Object> nodeAttributes) { try { - String metadata = collectMetadata(coredumpDirectory, nodeAttributes); + String metadata = getMetadata(context, coredumpDirectory, nodeAttributes); String coredumpId = coredumpDirectory.getFileName().toString(); coredumpReporter.reportCoredump(coredumpId, metadata); - finishProcessing(coredumpDirectory); - logger.info("Successfully reported coredump " + coredumpId); - } catch (Throwable e) { - logger.log(Level.WARNING, "Failed to report coredump " + coredumpDirectory, e); + finishProcessing(context, coredumpDirectory); + context.log(logger, "Successfully reported coredump " + coredumpId); + } catch (Exception e) { + throw new RuntimeException("Failed to process coredump " + coredumpDirectory, e); } } - private void enqueueCoredumpForProcessing(Path coredumpPath, Path processingCoredumpsPath) throws IOException { - // Make coredump readable - coredumpPath.toFile().setReadable(true, false); - - // Create new directory for this coredump and move it into it - Path folder = processingCoredumpsPath.resolve(UUID.randomUUID().toString()); - folder.toFile().mkdirs(); - Files.move(coredumpPath, folder.resolve(coredumpPath.getFileName())); - } - - String collectMetadata(Path coredumpDirectory, Map<String, Object> nodeAttributes) throws IOException { - Path metadataPath = coredumpDirectory.resolve(METADATA_FILE_NAME); - if (!Files.exists(metadataPath)) { - Path coredumpPath = FileHelper.listContentsOfDirectory(coredumpDirectory).stream().findFirst() - .orElseThrow(() -> new RuntimeException("No coredump file found in processing directory " + coredumpDirectory)); - Map<String, Object> metadata = coreCollector.collect(coredumpPath); + /** + * @return coredump metadata from metadata.json if present, otherwise attempts to get metadata using + * {@link CoreCollector} and stores it to metadata.json + */ + String getMetadata(NodeAgentContext context, Path coredumpDirectory, Map<String, Object> nodeAttributes) throws IOException { + UnixPath metadataPath = new UnixPath(coredumpDirectory.resolve(METADATA_FILE_NAME)); + if (!Files.exists(metadataPath.toPath())) { + Path coredumpFilePathOnHost = findCoredumpFileInProcessingDirectory(coredumpDirectory); + Path coredumpFilePathInContainer = context.pathInNodeFromPathOnHost(coredumpFilePathOnHost); + Map<String, Object> metadata = coreCollector.collect(context, coredumpFilePathInContainer); metadata.putAll(nodeAttributes); - Map<String, Object> fields = new HashMap<>(); - fields.put("fields", metadata); - - String metadataFields = objectMapper.writeValueAsString(fields); - Files.write(metadataPath, metadataFields.getBytes()); + String metadataFields = objectMapper.writeValueAsString(ImmutableMap.of("fields", metadata)); + metadataPath.writeUtf8File(metadataFields); return metadataFields; } else { - return new String(Files.readAllBytes(metadataPath)); + return metadataPath.readUtf8File(); } } - private void finishProcessing(Path coredumpDirectory) throws IOException { - Files.move(coredumpDirectory, doneCoredumpsPath.resolve(coredumpDirectory.getFileName())); - } - /** - * @return Path to directory where coredumps are temporarily moved while still being processed + * Compresses core file (and deletes the uncompressed core), then moves the entire core dump processing + * directory to {@link #doneCoredumpsPath} for archive */ - static Path getProcessingCoredumpsPath(Path coredumpsPath) { - return coredumpsPath.resolve(PROCESSING_DIRECTORY_NAME); + private void finishProcessing(NodeAgentContext context, Path coredumpDirectory) throws IOException { + Path coreFile = findCoredumpFileInProcessingDirectory(coredumpDirectory); + Path compressedCoreFile = coreFile.getParent().resolve(coreFile.getFileName() + ".lz4"); + terminal.newCommandLine(context) + .add(LZ4_PATH, "-f", coreFile.toString(), compressedCoreFile.toString()) + .execute(); + Files.delete(coreFile); + + Path newCoredumpDirectory = doneCoredumpsPath.resolve(coredumpDirectory.getFileName()); + Files.move(coredumpDirectory, newCoredumpDirectory); + } + + private Path findCoredumpFileInProcessingDirectory(Path coredumpProccessingDirectory) { + return FileFinder.files(coredumpProccessingDirectory) + .match(nameStartsWith(COREDUMP_FILENAME_PREFIX)) + .maxDepth(1) + .stream() + .map(FileFinder.FileAttributes::path) + .findFirst() + .orElseThrow(() -> new IllegalStateException( + "No coredump file found in processing directory " + coredumpProccessingDirectory)); } } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java index c130480ff9c..644ac587c34 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImpl.java @@ -9,7 +9,6 @@ import com.yahoo.vespa.hosted.dockerapi.metrics.GaugeWrapper; import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; -import com.yahoo.vespa.hosted.node.admin.maintenance.StorageMaintainer; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent; import com.yahoo.vespa.hosted.node.admin.util.PrefixLogger; @@ -56,15 +55,6 @@ public class NodeAdminImpl implements NodeAdmin { public NodeAdminImpl(DockerOperations dockerOperations, Function<String, NodeAgent> nodeAgentFactory, - StorageMaintainer storageMaintainer, - Runnable aclMaintainer, - MetricReceiverWrapper metricReceiver, - Clock clock) { - this(dockerOperations, nodeAgentFactory, aclMaintainer, metricReceiver, clock); - } - - public NodeAdminImpl(DockerOperations dockerOperations, - Function<String, NodeAgent> nodeAgentFactory, Runnable aclMaintainer, MetricReceiverWrapper metricReceiver, Clock clock) { diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java index 63f469635f8..f65f371ff67 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContext.java @@ -19,24 +19,42 @@ public interface NodeAgentContext extends TaskContext { AthenzService identity(); + /** - * Translates an absolute path in container to an absolute path in host. + * This method is the inverse of {@link #pathInNodeFromPathOnHost(Path)}} * * @param pathInNode absolute path in the container * @return the absolute path on host pointing at the same inode */ Path pathOnHostFromPathInNode(Path pathInNode); + /** @see #pathOnHostFromPathInNode(Path) */ default Path pathOnHostFromPathInNode(String pathInNode) { return pathOnHostFromPathInNode(Paths.get(pathInNode)); } + + /** + * This method is the inverse of {@link #pathOnHostFromPathInNode(Path)} + * + * @param pathOnHost absolute path on host + * @return the absolute path in the container pointing at the same inode + */ + Path pathInNodeFromPathOnHost(Path pathOnHost); + + /** @see #pathOnHostFromPathInNode(Path) */ + default Path pathInNodeFromPathOnHost(String pathOnHost) { + return pathInNodeFromPathOnHost(Paths.get(pathOnHost)); + } + + /** * @param relativePath relative path under Vespa home in container * @return the absolute path under Vespa home in the container */ Path pathInNodeUnderVespaHome(Path relativePath); + /** @see #pathInNodeUnderVespaHome(Path) */ default Path pathInNodeUnderVespaHome(String relativePath) { return pathInNodeUnderVespaHome(Paths.get(relativePath)); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java index 6d7110aeb51..d3c8b145488 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImpl.java @@ -59,15 +59,26 @@ public class NodeAgentContextImpl implements NodeAgentContext { @Override public Path pathOnHostFromPathInNode(Path pathInNode) { if (! pathInNode.isAbsolute()) - throw new IllegalArgumentException("Expected an absolute path in container, got: " + pathInNode); + throw new IllegalArgumentException("Expected an absolute path in the container, got: " + pathInNode); return pathToNodeRootOnHost.resolve(ROOT.relativize(pathInNode).toString()); } @Override + public Path pathInNodeFromPathOnHost(Path pathOnHost) { + if (! pathOnHost.isAbsolute()) + throw new IllegalArgumentException("Expected an absolute path on the host, got: " + pathOnHost); + + if (!pathOnHost.startsWith(pathToNodeRootOnHost)) + throw new IllegalArgumentException("Path " + pathOnHost + " does not exist in the container"); + + return ROOT.resolve(pathToNodeRootOnHost.relativize(pathOnHost).toString()); + } + + @Override public Path pathInNodeUnderVespaHome(Path relativePath) { if (relativePath.isAbsolute()) - throw new IllegalArgumentException("Expected a relative path to Vespa home, got: " + relativePath); + throw new IllegalArgumentException("Expected a relative path to the Vespa home, got: " + relativePath); return pathToVespaHome.resolve(relativePath); } diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java index 45af3a85810..3af78593a58 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImpl.java @@ -378,6 +378,7 @@ public class NodeAgentImpl implements NodeAgent { } } stopFilebeatSchedulerIfNeeded(); + storageMaintainer.handleCoreDumpsForContainer(context, node, Optional.of(existingContainer)); dockerOperations.removeContainer(existingContainer); containerState = ABSENT; context.log(logger, "Container successfully removed, new containerState is " + containerState); @@ -491,7 +492,7 @@ public class NodeAgentImpl implements NodeAgent { updateNodeRepoWithCurrentAttributes(node); break; case active: - storageMaintainer.handleCoreDumpsForContainer(context, node); + storageMaintainer.handleCoreDumpsForContainer(context, node, container); storageMaintainer.getDiskUsageFor(context) .map(diskUsage -> (double) diskUsage / BYTES_IN_GB / node.getMinDiskAvailableGb()) diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2.java index 172203a281a..4fdca85363f 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/ChildProcess2.java @@ -5,7 +5,7 @@ package com.yahoo.vespa.hosted.node.admin.task.util.process; /** * @author hakonhall */ -interface ChildProcess2 extends AutoCloseable { +public interface ChildProcess2 extends AutoCloseable { void waitForTermination(); int exitCode(); String getOutput(); diff --git a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java index 8e66c6e5a48..44b7da9367b 100644 --- a/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java +++ b/node-admin/src/main/java/com/yahoo/vespa/hosted/node/admin/task/util/process/CommandLine.java @@ -220,8 +220,10 @@ public class CommandLine { this.sigKillGracePeriod = period; return this; } + + public List<String> getArguments() { return Collections.unmodifiableList(arguments); } + // Accessor fields necessary for classes in this package. Could be public if necessary. - List<String> getArguments() { return Collections.unmodifiableList(arguments); } boolean getRedirectStderrToStdoutInsteadOfDiscard() { return redirectStderrToStdoutInsteadOfDiscard; } Predicate<Integer> getSuccessfulExitCodePredicate() { return successfulExitCodePredicate; } Charset getOutputEncoding() { return outputEncoding; } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java index 522bcc43444..3c316de94eb 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/DockerTester.java @@ -100,7 +100,7 @@ public class DockerTester implements AutoCloseable { Function<String, NodeAgent> nodeAgentFactory = (hostName) -> new NodeAgentImpl( NodeAgentContextImplTest.nodeAgentFromHostname(fileSystem, hostName), nodeRepositoryMock, orchestratorMock, dockerOperations, storageMaintainer, aclMaintainer, environment, clock, NODE_AGENT_SCAN_INTERVAL, athenzCredentialsMaintainer); - nodeAdmin = new NodeAdminImpl(dockerOperations, nodeAgentFactory, storageMaintainer, aclMaintainer, mr, Clock.systemUTC()); + nodeAdmin = new NodeAdminImpl(dockerOperations, nodeAgentFactory, aclMaintainer, mr, Clock.systemUTC()); nodeAdminStateUpdater = new NodeAdminStateUpdaterImpl(nodeRepositoryMock, orchestratorMock, storageMaintainer, nodeAdmin, DOCKER_HOST_HOSTNAME, clock, NODE_ADMIN_CONVERGE_STATE_INTERVAL, Optional.of(new ClassLocking())); diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java index 613b3cb5f5c..e91787ca540 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/integrationTests/StorageMaintainerMock.java @@ -2,6 +2,7 @@ package com.yahoo.vespa.hosted.node.admin.integrationTests; import com.yahoo.system.ProcessExecuter; +import com.yahoo.vespa.hosted.dockerapi.Container; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; import com.yahoo.vespa.hosted.node.admin.configserver.noderepository.NodeSpec; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; @@ -27,7 +28,7 @@ public class StorageMaintainerMock extends StorageMaintainer { } @Override - public void handleCoreDumpsForContainer(NodeAgentContext context, NodeSpec node) { + public void handleCoreDumpsForContainer(NodeAgentContext context, NodeSpec node, Optional<Container> container) { } @Override diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java index ca75a74cfcd..b5950646da9 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoreCollectorTest.java @@ -1,26 +1,20 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.maintenance.coredump; -import com.yahoo.collections.Pair; -import com.yahoo.system.ProcessExecuter; -import org.junit.Rule; +import com.yahoo.vespa.hosted.dockerapi.ProcessResult; +import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImplTest; import org.junit.Test; -import org.junit.rules.TemporaryFolder; -import java.io.IOException; -import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; import java.util.Arrays; import java.util.HashMap; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.stream.Collectors; -import static com.yahoo.vespa.hosted.node.admin.maintenance.coredump.CoreCollector.GDB_PATH; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; @@ -30,8 +24,10 @@ import static org.mockito.Mockito.when; * @author freva */ public class CoreCollectorTest { - private final ProcessExecuter processExecuter = mock(ProcessExecuter.class); - private final CoreCollector coreCollector = new CoreCollector(processExecuter); + private final String GDB_PATH = "/my/path/to/gdb"; + private final DockerOperations docker = mock(DockerOperations.class); + private final CoreCollector coreCollector = new CoreCollector(docker, Paths.get(GDB_PATH)); + private final NodeAgentContext context = NodeAgentContextImplTest.nodeAgentFromHostname("container-123.domain.tld"); private final Path TEST_CORE_PATH = Paths.get("/tmp/core.1234"); private final Path TEST_BIN_PATH = Paths.get("/usr/bin/program"); @@ -40,41 +36,30 @@ public class CoreCollectorTest { "#0 0x00000000004004d8 in main (argv=0x1) at main.c:4", "4\t printf(argv[3]);", "#0 0x00000000004004d8 in main (argv=0x1) at main.c:4"); - @Rule - public TemporaryFolder folder = new TemporaryFolder(); - - private void mockExec(String[] cmd, String output) throws IOException { - mockExec(cmd, output, ""); - } - - private void mockExec(String[] cmd, String output, String error) throws IOException { - when(processExecuter.exec(cmd)).thenReturn(new Pair<>(error.isEmpty() ? 0 : 1, output + error)); - } - @Test - public void extractsBinaryPathTest() throws IOException { + public void extractsBinaryPathTest() { final String[] cmd = {"file", TEST_CORE_PATH.toString()}; mockExec(cmd, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from " + "'/usr/bin/program'"); - assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(context, TEST_CORE_PATH)); mockExec(cmd, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from " + "'/usr/bin/program --foo --bar baz'"); - assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(context, TEST_CORE_PATH)); mockExec(cmd, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from " + "'/usr/bin//program'"); - assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(context, TEST_CORE_PATH)); mockExec(cmd, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, " + "from 'program', real uid: 0, effective uid: 0, real gid: 0, effective gid: 0, " + "execfn: '/usr/bin/program', platform: 'x86_64"); - assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(TEST_BIN_PATH, coreCollector.readBinPath(context, TEST_CORE_PATH)); Path fallbackResponse = Paths.get("/response/from/fallback"); @@ -82,57 +67,57 @@ public class CoreCollectorTest { "Core was generated by `/response/from/fallback'."); mockExec(cmd, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style"); - assertEquals(fallbackResponse, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(fallbackResponse, coreCollector.readBinPath(context, TEST_CORE_PATH)); mockExec(cmd, "", "Error code 1234"); - assertEquals(fallbackResponse, coreCollector.readBinPath(TEST_CORE_PATH)); + assertEquals(fallbackResponse, coreCollector.readBinPath(context, TEST_CORE_PATH)); } @Test - public void extractsBinaryPathUsingGdbTest() throws IOException { + public void extractsBinaryPathUsingGdbTest() { final String[] cmd = new String[]{"/bin/sh", "-c", GDB_PATH + " -n -batch -core /tmp/core.1234 | grep '^Core was generated by'"}; mockExec(cmd, "Core was generated by `/usr/bin/program-from-gdb --identity foo/search/cluster.content_'."); - assertEquals(Paths.get("/usr/bin/program-from-gdb"), coreCollector.readBinPathFallback(TEST_CORE_PATH)); + assertEquals(Paths.get("/usr/bin/program-from-gdb"), coreCollector.readBinPathFallback(context, TEST_CORE_PATH)); mockExec(cmd, "", "Error 123"); try { - coreCollector.readBinPathFallback(TEST_CORE_PATH); + coreCollector.readBinPathFallback(context, TEST_CORE_PATH); fail("Expected not to be able to get bin path"); } catch (RuntimeException e) { - assertEquals("Failed to extract binary path from GDB, result: (1,Error 123), command: " + - "[/bin/sh, -c, /usr/bin/gdb -n -batch -core /tmp/core.1234 | grep '^Core was generated by']", e.getMessage()); + assertEquals("Failed to extract binary path from GDB, result: ProcessResult { exitStatus=1 output= errors=Error 123 }, command: " + + "[/bin/sh, -c, /my/path/to/gdb -n -batch -core /tmp/core.1234 | grep '^Core was generated by']", e.getMessage()); } } @Test - public void extractsBacktraceUsingGdb() throws IOException { + public void extractsBacktraceUsingGdb() { mockExec(new String[]{GDB_PATH, "-n", "-ex", "bt", "-batch", "/usr/bin/program", "/tmp/core.1234"}, String.join("\n", GDB_BACKTRACE)); - assertEquals(GDB_BACKTRACE, coreCollector.readBacktrace(TEST_CORE_PATH, TEST_BIN_PATH, false)); + assertEquals(GDB_BACKTRACE, coreCollector.readBacktrace(context, TEST_CORE_PATH, TEST_BIN_PATH, false)); mockExec(new String[]{GDB_PATH, "-n", "-ex", "bt", "-batch", "/usr/bin/program", "/tmp/core.1234"}, "", "Failure"); try { - coreCollector.readBacktrace(TEST_CORE_PATH, TEST_BIN_PATH, false); + coreCollector.readBacktrace(context, TEST_CORE_PATH, TEST_BIN_PATH, false); fail("Expected not to be able to read backtrace"); } catch (RuntimeException e) { - assertEquals("Failed to read backtrace (1,Failure), Command: " + - "[/usr/bin/gdb, -n, -ex, bt, -batch, /usr/bin/program, /tmp/core.1234]", e.getMessage()); + assertEquals("Failed to read backtrace ProcessResult { exitStatus=1 output= errors=Failure }, Command: " + + "[/my/path/to/gdb, -n, -ex, bt, -batch, /usr/bin/program, /tmp/core.1234]", e.getMessage()); } } @Test - public void extractsBacktraceFromAllThreadsUsingGdb() throws IOException { + public void extractsBacktraceFromAllThreadsUsingGdb() { mockExec(new String[]{GDB_PATH, "-n", "-ex", "thread apply all bt", "-batch", "/usr/bin/program", "/tmp/core.1234"}, String.join("\n", GDB_BACKTRACE)); - assertEquals(GDB_BACKTRACE, coreCollector.readBacktrace(TEST_CORE_PATH, TEST_BIN_PATH, true)); + assertEquals(GDB_BACKTRACE, coreCollector.readBacktrace(context, TEST_CORE_PATH, TEST_BIN_PATH, true)); } @Test - public void collectsDataTest() throws IOException { + public void collectsDataTest() { mockExec(new String[]{"file", TEST_CORE_PATH.toString()}, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from " + "'/usr/bin/program'"); @@ -146,11 +131,11 @@ public class CoreCollectorTest { expectedData.put("bin_path", TEST_BIN_PATH.toString()); expectedData.put("backtrace", new ArrayList<>(GDB_BACKTRACE)); expectedData.put("backtrace_all_threads", new ArrayList<>(GDB_BACKTRACE)); - assertEquals(expectedData, coreCollector.collect(TEST_CORE_PATH)); + assertEquals(expectedData, coreCollector.collect(context, TEST_CORE_PATH)); } @Test - public void collectsPartialIfBacktraceFailsTest() throws IOException { + public void collectsPartialIfBacktraceFailsTest() { mockExec(new String[]{"file", TEST_CORE_PATH.toString()}, "/tmp/core.1234: ELF 64-bit LSB core file x86-64, version 1 (SYSV), SVR4-style, from " + "'/usr/bin/program'"); @@ -159,63 +144,15 @@ public class CoreCollectorTest { Map<String, Object> expectedData = new HashMap<>(); expectedData.put("bin_path", TEST_BIN_PATH.toString()); - assertEquals(expectedData, coreCollector.collect(TEST_CORE_PATH)); - } - - @Test - public void parseTotalMemoryTestTest() { - String memInfo = "MemTotal: 100000000 kB\nMemUsed: 1000000 kB\n"; - assertEquals(100000000, coreCollector.parseTotalMemorySize(memInfo)); - - String badMemInfo = "This string has no memTotal value"; - try { - coreCollector.parseTotalMemorySize(badMemInfo); - fail("Expected to fail on parsing"); - } catch (RuntimeException e) { - assertEquals("Could not parse meminfo: " + badMemInfo, e.getMessage()); - } + assertEquals(expectedData, coreCollector.collect(context, TEST_CORE_PATH)); } - @Test - public void testDeleteUncompressedFiles() throws IOException { - final String documentId = "UIDD-ABCD-EFGH"; - final String coreDumpFilename = "core.dump"; - - Path coredumpPath = folder.newFolder("crash").toPath().resolve(documentId); - coredumpPath.toFile().mkdirs(); - coredumpPath.resolve(coreDumpFilename).toFile().createNewFile(); - - Set<Path> expectedContentsOfCoredump = new HashSet<>(Arrays.asList( - coredumpPath.resolve(CoredumpHandler.METADATA_FILE_NAME), - coredumpPath.resolve(coreDumpFilename + ".lz4"))); - expectedContentsOfCoredump.forEach(path -> { - try { - path.toFile().createNewFile(); - } catch (IOException e) { e.printStackTrace();} - }); - coreCollector.deleteDecompressedCoredump(coredumpPath.resolve(coreDumpFilename)); - - assertEquals(expectedContentsOfCoredump, Files.list(coredumpPath).collect(Collectors.toSet())); + private void mockExec(String[] cmd, String output) { + mockExec(cmd, output, ""); } - @Test - public void testDeleteUncompressedFilesWithoutLz4() throws IOException { - final String documentId = "UIDD-ABCD-EFGH"; - final String coreDumpFilename = "core.dump"; - - Path coredumpPath = folder.newFolder("crash").toPath().resolve(documentId); - coredumpPath.toFile().mkdirs(); - - Set<Path> expectedContentsOfCoredump = new HashSet<>(Arrays.asList( - coredumpPath.resolve(CoredumpHandler.METADATA_FILE_NAME), - coredumpPath.resolve(coreDumpFilename))); - expectedContentsOfCoredump.forEach(path -> { - try { - path.toFile().createNewFile(); - } catch (IOException e) { e.printStackTrace();} - }); - coreCollector.deleteDecompressedCoredump(coredumpPath.resolve(coreDumpFilename)); - - assertEquals(expectedContentsOfCoredump, Files.list(coredumpPath).collect(Collectors.toSet())); + private void mockExec(String[] cmd, String output, String error) { + when(docker.executeCommandInContainerAsRoot(context.containerName(), cmd)) + .thenReturn(new ProcessResult(error.isEmpty() ? 0 : 1, output, error)); } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java index 8522112b0af..d567cac9a70 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/maintenance/coredump/CoredumpHandlerTest.java @@ -1,204 +1,204 @@ // Copyright 2018 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. package com.yahoo.vespa.hosted.node.admin.maintenance.coredump; +import com.google.common.collect.ImmutableMap; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContext; +import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentContextImplTest; +import com.yahoo.vespa.hosted.node.admin.task.util.file.UnixPath; +import com.yahoo.vespa.hosted.node.admin.task.util.process.TestChildProcess2; +import com.yahoo.vespa.hosted.node.admin.task.util.process.TestTerminal; +import com.yahoo.vespa.test.file.TestFileSystem; +import org.junit.After; import org.junit.Before; -import org.junit.Rule; import org.junit.Test; -import org.junit.rules.TemporaryFolder; import java.io.IOException; +import java.nio.file.FileSystem; import java.nio.file.Files; import java.nio.file.Path; +import java.nio.file.Paths; +import java.nio.file.attribute.FileTime; +import java.time.Duration; import java.time.Instant; import java.util.Arrays; import java.util.Collections; -import java.util.LinkedHashMap; -import java.util.List; +import java.util.HashMap; +import java.util.HashSet; import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.function.Supplier; import java.util.stream.Collectors; +import static com.yahoo.vespa.hosted.node.admin.task.util.file.IOExceptionUtil.uncheck; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; /** * @author freva */ public class CoredumpHandlerTest { + private final FileSystem fileSystem = TestFileSystem.create(); + private final Path donePath = fileSystem.getPath("/home/docker/dumps"); + private final NodeAgentContext context = NodeAgentContextImplTest.nodeAgentFromHostname(fileSystem, "container-123.domain.tld"); + private final Path crashPathInContainer = Paths.get("/var/crash"); + private final Path doneCoredumpsPath = fileSystem.getPath("/home/docker/dumps"); - private static final Map<String, Object> attributes = new LinkedHashMap<>(); - private static final Map<String, Object> metadata = new LinkedHashMap<>(); - private static final String expectedMetadataFileContents = "{\"fields\":{" + - "\"bin_path\":\"/bin/bash\"," + - "\"backtrace\":[\"call 1\",\"function 2\",\"something something\"]," + - "\"hostname\":\"host123.yahoo.com\"," + - "\"vespa_version\":\"6.48.4\"," + - "\"kernel_version\":\"2.6.32-573.22.1.el6.YAHOO.20160401.10.x86_64\"," + - "\"docker_image\":\"vespa/ci:6.48.4\"}}"; - - static { - attributes.put("hostname", "host123.yahoo.com"); - attributes.put("vespa_version", "6.48.4"); - attributes.put("kernel_version", "2.6.32-573.22.1.el6.YAHOO.20160401.10.x86_64"); - attributes.put("docker_image", "vespa/ci:6.48.4"); - - metadata.put("bin_path", "/bin/bash"); - metadata.put("backtrace", Arrays.asList("call 1", "function 2", "something something")); - } - - @Rule - public TemporaryFolder folder = new TemporaryFolder(); - + private final TestTerminal terminal = new TestTerminal(); private final CoreCollector coreCollector = mock(CoreCollector.class); private final CoredumpReporter coredumpReporter = mock(CoredumpReporter.class); - private CoredumpHandler coredumpHandler; - private Path crashPath; - private Path donePath; - private Path processingPath; + @SuppressWarnings("unchecked") + private final Supplier<String> coredumpIdSupplier = mock(Supplier.class); + private final CoredumpHandler coredumpHandler = new CoredumpHandler(terminal, coreCollector, coredumpReporter, + crashPathInContainer, doneCoredumpsPath, coredumpIdSupplier); - @Before - public void setup() throws IOException { - crashPath = folder.newFolder("crash").toPath(); - donePath = folder.newFolder("done").toPath(); - processingPath = CoredumpHandler.getProcessingCoredumpsPath(crashPath); - - coredumpHandler = new CoredumpHandler(coreCollector, coredumpReporter, donePath); - } @Test - public void ignoresIncompleteCoredumps() throws IOException { - Path coredumpPath = createCoredump(".core.dump", Instant.now()); - coredumpHandler.enqueueCoredumps(crashPath); - - // The 'processing' directory should be empty - assertFolderContents(processingPath); - - // The 'crash' directory should have 'processing' and the incomplete core dump in it - assertFolderContents(crashPath, processingPath.getFileName().toString(), coredumpPath.getFileName().toString()); + public void coredump_enqueue_test() throws IOException { + final Path crashPathOnHost = fileSystem.getPath("/home/docker/container-1/some/crash/path"); + final Path processingDir = fileSystem.getPath("/home/docker/container-1/some/other/processing"); + + Files.createDirectories(crashPathOnHost); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve(".bash.core.431")), FileTime.from(Instant.now())); + + assertFolderContents(crashPathOnHost, ".bash.core.431"); + Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir); + assertEquals(Optional.empty(), enqueuedPath); + + // bash.core.431 finished writing... and 2 more have since been written + Files.move(crashPathOnHost.resolve(".bash.core.431"), crashPathOnHost.resolve("bash.core.431")); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-proton.core.119")), FileTime.from(Instant.now().minus(Duration.ofMinutes(10)))); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-slobrok.core.673")), FileTime.from(Instant.now().minus(Duration.ofMinutes(5)))); + + when(coredumpIdSupplier.get()).thenReturn("id-123").thenReturn("id-321"); + enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir); + assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath); + assertFolderContents(crashPathOnHost, "bash.core.431", "vespa-slobrok.core.673"); + assertFolderContents(processingDir, "id-123"); + assertFolderContents(processingDir.resolve("id-123"), "dump_vespa-proton.core.119"); + verify(coredumpIdSupplier, times(1)).get(); + + // Enqueue another + enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir); + assertEquals(Optional.of(processingDir.resolve("id-321")), enqueuedPath); + assertFolderContents(crashPathOnHost, "bash.core.431"); + assertFolderContents(processingDir, "id-123", "id-321"); + assertFolderContents(processingDir.resolve("id-321"), "dump_vespa-slobrok.core.673"); + verify(coredumpIdSupplier, times(2)).get(); } @Test - public void startProcessingTest() throws IOException { - Path coredumpPath = createCoredump("core.dump", Instant.now()); - coredumpHandler.enqueueCoredumps(crashPath); - - // Contents of 'crash' should be only the 'processing' directory - assertFolderContents(crashPath, processingPath.getFileName().toString()); - - // The 'processing' directory should have 1 directory inside for the core.dump we just created - List<Path> processedCoredumps = Files.list(processingPath).collect(Collectors.toList()); - assertEquals(processedCoredumps.size(), 1); - - // Inside the coredump directory, there should be 1 file: core.dump - assertFolderContents(processedCoredumps.get(0), coredumpPath.getFileName().toString()); + public void coredump_to_process_test() throws IOException { + final Path crashPathOnHost = fileSystem.getPath("/home/docker/container-1/some/crash/path"); + final Path processingDir = fileSystem.getPath("/home/docker/container-1/some/other/processing"); + + // Initially there are no core dumps + Optional<Path> enqueuedPath = coredumpHandler.enqueueCoredump(crashPathOnHost, processingDir); + assertEquals(Optional.empty(), enqueuedPath); + + // 3 core dumps occur + Files.createDirectories(crashPathOnHost); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("bash.core.431")), FileTime.from(Instant.now())); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-proton.core.119")), FileTime.from(Instant.now().minus(Duration.ofMinutes(10)))); + Files.setLastModifiedTime(Files.createFile(crashPathOnHost.resolve("vespa-slobrok.core.673")), FileTime.from(Instant.now().minus(Duration.ofMinutes(5)))); + + when(coredumpIdSupplier.get()).thenReturn("id-123"); + enqueuedPath = coredumpHandler.getCoredumpToProcess(crashPathOnHost, processingDir); + assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath); + + // Running this again wont enqueue new core dumps as we are still processing the one enqueued previously + enqueuedPath = coredumpHandler.getCoredumpToProcess(crashPathOnHost, processingDir); + assertEquals(Optional.of(processingDir.resolve("id-123")), enqueuedPath); + verify(coredumpIdSupplier, times(1)).get(); } @Test - public void limitToProcessingOneCoredumpAtTheTimeTest() throws IOException { - final String oldestCoredump = "core.dump0"; - final Instant startTime = Instant.now(); - createCoredump(oldestCoredump, startTime.minusSeconds(3600)); - createCoredump("core.dump1", startTime.minusSeconds(1000)); - createCoredump("core.dump2", startTime); - coredumpHandler.enqueueCoredumps(crashPath); - - List<Path> processingCoredumps = Files.list(processingPath).collect(Collectors.toList()); - assertEquals(1, processingCoredumps.size()); - - // Make sure that the 1 coredump that we are processing is the oldest one - Set<String> filenamesInProcessingDirectory = Files.list(processingCoredumps.get(0)) - .map(file -> file.getFileName().toString()) - .collect(Collectors.toSet()); - assertEquals(Collections.singleton(oldestCoredump), filenamesInProcessingDirectory); + public void get_metadata_test() throws IOException { + Map<String, Object> metadata = new HashMap<>(); + metadata.put("bin_path", "/bin/bash"); + metadata.put("backtrace", Arrays.asList("call 1", "function 2", "something something")); - // Running enqueueCoredumps should not start processing any new coredumps as we already are processing one - coredumpHandler.enqueueCoredumps(crashPath); - assertEquals(processingCoredumps, Files.list(processingPath).collect(Collectors.toList())); - filenamesInProcessingDirectory = Files.list(processingCoredumps.get(0)) - .map(file -> file.getFileName().toString()) - .collect(Collectors.toSet()); - assertEquals(Collections.singleton(oldestCoredump), filenamesInProcessingDirectory); + Map<String, Object> attributes = ImmutableMap.of( + "hostname", "host123.yahoo.com", + "vespa_version", "6.48.4", + "kernel_version", "3.10.0-862.9.1.el7.x86_64", + "docker_image", "vespa/ci:6.48.4"); + + String expectedMetadataStr = "{\"fields\":{" + + "\"hostname\":\"host123.yahoo.com\"," + + "\"kernel_version\":\"3.10.0-862.9.1.el7.x86_64\"," + + "\"backtrace\":[\"call 1\",\"function 2\",\"something something\"]," + + "\"vespa_version\":\"6.48.4\"," + + "\"bin_path\":\"/bin/bash\"," + + "\"docker_image\":\"vespa/ci:6.48.4\"" + + "}}"; + + + Path coredumpDirectoryInContainer = Paths.get("/var/crash/id-123"); + Path coredumpDirectory = context.pathOnHostFromPathInNode(coredumpDirectoryInContainer); + Files.createDirectories(coredumpDirectory); + Files.createFile(coredumpDirectory.resolve("dump_core.456")); + when(coreCollector.collect(eq(context), eq(coredumpDirectoryInContainer.resolve("dump_core.456")))) + .thenReturn(metadata); + + assertEquals(expectedMetadataStr, coredumpHandler.getMetadata(context, coredumpDirectory, attributes)); + verify(coreCollector, times(1)).collect(any(), any()); + + // Calling it again will simply read the previously generated metadata from disk + assertEquals(expectedMetadataStr, coredumpHandler.getMetadata(context, coredumpDirectory, attributes)); + verify(coreCollector, times(1)).collect(any(), any()); } - @Test - public void coredumpMetadataCollectAndWriteTest() throws IOException { - createCoredump("core.dump", Instant.now()); - coredumpHandler.enqueueCoredumps(crashPath); - Path processingCoredumpPath = Files.list(processingPath).findFirst().orElseThrow(() -> - new RuntimeException("Expected to find directory with coredump in processing dir")); - when(coreCollector.collect(eq(processingCoredumpPath.resolve("core.dump")))).thenReturn(metadata); - - // Inside 'processing' directory, there should be a new directory containing 'core.dump' file - String returnedMetadata = coredumpHandler.collectMetadata(processingCoredumpPath, attributes); - String metadataFileContents = new String(Files.readAllBytes( - processingCoredumpPath.resolve(CoredumpHandler.METADATA_FILE_NAME))); - assertEquals(expectedMetadataFileContents, metadataFileContents); - assertEquals(expectedMetadataFileContents, returnedMetadata); + @Test(expected = IllegalStateException.class) + public void cant_get_metadata_if_no_core_file() throws IOException { + coredumpHandler.getMetadata(context, fileSystem.getPath("/fake/path"), Collections.emptyMap()); } @Test - public void coredumpMetadataReadIfExistsTest() throws IOException { - final String documentId = "UIDD-ABCD-EFGH"; - Path metadataPath = createProcessedCoredump(documentId); - - verifyZeroInteractions(coreCollector); - String returnedMetadata = coredumpHandler.collectMetadata(metadataPath.getParent(), attributes); - assertEquals(expectedMetadataFileContents, returnedMetadata); + public void process_single_coredump_test() throws IOException { + Path coredumpDirectory = fileSystem.getPath("/path/to/coredump/proccessing/id-123"); + Files.createDirectories(coredumpDirectory); + Files.write(coredumpDirectory.resolve("metadata.json"), "metadata".getBytes()); + Files.createFile(coredumpDirectory.resolve("dump_bash.core.431")); + assertFolderContents(coredumpDirectory, "metadata.json", "dump_bash.core.431"); + + terminal.interceptCommand("/usr/bin/lz4 -f /path/to/coredump/proccessing/id-123/dump_bash.core.431 " + + "/path/to/coredump/proccessing/id-123/dump_bash.core.431.lz4 2>&1", + commandLine -> { + uncheck(() -> Files.createFile(fileSystem.getPath(commandLine.getArguments().get(3)))); + return new TestChildProcess2(0, ""); + }); + coredumpHandler.processAndReportSingleCoredump(context, coredumpDirectory, Collections.emptyMap()); + verify(coreCollector, never()).collect(any(), any()); + verify(coredumpReporter, times(1)).reportCoredump(eq("id-123"), eq("metadata")); + assertFalse(Files.exists(coredumpDirectory)); + assertFolderContents(doneCoredumpsPath, "id-123"); + assertFolderContents(doneCoredumpsPath.resolve("id-123"), "metadata.json", "dump_bash.core.431.lz4"); } - @Test - public void reportSuccessCoredumpTest() throws IOException { - final String documentId = "UIDD-ABCD-EFGH"; - createProcessedCoredump(documentId); - - coredumpHandler.processAndReportCoredumps(crashPath, attributes); - verify(coredumpReporter).reportCoredump(eq(documentId), eq(expectedMetadataFileContents)); - - // The coredump should not have been moved out of 'processing' and into 'done' as the report failed - assertFolderContents(processingPath); - assertFolderContents(donePath.resolve(documentId), CoredumpHandler.METADATA_FILE_NAME); + @Before + public void setup() throws IOException { + Files.createDirectories(donePath); } - @Test - public void reportFailCoredumpTest() throws IOException { - final String documentId = "UIDD-ABCD-EFGH"; - Path metadataPath = createProcessedCoredump(documentId); - - doThrow(new RuntimeException()).when(coredumpReporter).reportCoredump(any(), any()); - coredumpHandler.processAndReportCoredumps(crashPath, attributes); - verify(coredumpReporter).reportCoredump(eq(documentId), eq(expectedMetadataFileContents)); - - // The coredump should not have been moved out of 'processing' and into 'done' as the report failed - assertFolderContents(donePath); - assertFolderContents(metadataPath.getParent(), CoredumpHandler.METADATA_FILE_NAME); + @After + public void teardown() { + terminal.verifyAllCommandsExecuted(); } - private static void assertFolderContents(Path pathToFolder, String... filenames) throws IOException { - Set<Path> expectedContentsOfFolder = Arrays.stream(filenames) - .map(pathToFolder::resolve) + private static void assertFolderContents(Path pathToFolder, String... filenames) { + Set<String> expectedContentsOfFolder = new HashSet<>(Arrays.asList(filenames)); + Set<String> actualContentsOfFolder = new UnixPath(pathToFolder) + .listContentsOfDirectory().stream() + .map(unixPath -> unixPath.toPath().getFileName().toString()) .collect(Collectors.toSet()); - Set<Path> actualContentsOfFolder = Files.list(pathToFolder).collect(Collectors.toSet()); assertEquals(expectedContentsOfFolder, actualContentsOfFolder); } - - private Path createCoredump(String coredumpName, Instant lastModified) throws IOException { - Path coredumpPath = crashPath.resolve(coredumpName); - coredumpPath.toFile().createNewFile(); - coredumpPath.toFile().setLastModified(lastModified.toEpochMilli()); - return coredumpPath; - } - - private Path createProcessedCoredump(String documentId) throws IOException { - Path coredumpPath = processingPath - .resolve(documentId) - .resolve(CoredumpHandler.METADATA_FILE_NAME); - coredumpPath.getParent().toFile().mkdirs(); - return Files.write(coredumpPath, expectedMetadataFileContents.getBytes()); - } } diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java index efd35cce00e..065b039c7fd 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeadmin/NodeAdminImplTest.java @@ -5,7 +5,6 @@ import com.yahoo.metrics.simple.MetricReceiver; import com.yahoo.test.ManualClock; import com.yahoo.vespa.hosted.dockerapi.metrics.MetricReceiverWrapper; import com.yahoo.vespa.hosted.node.admin.docker.DockerOperations; -import com.yahoo.vespa.hosted.node.admin.maintenance.StorageMaintainer; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgent; import com.yahoo.vespa.hosted.node.admin.nodeagent.NodeAgentImpl; import org.junit.Test; @@ -40,11 +39,10 @@ public class NodeAdminImplTest { private interface NodeAgentFactory extends Function<String, NodeAgent> {} private final DockerOperations dockerOperations = mock(DockerOperations.class); private final Function<String, NodeAgent> nodeAgentFactory = mock(NodeAgentFactory.class); - private final StorageMaintainer storageMaintainer = mock(StorageMaintainer.class); private final Runnable aclMaintainer = mock(Runnable.class); private final ManualClock clock = new ManualClock(); - private final NodeAdminImpl nodeAdmin = new NodeAdminImpl(dockerOperations, nodeAgentFactory, storageMaintainer, aclMaintainer, + private final NodeAdminImpl nodeAdmin = new NodeAdminImpl(dockerOperations, nodeAgentFactory, aclMaintainer, new MetricReceiverWrapper(MetricReceiver.nullImplementation), clock); @Test diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java index 2388e1e02b1..31ac8d1c114 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentContextImplTest.java @@ -15,23 +15,45 @@ import static org.junit.Assert.assertEquals; * @author freva */ public class NodeAgentContextImplTest { - private final NodeAgentContext context = nodeAgentFromHostname("container-1.domain.tld"); - + private final FileSystem fileSystem = TestFileSystem.create(); + private final NodeAgentContext context = nodeAgentFromHostname(fileSystem, "container-1.domain.tld"); @Test public void path_on_host_from_path_in_node_test() { assertEquals( "/home/docker/container-1", - context.pathOnHostFromPathInNode(Paths.get("/")).toString()); + context.pathOnHostFromPathInNode("/").toString()); assertEquals( "/home/docker/container-1/dev/null", - context.pathOnHostFromPathInNode(Paths.get("/dev/null")).toString()); + context.pathOnHostFromPathInNode("/dev/null").toString()); } @Test(expected=IllegalArgumentException.class) public void path_in_container_must_be_absolute() { - context.pathOnHostFromPathInNode(Paths.get("some/relative/path")); + context.pathOnHostFromPathInNode("some/relative/path"); + } + + @Test + public void path_in_node_from_path_on_host_test() { + assertEquals( + "/dev/null", + context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-1/dev/null")).toString()); + } + + @Test(expected=IllegalArgumentException.class) + public void path_on_host_must_be_absolute() { + context.pathInNodeFromPathOnHost("some/relative/path"); + } + + @Test(expected=IllegalArgumentException.class) + public void path_on_host_must_be_inside_container_storage_of_context() { + context.pathInNodeFromPathOnHost(fileSystem.getPath("/home/docker/container-2/dev/null")); + } + + @Test(expected=IllegalArgumentException.class) + public void path_on_host_must_be_inside_container_storage() { + context.pathInNodeFromPathOnHost(fileSystem.getPath("/home")); } @Test diff --git a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java index 99936f56596..fbb584ea1d9 100644 --- a/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java +++ b/node-admin/src/test/java/com/yahoo/vespa/hosted/node/admin/nodeagent/NodeAgentImplTest.java @@ -457,6 +457,8 @@ public class NodeAgentImplTest { nodeAgent.converge(); final InOrder inOrder = inOrder(storageMaintainer, dockerOperations, nodeRepository); + inOrder.verify(dockerOperations, times(1)).stopServices(eq(context.containerName())); + inOrder.verify(storageMaintainer, times(1)).handleCoreDumpsForContainer(eq(context), eq(node), any()); inOrder.verify(dockerOperations, times(1)).removeContainer(any()); inOrder.verify(storageMaintainer, times(1)).archiveNodeStorage(eq(context)); inOrder.verify(nodeRepository, times(1)).setNodeState(eq(hostName), eq(Node.State.ready)); diff --git a/searchlib/src/test/java/com/yahoo/searchlib/expression/IntegerResultNodeTestCase.java b/searchlib/src/test/java/com/yahoo/searchlib/expression/IntegerResultNodeTestCase.java index c9eaaa389cf..4e079ba2adb 100644 --- a/searchlib/src/test/java/com/yahoo/searchlib/expression/IntegerResultNodeTestCase.java +++ b/searchlib/src/test/java/com/yahoo/searchlib/expression/IntegerResultNodeTestCase.java @@ -5,6 +5,7 @@ import com.yahoo.vespa.objects.BufferSerializer; import com.yahoo.vespa.objects.ObjectDumper; import org.junit.Test; +import java.lang.reflect.InvocationTargetException; import java.util.Arrays; import java.util.List; @@ -107,10 +108,10 @@ public class IntegerResultNodeTestCase extends ResultNodeTest { } @Test - public void testSerialization() throws IllegalAccessException, InstantiationException { + public void testSerialization() throws IllegalAccessException, InstantiationException, NoSuchMethodException, InvocationTargetException { for (NumericResultNode node : getResultNodes(8)) { - assertThat(node.getInteger(), is(8l)); - NumericResultNode out = node.getClass().newInstance(); + assertThat(node.getInteger(), is(8L)); + NumericResultNode out = node.getClass().getConstructor().newInstance(); assertCorrectSerialization(node, out); assertThat(out.getInteger(), is(node.getInteger())); } diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/FeedParams.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/FeedParams.java index 4f5b30444a9..06edd222be2 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/FeedParams.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/FeedParams.java @@ -30,7 +30,7 @@ public final class FeedParams { * Enumeration of data formats that are acceptable by the OutputStream * returned by {@link com.yahoo.vespa.http.client.Session#stream(CharSequence)}. */ - public static enum DataFormat { + public enum DataFormat { /** UTF-8-encoded XML. Preamble is not necessary. */ XML_UTF8, JSON_UTF8 diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/SessionParams.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/SessionParams.java index 9a8640d09e2..eb3e8ec982b 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/SessionParams.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/config/SessionParams.java @@ -35,6 +35,7 @@ public final class SessionParams { * settings, use {@link #setConnectionParams(ConnectionParams)}. */ public static final class Builder { + private final List<Cluster> clusters = new LinkedList<>(); private FeedParams feedParams = new FeedParams.Builder().build(); private ConnectionParams connectionParams = new ConnectionParams.Builder().build(); diff --git a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java index c2e94e14486..e9852de215a 100644 --- a/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java +++ b/vespa-http-client/src/main/java/com/yahoo/vespa/http/client/core/communication/ApacheGatewayConnection.java @@ -160,8 +160,8 @@ class ApacheGatewayConnection implements GatewayConnection { } private ByteBuffer[] getDataWithStartAndEndOfFeed(List<Document> docs, int version) { - List<ByteBuffer> data = new ArrayList<ByteBuffer>(); - if (version == 2 || version == 3) { + List<ByteBuffer> data = new ArrayList<>(); + if (version == 2 || version == 3) { // TODO: Vespa 7: Remove support for version 2 for (Document doc : docs) { int operationSize = doc.size() + startOfFeed.length + endOfFeed.length; StringBuilder envelope = new StringBuilder(); diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandler.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandler.java index 61ff84cfdaa..52d999f1a52 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandler.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeedHandler.java @@ -43,7 +43,6 @@ import java.util.zip.GZIPInputStream; * Accept feeds from outside of the Vespa cluster. * * @author Steinar Knutsen - * @since 5.1 */ public class FeedHandler extends LoggingRequestHandler { @@ -131,7 +130,7 @@ public class FeedHandler extends LoggingRequestHandler { int version; if (washedClientVersions.contains("3")) { version = 3; - } else if (washedClientVersions.contains("2")) { + } else if (washedClientVersions.contains("2")) { // TODO: Vespa 7: Remove support for Version 2 version = 2; } else { return new Tuple2<>(new ErrorHttpResponse( diff --git a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeederSettings.java b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeederSettings.java index 060cd6ca0a6..874509dd2f8 100644 --- a/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeederSettings.java +++ b/vespaclient-container-plugin/src/main/java/com/yahoo/vespa/http/server/FeederSettings.java @@ -9,7 +9,7 @@ import com.yahoo.vespa.http.client.core.Headers; /** * Wrapper for the feed feederSettings read from HTTP request. * - * @author <a href="mailto:steinar@yahoo-inc.com">Steinar Knutsen</a> + * @author Steinar Knutsen */ public class FeederSettings { |