search

kubevirt / hyperconverged-cluster-operator

Operator pattern for managing multi-operator products
Apache License 2.0
154 stars 153 forks source link

API: Use new token generation SSP API and remove feature gate #3087

Closed akrejcir closed 2 weeks ago

akrejcir commented 2 months ago

What this PR does / why we need it: The token generation API was stabilized in the SSP, and feature gate was removed: https://github.com/kubevirt/ssp-operator/pull/1018

This PR removes the same feature gate from HCO, and adds a new field in the .spec to enable this feature.

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.

Jira Ticket:

https://issues.redhat.com/browse/CNV-45065

Release note:

- The feature gate "deployVmConsoleProxy" was deprecated and is now ignored.
- Added a new field ".spec.enableTokenGenerationApi" to enable the token generation API.
coveralls commented 2 months ago

Pull Request Test Coverage Report for Build 11362880275

Details

Totals Coverage Status
Change from base Build 11315476158: 0.007%
Covered Lines: 6062
Relevant Lines: 8357
💛 - Coveralls
akrejcir commented 2 months ago

Thanks for the PR @akrejcir

What will happen on upgrade, if the DeployVMConsoleProxy FG is set?

Good point, I did not consider update.

Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

nunnatsa commented 2 months ago

Thanks for the PR @akrejcir What will happen on upgrade, if the DeployVMConsoleProxy FG is set?

Good point, I did not consider update.

Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

hco-bot commented 2 months ago

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

kubevirt-bot commented 2 months ago

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

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2325009193): >hco-e2e-kv-smoke-gcp lane succeeded. >/override ci/prow/hco-e2e-kv-smoke-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
tiraboschi commented 2 months ago

Good point, I did not consider update. Does HCO have functionality to run some logic only during update? It would be good, if we can copy the value of the feature gate to the new field in .spec. But only during update, because in the new version, the feature gate will be ignored.

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

In assets/upgradePatches.json we are accumulating a set of jsonpatches to be executed during the upgrades (according to semver matching rules) on the spec stanza of the HCO CR. move and copy operations are also supported. Please take a look at https://github.com/evanphx/json-patch/blob/master/v5/patch_test.go for examples. I'd suggest to add there an upgradePatch to handle it during upgrades without the need for additional code.

akrejcir commented 1 month ago

...

HCO does have code that only run during upgrade, but we need to avoid modifying the HyperConverged spec. Another alternative is the mutation webhook. @tiraboschi WDYT?

In assets/upgradePatches.json we are accumulating a set of jsonpatches to be executed during the upgrades (according to semver matching rules) on the spec stanza of the HCO CR. move and copy operations are also supported. Please take a look at https://github.com/evanphx/json-patch/blob/master/v5/patch_test.go for examples. I'd suggest to add there an upgradePatch to handle it during upgrades without the need for additional code.

Thanks, I will add a patch there. Which version is checked by the semverRange field? Is it the version from which we are upgrading, or to which we are upgrading? It was not clear from the code.

hco-bot commented 1 month ago

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

kubevirt-bot commented 1 month ago

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

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2338879164): >hco-e2e-kv-smoke-gcp lane succeeded. >/override ci/prow/hco-e2e-kv-smoke-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
kubevirt-bot commented 1 month ago

@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
pull-hyperconverged-cluster-operator-e2e-k8s-1.29 25eac4980b7ddacd8ac38b6225fa224143dde121 link true /test pull-hyperconverged-cluster-operator-e2e-k8s-1.29
Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
hco-bot commented 1 month ago

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

kubevirt-bot commented 1 month ago

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

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2341479368): >hco-e2e-kv-smoke-gcp lane succeeded. >/override ci/prow/hco-e2e-kv-smoke-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
hco-bot commented 1 month ago

hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure hco-e2e-operator-sdk-aws lane succeeded. /override ci/prow/hco-e2e-operator-sdk-gcp

kubevirt-bot commented 1 month ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-gcp, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2385820293): >hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure >hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure >hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure >hco-e2e-operator-sdk-aws lane succeeded. >/override ci/prow/hco-e2e-operator-sdk-gcp > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
hco-bot commented 1 month ago

hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded. /override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws hco-e2e-operator-sdk-sno-azure lane succeeded. /override ci/prow/hco-e2e-operator-sdk-sno-aws

kubevirt-bot commented 1 month ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws, ci/prow/hco-e2e-operator-sdk-sno-aws

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2386083138): >hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded. >/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws >hco-e2e-operator-sdk-sno-azure lane succeeded. >/override ci/prow/hco-e2e-operator-sdk-sno-aws > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
akrejcir commented 1 month ago

