FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

import smaller root disk sizes by DaanHoogland · Pull Request #8488 · apache/cloudstack · GitHub

Repository navigation

import smaller root disk sizes - #8488

Closed
DaanHoogland wants to merge 1 commit into
apache:mainfrom
shapeblue:furtherUnmanagedFixes
Closed

DaanHoogland wants to merge 1 commit into
apache:mainfrom
shapeblue:furtherUnmanagedFixes

Conversation

DaanHoogland commented Jan 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

This PR tries to deal with on of the leftovers from #8465 as reported in #8439

  • when root size is 0 it is set to 1 to allow for creation of a disk.

still remaining and to be addressed separately:

  • manual adding an IP address on import fails. this PR adds some logging to try and explain the issue

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

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?

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

Copy link
Copy Markdown

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

codecov Bot commented Jan 10, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (59e78cb) 30.79% compared to head (1a37043) 30.81%.

Files Patch % Lines
.../apache/cloudstack/vm/UnmanagedVMsManagerImpl.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8488      +/-   ##
============================================
+ Coverage     30.79%   30.81%   +0.02%     
- Complexity    33990    34002      +12     
============================================
  Files          5341     5341              
  Lines        374961   374962       +1     
  Branches      54543    54543              
============================================
+ Hits         115452   115544      +92     
+ Misses       244241   244147      -94     
- Partials      15268    15271       +3     
Flag Coverage Δ
simulator-marvin-tests 24.72% <0.00%> (+0.03%) ⬆️
uitests 4.39% <ø> (ø)
unit-tests 16.48% <33.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

shwstppr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code LGTM

DaanHoogland mentioned this pull request Jan 10, 2024
1 of 13 tasks

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8265

DaanHoogland commented Jan 10, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

the rootsize issue seems solved (tested)
the available ips for the network return as an empty list however, so that part needs work;
networkModel.getAvailableIps(network, ipAddresses.getIp4Address()); returns an empty list instead of the list of available IP addresses, because of

2024-01-10 09:40:24,046 DEBUG [c.c.n.NetworkModelImpl] (API-Job-Executor-1:ctx-97ef3f2c job-60 ctx-e9d0a030) (logid:eb59ab23) network defaultGuestNetwork[e77313d2-97d5-4b08-bfcd-2a058f6a4ef0] has no CIDR, returning empty addrress set.

/me still looking
As it is fixing some issue I am adding it to the milestone though

DaanHoogland added this to the 4.19.0.0 milestone Jan 10, 2024
shwstppr self-requested a review January 10, 2024 11:29

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8270

