14240046899

Committed 03 Apr 2025 09:48AM UTC coverage: 72.296%. First build

Build # 14240046899

Build Type

Pull #3379

github

Committed by

nunnatsa

Commit Message

Fix a bug when setting a CPU quentity without a type

When setting, for example, the
`spec.resourceRequirements.storageWorkloads.limit` field to `"1.5"`, the
client set the value in CDI to `"1500m"`. Then HCO will always find this
value as not matches to the required value of `"1.5"`, and will
constantly try to update CDI.

This PR fixes this issue by turning the required objects to json and back
to objects. The json marshaling peforms the same change in the object.
Now when HCO will compare the required value with the existing one, both
will be at the newer format, with the quntity type, and HCO will
understand that no change was done, and it will not try to update the CR
again.

Also. This PR adds a mutex protection to the cache in the operand
handlers.

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Pull Request Pull Request #3379: Fix a bug when setting a CPU quantity without a type

Run Details

53 of 62 new or added lines in 7 files covered. (85.48%)

6524 of 9024 relevant lines covered (72.3%)

0.8 hits per line

Source File
Press 'n' to go to next uncovered line, 'b' for previous

80.49

/controllers/operands/deploymentHandler.go

package operands

import (
        "errors"
        "reflect"
        "sync"

        appsv1 "k8s.io/api/apps/v1"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        "k8s.io/apimachinery/pkg/runtime"
        "sigs.k8s.io/controller-runtime/pkg/client"

        "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"

        hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
        "github.com/kubevirt/hyperconverged-cluster-operator/controllers/common"
)

type newDeploymentFunc func(hc *hcov1beta1.HyperConverged) *appsv1.Deployment

func newDeploymentHandler(Client client.Client, Scheme *runtime.Scheme, deploymentGenerator newDeploymentFunc, hc *hcov1beta1.HyperConverged) Operand {
        return &genericOperand{
                Client: Client,
                Scheme: Scheme,
                crType: "Deployment",
                hooks:  newDeploymentHooks(deploymentGenerator, hc),
        }
}

type deploymentHooks struct {
        sync.Mutex
        deploymentGenerator newDeploymentFunc
        cache               *appsv1.Deployment
}

func newDeploymentHooks(deploymentGenerator newDeploymentFunc, hc *hcov1beta1.HyperConverged) *deploymentHooks {
        return &deploymentHooks{
                cache:               deploymentGenerator(hc),
                deploymentGenerator: deploymentGenerator,
        }
}

func (h *deploymentHooks) reset() {
        h.Lock()
        defer h.Unlock()

        h.cache = nil
}

func (h *deploymentHooks) getFullCr(hc *hcov1beta1.HyperConverged) (client.Object, error) {
        h.Lock()
        defer h.Unlock()

        if h.cache == nil {
                h.cache = h.deploymentGenerator(hc)
        }

        return h.cache, nil
}

func (h *deploymentHooks) getEmptyCr() client.Object {
        return &appsv1.Deployment{
                ObjectMeta: metav1.ObjectMeta{
                        Name: h.cache.Name,
                },
        }
}

func (h *deploymentHooks) justBeforeComplete(_ *common.HcoRequest) { /* no implementation */ }

func (h *deploymentHooks) updateCr(req *common.HcoRequest, Client client.Client, exists runtime.Object, required runtime.Object) (bool, bool, error) {
        deployment, ok1 := required.(*appsv1.Deployment)
        found, ok2 := exists.(*appsv1.Deployment)

        if !ok1 || !ok2 {
                return false, false, errors.New("can't convert to Deployment")
        }
        if !hasCorrectDeploymentFields(found, deployment) {
                if req.HCOTriggered {
                        req.Logger.Info("Updating existing Deployment to new opinionated values", "name", deployment.Name)
                } else {
                        req.Logger.Info("Reconciling an externally updated Deployment to its opinionated values", "name", deployment.Name)
                }
                if shouldRecreate(found, deployment) {
                        err := Client.Delete(req.Ctx, found, &client.DeleteOptions{})
                        if err != nil {
                                return false, false, err
                        }
                        err = Client.Create(req.Ctx, deployment, &client.CreateOptions{})
                        if err != nil {
                                return false, false, err
                        }
                        return true, !req.HCOTriggered, nil
                }
                util.MergeLabels(&deployment.ObjectMeta, &found.ObjectMeta)
                deployment.Spec.DeepCopyInto(&found.Spec)
                err := Client.Update(req.Ctx, found)
                if err != nil {
                        return false, false, err
                }
                return true, !req.HCOTriggered, nil
        }

        return false, false, nil
}

