fix: correctly delete proxy resources if namespace is not "kubevirt" #538

akrejcir · 2023-04-06T16:45:05Z

What this PR does / why we need it:
Cleanup() method of vm-console-proxy deletes resources from the correct namespace.

Release note:

None

openshift-ci · 2023-04-06T16:45:08Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

akrejcir · 2023-04-18T11:38:28Z

/cc @0xFelix @codingben @ksimon1

Cleanup() method of vm-console-proxy deletes resources from the correct namespace. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

sonarcloud · 2023-04-18T11:42:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

akrejcir · 2023-04-18T14:04:30Z

/test e2e-single-node-functests

kubevirt-bot · 2023-04-18T14:04:32Z

@akrejcir: No presubmit jobs available for kubevirt/ssp-operator@master

In response to this:

/test e2e-single-node-functests

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.

openshift-ci · 2023-04-18T15:58:16Z

@akrejcir: 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/e2e-single-node-functests	`b72b74f`	link	true	`/test e2e-single-node-functests`

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.

akrejcir · 2023-04-19T06:49:26Z

The e2e-single-node-functests failed when gathering logs after tests. The actual tests were successful:
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/kubevirt_ssp-operator/538/pull-ci-kubevirt-ssp-operator-master-e2e-single-node-functests/1648326650770755584

/override ci/prow/e2e-single-node-functests

kubevirt-bot · 2023-04-19T06:49:31Z

@akrejcir: Overrode contexts on behalf of akrejcir: ci/prow/e2e-single-node-functests

In response to this:

The e2e-single-node-functests failed when gathering logs after tests. The actual tests were successful:
https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/kubevirt_ssp-operator/538/pull-ci-kubevirt-ssp-operator-master-e2e-single-node-functests/1648326650770755584

/override ci/prow/e2e-single-node-functests

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.

codingben

/lgtm

codingben · 2023-04-19T13:54:05Z

internal/operands/vm-console-proxy/reconcile_test.go

+			ExpectResourceExists(route, request)
+
+			delete(request.Instance.Annotations, EnableAnnotation)
+			delete(request.Instance.Annotations, VmConsoleProxyNamespaceAnnotation)


What is the reason of deleting VmConsoleProxyNamespaceAnnotation if it's not going to be used in Reconcile?

Then without deleting it, it makes sense to write it as: should remove resources from the namespace once enable annotation removed.

Both annotations are for the proxy functionality, so I just deleted both.
We can improve it later, this PR is to unblock failing d/s tests.

lyarwood

/approve

This looks okay to unblock testing but I'd like to see my two comments discussed and possibly resolved in follow ups if you have time.

lyarwood · 2023-04-20T15:09:53Z

internal/operands/vm-console-proxy/reconcile.go

