Skip to content

CSTACKEX-25: ONTAP Primary storage pool creation - #5

Closed
sandeeplocharla wants to merge 35 commits into
mainfrom
feature/CSTACKEX-25
Closed

sandeeplocharla wants to merge 35 commits into
mainfrom
feature/CSTACKEX-25

Conversation

@sandeeplocharla

@sandeeplocharla sandeeplocharla commented Oct 13, 2025

Copy link
Copy Markdown
Collaborator

Description

This PR...

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Comment thread plugins/storage/volume/ontap/pom.xml Outdated
</parent>
<properties>
<spring-cloud.version>2021.0.7</spring-cloud.version>
<spring-boot.version>2.7.10</spring-boot.version>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this and below commented ones

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taken care of

private String name;
private List<AggregateDTO> aggregates;
private SvmDTO svm;
private Integer size;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make it of type Long. And please mention the comment stating "this value would always be in bytes".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


return null;

String url = dsInfos.get("url").toString();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add logger with input map values

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


// Additional details requested for ONTAP primary storage pool creation
@SuppressWarnings("unchecked")
Map<String, String> details = (Map<String, String>)dsInfos.get("details");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We have to check for empty and null both.
  2. We have to throw exceptions in case of failed validations.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added where its necessary

public boolean attachZone(DataStore dataStore, ZoneScope scope, Hypervisor.HypervisorType hypervisorType) {
return false;
List<HostVO> hostsToConnect = new ArrayList<>();
Hypervisor.HypervisorType[] hypervisorTypes = {Hypervisor.HypervisorType.XenServer, Hypervisor.HypervisorType.VMware, Hypervisor.HypervisorType.KVM};

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please limit support for the KVM hypervisor for now. I think that the remaining would be done in future releases.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@github-actions

Copy link
Copy Markdown

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

@sandeeplocharla sandeeplocharla changed the title CSTACKEX-25: Basic class structure CSTACKEX-25: ONTAP Primary storage pool creation Oct 17, 2025
}
}

if (storagePoolName == null || storagePoolName.isEmpty()) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add check for space

// TODO: scheme could be 'custom' in our case and we might have to ask 'protocol' separately to the user
if (scheme.equalsIgnoreCase(Constants.NFS)) {
parameters.setType(Storage.StoragePoolType.NetworkFilesystem);
} else if (scheme.equalsIgnoreCase(Constants.ISCSI)) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:// Compare it with custom
and specific protocol field should be included on UI and validated here

Username = username;
Password = password;
ManagementLIF = managementLIF;
OntapStorage.SVM = SVM;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should just be 'svm'

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add this in feign/model

import org.springframework.stereotype.Component;