// We need to check only certain fields in the deployment resource, since some of the fields
// are being set by k8s.
func hasCorrectDeploymentFields(found *appsv1.Deployment, required *appsv1.Deployment) bool {
        return util.CompareLabels(required, found) &&
                reflect.DeepEqual(found.Spec.Selector, required.Spec.Selector) &&
                reflect.DeepEqual(found.Spec.Replicas, required.Spec.Replicas) &&
                reflect.DeepEqual(found.Spec.Template.Spec.Containers, required.Spec.Template.Spec.Containers) &&
                reflect.DeepEqual(found.Spec.Template.Spec.ServiceAccountName, required.Spec.Template.Spec.ServiceAccountName) &&
                reflect.DeepEqual(found.Spec.Template.Spec.PriorityClassName, required.Spec.Template.Spec.PriorityClassName) &&
                reflect.DeepEqual(found.Spec.Template.Spec.Affinity, required.Spec.Template.Spec.Affinity) &&
                reflect.DeepEqual(found.Spec.Template.Spec.NodeSelector, required.Spec.Template.Spec.NodeSelector) &&
                reflect.DeepEqual(found.Spec.Template.Spec.Tolerations, required.Spec.Template.Spec.Tolerations)
}

func shouldRecreate(found, required *appsv1.Deployment) bool {
        // updating LabelSelector (it's immutable) would be rejected by API server; create new Deployment instead
        return !reflect.DeepEqual(found.Spec.Selector, required.Spec.Selector)
}