+}, PtrT interface {
+	*T
+	client.Object
+}, L any, T any](ctx context.Context, name string, cli client.Client, itemsFunc func(list PtrL) []T) ([]client.Object, error) {


Would love to see this simplified with better variable names if possible but that's something for a follow up as this works as expected at the moment.

Unfortunately these Ptr* type parameters are needed for creating generic objects and calling methods on them.
I would like to write it on one line like:

func findResourcesUsingLabels[PtrL interface { *L; client.ObjectList }, PtrT interface { *T; client.Object}, L any, T any](ctx context.Context, name string, cli client.Client, itemsFunc func(list PtrL) []T) ([]client.Object, error) {

But my IDE expands it automatically and I cannot turn it off.

I will try to simplify it. Do you have some suggestion?

lyarwood · 2023-04-20T15:23:26Z

internal/operands/vm-console-proxy/reconcile.go

+	}
+
+	// Filtering in a loop instead of using a FieldSelector in the List() call.
+	// It is only slightly inefficient, because all objects are already cached locally, so there is no API call.


Again something for a follow up but doesn't this duplicate what the client would be doing when using FieldSelector anyway? I didn't think it would fire off additional API calls to filter by fields.

Correct, but it was easier to implement here because of the internals of the client.

We use the cached client from the controller-runtime library. It watches and stores objects locally.
If we want to use a field selector for a specific type, we first need to add an index for the field name into the cache. These indexes are separate for every type of object [1].

I think the code would look like this:

const field = "metadata.name" var indexerFunc client.IndexerFunc = func(object client.Object) []string { return []string{object.GetName()} } mgr.GetFieldIndexer().IndexField(ctx, &v1.ServiceAccount{}, field, indexerFunc) mgr.GetFieldIndexer().IndexField(ctx, &v1.ConfigMap{}, field, indexerFunc) mgr.GetFieldIndexer().IndexField(ctx, &v1.Service{}, field, indexerFunc) mgr.GetFieldIndexer().IndexField(ctx, &appsv1.Deployment{}, field, indexerFunc) mgr.GetFieldIndexer().IndexField(ctx, &routev1.Route{}, field, indexerFunc)

I haven't tested it, so maybe it's incorrect.
I didn't like that we need to register the index for each type separately.

[1] -

ssp-operator/vendor/sigs.k8s.io/controller-runtime/pkg/client/interfaces.go

Lines 187 to 198 in 862a7ef

// FieldIndexer knows how to index over a particular "field" such that it

// can later be used by a field selector.

type FieldIndexer interface {

// IndexFields adds an index with the given field name on the given object type

// by using the given function to extract the value for that field. If you want

// compatibility with the Kubernetes API server, only return one key, and only use

// fields that the API server supports. Otherwise, you can return multiple keys,

// and "equality" in the field selector means that at least one key matches the value.

// The FieldIndexer will automatically take care of indexing over namespace

// and supporting efficient all-namespace queries.

IndexField(ctx context.Context, obj Object, field string, extractValue IndexerFunc) error

}

kubevirt-bot · 2023-04-20T15:25:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lyarwood

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~~ [lyarwood]

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

akrejcir · 2023-04-21T12:43:43Z

/cherry-pick release-v0.17

kubevirt-bot · 2023-04-21T12:44:19Z

@akrejcir: #538 failed to apply on top of branch "release-v0.17":

Applying: fix: correctly delete proxy resources if namespace is not "kubevirt"
Using index info to reconstruct a base tree...
M	internal/operands/vm-console-proxy/reconcile.go
M	internal/operands/vm-console-proxy/reconcile_test.go
M	internal/test-utils/test_utils.go
Falling back to patching base and 3-way merge...
Auto-merging internal/operands/vm-console-proxy/reconcile_test.go
CONFLICT (content): Merge conflict in internal/operands/vm-console-proxy/reconcile_test.go
Auto-merging internal/operands/vm-console-proxy/reconcile.go
CONFLICT (content): Merge conflict in internal/operands/vm-console-proxy/reconcile.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 fix: correctly delete proxy resources if namespace is not "kubevirt"
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-v0.17

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Apr 6, 2023

kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Apr 6, 2023

kubevirt-bot requested review from 0xFelix and lyarwood April 6, 2023 16:45

kubevirt-bot added the size/L label Apr 6, 2023

akrejcir force-pushed the fix-proxy-cleanup branch 2 times, most recently from f0b461c to 18c793b Compare April 18, 2023 11:37

akrejcir marked this pull request as ready for review April 18, 2023 11:37

kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 18, 2023

kubevirt-bot requested a review from ksimon1 April 18, 2023 11:38

kubevirt-bot requested a review from codingben April 18, 2023 11:38

fix: correctly delete proxy resources if namespace is not "kubevirt"

b72b74f

Cleanup() method of vm-console-proxy deletes resources from the correct namespace. Signed-off-by: Andrej Krejcir <akrejcir@redhat.com>

akrejcir force-pushed the fix-proxy-cleanup branch from 18c793b to b72b74f Compare April 18, 2023 11:41

codingben approved these changes Apr 19, 2023

View reviewed changes

kubevirt-bot assigned codingben Apr 19, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2023

codingben reviewed Apr 19, 2023

View reviewed changes

lyarwood reviewed Apr 20, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 20, 2023

kubevirt-bot merged commit d8c2346 into kubevirt:master Apr 20, 2023

akrejcir deleted the fix-proxy-cleanup branch April 21, 2023 12:21

akrejcir mentioned this pull request Apr 21, 2023

[release-v0.17] fix: correctly delete proxy resources if namespace is not "kubevirt" #541

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correctly delete proxy resources if namespace is not "kubevirt" #538

fix: correctly delete proxy resources if namespace is not "kubevirt" #538

akrejcir commented Apr 6, 2023 •

edited

Loading

openshift-ci bot commented Apr 6, 2023

akrejcir commented Apr 18, 2023

sonarcloud bot commented Apr 18, 2023

akrejcir commented Apr 18, 2023

kubevirt-bot commented Apr 18, 2023

openshift-ci bot commented Apr 18, 2023

akrejcir commented Apr 19, 2023

kubevirt-bot commented Apr 19, 2023

codingben left a comment

codingben Apr 19, 2023 •

edited

Loading

akrejcir Apr 20, 2023

lyarwood left a comment

lyarwood Apr 20, 2023

akrejcir Apr 21, 2023

lyarwood Apr 20, 2023

akrejcir Apr 21, 2023 •

edited

Loading

kubevirt-bot commented Apr 20, 2023

akrejcir commented Apr 21, 2023

kubevirt-bot commented Apr 21, 2023

	// FieldIndexer knows how to index over a particular "field" such that it
	// can later be used by a field selector.
	type FieldIndexer interface {
	// IndexFields adds an index with the given field name on the given object type
	// by using the given function to extract the value for that field. If you want
	// compatibility with the Kubernetes API server, only return one key, and only use
	// fields that the API server supports. Otherwise, you can return multiple keys,
	// and "equality" in the field selector means that at least one key matches the value.
	// The FieldIndexer will automatically take care of indexing over namespace
	// and supporting efficient all-namespace queries.
	IndexField(ctx context.Context, obj Object, field string, extractValue IndexerFunc) error
	}

fix: correctly delete proxy resources if namespace is not "kubevirt" #538

fix: correctly delete proxy resources if namespace is not "kubevirt" #538

Conversation

akrejcir commented Apr 6, 2023 • edited Loading

openshift-ci bot commented Apr 6, 2023

akrejcir commented Apr 18, 2023

sonarcloud bot commented Apr 18, 2023

akrejcir commented Apr 18, 2023

kubevirt-bot commented Apr 18, 2023

openshift-ci bot commented Apr 18, 2023

akrejcir commented Apr 19, 2023

kubevirt-bot commented Apr 19, 2023

codingben left a comment

Choose a reason for hiding this comment

codingben Apr 19, 2023 • edited Loading

Choose a reason for hiding this comment

akrejcir Apr 20, 2023

Choose a reason for hiding this comment

lyarwood left a comment

Choose a reason for hiding this comment

lyarwood Apr 20, 2023

Choose a reason for hiding this comment

akrejcir Apr 21, 2023

Choose a reason for hiding this comment

lyarwood Apr 20, 2023

Choose a reason for hiding this comment

akrejcir Apr 21, 2023 • edited Loading

Choose a reason for hiding this comment

kubevirt-bot commented Apr 20, 2023

akrejcir commented Apr 21, 2023

kubevirt-bot commented Apr 21, 2023

akrejcir commented Apr 6, 2023 •

edited

Loading

codingben Apr 19, 2023 •

edited

Loading

akrejcir Apr 21, 2023 •

edited

Loading