Update catalog if image changes #816
Conversation
openshift-ci-robot
added
approved
|
/retest |
njhale
changed the title
|
/lgtm |
|
/test e2e-aws-olm |
|
/retest |
|
/hold /retest |
openshift-ci-robot
added
the
do-not-merge/hold
|
/test e2e-aws-console-olm |
|
/retest |
|
/retest |
ecordell
force-pushed
the
udpate-catalog
branch
from
d39ffc2 to
9661ced
Compare
openshift-ci-robot
added
size/L
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
|
/test unit |
|
/test e2e-aws-olm |
this was causing the operatorgroup test to hang
cleanup function was hanging, so explicitly clean up here
openshift-ci-robot
added
size/XL
| @@ -25,5 +25,8 @@ metadata: | |||
| rbac.authorization.k8s.io/aggregate-to-view: "true" | |||
| rules: | |||
| - apiGroups: ["operators.coreos.com"] | |||
| resources: ["clusterserviceversions", "catalogsources", "installplans", "subscriptions", "packagemanifests"] | |||
| resources: ["clusterserviceversions", "catalogsources", "installplans", "subscriptions", "operatorgroups"] | |||
There was a problem hiding this comment.
This fixes the ui issue where project admins can't view the olm ui.
| @@ -323,22 +323,6 @@ func (o *Operator) handleDeletion(obj interface{}) { | |||
| return | |||
| } | |||
|
|
|||
| // Check the registry server | |||
There was a problem hiding this comment.
Removed this - all we need is the requeue that's done right below, and the main catalog sync takes care of this.
| @@ -429,17 +413,22 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) { | |||
| logger.Debug("catsrc configmap state good, checking registry pod") | |||
| } | |||
|
|
|||
| reconciler := o.reconciler.ReconcilerForSource(catsrc) | |||
| if reconciler == nil { | |||
| srcReconciler := o.reconciler.ReconcilerForSource(catsrc) | |||
There was a problem hiding this comment.
name was shadowing the package name
| // TODO: Add failure status on catalogsource and remove from sources | ||
| return fmt.Errorf("no reconciler for source type %s", catsrc.Spec.SourceType) | ||
| } | ||
|
|
||
| healthy, err := srcReconciler.CheckRegistryServer(catsrc) |
There was a problem hiding this comment.
this is the bulk of the feature change :) everything else is fixing flakes in the tests.
| } | ||
| } | ||
|
|
||
| a.copyQueueIndexer.Enqueue(outCSV) | ||
| if !outCSV.IsUncopiable() { |
There was a problem hiding this comment.
check for uncopiable before we enqueue, so we don't have to check on the dequeue
| @@ -1234,11 +1190,11 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst | |||
| } | |||
|
|
|||
| // parseStrategiesAndUpdateStatus returns a StrategyInstaller and a Strategy for a CSV if it can, else it sets a status on the CSV and returns | |||
| func (a *Operator) parseStrategiesAndUpdateStatus(csv *v1alpha1.ClusterServiceVersion) (install.StrategyInstaller, install.Strategy, install.Strategy) { | |||
| func (a *Operator) parseStrategiesAndUpdateStatus(csv *v1alpha1.ClusterServiceVersion) (install.StrategyInstaller, install.Strategy) { | |||
There was a problem hiding this comment.
findIntermediatesForDeletion was the only thing that needed to have both strategies.
| ownedRole.SetOwnerReferences([]metav1.OwnerReference{ownerutil.NonBlockingOwner(targetCSV)}) | ||
| if err := ownerutil.AddOwnerLabels(ownedRole, targetCSV); err != nil { | ||
| targetRole := ownedRole.DeepCopy() | ||
| targetRole.SetResourceVersion("0") |
There was a problem hiding this comment.
the resourceversion being set was causing creates to fail
honestly not sure why this was a flake and not just a flat out failure. I meant to go back and see why this ever worked.
There was a problem hiding this comment.
This makes me think we need some kind of "SafeDeepCopy".
There was a problem hiding this comment.
That's a good idea - like a CopyForCopy vs. CopyForUpdate
| @@ -242,7 +242,7 @@ func (a *Operator) requirementStatus(strategyDetailsDeployment *install.Strategy | |||
| } | |||
|
|
|||
| // permissionStatus checks whether the given CSV's RBAC requirements are met in its namespace | |||
| func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, csvNamespace string) (bool, []v1alpha1.RequirementStatus, error) { | |||
| func (a *Operator) permissionStatus(strategyDetailsDeployment *install.StrategyDetailsDeployment, ruleChecker install.RuleChecker, targetNamespace, serviceAccountNamespace string) (bool, []v1alpha1.RequirementStatus, error) { | |||
There was a problem hiding this comment.
another bug - the targetnamespace and serviceaccount namespace are different for multinamespace. again, not sure why this flaked instead of failing always.
(it looks like it would fail always, but resync would fix, so any test with MultiNamespace would be very slow)
|
|
||
| aCSV := newCSV(csvName, opGroupNamespace, "", semver.MustParse("0.0.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, namedStrategy) | ||
| createdCSV, err := crc.OperatorsV1alpha1().ClusterServiceVersions(opGroupNamespace).Create(&aCSV) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
the lack of ownerlabels meant the copying would fail
| @@ -369,24 +369,28 @@ func cleanupOLM(t *testing.T, namespace string) { | |||
| var err error | |||
| err = waitForEmptyList(func() (int, error) { | |||
| res, err := crc.OperatorsV1alpha1().ClusterServiceVersions(namespace).List(metav1.ListOptions{FieldSelector: nonPersistentCSVFieldSelector}) | |||
| t.Logf("%d %s remaining", len(res.Items), "csvs") | |||
There was a problem hiding this comment.
saw some cases where namespace cleanup failed, this helps to find those instances.
|
/test e2e-aws-console-olm |
|
/retest |
5 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest |
| @@ -914,7 +903,7 @@ func (o *Operator) createInstallPlan(namespace string, subs []*v1alpha1.Subscrip | |||
| func (o *Operator) requeueSubscription(name, namespace string) { | |||
| // we can build the key directly, will need to change if queue uses different key scheme | |||
| key := fmt.Sprintf("%s/%s", namespace, name) | |||
| o.subQueue.AddRateLimited(key) | |||
| o.subQueue.Add(key) | |||
There was a problem hiding this comment.
We shouldn't really need to rate limit our internal requeues - otherwise we can end up DOSing ourselves. In general we should only requeue when we have work to do
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alecmerdler, ecordell, jpeeler, tkashem 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 |
| @@ -418,6 +428,10 @@ func (a *Operator) ensureTenantRBAC(operatorNamespace, targetNamespace string, c | |||
| return err | |||
| } | |||
|
|
|||
| if len(ownedRoles) == 0 { | |||
There was a problem hiding this comment.
What makes this worth checking, but not the targetCSV? Perhaps this PR obsoletes #837, but the above was the biggest cache issue I saw.
|
/retest |
|
@ecordell: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. |
No description provided.