Remove old HCO metrics services and endpoints when upgrading #1711

machadovilaca · 2022-01-17T16:25:04Z

There is a bug when updating from v1.2.x to versions later than v1.4.x where services hyperconverged-cluster-operator-metrics and hyperconverged-cluster-webhook-metrics, and endpoint hyperconverged-cluster-operator-metrics are not being deleted. This PR ensures those objects are removed.

Signed-off-by: João Vilaça jvilaca@redhat.com

Reviewer Checklist

Reviewers are supposed to review the PR for every aspect below one by one. To check an item means the PR is either "OK" or "Not Applicable" in terms of that item. All items are supposed to be checked before merging a PR.

Release note:

Remove old HCO metrics services and endpoints when upgrading

coveralls · 2022-01-17T16:31:10Z

Pull Request Test Coverage Report for Build 1837254515

46 of 59 (77.97%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 85.216%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/controller/hyperconverged/hyperconverged_controller.go	46	59	77.97%

Totals
Change from base Build 1834040658:	-0.1%
Covered Lines:	3689
Relevant Lines:	4329

💛 - Coveralls

hco-bot · 2022-01-17T18:13:50Z

hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

kubevirt-bot · 2022-01-17T18:13:53Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-sno-azure

In response to this:

hco-e2e-image-index-sno-aws lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-azure

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

machadovilaca · 2022-01-17T18:40:46Z

/retest

hco-bot · 2022-01-17T20:12:36Z

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2022-01-17T20:12:39Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hco-bot · 2022-01-17T21:30:38Z

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

kubevirt-bot · 2022-01-17T21:30:40Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp

In response to this:

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiraboschi · 2022-01-18T15:48:48Z

/retest

tiraboschi · 2022-01-21T15:16:58Z

/rebase

hco-bot · 2022-01-21T16:48:30Z

okd-hco-e2e-image-index-gcp lane succeeded.
/override ci/prow/okd-hco-e2e-image-index-aws
hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp
hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

kubevirt-bot · 2022-01-21T16:48:35Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp, ci/prow/hco-e2e-image-index-sno-aws, ci/prow/okd-hco-e2e-image-index-aws

In response to this:

okd-hco-e2e-image-index-gcp lane succeeded.
/override ci/prow/okd-hco-e2e-image-index-aws
hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp
hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hco-bot · 2022-01-21T18:34:43Z

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

kubevirt-bot · 2022-01-21T18:34:45Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp

In response to this:

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tiraboschi · 2022-01-22T08:56:40Z

/rebase

hco-bot · 2022-01-22T10:32:33Z

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

kubevirt-bot · 2022-01-22T10:32:35Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-gcp

In response to this:

hco-e2e-image-index-azure, hco-e2e-image-index-aws lanes succeeded.
/override ci/prow/hco-e2e-image-index-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

hco-bot · 2022-01-22T12:16:32Z

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

kubevirt-bot · 2022-01-22T12:16:35Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-gcp

In response to this:

hco-e2e-kv-smoke-azure lane succeeded.
/override ci/prow/hco-e2e-kv-smoke-gcp

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nunnatsa · 2022-02-08T09:31:01Z

/retest

nunnatsa

I want to suggest something. Maybe, instead of going to the api-server each time to search for the services and the endpoint, converging the resources to object and so on, we can have a static list of these three objects, where we only need to loop over and delete its items? We can maybe also remove the deleted objects from the list so we could skip them next time, then this code will happen again only if the pod was restarted during upgrade.

@machadovilaca, what do you think?

nunnatsa · 2022-02-08T09:33:05Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+		"hyperconverged-cluster-operator-metrics",
+		"hyperconverged-cluster-webhook-metrics",


please use constants

nunnatsa · 2022-02-08T09:33:25Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+	}
+
+	anyEndpointRemoved, err := r.removeObjs(req, corev1.Endpoints{}, []string{
+		"hyperconverged-cluster-operator-metrics",


should use the same constant as above

nunnatsa · 2022-02-08T09:49:41Z

pkg/controller/hyperconverged/hyperconverged_controller.go

+	obj, err := r.getObj(req, objType, objName)
+	if err != nil {
+		return false, err
+	} else if obj == nil {
+		return false, nil
+	}
+


I think hcoutil.EnsureDeleted already does this logic. Please check if we can remove these lines and the getObj function.

nunnatsa · 2022-02-08T09:56:43Z

pkg/controller/hyperconverged/hyperconverged_controller_test.go

+					"hyperconverged-cluster-operator-metrics",
+					"hyperconverged-cluster-webhook-metrics",


use constant from above comment

nunnatsa · 2022-02-08T09:57:49Z

pkg/controller/hyperconverged/hyperconverged_controller_test.go

+				for _, service := range services {
+					serviceEvents = append(serviceEvents, commonTestUtils.MockEvent{
+						EventType: corev1.EventTypeNormal,
+						Reason:    "Killing",
+						Msg:       fmt.Sprintf("Removed %s Service", service),
+					})
+
+				}


It will work. I wonder if it won't be simpler just to add these two services as a golang structs.

Signed-off-by: João Vilaça <jvilaca@redhat.com>

sonarcloud · 2022-02-13T15:51:52Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

openshift-ci · 2022-02-13T16:45:05Z

@machadovilaca: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/hco-e2e-image-index-sno-aws	`36187aa`	link	false	`/test hco-e2e-image-index-sno-aws`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

hco-bot · 2022-02-13T17:16:57Z

hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

kubevirt-bot · 2022-02-13T17:16:59Z

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-sno-aws

In response to this:

hco-e2e-image-index-sno-azure lane succeeded.
/override ci/prow/hco-e2e-image-index-sno-aws

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nunnatsa · 2022-02-14T05:03:02Z

/override coverage/coveralls

kubevirt-bot · 2022-02-14T05:03:04Z

@nunnatsa: Overrode contexts on behalf of nunnatsa: coverage/coveralls

In response to this:

/override coverage/coveralls

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

nunnatsa · 2022-02-14T05:04:08Z

/approve

kubevirt-bot · 2022-02-14T05:04:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nunnatsa

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [nunnatsa]

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

tiraboschi · 2022-03-01T08:10:47Z

/cherry-pick release-1.6

kubevirt-bot · 2022-03-01T08:11:25Z

@tiraboschi: new pull request created: #1798

In response to this:

/cherry-pick release-1.6

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 17, 2022

kubevirt-bot requested review from nunnatsa and orenc1 January 17, 2022 16:25

kubevirt-bot added the size/L label Jan 17, 2022

hco-bot force-pushed the mv/bug-2037605 branch from 42e5f1a to 9dead91 Compare January 21, 2022 15:19

hco-bot force-pushed the mv/bug-2037605 branch from 9dead91 to 0fcd030 Compare January 22, 2022 08:59

nunnatsa reviewed Feb 8, 2022

View reviewed changes

kubevirt-bot added dco-signoff: no Indicates the PR's author has not DCO signed all their commits. and removed dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 13, 2022

machadovilaca force-pushed the mv/bug-2037605 branch from 3df48c4 to a662f21 Compare February 13, 2022 15:43

kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Feb 13, 2022

machadovilaca added 4 commits February 13, 2022 15:44

Remove old HCO metrics services and endpoints when upgrading

09413a6

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Add requested changes

15d8c3a

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Remove old HCO metrics services and endpoints when upgrading

0593685

Signed-off-by: João Vilaça <jvilaca@redhat.com>

Merge cleanup loops

0c60588

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the mv/bug-2037605 branch from a662f21 to e7368e5 Compare February 13, 2022 15:47

kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2022

Add RBAC to allow get/list/delete Endpoints

36187aa

Signed-off-by: João Vilaça <jvilaca@redhat.com>

machadovilaca force-pushed the mv/bug-2037605 branch from e7368e5 to 36187aa Compare February 13, 2022 15:51

nunnatsa approved these changes Feb 14, 2022

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2022

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 14, 2022

kubevirt-bot merged commit fe6dc53 into kubevirt:main Feb 14, 2022

kubevirt-bot mentioned this pull request Mar 1, 2022

[release-1.6] Remove old HCO metrics services and endpoints when upgrading #1798

Merged

		"hyperconverged-cluster-operator-metrics",
		"hyperconverged-cluster-webhook-metrics",

Remove old HCO metrics services and endpoints when upgrading #1711

Remove old HCO metrics services and endpoints when upgrading #1711

Conversation

machadovilaca commented Jan 17, 2022

coveralls commented Jan 17, 2022 • edited Loading

Pull Request Test Coverage Report for Build 1837254515

💛 - Coveralls

hco-bot commented Jan 17, 2022

kubevirt-bot commented Jan 17, 2022

machadovilaca commented Jan 17, 2022

hco-bot commented Jan 17, 2022

kubevirt-bot commented Jan 17, 2022

hco-bot commented Jan 17, 2022

kubevirt-bot commented Jan 17, 2022

tiraboschi commented Jan 18, 2022

tiraboschi commented Jan 21, 2022

hco-bot commented Jan 21, 2022

kubevirt-bot commented Jan 21, 2022

hco-bot commented Jan 21, 2022

kubevirt-bot commented Jan 21, 2022

tiraboschi commented Jan 22, 2022

hco-bot commented Jan 22, 2022

kubevirt-bot commented Jan 22, 2022

hco-bot commented Jan 22, 2022

kubevirt-bot commented Jan 22, 2022

nunnatsa commented Feb 8, 2022

nunnatsa left a comment

Choose a reason for hiding this comment

nunnatsa Feb 8, 2022

Choose a reason for hiding this comment

nunnatsa Feb 8, 2022

Choose a reason for hiding this comment

nunnatsa Feb 8, 2022

Choose a reason for hiding this comment

nunnatsa Feb 8, 2022

Choose a reason for hiding this comment

nunnatsa Feb 8, 2022

Choose a reason for hiding this comment

sonarcloud bot commented Feb 13, 2022

openshift-ci bot commented Feb 13, 2022 • edited Loading

hco-bot commented Feb 13, 2022

kubevirt-bot commented Feb 13, 2022

nunnatsa commented Feb 14, 2022

kubevirt-bot commented Feb 14, 2022

nunnatsa commented Feb 14, 2022

kubevirt-bot commented Feb 14, 2022

tiraboschi commented Mar 1, 2022

kubevirt-bot commented Mar 1, 2022

coveralls commented Jan 17, 2022 •

edited

Loading

openshift-ci bot commented Feb 13, 2022 •

edited

Loading