Skip to content

Conversation

@SadiJr
Copy link
Contributor

@SadiJr SadiJr commented Feb 15, 2023

Description

When deploying a new VR, using a network offering with CPU cap, this limitation is not respected in the VR, because in the definition and persistence of VR in database, the configuration of CPU cap of the network offering is not considered, and always uses the value false. This PR aims to fix this behavior, to respect the CPU cap of the network offering.

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

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

It was tested in a local lab:

  1. I created new isolated networks, and also created new VMs that use they;
  2. I executed an dumpxml in the VR definition;
  3. Before, the XML of the VR does not have the cpushared tag;
  4. Now, the definition of the VR have the cpushares tag, when using a network offering with CPU cap.

@codecov
Copy link

codecov bot commented Feb 15, 2023

Codecov Report

Merging #7234 (c69124b) into main (2f309b5) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##               main    #7234      +/-   ##
============================================
- Coverage     12.95%   12.95%   -0.01%     
  Complexity     8986     8986              
============================================
  Files          2728     2728              
  Lines        256647   256648       +1     
  Branches      40024    40024              
============================================
  Hits          33256    33256              
- Misses       219214   219215       +1     
  Partials       4177     4177              
Impacted Files Coverage Δ
...va/com/cloud/network/router/NetworkHelperImpl.java 3.81% <0.00%> (-0.01%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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.

CLGTM

@DaanHoogland
Copy link
Contributor

@ezntt please comment on why you approve. Did you test/review and if test, how?

@ezntt
Copy link
Contributor

ezntt commented Feb 24, 2023

Sure, @DaanHoogland, I approve the feature based on my testing, which was carried out using a similar approach to the PR owner.

I tested the change by creating a VR and verifying its dumpxml to confirm that cpushares tag was set correctly. Specifically, I confirmed that the tag:

<cputune>
    <shares>500</shares> 
</cputune>

were properly defined, which confirmed that the change was successful.

Copy link
Member

@weizhouapache weizhouapache left a comment

Choose a reason for hiding this comment

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

code lgtm

@DaanHoogland
Copy link
Contributor

Sure, @DaanHoogland, I approve the feature based on my testing, which was carried out using a similar approach to the PR owner.

I tested the change by creating a VR and verifying its dumpxml to confirm that cpushares tag was set correctly. Specifically, I confirmed that the tag:

<cputune>
    <shares>500</shares> 
</cputune>

were properly defined, which confirmed that the change was successful.

Great work @ezntt , we have a rule to have at least one lgtm based on code review and one lgtm based on 3rd party testing. Hence my question.

@DaanHoogland DaanHoogland added this to the 4.18.1.0 milestone Feb 27, 2023
Copy link
Contributor

@stephankruggg stephankruggg left a comment

Choose a reason for hiding this comment

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

CLGTM, not manually tested

@kiranchavala
Copy link
Contributor

kiranchavala commented Mar 8, 2023

@SadiJr @DaanHoogland

I have performed a manual test on 4.17.2 release and I observe that cpu cap is honoured

Let me know if any step is missing in reproducing the issue ?

Steps that I have followed

  1. Create router with the default system offering

Default-systemoffering

Login to the kvm host where the router is running and execute

virsh dumpxml <>

 <cputune>
    <shares>250</shares>
  </cputune>
  1. Create a system offering with cpu-cap enabled

systemoffering-test-cpu-cap

  1. Change the global setting value of "router.service.offering" to the uuid of system offering with cpu-cap enabled

  2. Create a router with the new system offering for router with cpu-cap

Observer the cpushare tag is present

virsh dumpxml <>

  <cputune>
    <shares>256</shares>
  </cputune>

@DaanHoogland
Copy link
Contributor

@blueorangutan package

@blueorangutan
Copy link

@DaanHoogland a [SF] 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.

@blueorangutan
Copy link

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

@DaanHoogland DaanHoogland merged commit 43a5d62 into apache:main May 23, 2023
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.

8 participants