Skip to content
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.

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

Fix memory setting issue when starting domain #1818

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tsuchiclay
Copy link

By passing a flag to libvirt's API to apply the memory size change to the domain's configuration file, the following issue was solved.

The problem was caused by passing only memory size to #memory= method in start_domain.rb. Specifying a flag in addition to the memory size causes libvirt's API, virDomainSetMemoryFlags, to be called with that flag, explicitly choosing whether to apply the size setting to the running domain or the config file for that domain. The missing flag was causing the above issue by attempting to change the memory size of a running domain that did not exist.

Therefore, a list of possible flags is introduced, and a flag that make changes to the domain's configuration file is selected and used.

Below are links to some related documents.

By passing a flag to libvirt's API to apply the memory size change
to the domain's configuration file, the following issue was solved.

 -  fail to update the domain.memory on existing running vm 'Call
    to virDomainSetMemory failed' vagrant-libvirt#1812
    vagrant-libvirt#1812

The problem was caused by passing only memory size to #memory= method
in start_domain.rb. Specifying a flag in addition to the memory size
causes libvirt's API, virDomainSetMemoryFlags, to be called with that
flag, explicitly choosing whether to apply the size setting to the
running domain or the config file for that domain. The missing flag was
causing the above issue by attempting to change the memory size of
a running domain that did not exist.

Therefore, a list of possible flags is introduced, and a flag that
make changes to the domain's configuration file is selected and used.

Below are links to some related documents.

 -  https://ruby.libvirt.org/api/Libvirt/Domain.html#method-i-memory-3D

 -  https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainSetMemoryFlags
Copy link
Contributor

@electrofelix electrofelix left a comment

Choose a reason for hiding this comment

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

Are you able to add an acceptance test that modified the memory and reloads? If you can add it in the first commit and then have this fix in the latter so that I can see the failure and then it passing it'll make it easy for me to just approve and merge without needing to wait until I've time to test it out myself.

It should be possible to copy

context 'with modified config' do
to add a reload test below it, just need to access the environment.workdir attribute for where to find the current run test Vagrantfile to modify before the reload call.

module ModImpactFlags
VIR_DOMAIN_AFFECT_CURRENT = 0 # affect current domain state
VIR_DOMAIN_AFFECT_LIVE = 1 # affect running domain state
VIR_DOMAIN_AFFECT_CONFIG = 2 # affect persistent domain state
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you keep these with the rest of domain_flags.rb

Copy link
Author

Choose a reason for hiding this comment

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

Yes, let's do that.

Copy link

github-actions bot commented Jun 2, 2024

Download the latest artifacts for this pull request here:

@coveralls
Copy link

Pull Request Test Coverage Report for Build 9043414875

Details

  • 0 of 1 (0.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 78.663%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/vagrant-libvirt/action/start_domain.rb 0 1 0.0%
Totals Coverage Status
Change from base Build 7342124073: 0.0%
Covered Lines: 3119
Relevant Lines: 3965

💛 - Coveralls

@tsuchiclay
Copy link
Author

tsuchiclay commented Jun 2, 2024

Are you able to add an acceptance test that modified the memory and reloads? If you can add it in the first commit and then have this fix in the latter so that I can see the failure and then it passing it'll make it easy for me to just approve and merge without needing to wait until I've time to test it out myself.

Thank you! I'm on vacation right now and will be able to work on it the day after tomorrow.
Anyway, I'll try to add it.
Would it be less messy to redo those as separate commits (completely different from the past ones already posted above)?
I mean re-create a new pull request and close this place.

Nvm. I'm an obvious newbie to git.

@electrofelix
Copy link
Contributor

As long as you're comfortable with rebase swapping the two commits it should be fine. There shouldn't be any of the same files touched by the two commits so it should be a clean swap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants