feat(module): add ability to edit or remove generic vmclass #1597
+1,238
−136
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideImplement stateful management of the generic VirtualMachineClass by tracking its creation in a module-state Secret, updating the create hook to respect user deletions, adding a state update hook, and introducing a Helm label cleanup hook while removing legacy static defaults. Sequence diagram for generic VirtualMachineClass creation and state trackingsequenceDiagram
participant Hook_create_generic_vmclass as Hook: create-generic-vmclass
participant K8s_API as K8s API
participant Secret_module_state as Secret: module-state
participant Hook_update_module_state as Hook: update-module-state
participant User
User->>K8s_API: Delete generic VirtualMachineClass
Hook_create_generic_vmclass->>K8s_API: Check if generic VMClass exists
alt VMClass does not exist
Hook_create_generic_vmclass->>Secret_module_state: Check generic-vmclass-created flag
alt generic-vmclass-created == true
Hook_create_generic_vmclass-->>K8s_API: Do nothing
else generic-vmclass-created != true
Hook_create_generic_vmclass->>K8s_API: Create generic VMClass
Hook_update_module_state->>Secret_module_state: Set generic-vmclass-created=true
end
else VMClass exists
Hook_create_generic_vmclass-->>K8s_API: Do nothing
end
Hook_update_module_state->>Secret_module_state: Update generic-vmclass-created flag if VMClass exists
Sequence diagram for Helm label cleanup on generic VMClasssequenceDiagram
participant Hook_drop_helm_labels_from_generic_vmclass as Hook: drop-helm-labels-from-generic-vmclass
participant K8s_API as K8s API
Hook_drop_helm_labels_from_generic_vmclass->>K8s_API: Get generic VMClass metadata
alt VMClass has Helm labels
Hook_drop_helm_labels_from_generic_vmclass->>K8s_API: Remove Helm labels from generic VMClass
else VMClass does not have Helm labels
Hook_drop_helm_labels_from_generic_vmclass-->>K8s_API: Do nothing
end
Entity relationship diagram for module-state Secret and VirtualMachineClasserDiagram
MODULE_STATE_SECRET {
string generic-vmclass-created
}
VIRTUAL_MACHINE_CLASS {
string name
map labels
CPU cpu
SizingPolicy[] sizingPolicies
}
MODULE_STATE_SECRET ||--o| VIRTUAL_MACHINE_CLASS : tracks creation
Class diagram for ModuleState and VMClassMetadata typesclassDiagram
class ModuleState {
+bool GenericVMClassCreated
+map<string, byte[]> ToSecretData()
+map<string, any> ToPatchData()
}
class VMClassMetadata {
+string Name
+map<string, string> Labels
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Member
Author
|
@sourcery-ai review |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In create-generic-vmclass/hook.go the constant createGenericVMClassHookName is both typo’d (“steate”) and unused—either correct it with a meaningful name or remove it to avoid confusion.
- The default VMClass spec (CPU model and sizing policies) is hard-coded in Reconcile; consider extracting these defaults into shared constants or config to simplify future adjustments.
- Base64 encode/decode logic is duplicated across both hooks—extract a small utility function to centralize this and reduce repetition.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In create-generic-vmclass/hook.go the constant createGenericVMClassHookName is both typo’d (“steate”) and unused—either correct it with a meaningful name or remove it to avoid confusion.
- The default VMClass spec (CPU model and sizing policies) is hard-coded in Reconcile; consider extracting these defaults into shared constants or config to simplify future adjustments.
- Base64 encode/decode logic is duplicated across both hooks—extract a small utility function to centralize this and reduce repetition.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fl64
force-pushed
the
chore/module/add-abbility-to-remove-vmclass
branch
from
e683936 to
dddb2c4
Compare
Member
Author
|
@sourcery-ai summary |
fl64
force-pushed
the
chore/module/add-abbility-to-remove-vmclass
branch
4 times, most recently
from
09a7345 to
7ac9050
Compare
fl64
force-pushed
the
chore/module/add-abbility-to-remove-vmclass
branch
4 times, most recently
from
d10961f to
28b219e
Compare
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
fl64
force-pushed
the
chore/module/add-abbility-to-remove-vmclass
branch
from
28b219e to
576270a
Compare
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
Signed-off-by: Pavel Tishkov <[email protected]>
fl64
changed the title
fl64
requested review from
Isteb4k,
nevermarine and
universal-itengineer
as code owners
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the update-module-state hook you always patch the secret even when the state hasn't changed; consider skipping the patch when newState equals currentState to reduce unnecessary API calls.
- The default VMClass spec is fully hardcoded in the create hook; extracting the sizing policies and CPU model into constants or making them configurable would improve maintainability.
- There's repeated base64 encoding/decoding and secret data handling logic across hooks; consider extracting these operations into a shared utility to avoid duplication.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the update-module-state hook you always patch the secret even when the state hasn't changed; consider skipping the patch when newState equals currentState to reduce unnecessary API calls.
- The default VMClass spec is fully hardcoded in the create hook; extracting the sizing policies and CPU model into constants or making them configurable would improve maintainability.
- There's repeated base64 encoding/decoding and secret data handling logic across hooks; consider extracting these operations into a shared utility to avoid duplication.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
diafour
reviewed
Oct 30, 2025
Comment on lines
39
to
40
| helmManagedByLabel = "app.kubernetes.io/managed-by" | ||
| helmHeritageLabel = "heritage" |
Member
There was a problem hiding this comment.
There are also 2 annotations to remove:
meta.helm.sh/release-name: <module name>
meta.helm.sh/release-namespace: <module namespace>
diafour
reviewed
Oct 30, 2025
Comment on lines
102
to
111
| if genericCreatedEncoded, exists := moduleStateData["generic-vmclass-created"]; exists { | ||
| if encodedStr, ok := genericCreatedEncoded.(string); ok { | ||
| if decodedBytes, err := base64.StdEncoding.DecodeString(encodedStr); err == nil { | ||
| genericCreated := string(decodedBytes) == "true" | ||
| if genericCreated { | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
There is a simpler solution: unmarshal into struct with []byte field, so encode/json package will automatically decode base64.
Or unmarshal into corev1.Secret.
JqFilter: `{"metadata": .metadata, "data": .data}`,
...
var moduleStateSecret corev1.Secret
if err := moduleStateSecrets[0].UnmarshalTo[&moduleStateSecret]; err != nil {
...
...
// Selected block of 10 lines will be 3 lines only:
if string(moduleStateSecret.Data["generic-vmclass-created"]) == "true" {
return nil
}
diafour
reviewed
Oct 30, 2025
| } | ||
| } | ||
|
|
||
| func Reconcile(_ context.Context, input *pkg.HookInput) error { |
Member
There was a problem hiding this comment.
It seems this hook can be combined with create-generic-vmclass. Same snapshots, same trigger.
Member
Author
There was a problem hiding this comment.
I think we will use this state in the future to preserve another state.
diafour
reviewed
Oct 30, 2025
Comment on lines
+123
to
+126
| Labels: map[string]string{ | ||
| "app": "virtualization-controller", | ||
| "module": settings.ModuleName, | ||
| }, |
Member
There was a problem hiding this comment.
May be add a label with some kind of "spec-version" for this vmclass?
Member
Author
There was a problem hiding this comment.
I don't understand, but why?
diafour
reviewed
Oct 30, 2025
| return nil | ||
| } | ||
|
|
||
| // if module-state secret exists and contains generic-vmclass-created=true, nothing to do |
Member
There was a problem hiding this comment.
Strange name for the flag. vmclass created, but there is no vmclass =) May be something like "vmclass-generic-was-initiated"?
Member
Author
There was a problem hiding this comment.
generic-vmclass-was-ever-created?)
diafour
reviewed
Nov 1, 2025
|
|
||
| var _ = registry.RegisterFunc(configDropHelmLabels, handlerDropHelmLabels) | ||
|
|
||
| var configDropHelmLabels = &pkg.HookConfig{ |
Member
There was a problem hiding this comment.
Suggested change
| var configDropHelmLabels = &pkg.HookConfig{ | |
| // This hook runs at start (ExecuteOnSynchronization: true) to drop helm labels before helm execution. | |
| var configDropHelmLabels = &pkg.HookConfig{ |
diafour
reviewed
Nov 1, 2025
| "module": settings.ModuleName, | ||
| }, | ||
| }, | ||
| ExecuteHookOnEvents: ptr.To(false), |
Member
There was a problem hiding this comment.
Suggested change
| ExecuteHookOnEvents: ptr.To(false), | |
| ExecuteHookOnEvents: ptr.To(false), | |
| ExecuteHookOnSynchronization: ptr.To(true), |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR adds the ability to safely edit or/and remove the generic VirtualMachineClass resource without it being automatically recreated by the virtualization module.
Why do we need it, and what problem does it solve?
Previously, if users deleted the generic VirtualMachineClass (either accidentally or intentionally), the virtualization module would automatically recreate it on the next restart or update. Also, it may stuck in Terminating phase.
What is the expected result?
Checklist
Changelog entries