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(vm): remove disk attachement capacity available #633

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

Conversation

eofff
Copy link
Contributor

@eofff eofff commented Jan 15, 2025

Description

Remove DiskAttachementCapacityAvailable condition, move it's logic to BlockDevicesReady new reason.

Why do we need it, and what problem does it solve?

What is the expected result?

Checklist

  • The code is covered by unit tests.
  • e2e tests passed.
  • Documentation updated according to the changes.
  • Changes were tested in the Kubernetes cluster manually.

@eofff eofff requested review from diafour and LopatinDmitr January 15, 2025 12:23
@eofff eofff self-assigned this Jan 15, 2025
@eofff eofff force-pushed the feat/vm/remove-disk-attachement-capacity-available-condition branch from a54e04d to 08f2c5b Compare January 15, 2025 12:27
Valeriy Khorunzhin added 2 commits January 15, 2025 15:39
Signed-off-by: Valeriy Khorunzhin <[email protected]>
Signed-off-by: Valeriy Khorunzhin <[email protected]>
@eofff eofff force-pushed the feat/vm/remove-disk-attachement-capacity-available-condition branch from 08f2c5b to 6d1ee4b Compare January 15, 2025 12:40
@@ -95,6 +97,20 @@ func (h *BlockDeviceHandler) Handle(ctx context.Context, s state.VirtualMachineS
return reconcile.Result{}, fmt.Errorf("unable to add block devices finalizers: %w", err)
}

// Get number of connected block devices
// If it greater limit then set condition to false
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is greater

cb.
Status(metav1.ConditionFalse).
Reason(vmcondition.ReasonBlockDeviceCapacityReached).
Message(fmt.Sprintf("Can not attach %d block devices (%d is maximum) to `VirtualMachine` %q", blockDeviceAttachedCount, common.VmBlockDeviceAttachedLimit, changed.Name))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to return here?

@@ -102,8 +101,7 @@ const (
ReasonVirtualMachineClassTerminating Reason = "VirtualMachineClassTerminating"
ReasonVirtualMachineClassNotExists Reason = "VirtalMachineClassNotExists"

ReasonBlockDeviceCapacityAvailable Reason = "BlockDeviceCapacityAvailable"
ReasonBlockDeviceCapacityReached Reason = "BlockDeviceCapacityReached"
ReasonBlockDeviceCapacityReached Reason = "BlockDeviceCapacityReached"
Copy link
Contributor

Choose a reason for hiding this comment

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

May be BlockDeviceLimitExceeded ?

Copy link
Contributor

@Isteb4k Isteb4k Jan 22, 2025

Choose a reason for hiding this comment

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

Describe what this reason means, when it occurs. It's important to add docs for new reasons/types here.

@@ -57,6 +58,7 @@ func NewBlockDeviceHandler(cl client.Client, recorder eventrecord.EventRecorderL
type BlockDeviceHandler struct {
client client.Client
recorder eventrecord.EventRecorderLogger
service IBlockDeviceService
Copy link
Contributor

Choose a reason for hiding this comment

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

What kind of service?
Need to rename


type EventRecorder = record.EventRecorder

type IBlockDeviceService interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just BlockDeviceService ?

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.

2 participants