/retest

akrejcir commented 1 month ago

Can you please take another look?

hco-bot commented 1 month ago

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

kubevirt-bot commented 1 month ago

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

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2388337632): >hco-e2e-kv-smoke-gcp lane succeeded. >/override ci/prow/hco-e2e-kv-smoke-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
nunnatsa commented 3 weeks ago

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I suggest to just replace the default value to true.

nunnatsa commented 3 weeks ago

And @akrejcir - sorry for the late response.

akrejcir commented 3 weeks ago

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I suggest to just replace the default value to true.

Thanks for the feedback. I don't have any opinion on HCO API change. I think it is up to you to decide what should change. Then, I will update this PR.

tiraboschi commented 3 weeks ago

@akrejcir, @tiraboschi

I'm not sure about this PR.

Do we really need a new enabler field to replace the existing feature gate? The term "feature gate" is wrongly used in HCO anyway, and we need to treat the feature gates as "enablers". I think this change will just create mess.

We want to introduce a new API, to fix the wrong feature gate naming anyway.

I agree with this: the name of the feature gate stanza is misleading but it's already used for other configuration knobs. For the sake of consistency, let's continue using it until we will be able to move all of them with the next version of the API.

I suggest to just replace the default value to true.

We need to understand if we simply want to propose a new default for future fresh deployments or if we want also to "silently" change it for all the cluster on upgrades.

sonarcloud[bot] commented 2 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

akrejcir commented 2 weeks ago

I'm sorry for the confusion, I've removed most of the code from this PR. Now it only uses the new SSP API without changing the HCO API.

I also did not change the default value for the feature gate, because I incorrectly assumed that we want to enable it by default. In the acceptance criteria of the epic https://issues.redhat.com/browse/CNV-34204 , it says that vm-console-proxy should not be enabled by default.

hco-bot commented 2 weeks ago

hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

kubevirt-bot commented 2 weeks ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2416546041): >hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
hco-bot commented 2 weeks ago

hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. /override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

kubevirt-bot commented 2 weeks ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2416568740): >hco-e2e-consecutive-operator-sdk-upgrades-aws lane succeeded. >/override ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
hco-bot commented 2 weeks ago

hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

kubevirt-bot commented 2 weeks ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2416669666): >hco-e2e-upgrade-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
akrejcir commented 2 weeks ago

/retest

openshift-ci[bot] commented 2 weeks ago

@akrejcir: The following tests 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-consecutive-operator-sdk-upgrades-azure 4862950d3fca7a1d0b645954063f9a2dd89123c7 link true /test hco-e2e-consecutive-operator-sdk-upgrades-azure
ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure 4862950d3fca7a1d0b645954063f9a2dd89123c7 link false /test hco-e2e-upgrade-operator-sdk-sno-azure
ci/prow/hco-e2e-kv-smoke-azure 4862950d3fca7a1d0b645954063f9a2dd89123c7 link true /test hco-e2e-kv-smoke-azure

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
hco-bot commented 2 weeks ago

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

kubevirt-bot commented 2 weeks ago

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

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2416888723): >hco-e2e-kv-smoke-gcp lane succeeded. >/override ci/prow/hco-e2e-kv-smoke-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
hco-bot commented 2 weeks ago

hco-e2e-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-operator-sdk-sno-azure hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded. /override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

kubevirt-bot commented 2 weeks ago

@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure

In response to [this](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/3087#issuecomment-2417123529): >hco-e2e-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-operator-sdk-sno-azure >hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded. >/override ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure > Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
dominikholler commented 2 weeks ago

For the sake of consistency, let's continue using it until we will be able to move all of them with the next version of the API.

@tiraboschi is there already an upstream github issue or an downstream epic to track this?

tiraboschi commented 2 weeks ago

For the sake of consistency, let's continue using it until we will be able to move all of them with the next version of the API.

@tiraboschi is there already an upstream github issue or an downstream epic to track this?

Yes: https://issues.redhat.com/browse/CNV-47166 for the design phase, currently targeted to v4.18. and https://issues.redhat.com/browse/CNV-17501 for the actual implementation, targeted to v4.19 (this requires a proper support of conversion webhook mechanism in OLMv1 to avoid any disruption on upgrades).

tiraboschi commented 2 weeks ago

/approve

kubevirt-bot commented 2 weeks ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tiraboschi

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](https://github.com/kubevirt/hyperconverged-cluster-operator/blob/main/OWNERS)~~ [tiraboschi] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment