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

feat(xo-server): implement rolling pool reboot #7242

Merged
merged 4 commits into from
Jan 25, 2024

Conversation

fbeauchamp
Copy link
Collaborator

Description

Short explanation of this PR (feel free to re-use commit message)

Checklist

  • Commit
    • Title follows commit conventions
    • Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
    • If bug fix, add Introduced by
  • Changelog
    • If visible by XOA users, add changelog entry
    • Update "Packages to release" in CHANGELOG.unreleased.md
  • PR
    • If UI changes, add screenshots
    • If not finished or not tested, open as Draft

@fbeauchamp fbeauchamp changed the title feat(xo-server): implement rolling pull reboot feat(xo-server): implement rolling pool reboot Dec 18, 2023
Copy link
Member

@MathieuRA MathieuRA left a comment

Choose a reason for hiding this comment

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

Since we are very close to the release, what do you think if for the rolling Pool Reboot we just copy/paste the necessary code, and think about how to mutualise the code right after the release?

packages/xo-server/src/xapi/mixins/patching.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/xapi/mixins/patching.mjs Outdated Show resolved Hide resolved
@fbeauchamp
Copy link
Collaborator Author

Since we are very close to the release, what do you think if for the rolling Pool Reboot we just copy/paste the necessary code, and think about how to mutualise the code right after the release?

if possible, I would like to try the refactoring, the rolling pool update method is already complex enough . If we're not confident on the code quality , we can still fallback to an additionnal flag on wednesday

@fbeauchamp fbeauchamp marked this pull request as ready for review December 19, 2023 08:59
@julien-f
Copy link
Member

I would love the logic to be mutualized with host_smartReboot as well.

@fbeauchamp fbeauchamp marked this pull request as draft December 19, 2023 09:12
@fbeauchamp fbeauchamp marked this pull request as ready for review December 19, 2023 13:06
@fbeauchamp
Copy link
Collaborator Author

I would love the logic to be mutualized with host_smartReboot as well.

how do you see things ? since the code logic seems quite different , I think I need more time to think this out. Each Vm will be handled exactly twice in smartreboot ( suspend, then resume), but an unlucky VM can be migrated a lot of times in case of rolling pull reboot . Also the error cases are different (allowed operation Vs local storage)

I think both feature should be blended : in case of rolling pool update/reboot , we could fall back to suspend for example

Copy link
Member

@MathieuRA MathieuRA left a comment

Choose a reason for hiding this comment

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

Not tested.

packages/xo-server/src/xo-mixins/pool.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/xapi/mixins/pool.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/xapi/mixins/patching.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/xapi/mixins/patching.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/xapi/mixins/patching.mjs Outdated Show resolved Hide resolved
packages/xo-server/src/api/pool.mjs Outdated Show resolved Hide resolved
@MathieuRA
Copy link
Member

Don't forget to rebase on master to get the "Rolling pool update" bug fix.

// host.$resident_VMs is outdated and returns resident VMs before the host.evacuate.
// this.getField is used in order not to get cached data.
const residentVmRefs = await this.getField('host', host.$ref, 'resident_VMs')

@fbeauchamp fbeauchamp force-pushed the feat_rolling_pull_reboot branch 2 times, most recently from 001e56f to da06a4f Compare December 21, 2023 10:32
@fbeauchamp
Copy link
Collaborator Author

test done : rolling reboot et rolling pull update works

@julien-f julien-f force-pushed the feat_rolling_pull_reboot branch 3 times, most recently from 28aa881 to 45844b7 Compare January 22, 2024 10:11
@julien-f julien-f force-pushed the feat_rolling_pull_reboot branch 2 times, most recently from 97ef0e8 to e0ce163 Compare January 24, 2024 08:57
fbeauchamp and others added 3 commits January 25, 2024 16:56
This makes it more clear that it is called only once and not one time per host or VM.
@julien-f julien-f merged commit d6abdb2 into master Jan 25, 2024
1 check passed
@julien-f julien-f deleted the feat_rolling_pull_reboot branch January 25, 2024 16:50
pdonias pushed a commit that referenced this pull request Jan 26, 2024
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