Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(CNV-31248): remove namespace annotation #634

Merged
merged 1 commit into from
Aug 31, 2023
Merged

refactor(CNV-31248): remove namespace annotation #634

merged 1 commit into from
Aug 31, 2023

Conversation

codingben
Copy link
Member

What this PR does / why we need it:

Remove vm-console-proxy-namespace annotation that
is no longer relevant since a new released VM
console proxy is no longer require namespace
to operate, by default it was kubevirt as it
required an access to virt-handler (and also
virt-handler certificates).

Jira-Url: https://issues.redhat.com/browse/CNV-31248

Release note:

Remove vm-console-proxy-namespace annotation

@kubevirt-bot 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 Jul 23, 2023
@codingben
Copy link
Member Author

/cc @0xFelix

@codingben codingben marked this pull request as draft July 23, 2023 16:07
@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L and removed size/M labels Jul 23, 2023
@codingben codingben marked this pull request as ready for review July 25, 2023 15:41
@kubevirt-bot 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 Jul 25, 2023
Copy link
Member

@0xFelix 0xFelix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC GetVmConsoleProxyNamespace should be dropped from the test suite and instead GetNamespace should be used, because vm-console-proxy is deployed in the same namespace as SSP?

@codingben
Copy link
Member Author

IIUC GetVmConsoleProxyNamespace should be dropped from the test suite and instead GetNamespace should be used, because vm-console-proxy is deployed in the same namespace as SSP?

You're right, thanks! I forgot about it.

@codingben codingben requested a review from 0xFelix July 26, 2023 09:37
@codingben codingben marked this pull request as draft July 26, 2023 10:14
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 26, 2023
@codingben
Copy link
Member Author

codingben commented Jul 26, 2023

There is an issue with functional tests, I'm trying to debug it. The tests aren't running locally and operator is stuck on validate create logs.

@codingben
Copy link
Member Author

Okay so the functional tests are failing because namespace annotation was removed, and a newer version of vm-console-proxy wasn't released yet, so it's failing to get the virt-handler certificates.

@akrejcir will do a new release of vm-console-proxy, now it's v0.2.0 [1].

[1] https://quay.io/repository/kubevirt/vm-console-proxy

@codingben codingben marked this pull request as ready for review August 7, 2023 09:55
@kubevirt-bot 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 Aug 7, 2023
@codingben
Copy link
Member Author

/retest

@codingben
Copy link
Member Author

This PR depends on #645, since we had major functionality changes in vm-console-proxy that wasn't updated in ssp-operator as well (e.g. there is no /token endpoint, but we have functional tests for it in ssp-operator that is failing).

@0xFelix
Copy link
Member

0xFelix commented Aug 9, 2023

Actually there is a /vnc endpoint now, which /token was renamed to. It should still be accessible, see https://github.com/kubevirt/vm-console-proxy/blob/main/docs/api.md#example.

@akrejcir
Copy link
Collaborator

PR #645 has been merged.

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023
@codingben
Copy link
Member Author

codingben commented Aug 30, 2023

PR #645 has been merged.

I rebased and resolved conflicts. make container-build successfully passed locally.

@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 30, 2023
@akrejcir
Copy link
Collaborator

The VmConsoleProxyNamespace() function in /tests/env/env.go is not used anymore and can be removed. The same applies for any mention of environment variable VM_CONSOLE_PROXY_NAMESPACE

@akrejcir
Copy link
Collaborator

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2023
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akrejcir

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2023
@codingben
Copy link
Member Author

I see CI job are failing and locally as well, functional tests:

Summarizing 5 Failures:
  [FAIL] VM Console Proxy Operand Resource creation created cluster resource [It] [test_id:9848] service account
  /home/*/GitHub/codingben/ssp-operator/tests/vm_console_proxy_test.go:156
  [FAIL] VM Console Proxy Operand Resource deletion recreate after delete [It] [test_id:9866] config map
  /home/*/GitHub/codingben/ssp-operator/tests/tests_common_test.go:105
  [FAIL] VM Console Proxy Operand Resource change should restore modified resource [It] [test_id:9871] config map
  /home/*/GitHub/codingben/ssp-operator/tests/tests_common_test.go:182
  [FAIL] VM Console Proxy Operand Resource change should restore modified app labels [It] [test_id:9884] config map
  /home/*/GitHub/codingben/ssp-operator/tests/labels_test.go:78
  [FAIL] VM Console Proxy Operand Accessing proxy [It] [test_id:TODO] should be able to access /vnc endpoint
  /home/*/GitHub/codingben/ssp-operator/tests/vm_console_proxy_test.go:336

Ran 35 of 289 Specs in 423.144 seconds
FAIL! -- 30 Passed | 5 Failed | 1 Pending | 253 Skipped

@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2023
Remove vm-console-proxy-namespace annotation that
is no longer relevant since a new released VM
console proxy is no longer require namespace
to operate, by default it was kubevirt as it
required an access to virt-handler (and also
virt-handler certificates).

Jira-Url: https://issues.redhat.com/browse/CNV-31248
Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@akrejcir
Copy link
Collaborator

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2023
@akrejcir
Copy link
Collaborator

/cherry-pick release-v0.18

@kubevirt-bot
Copy link
Contributor

@akrejcir: once the present PR merges, I will cherry-pick it on top of release-v0.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-v0.18

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 kubevirt-bot merged commit 72c7fba into kubevirt:main Aug 31, 2023
4 checks passed
@kubevirt-bot
Copy link
Contributor

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

Applying: refactor(CNV-31248): remove namespace annotation
Using index info to reconstruct a base tree...
M	automation/e2e-upgrade-functests/run.sh
M	data/olm-catalog/ssp-operator.clusterserviceversion.yaml
A	docs/configuration.md
M	internal/operands/vm-console-proxy/reconcile.go
Falling back to patching base and 3-way merge...
Auto-merging internal/operands/vm-console-proxy/reconcile.go
CONFLICT (modify/delete): docs/configuration.md deleted in HEAD and modified in refactor(CNV-31248): remove namespace annotation. Version refactor(CNV-31248): remove namespace annotation of docs/configuration.md left in tree.
Auto-merging data/olm-catalog/ssp-operator.clusterserviceversion.yaml
CONFLICT (content): Merge conflict in data/olm-catalog/ssp-operator.clusterserviceversion.yaml
Auto-merging automation/e2e-upgrade-functests/run.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 refactor(CNV-31248): remove namespace annotation
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.18

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants