Skip to content

Replicate rootDiskController behavior to dataDiskController. #4569

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
rohityadavcloud merged 1 commit into from
Jan 28, 2021
Merged

Replicate rootDiskController behavior to dataDiskController. #4569

rohityadavcloud merged 1 commit into from
Jan 28, 2021

Conversation

@GutoVeronezi
Copy link
Contributor

@GutoVeronezi GutoVeronezi commented Jan 5, 2021

Description

The bus type to data disk volumes is hardcoded to virtio or scsi, when using virtio-scsi (or, based on the template type). Therefore, there is no way to specify the bus type to data disk volumes (as we have for root disks).

This PR intends to replicate the rootDiskController behavior to dataDiskController, allowing the definition of the controller.

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

How Has This Been Tested?

It has been tested locally.

Replicate rootDiskController behavior to dataDiskController.
bb50dad 
The bus type to `data disk` volumes is hardcoded to `virtio` or `scsi`, when using virtio-scsi (or, based on the template type). Therefore, there is no way to specify the bus type to data disk volumes (as we have for root disks).
This PR intends to replicate the `rootDiskController` behavior to `dataDiskController`, allowing the definition of the controller.
RodrigoDLopez
RodrigoDLopez approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@RodrigoDLopez RodrigoDLopez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice Enhancement @GutoVeronezi
can you please explain to me how the local tests were carried out?

My tests consist of entering the details of the data disk (during the deployment process of an instance or via a graphical interface with a previously created instance) and then checking if the details informed are present in the dump XML of the target instance.

I performed manual tests with this branch. It is correct and working as it should.
LGTM

@RodrigoDLopez
Copy link
Contributor

RodrigoDLopez commented Jan 6, 2021

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Jan 6, 2021

@RodrigoDLopez a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Jan 6, 2021

Packaging result: ✔centos7 ✔centos8 ✔debian. JID-2527

@RodrigoDLopez
Copy link
Contributor

RodrigoDLopez commented Jan 6, 2021

@blueorangutan test

@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Jan 7, 2021

@RodrigoDLopez thanks for asking.

Actually the process is quite similar as which you described.

I made the whole process via GUI.

  1. I created an instance.

  2. With it stopped, I created a volume and attached to it.

  3. I setted the dataDiskController setting as ide and started the instance.

  4. After that, I checked if the detail informed was in the dump XML of the instance.
    What I got was <target dev = 'hda' bus = 'ide' />, which was what I expected.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jan 18, 2021

@blueorangutan test

@DaanHoogland DaanHoogland added this to the 4.16.0.0 milestone Jan 18, 2021
@blueorangutan
Copy link

blueorangutan commented Jan 18, 2021

@DaanHoogland a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

blueorangutan commented Jan 18, 2021

Trillian test result (tid-3388)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 32743 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4569-t3388-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Smoke tests completed. 86 look OK, 0 have error(s)
Only failed tests results shown below:

Test Result Time (s) Test File

DaanHoogland
DaanHoogland reviewed Jan 20, 2021
View reviewed changes
Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

one concern; the default used to be virtio and now it is null? otherwise the code looks good

@GutoVeronezi
Copy link
Contributor Author

GutoVeronezi commented Jan 21, 2021

one concern; the default used to be virtio and now it is null? otherwise the code looks good

@DaanHoogland
The method getDataDiskModelFromVMDetail returns null if the dataDiskController setting is not configured.
If the method returns null, then it will keep the same behavior as before, so the default still is virtio.

@DaanHoogland
Copy link
Contributor

DaanHoogland commented Jan 22, 2021

@GutoVeronezi you are right, i read with crooked eyes.

@rohityadavcloud rohityadavcloud merged commit 3a4a82d into apache:master Jan 28, 2021
@GutoVeronezi GutoVeronezi deleted the enable-set-data-disk-controller-from-vm-details branch January 28, 2021 11:30
@sureshanaparti sureshanaparti mentioned this pull request Dec 24, 2025
Open
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rohityadavcloud rohityadavcloud rohityadavcloud approved these changes

@DaanHoogland DaanHoogland DaanHoogland approved these changes

+1 more reviewer

@RodrigoDLopez RodrigoDLopez RodrigoDLopez approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

complexity:normal Severity:Minor type:enhancement

Projects

None yet

Milestone

4.16.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@GutoVeronezi @RodrigoDLopez @blueorangutan @DaanHoogland @rohityadavcloud