OCPBUGS-858: Package Server Manager should enforce expected csv values #378

awgreene · 2022-09-06T21:23:45Z

Problem:
The package-server-manifest (PSM) is a downstream specific component responsible for reconciling the package-server csv, which is defined here as a yaml file. When the PSM reconciles the package-server csv, it:

Attempts to generate the base of the expected CSV here
Passes the reconcile function as the mutateFn parameter and the expectedCSV as the obj parameter into into controller-runtime's createOrUpdate function. Controller runtime's createOrUpdate function will apply the mutateFn function against:
The obj you passed in if it does not already exist.
OR
The existing on cluster object.

In the case where the mutateFn is applied to the applied to the existing object, changes made to the package-server csv yaml are ignored because the mutateFn (ie the reconcileCSV function) doesn't consider changes to the manifest.

Solution:
Update the reconcileCSV function to:

Ensure that expected annotations and labels are present on the csv.
Ensure that the expected spec is present on the csv.

openshift-ci-robot · 2022-09-06T21:24:05Z

@awgreene: This pull request references Jira Issue OCPBUGS-867, which is invalid:

expected the bug to target the "4.12.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

…o the packageserver manifest

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.

awgreene · 2022-09-06T21:25:10Z

/jira refresh

openshift-ci-robot · 2022-09-06T21:25:16Z

@awgreene: This pull request references Jira Issue OCPBUGS-867, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.12.0) matches configured target version for branch (4.12.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

In response to this:

/jira refresh

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.

awgreene · 2022-09-06T21:25:31Z

WIP
/hold

openshift-ci-robot · 2022-09-06T21:29:16Z

@awgreene: This pull request references Jira Issue OCPBUGS-858, which is invalid:

expected the bug to target the "4.12.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

…o the packageserver manifest

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.

awgreene · 2022-09-06T21:30:45Z

/jira refresh

openshift-ci-robot · 2022-09-06T21:30:50Z

@awgreene: This pull request references Jira Issue OCPBUGS-858, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.12.0) matches configured target version for branch (4.12.0)
bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

In response to this:

/jira refresh

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.

awgreene · 2022-09-07T18:49:58Z

/retest

tylerslaton

/lgtm

tylerslaton · 2022-09-07T18:50:42Z

pkg/package-server-manager/controller.go

 		log.V(3).Info("no further updates are necessary to the packageserver csv")
 	}

+	// Ensure defaults


nit: Maybe expand a bit more on what it means to ensure these defaults here

openshift-ci · 2022-09-07T18:51:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: awgreene, tylerslaton

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

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

grokspawn · 2022-09-08T13:08:26Z

/hold until we get the qe-approved label

anik120 · 2022-09-08T13:17:30Z

I wonder if there's a way to launch a 4.9 cluster with this PR. It'd be ideal to do an upgrade testing from 4.9->4.12 with this PR, and make sure that for each OCP version, the correct CSV version is being deployed on upgrade.

perdasilva · 2022-09-08T13:51:00Z

pkg/package-server-manager/controller.go

 	}
-	if !ensureCSV(log, image, csv, highAvailabilityMode) {
+
+	if modified {


did you mean if !modified?

Yes, updated.

perdasilva · 2022-09-08T13:55:03Z

pkg/package-server-manager/controller_test.go

 	tt := []struct {
 		name            string
 		inputCSV        *olmv1alpha1.ClusterServiceVersion
 		expectedCSV     *olmv1alpha1.ClusterServiceVersion
 		highlyAvailable bool
-		want            bool
+		want            wanted


all test cases seem to set expectedErr to nil. Is it worth exercising the failure path as well?
Seems like it only happens when there are issues deserializing the csv

That's correct, all tests expect no error from this method. The request to add an error testcase seems reasonable, but the only instance where the ensureCSV function can return an error is when the manifests.NewPackageServerCSV function has an error unmarshalling the packageserver.yaml, which is hardcoded (as it should be IMO). While we could mock the function or update it to take a configurable path, it seemed like overkill and I didn't want to increase the scope of this PR given that it will need to be backported to 4.9.

If you feel strongly that we should test this path, I can update it when I'm back from vacation next week.

my preference would be to not increase the scope of this PR either, specially with the need to backport in mind. We could always have a follow up PR for the test

awgreene

Last minute additions

awgreene · 2022-09-08T19:02:31Z

pkg/package-server-manager/controller_test.go

+		csv.Spec.InstallStrategy.StrategySpec.DeploymentSpecs[0].Spec.Template.Spec.Affinity = affinity
+	}
+}
+func withRolloutStrategy(strategy *appsv1.RollingUpdateDeployment) func(*olmv1alpha1.ClusterServiceVersion) {


Shoot, @grokspawn could you change this to the name of the field?

Maybe withRollingUpdateStrategy? Open to suggestions

How about withRollingUpdateStrategy?

Funny that your comment came in after mine, but at least great minds think alike. :)

awgreene · 2022-09-08T19:06:47Z

pkg/package-server-manager/config.go

+
+	var modified bool
+
+	for k, v := range expectedCSV.GetLabels() {


We should add a check that the csv's annotations and label maps are not nil @grokspawn. If they are nil, set them to new maps and populate with the following for loops.

Something like

'''
if csv.GetAnnotations() == nil {
csv.SetAnnotations(make(map[string]string))
modified = true
}
'''

Could add a check if the expectedCSV has annotations, but it always should.

**Problem:** The package-server-manifest (PSM) is a downstream specific component responsible for reconciling the package-server csv, which is defined in code as a yaml file. When the PSM reconciles the package-server csv, it: - Attempts to generate the base of the expected CSV from the yaml mentioned above. - Passes the reconcileCSV function as the mutateFn parameter and the expectedCSV as the `obj` parameter into into controller-runtime's createOrUpdate function. Controller runtime's createOrUpdate function will apply the mutateFn function against: - The `obj` you passed in if it does not already exist. OR - The existing on cluster object. In the case where the mutateFn is applied to the applied to the existing object, changes made to the package-server csv yaml are ignored because the mutateFn (ie the reconcileCSV function) doesn't consider changes to the manifest. **Solution:** Update the reconcileCSV function to: - Ensure that expected annotations and labels are present on the csv. - Ensure that the expected spec is present on the csv.

grokspawn · 2022-09-08T19:55:26Z

@ qe, please take a look at the reproduction steps in OCPBUGS-867 for a (relatively) easy verification.

anik120 · 2022-09-09T01:17:57Z

/test e2e-gcp-ovn

openshift-ci · 2022-09-09T03:12:34Z

@awgreene: all tests passed!

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.

jianzhangbjz · 2022-09-09T09:09:53Z

I wonder if there's a way to launch a 4.9 cluster with this PR. It'd be ideal to do an upgrade testing from 4.9->4.12 with this PR, and make sure that for each OCP version, the correct CSV version is being deployed on upgrade.

Yes, but I guess no. In fact, I build a payload with this PR by using cluster-bot, and upgrade OCP 4.10.32 to it forcefully, it works well.

bandrade · 2022-09-09T14:25:16Z

/label qe-approved

dinhxuanvu

/lgtm

grokspawn · 2022-09-09T14:42:47Z

/hold cancel
got the qe-approved label 😄

openshift-ci-robot · 2022-09-09T14:46:34Z

@awgreene: All pull requests linked via external trackers have merged:

openshift/operator-framework-olm#378

Jira Issue OCPBUGS-858 has been moved to the MODIFIED state.

In response to this:

Problem:
The package-server-manifest (PSM) is a downstream specific component responsible for reconciling the package-server csv, which is defined here as a yaml file. When the PSM reconciles the package-server csv, it:

Attempts to generate the base of the expected CSV here

Passes the reconcile function as the mutateFn parameter and the expectedCSV as the obj parameter into into controller-runtime's createOrUpdate function. Controller runtime's createOrUpdate function will apply the mutateFn function against:

The obj you passed in if it does not already exist.
OR

The existing on cluster object.

In the case where the mutateFn is applied to the applied to the existing object, changes made to the package-server csv yaml are ignored because the mutateFn (ie the reconcileCSV function) doesn't consider changes to the manifest.

Solution:
Update the reconcileCSV function to:

Ensure that expected annotations and labels are present on the csv.

Ensure that the expected spec is present on the csv.

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.

jianzhangbjz · 2022-09-09T15:04:05Z

LGTM, details see: https://issues.redhat.com/browse/OCPBUGS-867 and https://issues.redhat.com/browse/OCPBUGS-858

anik120 · 2022-09-09T18:30:40Z

/cherry-pick release-4.11

for https://issues.redhat.com/browse/OCPBUGS-1103

openshift-cherrypick-robot · 2022-09-09T18:31:35Z

@anik120: new pull request created: #381

In response to this:

/cherry-pick release-4.11

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.

awgreene changed the title ~~Fixes an issue where package-server-manager doesn't respect changes t…~~ OCPBUGS-867: Fixes an issue where package-server-manager doesn't respect changes t… Sep 6, 2022

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Sep 6, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 6, 2022

openshift-ci bot requested review from exdx and gallettilance September 6, 2022 21:25

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2022

awgreene changed the title ~~OCPBUGS-867: Fixes an issue where package-server-manager doesn't respect changes t…~~ OCPBUGS-858: Fixes an issue where package-server-manager doesn't respect changes t… Sep 6, 2022

tylerslaton reviewed Sep 7, 2022

View reviewed changes

tylerslaton approved these changes Sep 7, 2022

View reviewed changes

openshift-ci bot assigned tylerslaton Sep 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 8, 2022

perdasilva reviewed Sep 8, 2022

View reviewed changes

awgreene force-pushed the fix-psm-update-ps branch from ba3ee3a to aa14628 Compare September 8, 2022 13:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2022

perdasilva reviewed Sep 8, 2022

View reviewed changes

awgreene force-pushed the fix-psm-update-ps branch from aa14628 to dd4850f Compare September 8, 2022 13:56

grokspawn force-pushed the fix-psm-update-ps branch from dd4850f to e144bda Compare September 8, 2022 18:37

awgreene commented Sep 8, 2022

View reviewed changes

grokspawn force-pushed the fix-psm-update-ps branch from e144bda to 2bb3d74 Compare September 8, 2022 19:52

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Sep 9, 2022

dinhxuanvu reviewed Sep 9, 2022

View reviewed changes

openshift-ci bot assigned dinhxuanvu Sep 9, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2022

openshift-merge-robot merged commit 208ce9a into openshift:master Sep 9, 2022

openshift-cherrypick-robot mentioned this pull request Sep 9, 2022

[release-4.11] OCPBUGS-1104: Package Server Manager should enforce expected csv values #381

Merged


		var modified bool

		for k, v := range expectedCSV.GetLabels() {

OCPBUGS-858: Package Server Manager should enforce expected csv values #378

OCPBUGS-858: Package Server Manager should enforce expected csv values #378

Uh oh!

Conversation

awgreene commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 6, 2022

Uh oh!

awgreene commented Sep 6, 2022

Uh oh!

openshift-ci-robot commented Sep 6, 2022

Uh oh!

awgreene commented Sep 6, 2022

Uh oh!

openshift-ci-robot commented Sep 6, 2022

Uh oh!

awgreene commented Sep 6, 2022

Uh oh!

openshift-ci-robot commented Sep 6, 2022

Uh oh!

awgreene commented Sep 7, 2022

Uh oh!

tylerslaton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Sep 7, 2022

Uh oh!

grokspawn commented Sep 8, 2022

Uh oh!

anik120 commented Sep 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awgreene Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awgreene left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awgreene Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awgreene Sep 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grokspawn commented Sep 8, 2022

Uh oh!

anik120 commented Sep 9, 2022

Uh oh!

openshift-ci bot commented Sep 9, 2022

Uh oh!

jianzhangbjz commented Sep 9, 2022

Uh oh!

bandrade commented Sep 9, 2022

Uh oh!

dinhxuanvu left a comment

Choose a reason for hiding this comment

Uh oh!

awgreene commented Sep 6, 2022 •

edited

Loading

awgreene Sep 8, 2022 •

edited

Loading

awgreene Sep 8, 2022 •

edited

Loading

awgreene Sep 8, 2022 •

edited

Loading

anik120 commented Sep 9, 2022 •

edited

Loading