Skip to content

engine/schema: fix CPU cap limitation for upgraded environment - #7808

Merged
yadvr merged 1 commit into
apache:4.18from
shapeblue:systemvm-cpucap-fix
Aug 7, 2023
Merged

yadvr merged 1 commit into
apache:4.18from
shapeblue:systemvm-cpucap-fix

Conversation

@yadvr

@yadvr yadvr commented Aug 3, 2023

Copy link
Copy Markdown
Member

CPU cap limitation was enabled as part of
#6420 that changes behaviour for existing environments.

The CPU cap limitation on KVM causes systemvms to not start or be really slow in nested and virtualised environments. We shouldn't change the defaults, but this could be documented or an operator can create new system offerings and use them.

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

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

In KVM env, post upgrade SSVM operations were slow and a couple of times SSVM didn't start. By setting the value to false, no cpu share/limitation was put on new SSVMs and they could start and work. On git history check, found this to be a recent change introduced between 4.17->4.18 via #6420. This should be reverted.

CPU cap limitation was enabled as part of
apache#6420 that changes behaviour
for existing environments. The CPU cap limitation on KVM causes
systemvms to not start or be really slow in nested and virtualised
environments.

Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
@yadvr

yadvr commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

@blueorangutan package

@blueorangutan

Copy link
Copy Markdown

@rohityadavcloud 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.

@codecov

codecov Bot commented Aug 3, 2023

Copy link
Copy Markdown

Codecov Report

Merging #7808 (12a0264) into 4.18 (f9b7bcf) will increase coverage by 0.00%.
Report is 14 commits behind head on 4.18.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               4.18    #7808   +/-   ##
=========================================
  Coverage     13.02%   13.02%           
- Complexity     9029     9031    +2     
=========================================
  Files          2720     2720           
  Lines        256998   257010   +12     
  Branches      40081    40083    +2     
=========================================
+ Hits          33462    33472   +10     
  Misses       219336   219336           
- Partials       4200     4202    +2     

see 3 files with indirect coverage changes

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

@blueorangutan

Copy link
Copy Markdown

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

@harikrishna-patnala harikrishna-patnala left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code LGTM.

agree with the point, one can create new system offerings and use them.

@yadvr

yadvr commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

@blueorangutan test

@blueorangutan

Copy link
Copy Markdown

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

@blueorangutan

Copy link
Copy Markdown

[SF] Trillian Build Failed (tid-7256)

@DaanHoogland DaanHoogland 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.

code looks good, @JoaoJandre please comment on this.

@yadvr

yadvr commented Aug 4, 2023

Copy link
Copy Markdown
Member Author

@blueorangutan test alma8 kvm-alma8

@blueorangutan

Copy link
Copy Markdown

@rohityadavcloud a [SF] Trillian-Jenkins test job (alma8 mgmt + kvm-alma8) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

[SF] Trillian test result (tid-7272)
Environment: kvm-alma8 (x2), Advanced Networking with Mgmt server a8
Total time taken: 40325 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7808-t7272-kvm-alma8.zip
Smoke tests completed. 99 look OK, 9 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File
test_01_add_delete_kubernetes_supported_version Error 0.14 test_kubernetes_supported_versions.py
ContextSuite context=TestResetVmOnReboot>:setup Error 0.00 test_reset_vm_on_reboot.py
ContextSuite context=TestListIdsParams>:setup Error 0.00 test_list_ids_parameter.py
ContextSuite context=TestCpuCapServiceOfferings>:setup Error 0.00 test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup Error 0.26 test_service_offerings.py
ContextSuite context=TestIsolatedNetworks>:setup Error 0.00 test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup Error 0.00 test_routers_network_ops.py
ContextSuite context=TestLoadBalance>:setup Error 0.00 test_loadbalance.py
ContextSuite context=TestVMWareStoragePolicies>:setup Error 0.00 test_storage_policy.py
ContextSuite context=TestMetrics>:setup Error 0.00 test_metrics_api.py
ContextSuite context=TestRouterIpTablesPolicies>:setup Error 0.00 test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup Error 0.00 test_routers_iptables_default_policy.py

@yadvr

yadvr commented Aug 6, 2023

Copy link
Copy Markdown
Member Author

@blueorangutan test alma8 kvm-alma8

@blueorangutan

Copy link
Copy Markdown

@rohityadavcloud a [SF] Trillian-Jenkins test job (alma8 mgmt + kvm-alma8) has been kicked to run smoke tests

@blueorangutan

Copy link
Copy Markdown

[SF] Trillian test result (tid-7295)
Environment: kvm-alma8 (x2), Advanced Networking with Mgmt server a8
Total time taken: 48997 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7808-t7295-kvm-alma8.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

@yadvr
yadvr merged commit 4f85c55 into apache:4.18 Aug 7, 2023
@yadvr
yadvr deleted the systemvm-cpucap-fix branch August 7, 2023 04:49
@JoaoJandre

Copy link
Copy Markdown
Contributor

Sorry for the delayed response @dahn, I somehow missed the notification.

I am against reverting #6420, as was explained there, keeping system VMs without CPU usage limits opens up space for different types of infrastructure attacks/misuses. Also, bugs like the ones fixed on #7826 and #6970 will affect much more the Cloud without a CPU cap on CPVMs, for example.

The problem reported by @rohityadavcloud, where system VMs fail to start, is caused by another situation. When a system VM is provisioned, ACS sends commands to the system VM to generate the CSR and subsequently generate the certificate; these commands have a timeout of 60 seconds (hardcoded). When the CPU cap is enabled for the system VMs, the request and certificate generation procedures can take more than 60 seconds each to complete, causing a timeout in the execution of the commands and the non-provisioning of certificates for the system VMs. Thus, generating a handshake failure when the ca.plugin.root.auth.strictness setting is true. This config should be externalized.

Also, I agree with @harikrishna-patnala, if the system VMs are too slow, you could just create new system offerings and use them. But the default offerings should protect users from attacks and possible bugs or misuses like the ones mentioned.

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