Skip to content

Commit 504c4c7

Browse files
Gupta, SuryaGupta, Surya
Gupta, Surya
authored and
Gupta, Surya
committed
[CSTACKEX-204] Address Review Comments
1 parent 060126d commit 504c4c7

4 files changed

Lines changed: 63 additions & 53 deletions

File tree

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/asup/OntapAsupManager.java‎

Lines changed: 21 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@
8484
* <p>Runs are booked one at a time: after each cycle starts, the next run is scheduled for
8585
* {@link OntapConfigurationManager#AsupIntervalHours} after that start, so production
8686
* intervals (hours) are start-to-start. REST work sits inside the interval; it is not added
87-
* after it. Editing either ONTAP ASUP setting re-books the pending run immediately, with no
87+
* after it. Editing the ONTAP ASUP interval re-books the pending run immediately, with no
8888
* management-server restart.</p>
8989
*/
9090
public class OntapAsupManager extends ManagerBase {
@@ -143,7 +143,7 @@ public class OntapAsupManager extends ManagerBase {
143143
@Override
144144
public boolean configure(String name, Map<String, Object> params) throws ConfigurationException {
145145
super.configure(name, params);
146-
// React to edits of the ONTAP ASUP settings so a new interval re-books the pending run
146+
// React to edits of ontap.autosupport.interval so a new value re-books the pending run
147147
// instead of waiting for it to fire on the old schedule.
148148
messageBus.subscribe(EventTypes.EVENT_CONFIGURATION_VALUE_EDIT, this::onAsupConfigEdited);
149149
return true;
@@ -152,8 +152,7 @@ public boolean configure(String name, Map<String, Object> params) throws Configu
152152
@Override
153153
public boolean start() {
154154
asupScheduler = Executors.newSingleThreadScheduledExecutor(new NamedThreadFactory("OntapAsup"));
155-
logger.info("OntapAsupManager started; ASUP telemetry enabled={}, interval={}h",
156-
OntapConfigurationManager.AsupEnabled.value(),
155+
logger.info("OntapAsupManager started; ASUP telemetry interval={}h",
157156
getAsupIntervalHours(OntapConfigurationManager.AsupIntervalHours.value()));
158157
scheduleNextRun();
159158
return super.start();
@@ -169,11 +168,12 @@ public boolean stop() {
169168

170169
/**
171170
* Books the single pending run for the moment the configured interval elapses, cancelling
172-
* whatever was booked before. Called at start-up, after every run, and whenever one of the
173-
* ONTAP ASUP settings is edited, so the live config always decides the next run.
171+
* whatever was booked before. Called at start-up, after every run, and whenever
172+
* {@code ontap.autosupport.interval} is edited, so the live config always decides the next run.
174173
*
175-
* <p>When ASUP is disabled nothing is booked; re-enabling it publishes a configuration-edit
176-
* event, which books a run again.</p>
174+
* <p>When the interval is {@link OntapStorageConstants#ASUP_DISABLED_INTERVAL_HOURS}
175+
* nothing is booked; setting a non-zero interval publishes a configuration-edit event,
176+
* which books a run again.</p>
177177
*/
178178
private synchronized void scheduleNextRun() {
179179
if (asupScheduler == null || asupScheduler.isShutdown()) {
@@ -183,9 +183,11 @@ private synchronized void scheduleNextRun() {
183183
pendingRun.cancel(false);
184184
pendingRun = null;
185185
}
186-
if (Boolean.FALSE.equals(OntapConfigurationManager.AsupEnabled.value())) {
187-
logger.debug("ONTAP ASUP: telemetry is disabled ({}=false); no run scheduled.",
188-
OntapConfigurationManager.AsupEnabled.key());
186+
int intervalHours = getAsupIntervalHours(OntapConfigurationManager.AsupIntervalHours.value());
187+
if (intervalHours == OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS) {
188+
logger.debug("ONTAP ASUP: telemetry is disabled ({}={}); no run scheduled.",
189+
OntapStorageConstants.ASUP_INTERVAL_CONFIG_KEY,
190+
OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS);
189191
return;
190192
}
191193
long delayMs = millisUntilNextPush();
@@ -218,8 +220,7 @@ private void onAsupConfigEdited(String senderAddress, String subject, Object arg
218220
return;
219221
}
220222
String updatedKey = ((Ternary<String, ConfigKey.Scope, Long>) args).first();
221-
if (!OntapConfigurationManager.AsupEnabled.key().equals(updatedKey)
222-
&& !OntapConfigurationManager.AsupIntervalHours.key().equals(updatedKey)) {
223+
if (!OntapConfigurationManager.AsupIntervalHours.key().equals(updatedKey)) {
223224
return;
224225
}
225226
logger.debug("ONTAP ASUP: [{}] was updated; re-booking the next push.", updatedKey);
@@ -618,15 +619,18 @@ private String defaultUnknown(String value) {
618619
}
619620

620621
/**
621-
* Returns a usable interval in hours. Out-of-range or missing DB values
622-
* (for example set outside the API) fall back to the default so ASUP is not
623-
* sent on an unintended cadence. The scheduler converts this to seconds via
624-
* {@link Duration#ofHours(long)}.
622+
* Returns a usable interval in hours. {@link OntapStorageConstants#ASUP_DISABLED_INTERVAL_HOURS}
623+
* means telemetry is off. Out-of-range or missing DB values (for example set outside the API)
624+
* fall back to the default so ASUP is not sent on an unintended cadence. The scheduler
625+
* converts a non-zero value to seconds via {@link Duration#ofHours(long)}.
625626
*/
626627
int getAsupIntervalHours(Integer configured) {
627628
if (configured == null) {
628629
return OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS;
629630
}
631+
if (configured == OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS) {
632+
return OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS;
633+
}
630634
if (configured < OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS
631635
|| configured > OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS) {
632636
logger.warn("ONTAP ASUP: {} value [{}] is outside [{}-{}]; using default [{}]",

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapConfigurationManager.java‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,21 +32,16 @@
3232
* construct new {@code ConfigKey} instances.
3333
*/
3434
public class OntapConfigurationManager implements Configurable {
35-
public static final ConfigKey<Boolean> AsupEnabled = new ConfigKey<>(
36-
OntapStorageConstants.ADVANCED_CONFIG_KEY_CATEGORY, Boolean.class,
37-
OntapStorageConstants.ASUP_ENABLED_CONFIG_KEY, OntapStorageConstants.ASUP_ENABLED_DEFAULT,
38-
OntapStorageConstants.ASUP_ENABLED_DESCRIPTION,
39-
true, ConfigKey.Scope.Global);
40-
4135
public static final ValidatedConfigKey<Integer> AsupIntervalHours = new ValidatedConfigKey<>(
4236
OntapStorageConstants.ADVANCED_CONFIG_KEY_CATEGORY, Integer.class,
4337
OntapStorageConstants.ASUP_INTERVAL_CONFIG_KEY,
4438
String.valueOf(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS),
4539
OntapStorageConstants.ASUP_INTERVAL_DESCRIPTION,
46-
true, ConfigKey.Scope.Global, null, asupIntervalValidator());
40+
true, ConfigKey.Scope.Global, null,
41+
OntapStorageConstants.ASUP_INTERVAL_DISPLAY_TEXT, asupIntervalValidator());
4742

4843
public static final ConfigKey<?>[] CONFIG_KEYS = new ConfigKey<?>[] {
49-
AsupEnabled, AsupIntervalHours
44+
AsupIntervalHours
5045
};
5146

5247
/**
@@ -60,7 +55,8 @@ private static Consumer<Integer> asupIntervalValidator() {
6055
}
6156

6257
/**
63-
* Rejects {@code ontap.asup.interval} values that are not integers in
58+
* Rejects {@code ontap.autosupport.interval} values that are not {@link
59+
* OntapStorageConstants#ASUP_DISABLED_INTERVAL_HOURS} (disabled) or integers in
6460
* [{@link OntapStorageConstants#ASUP_MIN_INTERVAL_HOURS},
6561
* {@link OntapStorageConstants#ASUP_MAX_INTERVAL_HOURS}] hours.
6662
* Invoked by {@link ValidatedConfigKey} when the setting is saved in Global Settings.
@@ -79,6 +75,9 @@ private static void validateAsupInterval(Object raw) {
7975
OntapStorageConstants.ASUP_INTERVAL_CONFIG_KEY + " must be an integer. "
8076
+ asupIntervalRangeMessage());
8177
}
78+
if (parsed == OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS) {
79+
return;
80+
}
8281
if (parsed < OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS
8382
|| parsed > OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS) {
8483
throw new InvalidParameterValueException(asupIntervalRangeMessage());
@@ -87,8 +86,9 @@ private static void validateAsupInterval(Object raw) {
8786

8887
private static String asupIntervalRangeMessage() {
8988
return String.format(
90-
"%s must be between %d and %d hours.",
89+
"%s must be %d to disable, or between %d and %d hours.",
9190
OntapStorageConstants.ASUP_INTERVAL_CONFIG_KEY,
91+
OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS,
9292
OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS,
9393
OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS);
9494
}

‎plugins/storage/volume/ontap/src/main/java/org/apache/cloudstack/storage/utils/OntapStorageConstants.java‎

Lines changed: 14 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -167,20 +167,19 @@ public class OntapStorageConstants {
167167
public static final String ASUP_TOTAL_LOGICAL_SIZE_BYTES = "totalLogicalSizeBytes";
168168
public static final String ASUP_VOLUME_SNAPSHOT_COUNT = "volumeSnapshotCount";
169169
public static final String ASUP_VM_SNAPSHOT_COUNT = "vmSnapshotCount";
170-
public static final String ASUP_GLOBAL_LOCK_NAME = "ontap.asup.push";
171-
public static final String ASUP_ENABLED_CONFIG_KEY = "ontap.asup.enabled";
172-
public static final String ASUP_ENABLED_DEFAULT = "true";
173-
public static final String ASUP_INTERVAL_CONFIG_KEY = "ontap.asup.interval";
174-
public static final int ASUP_MIN_INTERVAL_HOURS = 3;
175-
public static final int ASUP_MAX_INTERVAL_HOURS = 24;
176-
public static final int ASUP_DEFAULT_INTERVAL_HOURS = 12;
177-
178-
public static final String ASUP_ENABLED_DESCRIPTION =
179-
"Set to true to enable telemetry reporting from the CloudStack ONTAP plugin, or false to disable it. "
180-
+ "Changes take effect immediately and do not require a management server restart.";
170+
public static final String ASUP_GLOBAL_LOCK_NAME = "ontap.autosupport.push";
171+
public static final String ASUP_INTERVAL_CONFIG_KEY = "ontap.autosupport.interval";
172+
/** {@code ontap.autosupport.interval} value that disables telemetry reporting. */
173+
public static final int ASUP_DISABLED_INTERVAL_HOURS = 0;
174+
public static final int ASUP_MIN_INTERVAL_HOURS = 1;
175+
public static final int ASUP_MAX_INTERVAL_HOURS = 168; // once a week
176+
public static final int ASUP_DEFAULT_INTERVAL_HOURS = 24; // once a day
177+
public static final String ASUP_INTERVAL_DISPLAY_TEXT = "NetApp ONTAP AutoSupport interval";
178+
181179
public static final String ASUP_INTERVAL_DESCRIPTION =
182-
"Sets the telemetry reporting interval (in hours) for the CloudStack ONTAP plugin. "
183-
+ "Allowed range: " + ASUP_MIN_INTERVAL_HOURS + "-" + ASUP_MAX_INTERVAL_HOURS
184-
+ " hours, with a default of " + ASUP_DEFAULT_INTERVAL_HOURS + " hours. "
185-
+ "Changes take effect immediately and do not require a management server restart.";
180+
"The interval in hours between telemetry reports sent to the ONTAP cluster. "
181+
+ ASUP_DISABLED_INTERVAL_HOURS + " disables reporting; the default is "
182+
+ ASUP_DEFAULT_INTERVAL_HOURS + " (allowed range "
183+
+ ASUP_MIN_INTERVAL_HOURS + "-" + ASUP_MAX_INTERVAL_HOURS
184+
+ " hours). Changes take effect immediately and do not require a management server restart.";
186185
}

‎plugins/storage/volume/ontap/src/test/java/org/apache/cloudstack/storage/asup/OntapAsupManagerTest.java‎

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -519,30 +519,36 @@ void asupIntervalHours_defaultIsProductionValue() {
519519
OntapConfigurationManager.AsupIntervalHours.defaultValue());
520520
}
521521

522+
@Test
523+
void asupIntervalHours_keyAndDisplayText() {
524+
assertEquals(OntapStorageConstants.ASUP_INTERVAL_CONFIG_KEY,
525+
OntapConfigurationManager.AsupIntervalHours.key());
526+
assertEquals(OntapStorageConstants.ASUP_INTERVAL_DISPLAY_TEXT,
527+
OntapConfigurationManager.AsupIntervalHours.displayText());
528+
}
529+
522530
@Test
523531
void asupIntervalHours_descriptionIncludesAllowedRange() {
524532
String description = OntapConfigurationManager.AsupIntervalHours.description();
525533
assertTrue(description.contains(String.valueOf(OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS)));
526534
assertTrue(description.contains(String.valueOf(OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS)));
535+
assertTrue(description.contains(String.valueOf(OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS)));
527536
}
528537

529538
@Test
530-
void asupEnabled_defaultIsTrue() {
531-
assertEquals("true", OntapConfigurationManager.AsupEnabled.defaultValue());
532-
}
533-
534-
@Test
535-
void validateAsupInterval_acceptsMinMaxAndDefault() {
539+
void validateAsupInterval_acceptsDisabledMinMaxAndDefault() {
540+
OntapConfigurationManager.AsupIntervalHours.validateValue(String.valueOf(OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS));
536541
OntapConfigurationManager.AsupIntervalHours.validateValue(String.valueOf(OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS));
537542
OntapConfigurationManager.AsupIntervalHours.validateValue(String.valueOf(OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS));
538543
OntapConfigurationManager.AsupIntervalHours.validateValue(String.valueOf(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS));
539544
}
540545

541546
@Test
542547
void validateAsupInterval_rejectsOutOfRangeAndNonInteger() {
548+
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("1"));
543549
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("2"));
544-
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("0"));
545-
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("25"));
550+
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("3"));
551+
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("169"));
546552
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue("abc"));
547553
assertThrows(InvalidParameterValueException.class, () -> OntapConfigurationManager.AsupIntervalHours.validateValue(""));
548554
}
@@ -551,16 +557,17 @@ void validateAsupInterval_rejectsOutOfRangeAndNonInteger() {
551557
void getAsupIntervalHours_fallsBackOutsideRange() {
552558
assertEquals(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS,
553559
asupManager.getAsupIntervalHours(null));
554-
assertEquals(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS,
555-
asupManager.getAsupIntervalHours(0));
560+
assertEquals(OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS,
561+
asupManager.getAsupIntervalHours(OntapStorageConstants.ASUP_DISABLED_INTERVAL_HOURS));
556562
assertEquals(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS,
557563
asupManager.getAsupIntervalHours(2));
558564
assertEquals(OntapStorageConstants.ASUP_DEFAULT_INTERVAL_HOURS,
559-
asupManager.getAsupIntervalHours(25));
565+
asupManager.getAsupIntervalHours(169));
560566
assertEquals(OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS,
561567
asupManager.getAsupIntervalHours(OntapStorageConstants.ASUP_MIN_INTERVAL_HOURS));
562568
assertEquals(OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS,
563569
asupManager.getAsupIntervalHours(OntapStorageConstants.ASUP_MAX_INTERVAL_HOURS));
570+
assertEquals(25, asupManager.getAsupIntervalHours(25));
564571
}
565572

566573
// ──────────────────────────────────────────────────────────────────────────

0 commit comments

Comments
 (0)