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(vmclass): add events about available nodes and sizing policies changed #606

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions api/core/v1alpha2/events.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,6 @@ const (
// ReasonVMOPStarted is event reason that the operation is started
ReasonVMOPStarted = "VirtualMachineOperationStarted"

// ReasonVMClassInUse is event reason that VMClass is used by virtual machine.
ReasonVMClassInUse = "VirtualMachineClassInUse"

// ReasonVDStorageClassWasDeleted is event reason that VDStorageClass was deleted.
ReasonVDStorageClassWasDeleted = "VirtualDiskStorageClassWasDeleted"
// ReasonVDStorageClassNotFound is event reason that VDStorageClass not found.
Expand Down Expand Up @@ -95,4 +92,13 @@ const (

// ReasonDataSourceDiskProvisioningFailed is event reason that DataSource disk provisioning is failed.
ReasonDataSourceDiskProvisioningFailed = "DataSourceImportDiskProvisioningFailed"

// ReasonVMClassSizingPoliciesWasChanged is event reason that VMClass sizing policies was changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

were changed

ReasonVMClassSizingPoliciesWasChanged = "SizingPoliciesWasChanged"

// ReasonVMClassAvailableNodesListEmpty is event reason that VMClass has no available nodes.
ReasonVMClassAvailableNodesListEmpty = "AvailableNodesListEmpty"

// ReasonVMClassAvailableNodesWasUpdated is event reason that VMClass available nodes list was updated.
ReasonVMClassAvailableNodesWasUpdated = "AvailableNodesWasUpdated"
)
6 changes: 6 additions & 0 deletions api/core/v1alpha2/vmclasscondition/condition.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ func (t Type) String() string {
const (
TypeReady Type = "Ready"
TypeDiscovered Type = "Discovered"
TypeInUse Type = "InUse"
)

type Reason string
Expand All @@ -44,4 +45,9 @@ const (
ReasonDiscoverySucceeded Reason = "DiscoverySucceeded"
ReasonDiscoverySkip Reason = "DiscoverySkip"
ReasonDiscoveryFailed Reason = "DiscoveryFailed"

// ReasonVMClassInUse is event reason that VMClass is used by virtual machine.
ReasonVMClassInUse Reason = "VirtualMachineClassInUse"
// ReasonVMClassFree is event reason that VMClass not used by virtual machine.
ReasonVMClassFree Reason = "VirtualMachineClassFree"
)
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,23 @@ import (
"fmt"
"time"

corev1 "k8s.io/api/core/v1"
"k8s.io/client-go/tools/record"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/deckhouse/deckhouse/pkg/log"
"github.com/deckhouse/virtualization-controller/pkg/common/object"
"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
"github.com/deckhouse/virtualization-controller/pkg/controller/vmclass/internal/state"
"github.com/deckhouse/virtualization-controller/pkg/eventrecord"
virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmclasscondition"
)

const nameDeletionHandler = "DeletionHandler"

func NewDeletionHandler(client client.Client, recorder record.EventRecorder, logger *log.Logger) *DeletionHandler {
func NewDeletionHandler(client client.Client, recorder eventrecord.EventRecorderLogger, logger *log.Logger) *DeletionHandler {
return &DeletionHandler{
client: client,
recorder: recorder,
Expand All @@ -45,7 +47,7 @@ func NewDeletionHandler(client client.Client, recorder record.EventRecorder, log

type DeletionHandler struct {
client client.Client
recorder record.EventRecorder
recorder eventrecord.EventRecorderLogger
logger *log.Logger
}

Expand All @@ -58,16 +60,28 @@ func (h *DeletionHandler) Handle(ctx context.Context, s state.VirtualMachineClas
controllerutil.AddFinalizer(changed, virtv2.FinalizerVMCleanup)
return reconcile.Result{}, nil
}
cb := conditions.NewConditionBuilder(vmclasscondition.TypeInUse).Generation(changed.Generation)
defer func() { conditions.SetCondition(cb, &changed.Status.Conditions) }()
vms, err := s.VirtualMachines(ctx)
if err != nil {
return reconcile.Result{}, err
}
if len(vms) > 0 {
msg := fmt.Sprintf("VirtualMachineClass cannot be deleted, there are VMs that use it. %s...", object.NamespacedName(&vms[0]))
h.logger.Info(msg)
h.recorder.Event(changed, corev1.EventTypeWarning, virtv2.ReasonVMClassInUse, msg)
var vmNamespacedNames []string
for i := range vms {
vmNamespacedNames = append(vmNamespacedNames, object.NamespacedName(&vms[i]).String())
}
msg := fmt.Sprintf("VirtualMachineClass cannot be deleted, there are VMs that use it. %q", vmNamespacedNames)
cb.
Status(metav1.ConditionTrue).
Reason(vmclasscondition.ReasonVMClassInUse).
Message(msg)
return reconcile.Result{RequeueAfter: 60 * time.Second}, nil
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 subscribe to virtual machine deletion?

}
cb.
Status(metav1.ConditionFalse).
Reason(vmclasscondition.ReasonVMClassFree).
Message("")
Comment on lines +81 to +84
Copy link
Member

Choose a reason for hiding this comment

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

No need to add a separate condition "VMClassFree", vmclass is in a deletion state at this point, it will be deleted almost immediately after removing finalizers.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

h.logger.Info("Deletion observed: remove cleanup finalizer from VirtualMachineClass")
controllerutil.RemoveFinalizer(changed, virtv2.FinalizerVMCleanup)
return reconcile.Result{}, nil
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,22 @@ import (

"github.com/deckhouse/virtualization-controller/pkg/controller/conditions"
"github.com/deckhouse/virtualization-controller/pkg/controller/vmclass/internal/state"
"github.com/deckhouse/virtualization-controller/pkg/eventrecord"
virtv2 "github.com/deckhouse/virtualization/api/core/v1alpha2"
"github.com/deckhouse/virtualization/api/core/v1alpha2/vmclasscondition"
)

const nameDiscoveryHandler = "DiscoveryHandler"

func NewDiscoveryHandler() *DiscoveryHandler {
return &DiscoveryHandler{}
func NewDiscoveryHandler(recorder eventrecord.EventRecorderLogger) *DiscoveryHandler {
return &DiscoveryHandler{
recorder: recorder,
}
}

type DiscoveryHandler struct{}
type DiscoveryHandler struct {
recorder eventrecord.EventRecorderLogger
}

func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineClassState) (reconcile.Result, error) {
if s.VirtualMachineClass().IsEmpty() {
Expand Down Expand Up @@ -120,6 +125,28 @@ func (h *DiscoveryHandler) Handle(ctx context.Context, s state.VirtualMachineCla
sort.Strings(featuresEnabled)
sort.Strings(featuresNotEnabled)

addedNodes, removedNodes := NodeNamesDiff(current.Status.AvailableNodes, availableNodeNames)
if len(addedNodes) > 0 || len(removedNodes) > 0 {
if len(availableNodes) > 0 {
h.recorder.Eventf(
changed,
corev1.EventTypeNormal,
virtv2.ReasonVMClassAvailableNodesWasUpdated,
"List of available nodes was updated, added nodes: %q, removed nodes: %q",
addedNodes,
removedNodes,
)
} else {
h.recorder.Eventf(
changed,
corev1.EventTypeWarning,
virtv2.ReasonVMClassAvailableNodesListEmpty,
"List of available nodes was updated, now it's empty, removed nodes: %q",
removedNodes,
)
}
}

changed.Status.AvailableNodes = availableNodeNames
changed.Status.MaxAllocatableResources = h.maxAllocatableResources(availableNodes)
changed.Status.CpuFeatures = virtv2.CpuFeatures{
Expand Down Expand Up @@ -175,3 +202,32 @@ func (h *DiscoveryHandler) maxAllocatableResources(nodes []corev1.Node) corev1.R
}
return resourceList
}

func NodeNamesDiff(prev, current []string) (added, removed []string) {
added = make([]string, 0)
removed = make([]string, 0)
prevMap := make(map[string]struct{})
currentMap := make(map[string]struct{})

for _, n := range prev {
Copy link
Contributor

Choose a reason for hiding this comment

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

n usually means a number, but I suppose not in this context.

prevMap[n] = struct{}{}
}

for _, n := range current {
currentMap[n] = struct{}{}
}

for _, n := range prev {
if _, ok := currentMap[n]; !ok {
removed = append(removed, n)
}
}

for _, n := range current {
if _, ok := prevMap[n]; !ok {
added = append(added, n)
}
}

return added, removed
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
/*
Copyright 2024 Flant JSC

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package internal

import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

type nodeNamesDiffTestParams struct {
prev []string
current []string
added []string
removed []string
}

var _ = Describe("DiscoveryHandler Run", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

DiscoveryHandler doesn't have method Run

DescribeTable(
"NodeNamesDiffTest",
func(params nodeNamesDiffTestParams) {
calculatedAdded, calculatedRemoved := NodeNamesDiff(params.prev, params.current)
Expect(calculatedAdded).Should(Equal(params.added))
Expect(calculatedRemoved).Should(Equal(params.removed))
},
Entry(
"Should be no diff",
nodeNamesDiffTestParams{
prev: []string{
"node1",
"node2",
},
current: []string{
"node1",
"node2",
},
added: []string{},
removed: []string{},
},
),
Entry(
"Should be added node",
nodeNamesDiffTestParams{
prev: []string{
"node1",
"node2",
},
current: []string{
"node1",
"node2",
"node3",
},
added: []string{
"node3",
},
removed: []string{},
},
),
Entry(
"Should be removed node",
nodeNamesDiffTestParams{
prev: []string{
"node1",
"node2",
"node3",
},
current: []string{
"node1",
"node2",
},
added: []string{},
removed: []string{
"node3",
},
},
),
Entry(
"Should be added and removed node",
nodeNamesDiffTestParams{
prev: []string{
"node1",
"node2",
},
current: []string{
"node2",
"node3",
},
added: []string{
"node3",
},
removed: []string{
"node1",
},
},
),
Entry(
"Should be multiple added and removed node",
nodeNamesDiffTestParams{
prev: []string{
"node3",
"node4",
"node5",
},
current: []string{
"node1",
"node2",
"node3",
},
added: []string{
"node1",
"node2",
},
removed: []string{
"node4",
"node5",
},
},
),
)
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
Copyright 2024 Flant JSC

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package internal

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestInternal(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Internal")
}
Loading
Loading