Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ILM] Version the order of the actions in a phase. #97745

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,16 @@ public record LifecycleExecutionState(
String snapshotName,
String shrinkIndexName,
String snapshotIndexName,
String downsampleIndexName
String downsampleIndexName,
Integer actionsOrderVersion
) {

public LifecycleExecutionState {
if (actionsOrderVersion == null) {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should also enforce that it's > 0, to avoid weird -1 issues

actionsOrderVersion = 1;
}
}

public static final String ILM_CUSTOM_METADATA_KEY = "ilm";

private static final String PHASE = "phase";
Expand All @@ -58,6 +65,7 @@ public record LifecycleExecutionState(
private static final String SNAPSHOT_INDEX_NAME = "snapshot_index_name";
private static final String SHRINK_INDEX_NAME = "shrink_index_name";
private static final String DOWNSAMPLE_INDEX_NAME = "rollup_index_name";
private static final String ACTIONS_ORDER_VERSION = "actions_order_version";
Copy link
Member

Choose a reason for hiding this comment

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

I think we should go with action_order_version and avoid the awkward plural-action-but-singular-version noun


public static final LifecycleExecutionState EMPTY_STATE = LifecycleExecutionState.builder().build();

Expand All @@ -82,7 +90,8 @@ public static Builder builder(LifecycleExecutionState state) {
.setShrinkIndexName(state.shrinkIndexName)
.setSnapshotIndexName(state.snapshotIndexName)
.setDownsampleIndexName(state.downsampleIndexName)
.setStepTime(state.stepTime);
.setStepTime(state.stepTime)
.setActionsOrderVersion(state.actionsOrderVersion);
}

public static LifecycleExecutionState fromCustomMetadata(Map<String, String> customData) {
Expand Down Expand Up @@ -191,6 +200,10 @@ public static LifecycleExecutionState fromCustomMetadata(Map<String, String> cus
if (downsampleIndexName != null) {
builder.setDownsampleIndexName(downsampleIndexName);
}
String actionsOrderVersion = customData.get(ACTIONS_ORDER_VERSION);
if (actionsOrderVersion != null) {
builder.setActionsOrderVersion(Integer.parseInt(actionsOrderVersion));
}
return builder.build();
}

Expand Down Expand Up @@ -253,6 +266,9 @@ public Map<String, String> asMap() {
if (downsampleIndexName != null) {
result.put(DOWNSAMPLE_INDEX_NAME, downsampleIndexName);
}
if (actionsOrderVersion != null) {
result.put(ACTIONS_ORDER_VERSION, String.valueOf(actionsOrderVersion));
}
return Collections.unmodifiableMap(result);
}

Expand All @@ -274,6 +290,7 @@ public static class Builder {
private String shrinkIndexName;
private String snapshotIndexName;
private String downsampleIndexName;
private Integer actionsOrderVersion;

public Builder setPhase(String phase) {
this.phase = phase;
Expand Down Expand Up @@ -360,6 +377,11 @@ public Builder setDownsampleIndexName(String downsampleIndexName) {
return this;
}

public Builder setActionsOrderVersion(Integer actionsOrderVersion) {
this.actionsOrderVersion = actionsOrderVersion;
return this;
}

public LifecycleExecutionState build() {
return new LifecycleExecutionState(
phase,
Expand All @@ -378,7 +400,8 @@ public LifecycleExecutionState build() {
snapshotName,
shrinkIndexName,
snapshotIndexName,
downsampleIndexName
downsampleIndexName,
actionsOrderVersion
);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,10 +206,11 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
*
* @param client The Elasticsearch Client to use during execution of {@link AsyncActionStep}
* and {@link AsyncWaitStep} steps.
* @param actionsOrderVersion the version of the actions order to use
* @param licenseState The license state to use in actions and steps
* @return The list of {@link Step} objects in order of their execution.
*/
public List<Step> toSteps(Client client, XPackLicenseState licenseState) {
public List<Step> toSteps(Client client, int actionsOrderVersion, XPackLicenseState licenseState) {
List<Step> steps = new ArrayList<>();
List<Phase> orderedPhases = type.getOrderedPhases(phases);
ListIterator<Phase> phaseIterator = orderedPhases.listIterator(orderedPhases.size());
Expand All @@ -233,7 +234,7 @@ public List<Step> toSteps(Client client, XPackLicenseState licenseState) {
}

phase = previousPhase;
List<LifecycleAction> orderedActions = type.getOrderedActions(phase);
List<LifecycleAction> orderedActions = type.getOrderedActions(phase, actionsOrderVersion);
ListIterator<LifecycleAction> actionIterator = orderedActions.listIterator(orderedActions.size());
// add steps for each action, in reverse
while (actionIterator.hasPrevious()) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public interface LifecycleType extends NamedWriteable {
*/
List<Phase> getOrderedPhases(Map<String, Phase> phases);

List<LifecycleAction> getOrderedActions(Phase phase);
List<LifecycleAction> getOrderedActions(Phase phase, int orderVersion);

/**
* validates whether the specified <code>phases</code> are valid for this
Expand All @@ -31,4 +31,6 @@ public interface LifecycleType extends NamedWriteable {
* if a specific phase or lack of a specific phase is invalid.
*/
void validate(Collection<Phase> phases);

int getLatestActionsOrderVersion();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add javadocs for this?

}
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public static void refreshPhaseDefinition(
updatedPolicy.getModifiedDate()
);

// we're not updating the actions order version here as we're still executing the same phase (and the step keys are identical)
LifecycleExecutionState newExState = LifecycleExecutionState.builder(currentExState)
.setPhaseDefinition(Strings.toString(pei, false, false))
.build();
Expand Down Expand Up @@ -192,7 +193,9 @@ public static boolean isIndexPhaseDefinitionUpdatable(
final Step.StepKey currentStepKey = Step.getCurrentStepKey(executionState);
final String currentPhase = currentStepKey.phase();

final Set<Step.StepKey> newStepKeys = newPolicy.toSteps(client, licenseState)
// reading the same order version of the new policy as if we refresh the cached phase, being the same phase
// as it's currently executing, we won't update the actions order version
final Set<Step.StepKey> newStepKeys = newPolicy.toSteps(client, executionState.actionsOrderVersion(), licenseState)
.stream()
.map(Step::getKey)
.collect(Collectors.toCollection(LinkedHashSet::new));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

package org.elasticsearch.xpack.core.ilm;

import org.elasticsearch.common.util.set.Sets;

import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.stream.Stream;

/**
* Defines a record of the actions that are allowed for the timeseries lifecycle.
* The order of actions is versioned, newer versions must contain all previous defined actions and are
* allowed to make additive changes (we do not support removing actions, but actions to be removed
* must be converted to no-ops)
*/
public final class TimeseriesLifecycleActionsRegistry {

public static final int VERSION_ONE = 1;
// moves the downsample action after the migrate action in the warm and cold phases
public static final int VERSION_TWO = 2;
public static final int CURRENT_VERSION = VERSION_TWO;
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion, maybe we should call this LATEST_VERSION, what do you think?


public static final String HOT_PHASE = "hot";
public static final String WARM_PHASE = "warm";
public static final String COLD_PHASE = "cold";
public static final String FROZEN_PHASE = "frozen";
public static final String DELETE_PHASE = "delete";

public static final Map<Integer, List<String>> ORDERED_VALID_HOT_ACTIONS = Map.of(
VERSION_ONE,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
RolloverAction.NAME,
ReadOnlyAction.NAME,
DownsampleAction.NAME,
ShrinkAction.NAME,
ForceMergeAction.NAME,
SearchableSnapshotAction.NAME
).filter(Objects::nonNull).toList(),
VERSION_TWO,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
RolloverAction.NAME,
ReadOnlyAction.NAME,
DownsampleAction.NAME,
ShrinkAction.NAME,
ForceMergeAction.NAME,
SearchableSnapshotAction.NAME
).filter(Objects::nonNull).toList()
Comment on lines +38 to +59
Copy link
Member

Choose a reason for hiding this comment

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

Since these are the same, can we put them in a variable so it's clear here that the order isn't changing between versions?

);

public static final Map<Integer, List<String>> ORDERED_VALID_WARM_ACTIONS = Map.of(
VERSION_ONE,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
ReadOnlyAction.NAME,
DownsampleAction.NAME,
AllocateAction.NAME,
MigrateAction.NAME,
ShrinkAction.NAME,
ForceMergeAction.NAME
).filter(Objects::nonNull).toList(),
VERSION_TWO,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
ReadOnlyAction.NAME,
AllocateAction.NAME,
MigrateAction.NAME,
DownsampleAction.NAME,
Comment on lines +79 to +81
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a comment about the differences here, so that someone doesn't have to eyeball it?

ShrinkAction.NAME,
ForceMergeAction.NAME
).filter(Objects::nonNull).toList()
);

public static final Map<Integer, List<String>> ORDERED_VALID_COLD_ACTIONS = Map.of(
VERSION_ONE,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
ReadOnlyAction.NAME,
DownsampleAction.NAME,
SearchableSnapshotAction.NAME,
AllocateAction.NAME,
MigrateAction.NAME,
FreezeAction.NAME
).filter(Objects::nonNull).toList(),
VERSION_TWO,
Stream.of(
SetPriorityAction.NAME,
UnfollowAction.NAME,
ReadOnlyAction.NAME,
SearchableSnapshotAction.NAME,
AllocateAction.NAME,
MigrateAction.NAME,
DownsampleAction.NAME,
Comment on lines +105 to +107
Copy link
Member

Choose a reason for hiding this comment

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

Same here about adding a comment

FreezeAction.NAME
).filter(Objects::nonNull).toList()
);

public static final Map<Integer, List<String>> ORDERED_VALID_FROZEN_ACTIONS = Map.of(
VERSION_ONE,
List.of(UnfollowAction.NAME, SearchableSnapshotAction.NAME),
VERSION_TWO,
List.of(UnfollowAction.NAME, SearchableSnapshotAction.NAME)
);
public static final Map<Integer, List<String>> ORDERED_VALID_DELETE_ACTIONS = Map.of(
VERSION_ONE,
List.of(WaitForSnapshotAction.NAME, DeleteAction.NAME),
VERSION_TWO,
List.of(WaitForSnapshotAction.NAME, DeleteAction.NAME)
);

static final Set<String> VALID_HOT_ACTIONS = Sets.newHashSet(ORDERED_VALID_HOT_ACTIONS.get(CURRENT_VERSION));
static final Set<String> VALID_WARM_ACTIONS = Sets.newHashSet(ORDERED_VALID_WARM_ACTIONS.get(CURRENT_VERSION));
static final Set<String> VALID_COLD_ACTIONS = Sets.newHashSet(ORDERED_VALID_COLD_ACTIONS.get(CURRENT_VERSION));
static final Set<String> VALID_FROZEN_ACTIONS = Sets.newHashSet(ORDERED_VALID_FROZEN_ACTIONS.get(CURRENT_VERSION));
static final Set<String> VALID_DELETE_ACTIONS = Sets.newHashSet(ORDERED_VALID_DELETE_ACTIONS.get(CURRENT_VERSION));
Comment on lines +125 to +129
Copy link
Member

Choose a reason for hiding this comment

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

Are we concerned that a step may be considered valid but that a different version may not contain the action name? For example, we have to be careful not to remove any actions from later version because they will be considered "invalid" even if they exist in older action version orders.

Should we have an assert for this?


static final Map<String, Set<String>> ALLOWED_ACTIONS = Map.of(
HOT_PHASE,
VALID_HOT_ACTIONS,
WARM_PHASE,
VALID_WARM_ACTIONS,
COLD_PHASE,
VALID_COLD_ACTIONS,
DELETE_PHASE,
VALID_DELETE_ACTIONS,
FROZEN_PHASE,
VALID_FROZEN_ACTIONS
);
}
Loading