Skip to content

HypervisorController: Do not overwrite evicting #240

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.

Sign up for GitHub

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
fwiesel wants to merge 1 commit into
base: main
Choose a base branch
from
Open

HypervisorController: Do not overwrite evicting #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 12 additions & 6 deletions internal/controller/hypervisor_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,18 @@ func (hv *HypervisorController) Reconcile(ctx context.Context, req ctrl.Request)
nodeTerminationCondition := FindNodeStatusCondition(node.Status.Conditions, "Terminating")
if nodeTerminationCondition != nil && nodeTerminationCondition.Status == corev1.ConditionTrue {
// Node might be terminating, propagate condition to hypervisor
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeReady,
Status: metav1.ConditionFalse,
Reason: nodeTerminationCondition.Reason,
Message: nodeTerminationCondition.Message,
})

if readyCondition := meta.FindStatusCondition(hypervisor.Status.Conditions, kvmv1.ConditionTypeReady); readyCondition == nil || readyCondition.Status == metav1.ConditionTrue {
// Only set Terminating condition if Ready is still True, otherwise we might overwrite other controllers that already set Ready to False
// In particular if the hypervisor is evicting
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeReady,
Status: metav1.ConditionFalse,
Reason: nodeTerminationCondition.Reason,
Message: nodeTerminationCondition.Message,
})
}

meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeTerminating,
Status: metav1.ConditionStatus(nodeTerminationCondition.Status),
Expand Down
153 changes: 120 additions & 33 deletions internal/controller/hypervisor_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,13 @@ import (

var _ = Describe("Hypervisor Controller", func() {
const (
resourceName = "other-node"
topologyZone = "test-zone"
customTrait = "test-trait"
aggregate1 = "aggregate1"
workerGroupLabel = "worker.garden.sapcloud.io/group"
workerGroupValue = "test-group"
resourceName = "other-node"
topologyZone = "test-zone"
customTrait = "test-trait"
aggregate1 = "aggregate1"
workerGroupLabel = "worker.garden.sapcloud.io/group"
workerGroupValue = "test-group"
terminatingReason = "ScaleDown"
)
var (
hypervisorController *HypervisorController
Expand Down Expand Up @@ -264,7 +265,7 @@ var _ = Describe("Hypervisor Controller", func() {
})

Context("When reconciling a terminating node", func() {
It("should successfully reconcile the terminating node", func(ctx SpecContext) {
BeforeEach(func(ctx SpecContext) {
// Mark the node as terminating
resource.Finalizers = append(resource.Finalizers, "example.com/test-finalizer")
Expect(k8sClient.Update(ctx, resource)).To(Succeed())
Expand All @@ -281,42 +282,128 @@ var _ = Describe("Hypervisor Controller", func() {
resource.Status.Conditions = append(resource.Status.Conditions, corev1.NodeCondition{
Type: "Terminating",
Status: corev1.ConditionTrue,
Reason: "Terminating",
Reason: terminatingReason,
Message: "Node is terminating",
})
Expect(k8sClient.Status().Update(ctx, resource)).To(Succeed())

_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
NamespacedName: types.NamespacedName{Name: resource.Name},
})
Expect(err).NotTo(HaveOccurred())
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
Expect(hypervisor.Spec.Maintenance).To(Equal(kvmv1.MaintenanceTermination))
})

By("Reconciling the created resource")
for range 2 {
Context("and the Hypervisor resource does not exists", func() {
It("should successfully reconcile the terminating node", func(ctx SpecContext) {
_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
NamespacedName: types.NamespacedName{Name: resource.Name},
})
Expect(err).NotTo(HaveOccurred())
}
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
Expect(hypervisor.Spec.Maintenance).To(Equal(kvmv1.MaintenanceTermination))

By("Reconciling the created resource")
for range 2 {
_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
NamespacedName: types.NamespacedName{Name: resource.Name},
})
Expect(err).NotTo(HaveOccurred())
}

By("should have set the terminating condition on the Hypervisor resource")
// Get the Hypervisor resource
updatedHypervisor := &kvmv1.Hypervisor{}
Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
Expect(updatedHypervisor.Name).To(Equal(resource.Name))
Expect(updatedHypervisor.Status.Conditions).ToNot(BeNil())
condition := meta.FindStatusCondition(updatedHypervisor.Status.Conditions, kvmv1.ConditionTypeReady)
Expect(condition).ToNot(BeNil())
Expect(condition.Reason).To(Equal("Terminating"))
Expect(condition.Status).To(Equal(metav1.ConditionFalse))

condition = meta.FindStatusCondition(updatedHypervisor.Status.Conditions, kvmv1.ConditionTypeTerminating)
Expect(condition).ToNot(BeNil())
Expect(condition.Reason).To(Equal("Terminating"))
Expect(condition.Status).To(Equal(metav1.ConditionTrue))
By("should have set the terminating condition on the Hypervisor resource")
// Get the Hypervisor resource
updatedHypervisor := &kvmv1.Hypervisor{}
Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
Expect(updatedHypervisor.Status.Conditions).To(ContainElements(
SatisfyAll(
HaveField("Type", kvmv1.ConditionTypeReady),
HaveField("Reason", terminatingReason),
HaveField("Status", metav1.ConditionFalse),
),
SatisfyAll(
HaveField("Type", kvmv1.ConditionTypeTerminating),
HaveField("Reason", terminatingReason),
HaveField("Status", metav1.ConditionTrue),
),
))
})
})

Context("and the Hypervisor resource does exists", func() {
BeforeEach(func(ctx SpecContext) {
hypervisor := &kvmv1.Hypervisor{
ObjectMeta: metav1.ObjectMeta{
Name: resource.Name,
},
Spec: kvmv1.HypervisorSpec{
Maintenance: kvmv1.MaintenanceUnset,
},
}
Expect(k8sClient.Create(ctx, hypervisor)).To(Succeed())
DeferCleanup(func(ctx SpecContext) {
Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, hypervisor))).To(Succeed())
})
})

Context("and the Hypervisor resource already has a Ready Condition set to false", func() {
BeforeEach(func(ctx SpecContext) {
hypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, hypervisor)).To(Succeed())
meta.SetStatusCondition(&hypervisor.Status.Conditions, metav1.Condition{
Type: kvmv1.ConditionTypeReady,
Status: metav1.ConditionFalse,
Reason: "SomeOtherReason",
})
Expect(k8sClient.Status().Update(ctx, hypervisor)).To(Succeed())
})

It("should not update the existing Ready Condition with the new reason", func(ctx SpecContext) {
for range 2 {
_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
NamespacedName: types.NamespacedName{Name: resource.Name},
})
Expect(err).NotTo(HaveOccurred())
}

// Get the Hypervisor resource again
updatedHypervisor := &kvmv1.Hypervisor{}
Expect(k8sClient.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
Expect(updatedHypervisor.Status.Conditions).To(ContainElement(
SatisfyAll(
HaveField("Type", kvmv1.ConditionTypeReady),
HaveField("Reason", "SomeOtherReason"),
HaveField("Status", metav1.ConditionFalse),
),
))
})
})

It("should successfully reconcile the terminating node", func(ctx SpecContext) {
By("Reconciling the created resource")
for range 2 {
_, err := hypervisorController.Reconcile(ctx, ctrl.Request{
NamespacedName: types.NamespacedName{Name: resource.Name},
})
Expect(err).NotTo(HaveOccurred())
}

By("should have set the terminating condition on the Hypervisor resource")
// Get the Hypervisor resource
updatedHypervisor := &kvmv1.Hypervisor{}
Expect(hypervisorController.Get(ctx, hypervisorName, updatedHypervisor)).To(Succeed())
// Not sure, if that is a good idea, but that is the current behaviour
// We expect another operator to set the Maintenance field to Termination
Expect(updatedHypervisor.Spec.Maintenance).NotTo(Equal(kvmv1.MaintenanceTermination))
Expect(updatedHypervisor.Status.Conditions).To(ContainElements(
SatisfyAll(
HaveField("Type", kvmv1.ConditionTypeReady),
HaveField("Reason", terminatingReason),
HaveField("Status", metav1.ConditionFalse),
),
SatisfyAll(
HaveField("Type", kvmv1.ConditionTypeTerminating),
HaveField("Reason", terminatingReason),
HaveField("Status", metav1.ConditionTrue),
),
))
})
})
})
})