Copilot commented on code in PR #13049:
URL: https://github.com/apache/cloudstack/pull/13049#discussion_r3115392040
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
+ if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
+ try {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volume-snapshots?names=" + fullName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ });
Review Comment:
The `names=` query parameter is built via string concatenation without
URL-encoding. If `fullName`/`renamedName` contains reserved characters (e.g.,
`:` seen in `cloudstack::...`, or any future name changes), the request can be
mis-parsed by the HTTP stack or server. Build the URI with proper query
encoding (e.g., encode the `names` value or use a URI/query builder) before
calling `PATCH`.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
+ if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
+ try {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volume-snapshots?names=" + fullName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ });
+ } catch (CloudRuntimeException e) {
+ if (e.toString().contains("No such volume or snapshot")
+ || e.toString().contains("Volume does not exist")) {
+ return;
+ }
+ throw e;
+ }
Review Comment:
Matching error conditions via `e.toString().contains(...)` is brittle (it
can include class names, nested exceptions, and formatting changes). Prefer
checking a stable signal such as `e.getMessage()`, an HTTP status code/response
body field from the underlying API client (if accessible), or a dedicated
helper that inspects the FlashArray error payload. This will reduce false
positives/negatives and improve long-term reliability.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
Review Comment:
This comment reads as if FlashArray rejects *snapshot names* containing '.',
but snapshots inherently have `<volume>.<suffix>`. To avoid confusion, reword
to clarify that FlashArray rejects renaming snapshots to an arbitrary
timestamp-suffixed name (i.e., snapshot rename constraints differ), hence the
decision to skip rename and only set `destroyed=true`.
```suggestion
// Snapshots live under /volume-snapshots and already use the
reserved
// form <volume>.<suffix>. FlashArray snapshot rename constraints do
// not let us rename them to an arbitrary timestamp-suffixed name
// before deletion, so unlike volumes we skip rename and only mark
// them destroyed.
```
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
+ if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
+ try {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volume-snapshots?names=" + fullName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ });
+ } catch (CloudRuntimeException e) {
+ if (e.toString().contains("No such volume or snapshot")
+ || e.toString().contains("Volume does not exist")) {
+ return;
+ }
+ throw e;
+ }
+ return;
+ }
- // rename as we delete so it doesn't conflict if the template or
volume is ever recreated
- // pure keeps the volume(s) around in a Destroyed bucket for a period
of time post delete
+ // first make sure we are disconnected
+ removeVlunsAll(context, pod, dataObject.getExternalName());
+
+ // Rename then destroy: FlashArray keeps destroyed volumes in a recycle
+ // bin (default 24h) from which they can be recovered. Renaming with a
+ // deletion timestamp gives operators a forensic trail when browsing
the
+ // array - they can see when each destroyed copy was deleted on the
+ // CloudStack side. FlashArray rejects a single PATCH that combines
+ // {name, destroyed}, so the rename and the destroy must be issued as
+ // two separate requests each carrying only its own field.
String timestamp = new SimpleDateFormat("yyyyMMddHHmmss").format(new
java.util.Date());
- volume.setExternalName(fullName + "-" + timestamp);
+ String renamedName = fullName + "-" + timestamp;
try {
- PATCH("/volumes?names=" + fullName, volume, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ FlashArrayVolume rename = new FlashArrayVolume();
+ rename.setExternalName(renamedName);
+ PATCH("/volumes?names=" + fullName, rename, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
});
Review Comment:
The `names=` query parameter is built via string concatenation without
URL-encoding. If `fullName`/`renamedName` contains reserved characters (e.g.,
`:` seen in `cloudstack::...`, or any future name changes), the request can be
mis-parsed by the HTTP stack or server. Build the URI with proper query
encoding (e.g., encode the `names` value or use a URI/query builder) before
calling `PATCH`.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
+ if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
+ try {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volume-snapshots?names=" + fullName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ });
+ } catch (CloudRuntimeException e) {
+ if (e.toString().contains("No such volume or snapshot")
+ || e.toString().contains("Volume does not exist")) {
+ return;
+ }
+ throw e;
+ }
+ return;
+ }
- // rename as we delete so it doesn't conflict if the template or
volume is ever recreated
- // pure keeps the volume(s) around in a Destroyed bucket for a period
of time post delete
+ // first make sure we are disconnected
+ removeVlunsAll(context, pod, dataObject.getExternalName());
+
+ // Rename then destroy: FlashArray keeps destroyed volumes in a recycle
+ // bin (default 24h) from which they can be recovered. Renaming with a
+ // deletion timestamp gives operators a forensic trail when browsing
the
+ // array - they can see when each destroyed copy was deleted on the
+ // CloudStack side. FlashArray rejects a single PATCH that combines
+ // {name, destroyed}, so the rename and the destroy must be issued as
+ // two separate requests each carrying only its own field.
String timestamp = new SimpleDateFormat("yyyyMMddHHmmss").format(new
java.util.Date());
- volume.setExternalName(fullName + "-" + timestamp);
+ String renamedName = fullName + "-" + timestamp;
Review Comment:
The deletion timestamp uses the JVM default timezone, which can produce
inconsistent rename suffixes across management servers (or after a timezone/DST
change), making the operator-facing forensic trail harder to interpret.
Consider generating the timestamp in a fixed timezone (e.g., UTC) using
`java.time` (`Instant` + `DateTimeFormatter`) to ensure consistent, comparable
names.
##########
plugins/storage/volume/flasharray/src/main/java/org/apache/cloudstack/storage/datastore/adapter/flasharray/FlashArrayAdapter.java:
##########
@@ -200,25 +200,50 @@ public void detach(ProviderAdapterContext context,
ProviderAdapterDataObject dat
@Override
public void delete(ProviderAdapterContext context,
ProviderAdapterDataObject dataObject) {
- // first make sure we are disconnected
- removeVlunsAll(context, pod, dataObject.getExternalName());
String fullName = normalizeName(pod, dataObject.getExternalName());
- FlashArrayVolume volume = new FlashArrayVolume();
+ // Snapshots live under /volume-snapshots and have the reserved form
+ // <volume>.<suffix>: the FlashArray rejects renames to any name that
+ // includes '.' or is not pure alphanumeric+'_'+'-', so we cannot
+ // tag them with a timestamp on the way out. Just mark them destroyed.
+ if (dataObject.getType() == ProviderAdapterDataObject.Type.SNAPSHOT) {
+ try {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volume-snapshots?names=" + fullName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ });
+ } catch (CloudRuntimeException e) {
+ if (e.toString().contains("No such volume or snapshot")
+ || e.toString().contains("Volume does not exist")) {
+ return;
+ }
+ throw e;
+ }
+ return;
+ }
- // rename as we delete so it doesn't conflict if the template or
volume is ever recreated
- // pure keeps the volume(s) around in a Destroyed bucket for a period
of time post delete
+ // first make sure we are disconnected
+ removeVlunsAll(context, pod, dataObject.getExternalName());
+
+ // Rename then destroy: FlashArray keeps destroyed volumes in a recycle
+ // bin (default 24h) from which they can be recovered. Renaming with a
+ // deletion timestamp gives operators a forensic trail when browsing
the
+ // array - they can see when each destroyed copy was deleted on the
+ // CloudStack side. FlashArray rejects a single PATCH that combines
+ // {name, destroyed}, so the rename and the destroy must be issued as
+ // two separate requests each carrying only its own field.
String timestamp = new SimpleDateFormat("yyyyMMddHHmmss").format(new
java.util.Date());
- volume.setExternalName(fullName + "-" + timestamp);
+ String renamedName = fullName + "-" + timestamp;
try {
- PATCH("/volumes?names=" + fullName, volume, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ FlashArrayVolume rename = new FlashArrayVolume();
+ rename.setExternalName(renamedName);
+ PATCH("/volumes?names=" + fullName, rename, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
});
- // now delete it with new name
- volume.setDestroyed(true);
-
- PATCH("/volumes?names=" + fullName + "-" + timestamp, volume, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
+ FlashArrayVolume destroy = new FlashArrayVolume();
+ destroy.setDestroyed(true);
+ PATCH("/volumes?names=" + renamedName, destroy, new
TypeReference<FlashArrayList<FlashArrayVolume>>() {
});
Review Comment:
The `names=` query parameter is built via string concatenation without
URL-encoding. If `fullName`/`renamedName` contains reserved characters (e.g.,
`:` seen in `cloudstack::...`, or any future name changes), the request can be
mis-parsed by the HTTP stack or server. Build the URI with proper query
encoding (e.g., encode the `names` value or use a URI/query builder) before
calling `PATCH`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
