Skip to content

Commit

Permalink
[WorkloadUpdateController]Remove updater from WorkloadUpdateController
Browse files Browse the repository at this point in the history 
The updater package was originally used to prevent
repeated update failures or no-op updates to CRD
status during long upgrades, particularly when
upgrading from a version where a CRD did not include
status as a subresource to a version where it did.
However, since KubeVirt CRD has included
status as a subresource for a long time, this concern
is no longer relevant.
Therefore, we can safely remove the usage of
the updater package.

Signed-off-by: bmordeha <bmordeha@redhat.com>
  • Loading branch information
@Barakmor1
Barakmor1 committed Sep 5, 2024
1 parent 352adc3 commit 8d8b848
Show file tree
Hide file tree
Showing 5 changed files with 1 addition and 121 deletions.
22 changes: 0 additions & 22 deletions pkg/util/status/status.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,28 +302,6 @@ func NewVMIRSStatusUpdater(cli kubecli.KubevirtClient) *VMIRSStatusUpdater {
}
}

type KVStatusUpdater struct {
updater updater
}

func (v *KVStatusUpdater) UpdateStatus(kv *v1.KubeVirt) error {
return v.updater.update(kv)
}

func (v *KVStatusUpdater) PatchStatus(kv *v1.KubeVirt, pt types.PatchType, data []byte) error {
return v.updater.patch(kv, pt, data, &metav1.PatchOptions{})
}

func NewKubeVirtStatusUpdater(cli kubecli.KubevirtClient) *KVStatusUpdater {
return &KVStatusUpdater{
updater: updater{
lock: sync.Mutex{},
subresource: true,
cli: cli,
},
}
}

type VMExportStatusUpdater struct {
updater
}
Expand Down
93 changes: 0 additions & 93 deletions pkg/util/status/status_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,57 +37,6 @@ var _ = Describe("Status", func() {
virtClient.EXPECT().VirtualMachine(gomock.Any()).Return(vmInterface).AnyTimes()
})
Context("starting with the assumption of /status being present", func() {

Context("for PUT operations", func() {
It("should continuously use the /status subresource if no errors occur", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(2)
Expect(updater.UpdateStatus(kv)).To(Succeed())
Expect(updater.UpdateStatus(kv)).To(Succeed())
})

It("should fall back on a 404 error on the /status subresource to an ordinary update", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, errors.NewNotFound(schema.GroupResource{}, "something")).Times(1)
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(2)
Expect(updater.UpdateStatus(kv)).To(Succeed())
Expect(updater.UpdateStatus(kv)).To(Succeed())
})

It("should fall back on a 404 error on the /status subresource to an ordinary update but keep in mind that objects may have disappeared", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, errors.NewNotFound(schema.GroupResource{}, "something")).Times(1)
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, errors.NewNotFound(schema.GroupResource{}, "something")).Times(1)
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(1)
Expect(updater.UpdateStatus(kv)).To(Succeed())
})

It("should fall back on a 404 error on the /status subresource to an ordinary update but keep in mind that the subresource may get enabled directly afterwards", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
newKV := kv.DeepCopy()
newKV.Status.Phase = v1.KubeVirtPhaseDeleted
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, errors.NewNotFound(schema.GroupResource{}, "something")).Times(1)
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(newKV, nil).Times(1)
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(1)
Expect(updater.UpdateStatus(kv)).To(Succeed())
})

It("should stick with /status if an arbitrary error occurs", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, fmt.Errorf("I am not 404")).Times(1)
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(1)
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
Expect(updater.UpdateStatus(kv)).To(Succeed())
})
})

