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

Prevent instance migration #1306

Closed
bensmrs opened this issue Oct 15, 2024 · 5 comments · Fixed by #1339
Closed

Prevent instance migration #1306

bensmrs opened this issue Oct 15, 2024 · 5 comments · Fixed by #1339
Assignees
Labels
Bug Confirmed to be a bug
Milestone

Comments

@bensmrs
Copy link
Contributor

bensmrs commented Oct 15, 2024

Instance placement scriptlets offer interesting ways to control how instances are placed on cluster nodes. While creation, relocation (due to a node being down), and evacuation are handled, there is to the best of my knowledge no way to block a migration to an unwanted cluster node. So I have two questions:

  • How to lock an instance to a specific node in a cluster, without creating a dedicated project attached to a dedicated group?
  • Could the instance placement scriptlet be modified so that it includes an optional parameter (or request property) target containing the requested migration target, allowing us to call fail depending on different decision factors? (I can work on this one)
@stgraber
Copy link
Member

stgraber commented Oct 18, 2024

How to lock an instance to a specific node in a cluster, without creating a dedicated project attached to a dedicated group?

Easiest way to do this would be to use --target during creation and then set cluster.evacuate to stop so it never gets migrated away during evacuation or recovery.

Could the instance placement scriptlet be modified so that it includes an optional parameter (or request property) target containing the requested migration target, allowing us to call fail depending on different decision factors? (I can work on this one)

I think that starting with Incus 6.6 we call the scriptlet even with --target=NAME so that should let you perform your check there.

As for failing, you should be able to either call the fail function or do return "Placement rejected" which will also be treated as an error.

@stgraber stgraber added the Incomplete Waiting on more information from reporter label Oct 18, 2024
@stgraber
Copy link
Member

stgraber commented Oct 18, 2024

Marking as incomplete as we may not have anything to add to Incus for that.

@bensmrs
Copy link
Contributor Author

bensmrs commented Oct 25, 2024

Ooooh, I didn’t actually see that the candidate_members was actually populated with the --target! Maybe the documentation could be improved, as the behavior is not suggested.
However, set_target doesn’t work as I’d expect:

  • incus move instance5 --target node3 --project test
  • The scriptlet gets called, with node3 as its candidate_members singleton (first part of my answer; which I didn’t know happened)
  • set_target(node5) does nothing; no error, no warning, and the instance is still moved to node3.

I get that set_target might not get called in this scenario, but maybe an error could help admins…

@stgraber
Copy link
Member

Hmm, yeah, we should have set_target fail if it's passed a server that's not within the candidate_members.

@bensmrs
Copy link
Contributor Author

bensmrs commented Oct 28, 2024

So, after reading the code, it appears that a wrong target does actually trigger a warning, and returns a string instead of None, so we can “catch” errors. It depends on how defensive we want to be. Python isn’t really in favor of defensive programming, but it has catching mechanisms that aren’t implemented in Starlark, so raising an error here would be fatal to the scriptlet. On the other hand, not raising one would fail silently, which may lead to safety issues, given the nature of the project. I’m opening a (very short) PR, but both arguments look valid to me, so I’ll let you be the judge.

@stgraber stgraber added Bug Confirmed to be a bug and removed Incomplete Waiting on more information from reporter labels Oct 28, 2024
@stgraber stgraber added this to the incus-6.7 milestone Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed to be a bug
Development

Successfully merging a pull request may close this issue.

2 participants