Skip to content

Add support for volumeAttributesClassName on persistent storage #11210

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

Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
editableEnabled = false,
builderPackage = Constants.FABRIC8_KUBERNETES_API
)
@JsonPropertyOrder({"id", "type", "size", "kraftMetadata", "class", "selector", "deleteClaim", "overrides"})
@JsonPropertyOrder({"id", "type", "size", "kraftMetadata", "class", "volumeAttributesClass", "selector", "deleteClaim", "overrides"})
@JsonInclude(JsonInclude.Include.NON_DEFAULT)
@EqualsAndHashCode(callSuper = true)
@ToString(callSuper = true)
Expand All @@ -36,6 +36,7 @@ public class PersistentClaimStorage extends SingleVolumeStorage {
private Map<String, String> selector;
private boolean deleteClaim;
private List<PersistentClaimStorageOverride> overrides;
private String volumeAttributesClass;

@Description("Must be `" + TYPE_PERSISTENT_CLAIM + "`")
@Override
Expand Down Expand Up @@ -125,4 +126,13 @@ public List<PersistentClaimStorageOverride> getOverrides() {
public void setOverrides(List<PersistentClaimStorageOverride> overrides) {
this.overrides = overrides;
}

@Description("Specifies `VolumeAttributeClass` name for dynamic volume allocation.")
public String getVolumeAttributesClass() {
return this.volumeAttributesClass;
}

public void setVolumeAttributesClass(String volumeAttributesClass) {
this.volumeAttributesClass = volumeAttributesClass;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ private static PersistentVolumeClaim createPersistentVolumeClaim(
.withRequests(requests)
.endResources()
.withStorageClassName(storage.getStorageClass())
.withVolumeAttributesClassName(storage.getVolumeAttributesClass())
.withSelector(storageSelector)
.withVolumeMode("Filesystem")
.endSpec()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import io.strimzi.operator.common.model.AbstractJsonDiff;

import java.util.HashSet;
import java.util.Objects;
import java.util.Set;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand All @@ -36,6 +37,7 @@ public class StorageDiff extends AbstractJsonDiff {
private final boolean volumesAddedOrRemoved;
private final boolean tooManyKRaftMetadataVolumes;
private final boolean duplicateVolumeIds;
private final boolean volumeAttributesClassChanged;

/**
* Diffs the storage for allowed or not allowed changes. Examples of allowed changes is increasing volume size or
Expand Down Expand Up @@ -70,6 +72,7 @@ private StorageDiff(Reconciliation reconciliation, Storage current, Storage desi
boolean volumesAddedOrRemoved = false;
boolean tooManyKRaftMetadataVolumes = false;
boolean duplicateVolumeIds = false;
boolean volumeAttributesClassChanged = false;
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to know it changed? Isn't it sufficient to ignore the change? We do not seem to use this anywhere in the production code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It is only being used in tests. Can remove it and the related tests. I'll just add some KafkaAssemblyOperator tests as well

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 the tests for the StorageDiff can just test that it does not report any issues when the volume class atrributes change, or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right... Didn't think this through before replying. Thanks


if (current instanceof JbodStorage currentJbodStorage && desired instanceof JbodStorage desiredJbodStorage) {
Set<Integer> volumeIds = new HashSet<>();
Expand Down Expand Up @@ -99,6 +102,7 @@ private StorageDiff(Reconciliation reconciliation, Storage current, Storage desi
changesType |= diff.changesType();
shrinkSize |= diff.shrinkSize();
isEmpty &= diff.isEmpty();
volumeAttributesClassChanged |= diff.volumeAttributesClassChanged();
}
} else {
JsonNode source = PATCH_MAPPER.valueToTree(current == null ? "{}" : current);
Expand All @@ -115,16 +119,25 @@ private StorageDiff(Reconciliation reconciliation, Storage current, Storage desi
continue;
}

// It might be possible to increase the volume size, but never to shrink volumes
// When size changes, we need to detect whether it is shrinking or increasing
if (pathValue.endsWith("/size") && desired.getType().equals(current.getType()) && current instanceof PersistentClaimStorage persistentCurrent && desired instanceof PersistentClaimStorage persistentDesired) {
if (current instanceof PersistentClaimStorage persistentCurrent && desired instanceof PersistentClaimStorage persistentDesired
&& desired.getType().equals(current.getType())) {
// It might be possible to increase the volume size, but never to shrink volumes
// When size changes, we need to detect whether it is shrinking or increasing
if (pathValue.endsWith("/size")) {

long currentSize = StorageUtils.convertToMillibytes(persistentCurrent.getSize());
long desiredSize = StorageUtils.convertToMillibytes(persistentDesired.getSize());
long currentSize = StorageUtils.convertToMillibytes(persistentCurrent.getSize());
long desiredSize = StorageUtils.convertToMillibytes(persistentDesired.getSize());

if (currentSize > desiredSize) {
shrinkSize = true;
} else {
if (currentSize > desiredSize) {
shrinkSize = true;
} else {
continue;
}
}

if (pathValue.endsWith("/volumeAttributesClass") &&
!Objects.equals(persistentCurrent.getVolumeAttributesClass(), persistentDesired.getVolumeAttributesClass())) {
volumeAttributesClassChanged = true;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't here be check that it's really different in comparison current/desired?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I didn't think about that. Will change

continue;
}
}
Expand All @@ -148,6 +161,7 @@ private StorageDiff(Reconciliation reconciliation, Storage current, Storage desi
this.volumesAddedOrRemoved = volumesAddedOrRemoved;
this.tooManyKRaftMetadataVolumes = tooManyKRaftMetadataVolumes;
this.duplicateVolumeIds = duplicateVolumeIds;
this.volumeAttributesClassChanged = volumeAttributesClassChanged;
}

/**
Expand Down Expand Up @@ -214,6 +228,15 @@ protected boolean shrinkSize() {
return shrinkSize;
}

/**
* Returns true if there's a difference in {@code /volumeAttributesClass}
*
* @return true when the volumeAttributesClass changes
*/
protected boolean volumeAttributesClassChanged() {
return volumeAttributesClassChanged;
}

/**
* Returns true if some JBOD volumes were added or removed
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public class PersistentVolumeClaimUtilsTest {
.build();
private final static PersistentClaimStorage PERSISTENT_CLAIM_STORAGE = new PersistentClaimStorageBuilder()
.withStorageClass("my-storage-class")
.withVolumeAttributesClass("my-volume-attributes-class")
.withSize("100Gi")
.build();
private final static Set<NodeRef> SINGLE_NODE = Set.of(new NodeRef(NAME + "-" + 0, 0, null, false, true));
Expand Down Expand Up @@ -106,6 +107,7 @@ public void testPersistentClaimStorage() {
assertThat(pvcs.get(0).getSpec().getSelector(), is(nullValue()));
assertThat(pvcs.get(0).getSpec().getResources().getRequests(), is(Map.of("storage", new Quantity("100Gi", null))));
assertThat(pvcs.get(0).getSpec().getStorageClassName(), is("my-storage-class"));
assertThat(pvcs.get(0).getSpec().getVolumeAttributesClassName(), is("my-volume-attributes-class"));
}

@ParallelTest
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,15 @@ public void testJbodDiff() {
assertThat(diff.isEmpty(), is(true));
assertThat(diff.shrinkSize(), is(false));
assertThat(diff.isVolumesAddedOrRemoved(), is(false));
assertThat(diff.volumeAttributesClassChanged(), is(false));
assertThat(diff.issuesDetected(), is(false));

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, jbod, jbod2, Set.of(0, 1, 5), Set.of(0, 1, 5));
assertThat(diff.changesType(), is(false));
assertThat(diff.isEmpty(), is(false));
assertThat(diff.shrinkSize(), is(false));
assertThat(diff.isVolumesAddedOrRemoved(), is(false));
assertThat(diff.volumeAttributesClassChanged(), is(false));
assertThat(diff.issuesDetected(), is(true));
}

Expand All @@ -56,11 +58,13 @@ public void testPersistentDiff() {
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(true));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(false));

assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(true));
}

Expand Down Expand Up @@ -96,16 +100,19 @@ public void testOverridesAreIgnoredInDiff() {
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(true));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(false));

assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(true));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(false));

assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(true));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(false));
}

Expand All @@ -132,6 +139,7 @@ public void testEphemeralDiff() {
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, ephemeral, ephemeral, Set.of(0, 1, 5), Set.of(0, 1, 5)).changesType(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, ephemeral, ephemeral, Set.of(0, 1, 5), Set.of(0, 1, 5)).isEmpty(), is(true));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, ephemeral, ephemeral, Set.of(0, 1, 5), Set.of(0, 1, 5)).shrinkSize(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, ephemeral, ephemeral, Set.of(0, 1, 5), Set.of(0, 1, 5)).volumeAttributesClassChanged(), is(false));
assertThat(new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, ephemeral, ephemeral, Set.of(0, 1, 5), Set.of(0, 1, 5)).issuesDetected(), is(false));
}

Expand Down Expand Up @@ -363,4 +371,48 @@ public void testDuplicateVolumeIds() {
assertThat(diff.isDuplicateVolumeIds(), is(true));
assertThat(diff.issuesDetected(), is(true));
}

@ParallelTest
public void testVolumeAttributesChanges() {
Storage persistent = new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withDeleteClaim(false).withId(0).withSize("100Gi").build();
Storage persistent2 = new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withDeleteClaim(false).withId(0).withSize("100Gi").withVolumeAttributesClass("vac-example").build();
Storage persistent3 = new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withDeleteClaim(false).withId(0).withSize("100Gi").withVolumeAttributesClass("vac-example-2").build();

StorageDiff diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(false));
assertThat(diff.issuesDetected(), is(false));

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent, persistent2, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(true));
assertThat(diff.issuesDetected(), is(false));

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, persistent2, persistent3, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(true));
assertThat(diff.issuesDetected(), is(false));

Storage jbod = new JbodStorageBuilder().withVolumes(
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(0).withVolumeAttributesClass("vac-example-1").build(),
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(1).withVolumeAttributesClass("vac-example-2").build()
).build();
Storage jbod2 = new JbodStorageBuilder().withVolumes(
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(0).build(),
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(1).withVolumeAttributesClass("vac-example-2").build()
).build();
Storage jbod3 = new JbodStorageBuilder().withVolumes(
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(0).withVolumeAttributesClass("vac-example-2").build(),
new PersistentClaimStorageBuilder().withStorageClass("gp2-ssd").withId(1).withVolumeAttributesClass("vac-example-2").build()
).build();

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, jbod, jbod, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(false));
assertThat(diff.issuesDetected(), is(false));

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, jbod, jbod2, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(true));
assertThat(diff.issuesDetected(), is(false));

diff = new StorageDiff(Reconciliation.DUMMY_RECONCILIATION, jbod, jbod3, Set.of(0, 1, 2), Set.of(0, 1, 2));
assertThat(diff.volumeAttributesClassChanged(), is(true));
assertThat(diff.issuesDetected(), is(false));
}
}
3 changes: 3 additions & 0 deletions documentation/modules/appendix_crds.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -671,6 +671,9 @@ It must have the value `persistent-claim` for the type `PersistentClaimStorage`.
|class
|string
|The storage class to use for dynamic volume allocation.
|volumeAttributesClass
|string
|Specifies `VolumeAttributeClass` name for dynamic volume allocation.
|selector
|map
|Specifies a specific persistent volume to use. It contains key:value pairs representing labels for selecting such a volume.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -599,6 +599,9 @@ spec:
- persistent-claim
- jbod
description: "Storage type, must be either 'ephemeral', 'persistent-claim', or 'jbod'."
volumeAttributesClass:
type: string
description: Specifies `VolumeAttributeClass` name for dynamic volume allocation.
volumes:
type: array
items:
Expand Down Expand Up @@ -649,6 +652,9 @@ spec:
- ephemeral
- persistent-claim
description: "Storage type, must be either 'ephemeral' or 'persistent-claim'."
volumeAttributesClass:
type: string
description: Specifies `VolumeAttributeClass` name for dynamic volume allocation.
required:
- type
description: List of volumes as Storage objects representing the JBOD disks array.
Expand Down Expand Up @@ -2348,6 +2354,9 @@ spec:
- ephemeral
- persistent-claim
description: "Storage type, must be either 'ephemeral' or 'persistent-claim'."
volumeAttributesClass:
type: string
description: Specifies `VolumeAttributeClass` name for dynamic volume allocation.
required:
- type
description: Storage configuration (disk). Cannot be updated.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,9 @@ spec:
- persistent-claim
- jbod
description: "Storage type, must be either 'ephemeral', 'persistent-claim', or 'jbod'."
volumeAttributesClass:
type: string
description: Specifies `VolumeAttributeClass` name for dynamic volume allocation.
volumes:
type: array
items:
Expand Down Expand Up @@ -161,6 +164,9 @@ spec:
- ephemeral
- persistent-claim
description: "Storage type, must be either 'ephemeral' or 'persistent-claim'."
volumeAttributesClass:
type: string
description: Specifies `VolumeAttributeClass` name for dynamic volume allocation.
required:
- type
description: List of volumes as Storage objects representing the JBOD disks array.
Expand Down
Loading