1	package operands
2
3	import (
4	"errors"
5	"reflect"
6	"sync"
7
8	appsv1 "k8s.io/api/apps/v1"
9	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10	"k8s.io/apimachinery/pkg/runtime"
11	"sigs.k8s.io/controller-runtime/pkg/client"
12
13	"github.com/kubevirt/hyperconverged-cluster-operator/pkg/util"
14
15	hcov1beta1 "github.com/kubevirt/hyperconverged-cluster-operator/api/v1beta1"
16	"github.com/kubevirt/hyperconverged-cluster-operator/controllers/common"
17	)
18
19	type newDeploymentFunc func(hc hcov1beta1.HyperConverged) appsv1.Deployment
20
21	func newDeploymentHandler(Client client.Client, Scheme runtime.Scheme, deploymentGenerator newDeploymentFunc, hc hcov1beta1.HyperConverged) Operand {	1✔
22	return &genericOperand{	1✔
23	Client: Client,	1✔
24	Scheme: Scheme,	1✔
25	crType: "Deployment",	1✔
26	hooks: newDeploymentHooks(deploymentGenerator, hc),	1✔
27	}	1✔
28	}	1✔
29
30	type deploymentHooks struct {
31	sync.Mutex
32	deploymentGenerator newDeploymentFunc
33	cache *appsv1.Deployment
34	}
35
36	func newDeploymentHooks(deploymentGenerator newDeploymentFunc, hc hcov1beta1.HyperConverged) deploymentHooks {	1✔
37	return &deploymentHooks{	1✔
38	cache: deploymentGenerator(hc),	1✔
39	deploymentGenerator: deploymentGenerator,	1✔
40	}	1✔
41	}	1✔
42
43	func (h *deploymentHooks) reset() {	×
NEW 44	h.Lock()	×
NEW 45	defer h.Unlock()	×
NEW 46		×
47	h.cache = nil	×
48	}	×
49
50	func (h deploymentHooks) getFullCr(hc hcov1beta1.HyperConverged) (client.Object, error) {	1✔
51	h.Lock()	1✔
52	defer h.Unlock()	1✔
53		1✔
54	if h.cache == nil {	1✔
55	h.cache = h.deploymentGenerator(hc)	×
56	}	×
57
58	return h.cache, nil	1✔
59	}
60
61	func (h *deploymentHooks) getEmptyCr() client.Object {	1✔
62	return &appsv1.Deployment{	1✔
63	ObjectMeta: metav1.ObjectMeta{	1✔
64	Name: h.cache.Name,	1✔
65	},	1✔
66	}	1✔
67	}	1✔
68
69	func (h deploymentHooks) justBeforeComplete(_ common.HcoRequest) { /* no implementation */ }	1✔
70
71	func (h deploymentHooks) updateCr(req common.HcoRequest, Client client.Client, exists runtime.Object, required runtime.Object) (bool, bool, error) {	1✔
72	deployment, ok1 := required.(*appsv1.Deployment)	1✔
73	found, ok2 := exists.(*appsv1.Deployment)	1✔
74		1✔
75	if !ok1 \|\| !ok2 {	1✔
76	return false, false, errors.New("can't convert to Deployment")	×
77	}	×
78	if !hasCorrectDeploymentFields(found, deployment) {	2✔
79	if req.HCOTriggered {	2✔
80	req.Logger.Info("Updating existing Deployment to new opinionated values", "name", deployment.Name)	1✔
81	} else {	2✔
82	req.Logger.Info("Reconciling an externally updated Deployment to its opinionated values", "name", deployment.Name)	1✔
83	}	1✔
84	if shouldRecreate(found, deployment) {	2✔
85	err := Client.Delete(req.Ctx, found, &client.DeleteOptions{})	1✔
86	if err != nil {	1✔
87	return false, false, err	×
88	}	×
89	err = Client.Create(req.Ctx, deployment, &client.CreateOptions{})	1✔
90	if err != nil {	1✔
91	return false, false, err	×
92	}	×
93	return true, !req.HCOTriggered, nil	1✔
94	}
95	util.MergeLabels(&deployment.ObjectMeta, &found.ObjectMeta)	1✔
96	deployment.Spec.DeepCopyInto(&found.Spec)	1✔
97	err := Client.Update(req.Ctx, found)	1✔
98	if err != nil {	1✔
99	return false, false, err	×
100	}	×
101	return true, !req.HCOTriggered, nil	1✔
102	}
103
104	return false, false, nil	1✔
105	}
106
107	// We need to check only certain fields in the deployment resource, since some of the fields
108	// are being set by k8s.
109	func hasCorrectDeploymentFields(found appsv1.Deployment, required appsv1.Deployment) bool {	1✔
110	return util.CompareLabels(required, found) &&	1✔
111	reflect.DeepEqual(found.Spec.Selector, required.Spec.Selector) &&	1✔
112	reflect.DeepEqual(found.Spec.Replicas, required.Spec.Replicas) &&	1✔
113	reflect.DeepEqual(found.Spec.Template.Spec.Containers, required.Spec.Template.Spec.Containers) &&	1✔
114	reflect.DeepEqual(found.Spec.Template.Spec.ServiceAccountName, required.Spec.Template.Spec.ServiceAccountName) &&	1✔
115	reflect.DeepEqual(found.Spec.Template.Spec.PriorityClassName, required.Spec.Template.Spec.PriorityClassName) &&	1✔
116	reflect.DeepEqual(found.Spec.Template.Spec.Affinity, required.Spec.Template.Spec.Affinity) &&	1✔
117	reflect.DeepEqual(found.Spec.Template.Spec.NodeSelector, required.Spec.Template.Spec.NodeSelector) &&	1✔
118	reflect.DeepEqual(found.Spec.Template.Spec.Tolerations, required.Spec.Template.Spec.Tolerations)	1✔
119	}	1✔
120
121	func shouldRecreate(found, required *appsv1.Deployment) bool {	1✔
122	// updating LabelSelector (it's immutable) would be rejected by API server; create new Deployment instead	1✔
123	return !reflect.DeepEqual(found.Spec.Selector, required.Spec.Selector)	1✔
124	}	1✔

kubevirt / hyperconverged-cluster-operator / 14240046899

Source File Press 'n' to go to next uncovered line, 'b' for previous

Source File
Press 'n' to go to next uncovered line, 'b' for previous