POC: block operator upgrade when detecting outdated vms #3171

dasionov · 2024-11-20T15:49:55Z

This PR introduces the logic necessary for blocking hco-operator upgrade when we detect that there are outdated vms in the cluster

step 1: list all virt-controller pods
step 2: loop over the pods and perform http request to the /metrics endpoint to extract all the metrics data
step 3: use regex to make sure no one is using rhel8 machine types
step 4: set upgradeable false when we do detect rhel8 machines

the logic right now is simple and straight forward, and it can be further improved

Release note:

None

kubevirt-bot · 2024-11-20T15:50:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign tiraboschi for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dasionov · 2024-11-20T15:51:43Z

/cc @enp0s3 @jean-edouard @Barakmor1 @nunnatsa

coveralls · 2024-11-20T16:01:22Z

Pull Request Test Coverage Report for Build 11952461865

Details

14 of 83 (16.87%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage decreased (-0.5%) to 71.412%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
controllers/hyperconverged/hyperconverged_controller.go	14	83	16.87%

Files with Coverage Reduction	New Missed Lines	%
controllers/operands/quickStart.go	1	82.14%

Totals
Change from base Build 11951479920:	-0.5%
Covered Lines:	6045
Relevant Lines:	8465

💛 - Coveralls

nunnatsa

I know it's a POC, but still have several comments inline.

nunnatsa · 2024-11-21T08:44:44Z

controllers/hyperconverged/hyperconverged_controller.go

@@ -1330,6 +1340,66 @@ func (r *ReconcileHyperConverged) deleteObj(req *common.HcoRequest, obj client.O
 	return removed, nil
 }

+func (r *ReconcileHyperConverged) EvaluateUpgradeEligibility(req *common.HcoRequest) error {


This method is called only locally and should not be exposed.

Please rename:

Suggested change

func (r *ReconcileHyperConverged) EvaluateUpgradeEligibility(req *common.HcoRequest) error {

func (r *ReconcileHyperConverged) evaluateUpgradeEligibility(req *common.HcoRequest) error {

nunnatsa · 2024-11-21T08:46:25Z

controllers/hyperconverged/hyperconverged_controller.go

+	podList := &corev1.PodList{}
+	listOpts := []client.ListOption{
+		client.InNamespace(req.Namespace),
+		client.MatchingLabels{"kubevirt.io": "virt-controller"},
+	}
+
+	if err := r.client.List(req.Ctx, podList, listOpts...); err != nil {
+		req.Logger.Info("Failed to list virt-controller pods", "namespace", req.Namespace, "error", err)
+		return fmt.Errorf("failed to list virt-controller pods: %w", err)
+	}
+
+	if len(podList.Items) == 0 {
+		req.Logger.Info("No virt-controller pods found", "namespace", req.Namespace)
+		return nil
+	}


Question: does each pod collects only several VM info? In another words: can we query the service instead of the pod itself?

we don't have a service for virt-controller as far as i know, only for virt-api
but i assume i can query just one of the pods since they all should have the same metrics data

nunnatsa · 2024-11-21T08:47:58Z

controllers/hyperconverged/hyperconverged_controller.go

+	for _, pod := range podList.Items {
+		if pod.Status.PodIP == "" {
+			continue
+		}
+		metricsURL := fmt.Sprintf("https://%s:%d/metrics", pod.Status.PodIP, 8443)
+
+		resp, err := httpClient.Get(metricsURL)
+		if err != nil {
+			req.Logger.Info("Failed to query metrics from pod", "pod", pod.Name, "error", err)
+			continue
+		}
+
+		if resp.StatusCode != http.StatusOK {
+			req.Logger.Info("Metrics endpoint returned non-200 status", "pod", pod.Name, "status", resp.StatusCode)
+			resp.Body.Close()
+			continue
+		}
+
+		body, err := io.ReadAll(resp.Body)
+		resp.Body.Close()
+		if err != nil {
+			req.Logger.Info("Failed to read metrics response from pod", "pod", pod.Name, "error", err)
+			continue
+		}
+
+		virtMetricsData := string(body)
+		rhel8Regex := regexp.MustCompile(`.*rhel8.*`)
+		if rhel8Regex.MatchString(virtMetricsData) {
+			req.Logger.Info("Detected outdated machine type in metrics", "pod", pod.Name, "matched", rhel8Regex.FindString(virtMetricsData))
+			req.Upgradeable = false
+			return nil
+		}
+	}


Eventually, we'll need to do it a-synchronized

nunnatsa · 2024-11-21T08:52:49Z

controllers/hyperconverged/hyperconverged_controller.go

+		}
+
+		virtMetricsData := string(body)
+		rhel8Regex := regexp.MustCompile(`.*rhel8.*`)


never create regex within a loop. creating a regex variable is an expensive operation. The value of this regex will never changed during runtime. Please take it out of this method as a regular variable, that will be initialized only once.

just do:

var rhel8Regex = regexp.MustCompile(`.*rhel8.*`)

just above the method.

the regex will be created each reconcile loop then, that's ok?

if it will be defined in the package scope, out of the method, then no. It will only be initialized once, on boot.

nunnatsa

I know it's a POC, but still have several comments inline.

nunnatsa · 2024-11-21T10:32:06Z

controllers/hyperconverged/hyperconverged_controller.go

+		}
+
+		if resp.StatusCode != http.StatusOK {
+			req.Logger.Info("Metrics endpoint returned non-200 status", "pod", pod.Name, "status", resp.StatusCode)


Not sure about the term "non-200" in an error message.

WDYT about "Metrics endpoint returned error" ?

sounds good

nunnatsa · 2024-11-21T10:32:23Z

controllers/hyperconverged/hyperconverged_controller.go

+
+		if resp.StatusCode != http.StatusOK {
+			req.Logger.Info("Metrics endpoint returned non-200 status", "pod", pod.Name, "status", resp.StatusCode)
+			resp.Body.Close()


use defer instead

i did use that at first, but got a warning said that it's not safe to use defer in a loop, and it might cause a resources leek

right. so we can move the loop content into a new method. That will work.

nunnatsa · 2024-11-21T10:32:33Z

controllers/hyperconverged/hyperconverged_controller.go

+		}
+
+		body, err := io.ReadAll(resp.Body)
+		resp.Body.Close()


use defer instead

nunnatsa · 2024-11-21T10:40:23Z

controllers/hyperconverged/hyperconverged_controller.go

+			continue
+		}
+
+		body, err := io.ReadAll(resp.Body)


I think it's better to use a scanner. The body can be big.

You can do something like

scanner := bufio.NewScanner(resp.Body) ... for scanner.Scan() { if rhel8Regex.MatchString(scanner.Text()) { req.Logger.Info("Detected outdated machine type in metrics", "pod", pod.Name, "matched", rhel8Regex.FindString(virtMetricsData)) req.Upgradeable = false return nil } }

nunnatsa · 2024-11-21T11:22:49Z

Another comment I forgot to write: The current HTTP client code does not use any type of timeout. Use a context (derived from req.Context) with 3-seconds timeout. Sending HTTP request w/o timeout may be risky.

Signed-off-by: Daniel Sionov <[email protected]>

sonarqubecloud · 2024-11-21T11:39:18Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

dasionov marked this pull request as draft November 20, 2024 15:49

kubevirt-bot requested review from avlitman and nunnatsa November 20, 2024 15:50

kubevirt-bot requested review from Barakmor1, enp0s3 and jean-edouard November 20, 2024 15:51

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 20, 2024

dasionov force-pushed the block_upgrade_for_rhel8_vms branch 2 times, most recently from 74747cc to d4ec71b Compare November 20, 2024 15:57

dasionov force-pushed the block_upgrade_for_rhel8_vms branch from d4ec71b to ed2b6ed Compare November 20, 2024 16:03

nunnatsa reviewed Nov 21, 2024

View reviewed changes

dasionov force-pushed the block_upgrade_for_rhel8_vms branch from ed2b6ed to a15db2d Compare November 21, 2024 11:06

dasionov force-pushed the block_upgrade_for_rhel8_vms branch from a15db2d to eefac66 Compare November 21, 2024 11:33

POC: block operator upgrade when detecting outdated vms

dd7f177

Signed-off-by: Daniel Sionov <[email protected]>

dasionov force-pushed the block_upgrade_for_rhel8_vms branch from eefac66 to dd7f177 Compare November 21, 2024 11:38

kubevirt-bot added size/L and removed size/M labels Nov 21, 2024

dasionov mentioned this pull request Dec 4, 2024

virt-operator: Add a service component to virt-controller kubevirt/kubevirt#13348

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POC: block operator upgrade when detecting outdated vms #3171

POC: block operator upgrade when detecting outdated vms #3171

dasionov commented Nov 20, 2024 •

edited

Loading

kubevirt-bot commented Nov 20, 2024

dasionov commented Nov 20, 2024

coveralls commented Nov 20, 2024 •

edited

Loading

nunnatsa left a comment

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024 •

edited

Loading

nunnatsa Nov 21, 2024

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024 •

edited

Loading

nunnatsa Nov 21, 2024

nunnatsa left a comment

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024

nunnatsa Nov 21, 2024

nunnatsa Nov 21, 2024

nunnatsa Nov 21, 2024

dasionov Nov 21, 2024

nunnatsa commented Nov 21, 2024

sonarqubecloud bot commented Nov 21, 2024

	func (r ReconcileHyperConverged) EvaluateUpgradeEligibility(req common.HcoRequest) error {
	func (r ReconcileHyperConverged) evaluateUpgradeEligibility(req common.HcoRequest) error {

POC: block operator upgrade when detecting outdated vms #3171

Are you sure you want to change the base?

POC: block operator upgrade when detecting outdated vms #3171

Conversation

dasionov commented Nov 20, 2024 • edited Loading

kubevirt-bot commented Nov 20, 2024

dasionov commented Nov 20, 2024

coveralls commented Nov 20, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11952461865

Details

💛 - Coveralls

nunnatsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasionov Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dasionov Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunnatsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunnatsa commented Nov 21, 2024

sonarqubecloud bot commented Nov 21, 2024

Quality Gate passed

dasionov commented Nov 20, 2024 •

edited

Loading

coveralls commented Nov 20, 2024 •

edited

Loading

dasionov Nov 21, 2024 •

edited

Loading

dasionov Nov 21, 2024 •

edited

Loading