-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix(CNV-29761): ensure that tekton crd exists #567
Conversation
I'm trying to solve an error that occurred here [1] in a job of [2] PR:
[1] https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/kubevirt_hyperconverged-cluster-operator/2349/pull-ci-kubevirt-hyperconverged-cluster-operator-main-hco-e2e-upgrade-prev-index-aws/1665732018995990528/artifacts/hco-e2e-upgrade-prev-index-aws/e2e-upgrade-test/artifacts/kubevirt-must-gather/quay-io-kubevirt-must-gather-sha256-28fe925c8305c0acda782029f983d241d688366dea7a9c83321db8d210402a3b/namespaces/kubevirt-hyperconverged/pods/ssp-operator-6bc677c64c-6z5r8/manager/manager/logs/current.log |
This PR is related to #566 as the error appears in the same log. From HCO CI:
|
/lgtm |
/test e2e-single-node-functests |
@codingben: No presubmit jobs available for kubevirt/ssp-operator@master In response to this:
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. |
We can also consider #570 as an alternative solution to that potential issue, since |
/test win11-pipeline-example-test |
@codingben: No presubmit jobs available for kubevirt/ssp-operator@master In response to this:
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. |
/test win11-pipeline-example-test |
@codingben: No presubmit jobs available for kubevirt/ssp-operator@master In response to this:
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. |
Yesterday I've reproduced that error in CNV QE cluster, it happened during CNV product removal process. After I've installed Tekton CRD, it's no longer complained about an error, so I'm pretty sure this is caused by defined |
// Solution to optional Tekton CRD is not implemented yet. | ||
// Until then, do not check if Tekton CRD exists. | ||
// | ||
// if !request.CrdList.CrdExists(tektonCrd) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason why not use this check instead of checking if feature gate is enabled? Does it not work?
ClusterTasks
should be removed on Cleanup()
even if the feature gate is disabled, @ksimon1 is that right?
When Tekton CRD does not exist then there are no ClusterTasks
to cleanup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, if crd does not exists, then there are no tasks / clusterTasks/ pipelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason why not use this check instead of checking if feature gate is enabled? Does it not work?
We're not watching Tekton CRD, because then it'll throw an error because it thinks it's required CRD and not an optional. Please see #561.
ClusterTasks should be removed on Cleanup() even if the feature gate is disabled, @ksimon1 is that right?
Yes, but then Tekton CRD is required. I assume that if the customer is using TTO, customer likely will have Tekton CRD already installed so the Feature Gate in ssp-operator will be enabled as well.
When Tekton CRD does not exist then there are no ClusterTasks to cleanup.
Yes, but we'll need to have !request.CrdList.CrdExists(tektonCrd)
for it, which doesn't work now because we don't have a solution to optional CRDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, CrdList
interface watches all CustomResourceDefinition
objects in the cluster. The method CrdList.CrdExists()
can be used to check if any CRD exists in the cluster.
In this case, we can use it to check if Tekton CRD is present, and only delete ClusterTasks
if it is.
We're not watching Tekton CRD, because then it'll throw an error because it thinks it's required CRD and not an optional. Please see #561.
We are not watching objects defined in the Tekton CRD. But we are watching the CRD itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're absolutely right. Let's use CrdList
interface then.
@@ -215,7 +227,7 @@ func (t *tektonTasks) Cleanup(request *common.Request) ([]common.CleanupResult, | |||
func listDeprecatedClusterTasks(request *common.Request) ([]pipeline.ClusterTask, error) { | |||
deprecatedClusterTasks := &pipeline.ClusterTaskList{} | |||
err := request.Client.List(request.Context, deprecatedClusterTasks, &client.MatchingLabels{ | |||
common.AppKubernetesManagedByLabel: common.AppKubernetesManagedByValue, | |||
common.AppKubernetesManagedByLabel: common.TektonAppKubernetesManagedByValue, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it makes sense to check for both values, please use In
operation on the label selector:
labelRequirement, err := labels.NewRequirement(common.AppKubernetesManagedByLabel,
selection.In, []string{common.TektonAppKubernetesManagedByValue, common.AppKubernetesManagedByValue})
if err != nil {
panic(err)
}
err = request.Client.List(request.Context, deprecatedClusterTasks, &client.MatchingLabelsSelector{
Selector: labels.NewSelector().Add(*labelRequirement),
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice piece of code! Thanks :) ssp-operator will not deploy ClusterTasks (just Tasks) as it was deprecated, only TTO was deploying ClusterTasks, so it makes sense to just use common.TektonAppKubernetesManagedByValue
.
/lgtm |
In the |
Checks if Tekton CRD is installed during Tekton operands reconciliation and during cleanup process when listing deprecated ClusterTasks. Signed-off-by: Ben Oukhanov <boukhanov@redhat.com>
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@akrejcir Yep, better to be safe than to be worried. Done. |
Thanks. |
/approve |
[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 |
/test win2k22-pipeline-example-test |
@codingben: No presubmit jobs available for kubevirt/ssp-operator@master In response to this:
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. |
What this PR does / why we need it:
Checks if Tekton CRD is installed during Tekton
operands reconciliation and during cleanup
process when listing deprecated ClusterTasks.
Release note: