Conversation
* noVNC: support Spanish Latin American keyboard * Update server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
…#12559) * Fix template details deletion while updating template from UI * update the latest template details before submit
* 4.22: Fixes issue with loading Capacity dashboard when mulitple backup providers configured (apache#12550)
…/domain) (apache#12294) * API modifications for passwordchangerequired * ui login flow for passwordchangerequired * add passwordchangerequired in listUsers API response, it will be used in UI to render reset password form * cleanup redundant LOGIN_SOURCE and limiting apis for first time login * address copilot comments * allow enforcing password change for all role types and update reset pwd flow for passwordchangerequired * address review comments * add unit tests * cleanup ispasswordchangerequired from user_view * address review comments * 1. Allow enforcing password change while creating user 2. Admin can enforce password change on next login with out resetting password * address review comment, add unit test * improve code coverage * fix pre-commit license issue * 1. allow enter key to submit change password form 2. hide force password reset for disabled/locked user in ui * 1. throw exception when force reset password is done for locked/disabled user/account 2. ui validation on current and new password being same 3. allow enforce change password for add user until saml is not enabled * allow oauth login to skip force password change
* 4.22: Fix issue when restoring backup after migration of volume (apache#12549) Usage: Heartbeat should not schedule usage job when a job is already running (apache#12616) Allow limit queries without random ordering (apache#12598) engine/schema: fix cluster/zone settings with encrypted values (apache#12626) Fix injection of preset variables into the JS interpreter (apache#12515) Fix issue with multiple KVM Host entries in host table (apache#12589) Add a Prometheus metric to track host certificate expiry (apache#12613) ssvm: delete temp directory while deleting entity download url (apache#12562)
…PCI class (apache#12981) * Fix domain parsing for GPU * Add Display controller to GPU class check this adds support for the amd instinct mi2xx accelorator crards in the discovery script. Co-authored-by: Piet Braat <piet@phiea.nl>
…as cut by GROUP_CONCAT (apache#12777)
…he#12964) * Introduce configurable timeout to Create NAS backup * use timeout set via "commands.timeout"
* Unhide setting 'js.interpretation.enabled' * Fix grammar mistake
… StoragePool with ONTAP storage (apache#12563) * Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain <Rajiv.Jain@netapp.com> Create & Delete, Enable & Disable, Enter & Cancel maintenance of Primary StoragePool with ONTAP storage Co-authored-by: Rajiv Jain<rajiv1@netapp.com> Edited readme file Fixed license check issue Removed dependency that's not conforming with ACF guidelines * Fixed the initial review comments * Fixed some rebase issues --------- Co-authored-by: Locharla, Sandeep <Sandeep.Locharla@netapp.com>
* New config.json variable to set the ACS default language * Address review
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
sandeeplocharla
left a comment
There was a problem hiding this comment.
There was a comment on public PR requesting the following modularisation in HostListener:
Logic in public boolean hostDisconnected(Host host, StoragePool pool) should be refactored to * public boolean hostDisconnected(long hostId, long poolId)* and public boolean hostDisconnected(long hostId, long poolId) should be called from public boolean hostDisconnected(Host host, StoragePool pool)
| @@ -343,11 +341,12 @@ private void deleteOntapSnapshot(SnapshotInfo snapshotInfo, CommandResult comman | |||
|
|
|||
| @Override | |||
| public void copyAsync(DataObject srcData, DataObject destData, AsyncCompletionCallback<CopyCommandResult> callback) { | |||
| throw new UnsupportedOperationException("Copy operation is not supported for ONTAP primary storage."); | |||
There was a problem hiding this comment.
This might cause an issue while creating instances, please keep an eye on this while testing
There was a problem hiding this comment.
can you share the reason for this suspect ?
There was a problem hiding this comment.
I was suspecting this method to be called during instance creation, but I went back and checked the code. Since, we're returning canCopy as false, there shouldn't be an issue
| ProtocolType protocol = ProtocolType.valueOf(details.get(Constants.PROTOCOL)); | ||
| // Connect to ONTAP and create volume | ||
| long volumeSize = Long.parseLong(details.get(Constants.SIZE)); | ||
| // long volumeSize = Long.parseLong(details.get(OntapStorageConstants.SIZE)); |
There was a problem hiding this comment.
This could be deleted
| s_logger.info("Validating SVM state and protocol settings..."); | ||
| if (!Objects.equals(svm.getState(), Constants.RUNNING)) { | ||
| s_logger.error("SVM " + svmName + " is not in running state."); | ||
| logger.info("Validating SVM state and protocol settings..."); |
There was a problem hiding this comment.
Received a comment on the community PR to modularize the validations and choosing aggregates. Please see if it would be possible to take it up
There was a problem hiding this comment.
refer my previous response on similar comment earlier
There was a problem hiding this comment.
Sure, will create a jira
| s_logger.error("assignExportPolicyToVolume: Job to update volume " + volumeUuid + " did not complete within expected time."); | ||
| while(createVolumeJob == null || !createVolumeJob.getState().equals(OntapStorageConstants.JOB_SUCCESS)) { | ||
| if(jobRetryCount >= OntapStorageConstants.JOB_MAX_RETRIES) { | ||
| logger.error("Job to update volume " + volumeUuid + " did not complete within expected time."); |
There was a problem hiding this comment.
Please add method name here and other places like in warn & trace logs also (wherever missing)
| throw new CloudRuntimeException("Failed to assign export policy: " + e.getMessage()); | ||
| } catch (Exception e) { | ||
| s_logger.error("assignExportPolicyToVolume: Exception while assigning export policy to volume: {}", volumeUuid, e); | ||
| logger.error("assignExportPolicyToVolume: Exception while assigning export policy to volume: {}", volumeUuid, e); | ||
| throw new CloudRuntimeException("Failed to assign export policy: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private boolean createFile(String volumeUuid, String filePath, FileInfo fileInfo) { |
There was a problem hiding this comment.
Please check if we are using "createFile", "deleteFile" and "updateFile". If not make sure to not include these while raising community PR
| return volumeUuid; | ||
| }catch (Exception e){ | ||
| s_logger.error("updateCloudStackVolumeMetadata: Exception while updating volumeInfo: {} in volume: {}", dataStoreId, volumeInfo.getUuid(), e); | ||
| logger.error("updateCloudStackVolumeMetadata: Exception while updating volumeInfo: {} in volume: {}", dataStoreId, volumeInfo.getUuid(), e); | ||
| throw new CloudRuntimeException("Exception while updating volumeInfo: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private Answer createVolumeOnKVMHost(DataObject volumeInfo) { |
There was a problem hiding this comment.
Btw, just an observation, probably these methods should've been *FileOnKVMHost
There was a problem hiding this comment.
It varies from the developer's perception. I am fine with both the conventions. One is from CloudStack perception, where we refer to them as CS-volume, aka volume, and from the storage side, file for NAS.
There was a problem hiding this comment.
Sure, just a thought as the name has KVMHost reference, felt it would be more appropriate to have File instead of Volume
| } | ||
| } catch (Exception e) { | ||
| s_logger.warn("getLogicalAccess: LunMap not found for Lun: {} and igroup: {} ({}).", lunName, igroupName, e.getMessage()); | ||
| logger.warn("getLogicalAccess: LunMap not found for Lun: {} and igroup: {} ({}).", lunName, igroupName, e.getMessage()); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| @Override | ||
| public String ensureLunMapped(String svmName, String lunName, String accessGroupName) { |
There was a problem hiding this comment.
Reminder: To be refactored
| @@ -98,43 +97,15 @@ public DataStore initialize(Map<String, Object> dsInfos) { | |||
| String tags = (String) dsInfos.get("tags"); | |||
| Boolean isTagARule = (Boolean) dsInfos.get("isTagARule"); | |||
There was a problem hiding this comment.
There was a suggestion on community PR to introduce StorageAccessGroups here
String storageAccessGroups = (String)dsInfos.get(ApiConstants.STORAGE_ACCESS_GROUPS); ... parameters.setStorageAccessGroups(storageAccessGroups);
There was a problem hiding this comment.
Can you create a Jira ticket consolidating all these community pending comments? we will pick them together.
|
Changes looks good. Please add some tesing details. |
| @@ -343,11 +341,12 @@ private void deleteOntapSnapshot(SnapshotInfo snapshotInfo, CommandResult comman | |||
|
|
|||
| @Override | |||
| public void copyAsync(DataObject srcData, DataObject destData, AsyncCompletionCallback<CopyCommandResult> callback) { | |||
| throw new UnsupportedOperationException("Copy operation is not supported for ONTAP primary storage."); | |||
There was a problem hiding this comment.
can you share the reason for this suspect ?
| @@ -98,43 +97,15 @@ public DataStore initialize(Map<String, Object> dsInfos) { | |||
| String tags = (String) dsInfos.get("tags"); | |||
| Boolean isTagARule = (Boolean) dsInfos.get("isTagARule"); | |||
There was a problem hiding this comment.
Can you create a Jira ticket consolidating all these community pending comments? we will pick them together.
| s_logger.info("Validating SVM state and protocol settings..."); | ||
| if (!Objects.equals(svm.getState(), Constants.RUNNING)) { | ||
| s_logger.error("SVM " + svmName + " is not in running state."); | ||
| logger.info("Validating SVM state and protocol settings..."); |
There was a problem hiding this comment.
refer my previous response on similar comment earlier
| @@ -237,49 +257,61 @@ public Volume createStorageVolume(String volumeName, Long size) { | |||
| } | |||
| String jobUUID = jobResponse.getJob().getUuid(); | |||
|
|
|||
| //Create URI for GET Job API | |||
| Boolean jobSucceeded = jobPollForSuccess(jobUUID, 10, 1); | |||
| Boolean jobSucceeded = jobPollForSuccess(jobUUID,OntapStorageConstants.JOB_MAX_RETRIES, OntapStorageConstants.CREATE_VOLUME_CHECK_SLEEP_TIME); | |||
There was a problem hiding this comment.
this needs to check since constants have more values for retries and sleep-time as 10 and 1
| return volumeUuid; | ||
| }catch (Exception e){ | ||
| s_logger.error("updateCloudStackVolumeMetadata: Exception while updating volumeInfo: {} in volume: {}", dataStoreId, volumeInfo.getUuid(), e); | ||
| logger.error("updateCloudStackVolumeMetadata: Exception while updating volumeInfo: {} in volume: {}", dataStoreId, volumeInfo.getUuid(), e); | ||
| throw new CloudRuntimeException("Exception while updating volumeInfo: " + e.getMessage()); | ||
| } | ||
| } | ||
|
|
||
| private Answer createVolumeOnKVMHost(DataObject volumeInfo) { |
There was a problem hiding this comment.
It varies from the developer's perception. I am fine with both the conventions. One is from CloudStack perception, where we refer to them as CS-volume, aka volume, and from the storage side, file for NAS.
suryag1201
left a comment
There was a problem hiding this comment.
1-volume/ontap/src/main/java/org/apache/cloudstack/storage/feign/model/ OntapStorage.java This file, i have also modified for storageIP and disaggregated flag
| throw new CloudRuntimeException("ONTAP primary storage creation failed, missing detail(s): " + missing); | ||
| } | ||
| details.put(OntapStorageConstants.SIZE, capacityBytes.toString()); | ||
| details.putIfAbsent(OntapStorageConstants.IS_DISAGGREGATED, "false"); |
There was a problem hiding this comment.
this field check we have removed it
| if (providerName == null || providerName.isEmpty()) { | ||
| throw new CloudRuntimeException("Provider name is null or empty, cannot create primary storage"); | ||
| } | ||
| capacityBytes = validateInitializeInputs(capacityBytes, podId, clusterId, zoneId, storagePoolName, providerName, managed, details); |
There was a problem hiding this comment.
why to return capacityBytes, can we set inside the validateInitializeInputs methods.
Also, method name is not aligning with the operation is is doing as it is also setting the values
There was a problem hiding this comment.
We can take this as part of a refactor separately, pls add a task in the existing Jira or add a new jira.
This PR is focused on rebasing the available changes.
suryag1201
left a comment
There was a problem hiding this comment.
please check the changes for ui/src/views/infra/AddPrimaryStorage.vue this file also
Could you please specify what to check? |
Description
This PR...
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?