@Component
public class StorageProviderManager {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change the name to StorageProviderFactory

public StorageProviderManager(OntapStorage ontapStorage) {
String protocol = ontapStorage.getProtocol();
s_logger.info("Initializing StorageProviderManager with protocol: " + protocol);
if (protocol.equalsIgnoreCase(Constants.NFS)) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change this to switch and add isDisaggregated flag to OntapStoragr, add the respective condition here

try {
// Call the SVM API to check if the SVM exists
Svm svm = null;
URI url = URI.create(Constants.HTTPS + storage.getManagementLIF() + Constants.GETSVMs);

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create a common method to Utility.java for uri creation

details.get(Constants.MANAGEMENTLIF), details.get(Constants.SVMNAME), scheme); //TODO: Here the passing 'scheme' might need a re-look
StorageProviderManager storageProviderManager = new StorageProviderManager(ontapStorage);
StorageStrategy storageStrategy = storageProviderManager.getStrategy();
boolean isValid = storageStrategy.connect();

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

put this in try catch

boolean isValid = storageStrategy.connect();
if (isValid) {
// String volumeName = storagePoolName + "_vol"; //TODO: Figure out a better naming convention
storageStrategy.createVolume(storagePoolName, Long.parseLong((details.get("size")))); // TODO: size should be in bytes, so see if conversion is needed

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try catch block

public boolean attachCluster(DataStore dataStore, ClusterScope scope) {
logger.debug("In attachCluster for ONTAP primary storage");
PrimaryDataStoreInfo primarystore = (PrimaryDataStoreInfo)dataStore;
List<HostVO> hostsToConnect = _resourceMgr.getEligibleUpAndEnabledHostsInClusterForStorageConnection(primarystore);

@sandeeplocharla sandeeplocharla Oct 17, 2025

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add check to see if the host supports iscsi, because the scheme could be 'custom'

for (HostVO host : hostsToConnect) {
// TODO: Fetch the host IQN and add to the initiator group on ONTAP cluster
try {
_storageMgr.connectHostToSharedPool(host, dataStore.getId());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read about connectHostsToPool and decide on which one to use

@rajiv-jain-netapp rajiv-jain-netapp left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please exclude the files which are taken from other PR and repost.

sureshanaparti and others added 26 commits October 21, 2025 08:15
* Return details of the storage pool in the response including url, and update capacityBytes and capacityIops if applicable while creating storage pool

* Added capacitybytes parameter to the storage pool response in sync with the capacityiops response parameter and createStoragePool cmd request parameter (existing disksizetotal parameter in the storage pool response can be deprecated)

* Don't keep url in details

* Persist the capacityBytes and capacityIops in the storage_pool_details table while creating storage pool as well, for consistency - as these are updated with during update storage pool

* rebase with main fixes
…pache#11488)

Addresses apache#11483

Signed-off-by: Abhishek Kumar <abhishek.mrt22@gmail.com>
Co-authored-by: Henrique Sato <henrique.sato@scclouds.com.br>
apache#11773)

* storage: change storage pool to Up state when cancel storage migration

* Update 11773: connect host to shared pool after cancelling storage migration

* Update 11773: update db only

* Update 11773: skip capacity update for storpool
… added EOF fixes + correcting license header
* CSTACKEX-29 Cluster, SVM and Aggr Feign Client

* CSTACKEX-29 Change the endpoint method name in feign client

* CSTACKEX-29 Make the alignment proper

* CSTACKEX-29 Added License Info

* CSTACKEX-29 Resolve Review Comments

* CSTACKEX-29 Remove Component Annotation from datastoredriverclass

* CSTACKEX-29 Resolve Style check issues

* CSTACKEX-29 Resolve ALL Style issues

* CSTACKEX-29 Resolve Precommits Issues

* CSTACKEX-29 Added Method comments and change the ontap response class name

---------

Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
* CSTACKEX-31 NAS and Job Feign Client and POJOs

* CSTACKEX-31 Fixed Checks Issues

* CSTACKEX-31 Resolve Review Comments

* CSTACKEX-31 Resolve Review Comments

* CSTACKEX-31 Resolve Review Comments

* CSTACKEX-31 Added Aggr and size to volume model

* CSTACKEX-31 Change the export policy endpoint path

* CSTACKEX-31 Fixed check styles

---------

Co-authored-by: Gupta, Surya <Surya.Gupta@netapp.com>
…ommits.

# This is the 1st commit message:

CSTACKEX-25: Basic class structure

# This is the commit message #2:

Add PrimaryStoragePool base code

# This is the commit message #3:

CSTACKEX-25: Create Volume code basic code added

# This is the commit message #4:

CSTACKEX-25: additional logic for Primary storage pool creation

# This is the commit message #5:

CSTACKEX-29 Cluster, SVM and Aggr Feign Client

# This is the commit message #6:

CSTACKEX-29 Added License Info

# This is the commit message #7:

CSTACKEX-29 Resolve Review Comments

# This is the commit message #8:

CSTACKEX-29 Resolve Style check issues

� This is the commit message #9:

CSTACKEX-29 Resolve Style check issues

� This is the commit message #10:

CSTACKEX-29 Resolve Precommits Issues

# This is the commit message #11:

CSTACKEX-29 Resolve Precommits Issues
Add PrimaryStoragePool base code

CSTACKEX-25: Create Volume code basic code added

CSTACKEX-25: additional logic for Primary storage pool creation

CSTACKEX-29 Cluster, SVM and Aggr Feign Client

CSTACKEX-29 Added License Info

CSTACKEX-29 Resolve Review Comments

CSTACKEX-29 Resolve Style check issues

� This is the commit message #9:

CSTACKEX-29 Resolve Style check issues

� This is the commit message #10:

CSTACKEX-29 Resolve Precommits Issues

CSTACKEX-29 Resolve Precommits Issues

CSTACKEX-25: PrimaryStoragePool create workflow almost done

CSTACKEX-25: PrimaryStoragePool create workflow almost done

CSTACKEX-25: Added license string to new file
piyush5netapp pushed a commit that referenced this pull request Dec 4, 2025
# This is the 1st commit message:

NFS Cloudstack volume and export policy utils

# This is the commit message #2:

Licencse add in files

# This is the commit message #3:

accessgroup create recode

# This is the commit message #4:

creatacessgroup for NFS impl

# This is the commit message #5:

storage pool mounting on host

# This is the commit message #6:

storage pool mounting on host 1

# This is the commit message #7:

vm restart issue

# This is the commit message #8:

vm restart issue 1

# This is the commit message #9:

vm restart issue 2

# This is the commit message #10:

vm instance creation test1

# This is the commit message #11:

vm instance creation test4
sandeeplocharla pushed a commit that referenced this pull request Sep 16, 2026
* storage: enable RBD/Ceph volume encryption support (shared base)

Flip StoragePoolType.RBD from EncryptionSupport.Unsupported to Hypervisor so the
existing encryption control plane (allocator, endpoint selector, offerings) treats
RBD pools as encryption-capable.

The agent-side encrypted RBD create path is not implemented yet; it is delivered by
two follow-up tracks (qemu-native engine='qemu' and ceph-native engine='librbd').
Until then, fail closed at the two RBD create chokepoints in LibvirtStorageAdaptor
(createPhysicalDisk and createDiskFromTemplate) when a passphrase is present, so we
never silently produce a plaintext volume that the control plane believes is encrypted.

No change for existing unencrypted RBD volumes (guards only fire when a passphrase
is set; supportsEncryption() only affects volumes that require encryption).

* kvm: Ceph-native LUKS2 encryption for RBD volumes (engine='librbd')

Implements encrypted RBD data and root disks using librbd's native LUKS2
encryption, decrypted at runtime by libvirt/qemu via <encryption
engine='librbd'>. CloudStack manages the passphrase (existing model).

- RbdEncryption: isolated helper wrapping `rbd encryption format luks2`,
  cephx via --id + keyfile (secret not on the command line), LUKS passphrase
  via KeyFile. Kept separate so the CLI can later be swapped for a JNA binding
  (rados-java has no rbd_encryption_format API).
- LibvirtStorageAdaptor: create/clone the raw RBD image, then apply
  `rbd encryption format luks2`; mark the disk LUKS2 so encrypt_format
  propagates to the volume. Replaces the fail-closed guards.
- QemuObject.EncryptFormat: add LUKS2.
- LibvirtVMDef: render <encryption format='luks2' engine='librbd'>; the
  encrypt details now carry an optional engine.
- attach (KVMStorageProcessor) and boot (LibvirtComputingResource): set
  engine='librbd' for RBD-backed encrypted volumes.

NOTE: the CoW-clone-then-format path (encrypted root from an unencrypted
template) needs live-cluster validation for the parent-grow / usable-size
behaviour described in the Ceph image-encryption docs.

Builds: api + plugins/hypervisors/kvm (JDK11).

* kvm: gate host encryption probe on librbd support for RBD

hostSupportsVolumeEncryption() now advertises encryption capability if the
host supports EITHER qemu-native LUKS (qemu-img LUKS + cryptsetup) OR librbd
native encryption (rbd CLI with the encryption subcommand). Previously a
Ceph-only host that lacked cryptsetup would not advertise encryption even
though librbd can encrypt RBD volumes.

Split into hostSupportsQemuNativeVolumeEncryption() and
hostSupportsRbdVolumeEncryption(); kept HOST_VOLUME_ENCRYPTION as the single
host-wide flag (documented limitation: not per-pool).

* kvm: resize support for librbd-encrypted RBD volumes (#5)

Encrypted RBD volumes are encrypted natively by librbd and must be resized
with `rbd resize --encryption-passphrase-file` so librbd grows the encrypted
payload and keeps the LUKS header consistent. The existing encrypted-resize
path (resizeEncryptedQcowFile) uses qemu-img --object secret, which is for
qemu-native LUKS and does not fit the librbd LUKS2 layout.

- RbdEncryption.resize(): new `rbd resize` wrapper (cephx via --id + keyfile,
  passphrase via KeyFile, optional --allow-shrink).
- LibvirtResizeVolumeCommandWrapper: detect encrypted RBD and route to the rbd
  resize path, bypassing the libvirt v.resize and qemu-img paths.

Snapshot/revert, RBD<->RBD copy, and migration of encrypted RBD volumes need
no code changes: they operate on the raw (LUKS-containing) image at the block
level, and the destination passphrase secret is already created engine-agnostic
in LibvirtPrepareForMigrationCommandWrapper. These still require live validation.

Builds: plugins/hypervisors/kvm (JDK11).

* kvm: route online resize of encrypted RBD through virsh blockresize

For a running VM, an librbd-encrypted RBD volume must be resized in-band by
qemu/librbd, not out-of-band by the rbd CLI. Gate the CLI rbd-resize path on
!vmIsRunning so:
 - offline -> `rbd resize --encryption-passphrase-file` (librbd-aware), and
 - online  -> existing NOTIFYONLY path -> virsh blockresize, where qemu's
   block_resize delegates to librbd to grow the encrypted payload and notify
   the guest in one step (no passphrase needed; qemu holds the secret).

This avoids notify-less out-of-band growth and qemu/librbd size divergence
while the image is open. Online behaviour still needs live validation that
blockresize resizes the encrypted payload for engine='librbd' disks.

* kvm: encrypted RBD root disks (thin CoW clone + full-copy fallback)

Root disks could not be encrypted: cloning a plaintext template and then
`rbd encryption format`ing the clone leaves the inherited OS data unreadable
(the LUKS header offsets it), so the guest could not mount root.

Fix, in createDiskFromTemplateOnRBD, with two paths:
- Option A (same-cluster cached RBD template): grow the template base to
  reserve LUKS2 header space, snapshot+protect it (cloudstack-base-snap-luks),
  clone from it, apply the LUKS2 header, resize the clone to the requested
  size. Inherited template data stays readable through the clone's encryption
  and the clone is a thin CoW image (only the header is written).
- Option B (first-use / non-RBD template): create an empty image, apply a
  LUKS2 header, then import the template THROUGH the encryption layer via
  RbdEncryption.importTemplate (qemu-img convert -n into encrypt.key-secret).
  Correct but a full copy.

Validated end-to-end on Ubuntu 26.04 / libvirt 12.0.0: both boot; A is thin
(3.5 GiB provisioned, ~120 MiB used); LUKS2 verified at rest on Ceph.

* kvm: harden and align librbd-encrypted RBD volume code

Review pass over the librbd LUKS2 encryption feature to fix latent issues
and bring it in line with CloudStack conventions:

- RbdEncryption: reject empty/null passphrase with a clear error; round
  rbd --size up to MiB so a non-aligned request never shrinks the volume
  below what was asked for; create the temporary cephx conf/keyring 0600
  explicitly instead of relying on the umask.
- LibvirtStorageAdaptor: close Rados/IoCTX/RbdImage in a finally block on
  the encrypted-root paths (mirrors deleteVolume) so handles are not
  leaked on exceptions; use parameterized log messages instead of string
  concatenation; extract the encrypted-root Option A/B logic into
  createEncryptedRootCoWClone / createEncryptedRootFullCopy.
- RbdEncryption: use an instance logger (matching the plugin convention)
  and split argv construction into build{Format,Resize,Convert}Script so
  the generated commands can be unit-tested.

* kvm: add RbdEncryption unit tests

Assert the rbd/qemu-img argv built for format, resize and
convert-through-encryption (RBD and file sources), and that empty/null
passphrases are rejected. Command construction is verified without a
live Ceph cluster.

* kvm: refuse encrypted RBD hot-plug on libvirt < 10.1.0

libvirt 10.0.0 has an object apply-order bug (fixed in 10.1.0) that breaks
hot-plug of an encrypted rbd blockdev: on attach the disk is opened before its
LUKS secret object is defined, so the attach fails with "No secret with id
'...-format-encryption-secret0'". Booting a VM from an encrypted RBD disk is
unaffected (the QEMU command line resolves all -object before -blockdev).

Refuse the attach up front with a clear error (mirroring the existing
openvswitch/io_uring libvirt-version gates) instead of letting libvirt fail
opaquely. Only the RBD hot-plug path is gated; boot/root/detach are untouched.

* docs: add PendingReleaseNotes entry for librbd-encrypted RBD volumes

* kvm: route the encrypted RBD template import through QemuImg

RbdEncryption built its own 'qemu-img convert' command line, which
duplicated qemu-img knowledge outside of QemuImg. QemuImg could only
write to a plain filename destination, so importing a template through
the librbd encryption layer was not expressible with it.

QemuImg now supports a destination described by image options
(--target-image-opts, with -n implied since such a target always exists
already), exposed as convertIntoExistingTarget(). QemuImageOptions can
render its parameters under either image-opts flag.

RbdEncryption.importTemplate now composes QemuImageOptions and a
QemuObject secret and delegates to QemuImg; its hand-built convert
script is removed. The rbd CLI calls (encryption format, resize,
support probe) stay, as qemu-img cannot perform them.

No functional change to the generated command.

* kvm: use readable variable names in the encrypted RBD root helpers

Review feedback: single-letter and abbreviated names are hard to read.
Renamed in the two methods added by this PR only (renaming the rest of
the class is out of scope here): r -> radosConnection, io -> ioContext,
rbd -> rbdClient, base -> templateImage, s -> snapshotInfo, encSnap ->
luksReservedSnapshotName, haveEncSnap -> luksSnapshotExists, createSize
-> imageSizeWithLuksHeader, srcIsRbd -> sourceIsRbdPool.

No functional change.

* server, kvm: report and require RBD volume encryption separately

Review feedback: distinguish the two volume encryption mechanisms
instead of advertising them under one host flag.

host.volume.encryption goes back to meaning qemu-native LUKS only
(qemu-img LUKS + cryptsetup), as it did before this PR, and hosts now
additionally report host.volume.encryption.rbd for librbd encryption
(rbd encryption format).

The deployment planner requires the flag matching the pool type of each
encrypted volume - librbd for volumes on RBD pools, qemu-native for any
other pool type - at all three places it validated encryption support
before. The pool is taken from the pools proposed alongside the host
when present, so first deployments are matched accurately too; an
encrypted volume with no pool yet accepts either mechanism and the
storage pool allocator picks a pool the host can serve.

This also stops a host whose librbd is too old for 'rbd encryption
format' from being selected for encrypted RBD volumes; it previously
advertised encryption through the qemu stack and the VM failed to
start.

---------

Co-authored-by: Václav Rozsypálek <vaclav.rozsypalek@master.cz>
Co-authored-by: calvix <7136358+calvix@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.