[CSTACKEX-204] ASUP Implementation#69
Conversation
suryag1201
requested review from
piyush5netapp,
rajiv-jain-netapp and
sandeeplocharla
as code owners
| */ | ||
| public class OntapAsupManager extends ManagerBase implements Configurable { | ||
|
|
||
| public static final ConfigKey<Boolean> AsupEnabled = new ConfigKey<>( |
There was a problem hiding this comment.
Shall we have one common place for taking config values, instead of creating separate instances everytime we need it? There were few comments on our earlier community PRs to have few fields as configs too.
| OntapStorageConstants.ASUP_ENABLED_DESCRIPTION, | ||
| true, ConfigKey.Scope.Global); | ||
|
|
||
| public static final ConfigKey<Integer> AsupIntervalSeconds = new ConfigKey<>( |
There was a problem hiding this comment.
Changing any config in GlobalSettings doesn't need restarting of Jetty. Is it the same behaviour here?
Also, the config can be changed in 'GlobalSettings' under 'Configuration' on the UI, isn't it?
|
|
||
| // event-id 1: CloudStack storage pool -> backing ONTAP volume mapping, once per pool. | ||
| // The description also includes disk usage and snapshot telemetry | ||
| EmsApplicationLog poolMessage = buildBaseMessage(computerName, appVersion); |
There was a problem hiding this comment.
This event gets sent every 1 min right? Not as the comment saying that it gets sent only once per pool. We need to keep sending the change in pool size. But, isn't 1 min too aggressive?
| */ | ||
| private String buildPoolDescription(StoragePoolVO pool, Map<String, String> details, | ||
| String clusterUuid) { | ||
| Map<String, Object> payload = new LinkedHashMap<>(); |
There was a problem hiding this comment.
Would ONTAP EMS saves only the diff from the previous event? If all the kv's are not needed all the time, should we send few identifiers along with the actual data change as payload?
|
|
||
| @Override | ||
| public Long getDelay() { | ||
| return ASUP_POLL_CHECK_INTERVAL_MS; |
There was a problem hiding this comment.
Should we not set the value from the config here, instead of constant?
| long snapCount = 0; | ||
| if (snapshots != null) { | ||
| for (SnapshotVO snap : snapshots) { | ||
| if (!com.cloud.storage.Snapshot.State.Destroyed.equals(snap.getState())) { |
There was a problem hiding this comment.
Please import this instead.
2 participants
Description
This PR...
ASUP Implementation
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?