shwstppr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Disk size change looks okay but not sure about IP check

}
for (String usedIp : usedIps) {
// only half witted support for ipv6 here :(
if (usedIp.equals(ipAddresses.getIp4Address()) || usedIp.equals(ipAddresses.getIp6Address())) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Will this allow passing any random IP outside of the network?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

good point, maybe add a cidr check as well. The issue was that the network did not have a cidr though. Not sure how to handle that except for just accepting the ip.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@DaanHoogland then would it make sense to just fix/merge the root disk size issue and address the specific case later?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

yes, but can you check again? I can split in two PRs if it still needs work

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

splitting out as dragons be there

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8273

DaanHoogland changed the title import unmanaged instance issues adressed import smaller root disk sizes Jan 10, 2024

nvazquez left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Code LGTM

Copy link
Copy Markdown
Contributor

@blueorangutan package

Copy link
Copy Markdown

@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Copy link
Copy Markdown
Contributor

@blueorangutan package

Copy link
Copy Markdown

@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

sureshanaparti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

code lgtm

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8284

DaanHoogland marked this pull request as ready for review January 11, 2024 08:04

Copy link
Copy Markdown
Contributor Author

@blueorangutan package

Copy link
Copy Markdown

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8285

Copy link
Copy Markdown
Contributor

Tested this. Import works fine but volume size is returned as 0 for imported VM with smaller size volumes.

listUnmanagedInstances output,

{
    "listunmanagedinstancesresponse": {
        "count": 1,
        "unmanagedinstance": [
            {
                "name": "i-2-36-VM",
                "clusterid": "4c948570-9ceb-465a-829a-88eb777ce13f",
                "clustername": "p1-c2",
                "hostid": "9d1e614a-b9b6-4b57-8df5-8dd49a2501f3",
                "hostname": "ref-trl-5272-v-mr8-abhishek-kumar-kvm1",
                "powerstate": "PowerOn",
                "cpunumber": 1,
                "cpucorepersocket": -1,
                "cpuspeed": 250,
                "memory": 512,
                "disk": [
                    {
                        "id": "0",
                        "label": "hda",
                        "capacity": 52428800,
                        "imagepath": "/mnt/ef4f8bb2-c104-30cc-a3a2-087548c11d6f/3447c0bb-1961-4bb0-bcb3-2e06c7d35c9b",
                        "controller": "ide",
                        "position": 0,
                        "datastorename": "/acs/primary/ref-trl-5272-v-Mr8-abhishek-kumar/ref-trl-5272-v-Mr8-abhishek-kumar-kvm-pri1",
                        "datastorehost": "10.1.32.4",
                        "datastorepath": "/acs/primary/ref-trl-5272-v-Mr8-abhishek-kumar/ref-trl-5272-v-Mr8-abhishek-kumar-kvm-pri1",
                        "datastoretype": "file"
                    }
                ],
                "nic": [
                    {
                        "id": "0",
                        "networkname": "vnet0",
                        "macaddress": "02:01:00:cd:00:05",
                        "vlanid": -1,
                        "adaptertype": "e1000"
                    }
                ]
            }
        ]
    }
}

Import called with params,

name=i-2-36-VM&clusterid=4c948570-9ceb-465a-829a-88eb777ce13f&displayname=test&zoneid=b4a732bb-e837-4682-bbb9-b845b04a17dd&importsource=unmanaged&hypervisor=kvm&host=&username=&password=&diskpath=&temppath=&serviceofferingid=cec984be-1c57-468b-b1b1-94ea3cef705e&templateid=9e2ebcc2-d325-456e-8cad-1e1485e32d6a&nicnetworklist[0].nic=0&nicnetworklist[0].network=074dd540-2414-4ce2-9a51-f1888b8839c4

Template used in Macchinina

ListVM output for the imported VM,

{
    "listvirtualmachinesmetricsresponse": {
        "count": 1,
        "virtualmachine": [
            {
                "cputotal": "0.5 Ghz",
                "memorytotal": "0.50 GiB",
                "networkread": "0.01 MiB",
                "networkwrite": "0.00 MiB",
                "diskread": "0.00 MiB",
                "diskwrite": "0.00 MiB",
                "diskiopstotal": 0,
                "id": "62f777a5-9dbb-472f-b70b-a2635bbaa2f0",
                "name": "test",
                "displayname": "test",
                "account": "admin",
                "userid": "c77dbaa7-af98-11ee-847d-1e0058000a5d",
                "username": "admin",
                "domainid": "a2138f1e-af98-11ee-847d-1e0058000a5d",
                "domain": "ROOT",
                "created": "2024-01-11T11:43:01+0000",
                "state": "Running",
                "haenable": false,
                "zoneid": "b4a732bb-e837-4682-bbb9-b845b04a17dd",
                "zonename": "ref-trl-5272-v-Mr8-abhishek-kumar",
                "hostid": "9d1e614a-b9b6-4b57-8df5-8dd49a2501f3",
                "hostname": "ref-trl-5272-v-mr8-abhishek-kumar-kvm1",
                "hostcontrolstate": "Enabled",
                "templateid": "9e2ebcc2-d325-456e-8cad-1e1485e32d6a",
                "templatename": "macc-kvm",
                "templatetype": "USER",
                "templatedisplaytext": "macc-kvm",
                "passwordenabled": false,
                "serviceofferingid": "cec984be-1c57-468b-b1b1-94ea3cef705e",
                "serviceofferingname": "Small Instance",
                "cpunumber": 1,
                "cpuspeed": 500,
                "memory": 512,
                "cpuused": "3.17%",
                "networkkbsread": 7,
                "networkkbswrite": 0,
                "diskkbsread": 0,
                "diskkbswrite": 0,
                "memorykbs": 524288,
                "memoryintfreekbs": -1,
                "memorytargetkbs": 524288,
                "diskioread": 0,
                "diskiowrite": 0,
                "guestosid": "a23b8c54-af98-11ee-847d-1e0058000a5d",
                "rootdeviceid": 0,
                "rootdevicetype": "ROOT",
                "securitygroup": [],
                "nic": [
                    {
                        "id": "947586e0-5c81-4a1f-980c-60ba6f0ae4ff",
                        "networkid": "074dd540-2414-4ce2-9a51-f1888b8839c4",
                        "networkname": "testL2",
                        "isolationuri": "vlan://1760",
                        "broadcasturi": "vlan://1760",
                        "traffictype": "Guest",
                        "type": "L2",
                        "isdefault": true,
                        "macaddress": "02:01:00:cd:00:05",
                        "secondaryip": [],
                        "extradhcpoption": [],
                        "deviceid": "0"
                    }
                ],
                "hypervisor": "KVM",
                "instancename": "i-2-36-VM",
                "details": {
                    "deployvm": "true",
                    "kvm.vnc.password": "lqU9pMsfaaaaaaaaaaaaaa",
                    "rootdisksize": "1",
                    "nicAdapter": "e1000",
                    "rootDiskController": "ide"
                },
                "affinitygroup": [],
                "displayvm": true,
                "isdynamicallyscalable": false,
                "ostypeid": "a23b8c54-af98-11ee-847d-1e0058000a5d",
                "osdisplayname": "Other Linux (64-bit)",
                "pooltype": "NetworkFilesystem",
                "receivedbytes": 0,
                "sentbytes": 0,
                "tags": [],
                "hasannotations": false
            }
        ]
    }
}

List volumes output for the VM volume,

{
    "listvolumesresponse": {
        "count": 1,
        "volume": [
            {
                "sizegb": "0.00 GiB",
                "id": "c888227f-6676-487c-9625-b09e199abdb3",
                "name": "ROOT-39",
                "zoneid": "b4a732bb-e837-4682-bbb9-b845b04a17dd",
                "zonename": "ref-trl-5272-v-Mr8-abhishek-kumar",
                "type": "ROOT",
                "deviceid": 0,
                "virtualmachineid": "62f777a5-9dbb-472f-b70b-a2635bbaa2f0",
                "templateid": "9e2ebcc2-d325-456e-8cad-1e1485e32d6a",
                "templatename": "macc-kvm",
                "templatedisplaytext": "macc-kvm",
                "vmname": "test",
                "vmdisplayname": "test",
                "vmstate": "Running",
                "vmtype": "User",
                "provisioningtype": "thin",
                "size": 0,
                "created": "2024-01-11T11:43:01+0000",
                "state": "Ready",
                "account": "admin",
                "domainid": "a2138f1e-af98-11ee-847d-1e0058000a5d",
                "domain": "ROOT",
                "storagetype": "Shared (ROOT)",
                "hypervisor": "KVM",
                "storage": "ref-trl-5272-v-Mr8-abhishek-kumar-kvm-pri1",
                "attached": "2024-01-11T11:43:01+0000",
                "serviceofferingid": "1",
                "serviceofferingname": "Small Instance",
                "serviceofferingdisplaytext": "Small Instance",
                "isextractable": false,
                "displayvolume": true,
                "path": "/mnt/ef4f8bb2-c104-30cc-a3a2-087548c11d6f/3447c0bb-1961-4bb0-bcb3-2e06c7d35c9b",
                "storageid": "ef4f8bb2-c104-30cc-a3a2-087548c11d6f",
                "quiescevm": false,
                "supportsstoragesnapshot": false,
                "physicalsize": 1052672,
                "virtualsize": 52428800,
                "utilization": "2.0%",
                "clusterid": "4c948570-9ceb-465a-829a-88eb777ce13f",
                "clustername": "p1-c2",
                "podid": "639ef97b-fa03-4de1-8d55-c262234fc033",
                "podname": "Pod1",
                "tags": [],
                "hasannotations": false
            }
        ]
    }
}

DaanHoogland commented Jan 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

ok, @shwstppr so there is another place where rounding is done. Should this block this PR or an RC? Or should we create a left-over ticket and move on for now? (cc @nvazquez )

ad. I have found a second spot where the rounding error occurred and am retesting

DaanHoogland force-pushed the furtherUnmanagedFixes branch from f045990 to 1a37043 Compare January 11, 2024 13:08

Copy link
Copy Markdown

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8290

Copy link
Copy Markdown
Contributor Author

ok, @shwstppr so there is another place where rounding is done. Should this block this PR or an RC? Or should we create a left-over ticket and move on for now? (cc @nvazquez )

ad. I have found a second spot where the rounding error occurred and am retesting

this is now solved is a way, but the size is now rounded to 1Gb. this seems to be a limitation of the feature. I am not sure if there is something to be done about that (@nvazquez ?)

Copy link
Copy Markdown
Contributor

@DaanHoogland I tested on 4.18.1 and the problem from #8488 (comment) exists there as well. Should we merge without the last change?
I'm testing #8500 it should probably fix the issue. In code we are currently unnecessarily doing conversion twice (probably I added that 😶‍🌫️ )

Copy link
Copy Markdown
Contributor Author

@DaanHoogland I tested on 4.18.1 and the problem from #8488 (comment) exists there as well. Should we merge without the last change? I'm testing #8500 it should probably fix the issue. In code we are currently unnecessarily doing conversion twice (probably I added that 😶‍🌫️ )

ok, by the look of c01e162 we can abandon this PR.

Copy link
Copy Markdown
Contributor Author

this is incorporated in #8500 as far as needed

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

5 participants


Back | FazBrowse Home | New Git URL