Context("for PATCH operations", func() {
It("should continuously use the /status subresource if no errors occur", func() {
updater := NewVMStatusUpdater(virtClient)
Expand Down Expand Up @@ -146,48 +95,6 @@ var _ = Describe("Status", func() {
})

Context("starting with the assumption that /status is not present", func() {

Context("for PUT operations", func() {
It("should stick with a normal update if the status did change", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
updater.updater.subresource = false
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(2)
Expect(updater.UpdateStatus(kv)).To(Succeed())
Expect(updater.UpdateStatus(kv)).To(Succeed())
})

It("should stick with a normal update if we get a 404 error", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
updater.updater.subresource = false
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, errors.NewNotFound(schema.GroupResource{}, "something")).Times(2)
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
})

It("should stick with a normal update if we get an arbitrary error", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
updater.updater.subresource = false
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, fmt.Errorf("I am not 404")).Times(2)
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
Expect(updater.UpdateStatus(kv)).ToNot(Succeed())
})

It("should fall back to /status if the status did not change and stick to it", func() {
updater := NewKubeVirtStatusUpdater(virtClient)
updater.updater.subresource = false
kv := &v1.KubeVirt{Status: v1.KubeVirtStatus{Phase: v1.KubeVirtPhaseDeployed}}
oldKV := kv.DeepCopy()
oldKV.Status.Phase = v1.KubeVirtPhaseDeploying
kvInterface.EXPECT().Update(context.Background(), kv, metav1.UpdateOptions{}).Return(oldKV, nil).Times(1)
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(1)
Expect(updater.UpdateStatus(kv)).To(Succeed())
kvInterface.EXPECT().UpdateStatus(context.Background(), kv, metav1.UpdateOptions{}).Return(kv, nil).Times(1)
Expect(updater.UpdateStatus(kv)).To(Succeed())
})
})
Context("for PATCH operations", func() {
It("should stick with a normal update if the resource version did change", func() {
updater := NewVMStatusUpdater(virtClient)
Expand Down
1 change: 0 additions & 1 deletion pkg/virt-controller/watch/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ go_library(
"//pkg/util/migrations:go_default_library",
"//pkg/util/pdbs:go_default_library",
"//pkg/util/ratelimiter:go_default_library",
"//pkg/util/status:go_default_library",
"//pkg/util/tls:go_default_library",
"//pkg/util/trace:go_default_library",
"//pkg/virt-config:go_default_library",
Expand Down
1 change: 0 additions & 1 deletion pkg/virt-controller/watch/workload-updater/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ go_library(
"//pkg/controller:go_default_library",
"//pkg/monitoring/metrics/virt-controller:go_default_library",
"//pkg/util/migrations:go_default_library",
"//pkg/util/status:go_default_library",
"//pkg/virt-config:go_default_library",
"//pkg/virt-controller/watch/volume-migration:go_default_library",
"//staging/src/kubevirt.io/api/core/v1:go_default_library",
Expand Down
5 changes: 1 addition & 4 deletions pkg/virt-controller/watch/workload-updater/workload-updater.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import (
"kubevirt.io/kubevirt/pkg/controller"
metrics "kubevirt.io/kubevirt/pkg/monitoring/metrics/virt-controller"
migrationutils "kubevirt.io/kubevirt/pkg/util/migrations"
"kubevirt.io/kubevirt/pkg/util/status"
virtconfig "kubevirt.io/kubevirt/pkg/virt-config"
volumemig "kubevirt.io/kubevirt/pkg/virt-controller/watch/volume-migration"
)
Expand Down Expand Up @@ -70,7 +69,6 @@ type WorkloadUpdateController struct {
migrationExpectations *controller.UIDTrackingControllerExpectations
kubeVirtStore cache.Store
clusterConfig *virtconfig.ClusterConfig
statusUpdater *status.KVStatusUpdater
launcherImage string

lastDeletionBatch time.Time
Expand Down Expand Up @@ -111,7 +109,6 @@ func NewWorkloadUpdateController(
kubeVirtStore: kubeVirtInformer.GetStore(),
recorder: recorder,
clientset: clientset,
statusUpdater: status.NewKubeVirtStatusUpdater(clientset),
launcherImage: launcherImage,
migrationExpectations: controller.NewUIDTrackingControllerExpectations(controller.NewControllerExpectations()),
clusterConfig: clusterConfig,
Expand Down Expand Up @@ -484,7 +481,7 @@ func (c *WorkloadUpdateController) sync(kv *virtv1.KubeVirt) error {
if err != nil {
return err
}
err = c.statusUpdater.PatchStatus(kv, types.JSONPatchType, patchBytes)
_, err = c.clientset.KubeVirt(kv.Namespace).PatchStatus(context.Background(), kv.Name, types.JSONPatchType, patchBytes, metav1.PatchOptions{})
if err != nil {
return fmt.Errorf("unable to patch kubevirt obj status to update the outdatedVirtualMachineInstanceWorkloads valued: %v", err)
}
Expand Down

0 comments on commit 8d8b848

Please sign in to comment.