Skip to content

Allow multiple nodes have templates (use local storage) #467

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Atoms
Copy link

@Atoms Atoms commented Mar 25, 2025

Issue #, if available:
fixes #451

  • Adjusted order of how scheduling happens

    • First we determine if we have usable template (fail fast if not)
  • If we have usable template, determine how we need to schedule

    • if Target is provided schedule on that node if template is there
    • if Target and allowedNodes missing assume we want all nodes to use in scheduling (discover all nodes)
    • if templates are on more nodes then is in allowedNodes schedule only on allowedNodes
    • if templates missing on some allowedNodes still continue to schedule on nodes where template exists.

LocalStorage:

  • If localstorage (we have no easy way to determine if template is no localstorage):
    • multiple templates can be found
    • assign options.Node same as options.Target as we will use template from same node.
    • templateID and SourceNode cannot be used in this scenario
  • If !localstorage
    • only 1 template can be found, if multiple found fail
    • schedule vm on any allowedNode (either Target or allowedNodes or discovered nodes)
    • templateID and SourceNode only can be used in this scenario

Fixed existing tests to reflect multiple scenarios (moving out templateID and SourceNode from setupReconcilerTest

  • fixed yamlfmt

createVM had became too complex (gocyclo) as i've written in code for now it should be function to revisit and get into smaller chunks.

Copy link

Tests

Please note that running unit and e2e tests requires manual approval from a team member.

e2e tests

We use labels to control which e2e tests contexts are run:

Label Behaviour
none Run Generic tests only
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels Do not run any e2e tests
e2e/flatcar run Flatcar e2e tests Add Flatcar tests

ℹ️ Ask a team member to add the requested labels if you don't have enough permissions.

for k, v := range m {
return k, v
}
return "", 0 // Fallback (should not happen if len(m) == 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this should not happen then perhaps it ought to return an error instead of failing silently.

@Atoms
Copy link
Author

Atoms commented Mar 27, 2025

did more intensive testing, not all cases now work, so will be making some changes there.

@Atoms
Copy link
Author

Atoms commented Apr 14, 2025

fully reworked logic is now pushed, also updated description.

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.

Local storage templates per Proxmox Node
2 participants