Allow multiple nodes have templates (use local storage) #467
Conversation
Atoms
requested review from
65278,
avorima,
mcbenjemaa,
pborn-ionos and
wikkyk
as code owners
TestsPlease note that running unit and e2e tests requires manual approval from a team member. e2e testsWe use labels to control which e2e tests contexts are run:
ℹ️ Ask a team member to add the requested labels if you don't have enough permissions. |
|
did more intensive testing, not all cases now work, so will be making some changes there. |
|
fully reworked logic is now pushed, also updated description. |
There was a problem hiding this comment.
Please make better use of validation rules instead of the overriding logic or erroring out. Early failure is better UX.
Make sure the examples are still correct.
This feature deserves documentation :)
This is close. I like the overall idea and implementation but it can be simplified. Please avoid just reformatting unrelated files as that makes the PR larger and thus harder to review plus it can mess up rebasing/reverting/... .
|
rebased on main |
|
any comments? |
|
Hello guys, anything I can help with here? For now, I'll at least try to make a build and test this. I'm quite looking forward to this feature. Thanks a lot @Atoms. |
|
Looking forward to it too and happy to test. |
| warnings = append(warnings, "localStorage is mutually exclusive with templateID/sourceNode") | ||
| return warnings, apierrors.NewBadRequest("localStorage is mutually exclusive with templateID/sourceNode") |
There was a problem hiding this comment.
Since you can't see the sonarqube report (sorry about that): it's complaining about this string constant being repeated 5 times. Personally, I don't really see the point in setting warnings here at all only to return the same warning again as an error. This also applies to ValidateUpdate below. validateLocalStorage should return an error that can be returned by ValidateCreate and ValidateUpdate directly.
| warnings = append(warnings, "localStorage is mutually exclusive with templateID/sourceNode") | ||
| return warnings, apierrors.NewBadRequest("localStorage is mutually exclusive with templateID/sourceNode") |
| func (c *APIClient) FindVMTemplateByTags(ctx context.Context, templateTags []string) (string, int32, error) { | ||
| vmTemplates := make([]*proxmox.ClusterResource, 0) | ||
| // FindVMTemplatesByTags finds VM templates by tags across the whole cluster. | ||
| func (c *APIClient) FindVMTemplatesByTags(ctx context.Context, templateTags []string, allowedNodes []string, localStorage bool) (map[string]int32, error) { |
There was a problem hiding this comment.
Again, since you can't see the sonarqube report: It finds this function too complex ("Refactor this method to reduce its Cognitive Complexity from 21 to the 15 allowed"). While indeed there are many branches here, they're all fairly straightforward and there isn't really a better place to put them.
| func (c *APIClient) FindVMTemplatesByTags(ctx context.Context, templateTags []string, allowedNodes []string, localStorage bool) (map[string]int32, error) { | ||
| templates := make(map[string]int32) | ||
|
|
||
| // if for some reason there is not tags, we fail early and return error |
There was a problem hiding this comment.
| // if for some reason there is not tags, we fail early and return error | |
| // if for some reason there are no tags, fail early and return error |
| vmTags := strings.Split(vm.Tags, ";") | ||
| slices.Sort(vmTags) | ||
|
|
||
| // if localstorage - template should be on all allowed nodes |
There was a problem hiding this comment.
| // if localstorage - template should be on all allowed nodes | |
| // if localstorage, the template should be on all allowed nodes |
|
We are focusing on releasing v0.8/v1alpha2 and aren't planning to add any new features to the v0.7 branch, so I think |
wikkyk
force-pushed
the
multiple-node-templates
branch
from
fac81f9 to
e393e18
Compare
|
|
Should i rebase on v1alpha2 branch then ? |
|
No, that branch is not ready yet. We'll rebase this PR once it's ready. Please hold on tight :) |
This adds supoprt for PR ionos-cloud/cluster-api-provider-proxmox#467
|
Hi guys, any news for this PR? This is most useful feature for homelab usage |
5 participants
Issue #, if available:
fixes #451
Adjusted order of how scheduling happens
If we have usable template, determine how we need to schedule
Targetis provided schedule on that node if template is thereTargetandallowedNodesmissing assume we want all nodes to use in scheduling (discover all nodes)allowedNodesschedule only onallowedNodesallowedNodesstill continue to schedule on nodes where template exists.LocalStorage:
options.Targetas we will use template from same node.Fixed existing tests to reflect multiple scenarios (moving out
templateIDandSourceNodefromsetupReconcilerTestcreateVMhad became too complex (gocyclo) as i've written in code for now it should be function to revisit and get into smaller chunks.