Skip to content

Commit 540d40c

Browse files
committed
flasharray: address Copilot review on delete()
- Use e.getMessage() instead of e.toString() in the delete-time benign-error catches so the substring check is not coupled to JVM exception formatting. - Generate the deletion-timestamp suffix in UTC via DateTimeFormatter.withZone(ZoneOffset.UTC) so the rename is stable across management servers in different timezones (and across DST changes). - Sharpen the inline comment on snapshot deletion to spell out why the rename is skipped: FlashArray volume/snapshot names must match [A-Za-z0-9_-] and start/end with an alphanumeric, so the rename target would have an embedded '.' which the array rejects. Also drop the now-unused java.text.SimpleDateFormat import. Signed-off-by: Eugenio Grosso <eugenio.grosso@gmail.com>
1 parent 41d00d4 commit 540d40c

1 file changed

Lines changed: 23 additions & 10 deletions

File tree

  • plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray

plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@
2323
import java.security.KeyManagementException;
2424
import java.security.KeyStoreException;
2525
import java.security.NoSuchAlgorithmException;
26-
import java.text.SimpleDateFormat;
26+
import java.time.ZoneOffset;
27+
import java.time.format.DateTimeFormatter;
2728
import java.util.ArrayList;
2829
import java.util.HashMap;
2930
import java.util.Map;
@@ -88,6 +89,9 @@ public class FlashArrayAdapter implements ProviderAdapter {
8889
static final ObjectMapper mapper = new ObjectMapper();
8990
public String pod = null;
9091
public String hostgroup = null;
92+
private static final DateTimeFormatter DELETION_TIMESTAMP_FORMAT =
93+
DateTimeFormatter.ofPattern("yyyyMMddHHmmss").withZone(ZoneOffset.UTC);
94+
9195
private String username;
9296
private String password;
9397
private String accessToken;
@@ -202,20 +206,24 @@ public void detach(ProviderAdapterContext context, ProviderAdapterDataObject dat
202206
public void delete(ProviderAdapterContext context, ProviderAdapterDataObject dataObject) {
203207
String fullName = normalizeName(pod, dataObject.getExternalName());
204208

205-
// Snapshots live under /volume-snapshots and already use the reserved
206-
// form <volume>.<suffix>. FlashArray snapshot rename constraints do
207-
// not let us rename them to an arbitrary timestamp-suffixed name
208-
// before deletion, so unlike volumes we skip rename and only mark
209-
// them destroyed.
209+
// Snapshots live under /volume-snapshots and already use the
210+
// reserved form <volume>.<suffix>. FlashArray volume/snapshot names
211+
// must match [A-Za-z0-9_-] and start/end with an alphanumeric, so
212+
// appending our usual deletion-timestamp suffix to a snapshot name
213+
// would produce a target like "<vol>.<n>-<ts>" - the embedded "."
214+
// is rejected by the array. We therefore skip the rename for
215+
// snapshots and only mark them destroyed; the array's own ".N"
216+
// suffix already disambiguates them in the recycle bin.
210217
if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
211218
try {
212219
FlashArrayVolume destroy = new FlashArrayVolume();
213220
destroy.setDestroyed(true);
214221
PATCH("/volume-snapshots?names=" + fullName, destroy, new TypeReference<FlashArrayList<FlashArrayVolume>>() {
215222
});
216223
} catch (CloudRuntimeException e) {
217-
if (e.toString().contains("No such volume or snapshot")
218-
|| e.toString().contains("Volume does not exist")) {
224+
String msg = e.getMessage();
225+
if (msg != null && (msg.contains("No such volume or snapshot")
226+
|| msg.contains("Volume does not exist"))) {
219227
return;
220228
}
221229
throw e;
@@ -233,7 +241,11 @@ public void delete(ProviderAdapterContext context, ProviderAdapterDataObject dat
233241
// CloudStack side. FlashArray rejects a single PATCH that combines
234242
// {name, destroyed}, so the rename and the destroy must be issued as
235243
// two separate requests each carrying only its own field.
236-
String timestamp = new SimpleDateFormat("yyyyMMddHHmmss").format(new java.util.Date());
244+
// Use UTC so the rename suffix is stable regardless of the management
245+
// server's local timezone or DST changes - operators correlating the
246+
// CloudStack delete event with the array's audit log get a consistent
247+
// wall-clock value.
248+
String timestamp = DELETION_TIMESTAMP_FORMAT.format(java.time.Instant.now());
237249
String renamedName = fullName + "-" + timestamp;
238250

239251
try {
@@ -247,7 +259,8 @@ public void delete(ProviderAdapterContext context, ProviderAdapterDataObject dat
247259
PATCH("/volumes?names=" + renamedName, destroy, new TypeReference<FlashArrayList<FlashArrayVolume>>() {
248260
});
249261
} catch (CloudRuntimeException e) {
250-
if (e.toString().contains("Volume does not exist")) {
262+
String msg = e.getMessage();
263+
if (msg != null && msg.contains("Volume does not exist")) {
251264
return;
252265
} else {
253266
throw e;

0 commit comments

Comments
 (0)