Allow excluding manifests from CVO payload by csrwng · Pull Request #252 · openshift/cluster-version-operator

csrwng · 2019-10-17T17:02:26Z

Allows specifying an identifier with the EXCLUDE_MANIFESTS environment variable that will be used to skip manifests that contain an annotation like exclude.release.openshift.io/<identifier>=true

csrwng · 2019-11-04T12:50:36Z

@crawford @abhinavdahiya this should be ready for a review. This is a first step towards introducing the concept of profiles where each component will declare what files go in a given profile. When that is introduced, these flags will be removed.

abhinavdahiya · 2019-11-04T22:13:05Z

this should be ready for a review. This is a first step towards introducing the concept of profiles where each component will declare what files go in a given profile.

Is there a backing enhancement that provides some context on how this change is on the path to profiles.

Who can set these cli flags? How do we find out about these flags are being set?
Setting these flags should probably make your clusters Not Upgradeable for now.

Also this change overlaps with what clusterversion.overrides do? Why do we this new feature when we can achieve the same thing with clusterversion API

abhinavdahiya · 2019-11-04T22:14:33Z

 	// updated by the run method only
 	payload *payload.Update
+
+	excludeFilter []string


why is this called a filter but is a string slice? this doesn't really provide context on how to use this to filter.

It's just a slice of regular expressions. I can add a comment explaining what they are.

maybe plural Filters?

abhinavdahiya · 2019-11-04T22:15:11Z

+	return false
+}
+
+func LoadUpdateWithFilters(dir, releaseImage string, excludes, includes []string) (*Update, error) {


the name is LoadUpdateWithFilters, but it doen't take any filters though...

It does (excludes, includes)

abhinavdahiya · 2019-11-04T22:16:20Z

Who can set these cli flags? How do we find out about these flags are being set?
Setting these flags should probably make your clusters Not Upgradeable for now.

if these flags are not supposed to be set by any Openshift 4 user, these should be behind a build time flag.

csrwng · 2019-11-05T06:53:18Z

Is there a backing enhancement that provides some context on how this change is on the path to profiles.

There isn't an enhancement proposal for profiles yet, I will create one. The target for profiles would be 4.4.

Who can set these cli flags? How do we find out about these flags are being set?

These flags would be set in the deployment for the CVO in a hosted environment, where the cvo runs as a pod in a management server but operates on a user cluster control plane (also hosted as pods on the management server)

Setting these flags should probably make your clusters Not Upgradeable for now.

Ack, will make that change.

Also this change overlaps with what clusterversion.overrides do? Why do we this new feature when we can achieve the same thing with clusterversion API

One of the benefits of hosted clusters is enabling cluster admin rights for users to their own cluster and making it hard for them to bring down the control plane on their own. Having the clusterversion resource (inside the user cluster) control which manifests get laid down puts too much of the responsibility on the user to keep it in a good working state.

csrwng · 2019-11-05T06:54:43Z

if these flags are not supposed to be set by any Openshift 4 user, these should be behind a build time flag.

The release is meant to be run unchanged in hosted mode, it won't get its own build.

csrwng · 2019-11-05T06:58:24Z

@abhinavdahiya thinking more about this.... since this is only a temporary enhancement that will be removed once we add a way to specify a profile, maybe instead of adding flags it should just be a set of env vars, wdyt?

csrwng · 2019-11-07T13:26:50Z

@abhinavdahiya I've now simplified this some and removed the includes part (less to remove later). We should only need to specify what to exclude. I've also removed the additional command flags and switched to reading environment variables to specify the regular expressions for manifest exclusion. I did not make the change to switch to not Upgradeable when these filters are specified because we will still need the CVO to apply upgrades to non-control plane components.

We need to get this in for the current release. Please let me know what else you'd like to see changed so we can get this merged.

/cc @derekwaynecarr

abhinavdahiya · 2019-11-08T00:05:27Z

Who can set these cli flags? How do we find out about these flags are being set?

These flags would be set in the deployment for the CVO in a hosted environment, where the cvo runs as a pod in a management server but operates on a user cluster control plane (also hosted as pods on the management server)

So that means the customer is not allowed to set/effect these values in external-hosted-profile.

but what about self-hosted-profile clusters, there the users have permission and access to modify their cluster-version-operator deployment to add these flags/envs. How do we prevent that or if we can't prevent that, how do we know the customer is doing this.

Also this change overlaps with what clusterversion.overrides do? Why do we this new feature when we can achieve the same thing with clusterversion API

One of the benefits of hosted clusters is enabling cluster admin rights for users to their own cluster and making it hard for them to bring down the control plane on their own. Having the clusterversion resource (inside the user cluster) control which manifests get laid down puts too much of the responsibility on the user to keep it in a good working state.

So the users are allowed to modify the clusterversion object in external-hosted-profile? which will affect how cvo behaves, and affect how it managed the cluster components, but we think using overrides section would be too much access to the user?

@abhinavdahiya I've now simplified this some and removed the includes part (less to remove later). We should only need to specify what to exclude. I've also removed the additional command flags and switched to reading environment variables to specify the regular expressions for manifest exclusion. I did not make the change to switch to not Upgradeable when these filters are specified because we will still need the CVO to apply upgrades to non-control plane components.

As I mentioned above, there is a chance users will set these values in self-hosted cases, and not knowing how/when these are being set is a blocker in my opinion.

We need to get this in for the current release.

current? 4.3 or 4.4 ?

Please let me know what else you'd like to see changed so we can get this merged.

/cc @derekwaynecarr

abhinavdahiya · 2019-11-08T00:07:14Z

+
+	// excludeManifestRegularExpressions is a set of regular expresions that are used
+	// to exclude manifest file names that match them.
+	excludeManifestRegularExpressions []string


why not store regex compiled here? compiling these regex every time is known to cause problems.

csrwng · 2019-11-08T10:31:32Z

but what about self-hosted-profile clusters, there the users have permission and access to modify their cluster-version-operator deployment to add these flags/envs. How do we prevent that or if we can't prevent that, how do we know the customer is doing this.

Users can do this today. They can modify the cvo deployment in all kinds of ways. They can specify an alternate image, alternate namespace or a different payload directory (https://github.com/openshift/cluster-version-operator/blob/master/pkg/start/start.go#L85-L87).
How are these variables different?

So the users are allowed to modify the clusterversion object in external-hosted-profile? which will affect how cvo behaves, and affect how it managed the cluster components, but we think using overrides section would be too much access to the user?

Yes they will be able to change the clusterversion resource and if they like, add additional overrides, but they can't add things that are already excluded on the management side.

As I mentioned above, there is a chance users will set these values in self-hosted cases, and not knowing how/when these are being set is a blocker in my opinion.

Again, not sure how it changes what you can do today. If you are privileged enough to change the cluster version operator deployment, then there's really not much we can do to stop you from shooting yourself in the foot.

current? 4.3 or 4.4 ?

4.3 (we have exception approval)

csrwng · 2019-11-08T15:49:14Z

Pushed update to parse regular expressions once on startup

csrwng · 2019-11-08T22:50:27Z

@abhinavdahiya I've submitted a change to make the cvo not upgradable when manifest excludes are set (verified that it doesn't affect upgrades for externally hosted mode). Please let me know if this addresses your concern.

abhinavdahiya · 2019-11-11T20:59:51Z

+		return nil
+	}
+	return &configv1.ClusterOperatorStatusCondition{
+		Type:    configv1.ClusterStatusConditionType("UpgradableManifestExcludes"),


https://github.com/openshift/cluster-version-operator/pull/252/files#diff-d9212d08828a9f9d8b54a2e90f31500aR195
the conditions need to start with Upgradeable

UpgradableManifestExcludes -> UpgradeableManifestExcludes

hmm, also this conditions names is not that great.

abhinavdahiya · 2019-11-11T21:02:41Z

+		Type:    configv1.ClusterStatusConditionType("UpgradableManifestExcludes"),
+		Status:  configv1.ConditionFalse,
+		Reason:  "ManifestExcludesSet",
+		Message: "Certain manifests are excluded from the operator's payload. The cluster cannot be upgraded.",


https://github.com/openshift/cluster-version-operator/pull/252/files#diff-d9212d08828a9f9d8b54a2e90f31500aR215

based on that, something like Excluding manifests from the release image prevents upgrades. Please remove the exclude filters before continuing.

I don't like regex on manifests, I'd like this to be a higher level name that manifests are associated with. If that's the case, then this shouldn't even be a condition because expectation is that they are tested and we don't need this alert.

abhinavdahiya · 2019-11-11T21:04:15Z

/retest

smarterclayton · 2019-11-11T21:07:06Z

+
+	// excludeManifestsFilter is a set of regular expresions that are used
+	// to exclude manifest file names that match them.
+	excludeManifestsFilter payload.RegexpFilter


I think the payload should be required to identify the manifests that match a string, rather than a regex. I do not like regex and I don't want arbitrary regular expressions.

ack, string matching should be sufficient for what we need, will switch to that

I think the payload should be required to identify the manifests that match a string...

I'm trying to catch up with the previous discussion in this PR, but if the folks setting this are creating release images, can't they just remove the manifests they don't want pushed? And if they aren't writing to release images, why is the existing overrides insufficient. Do we have the enhancement explaining how this is expected to fit together yet?

We don't want a separate release image for this case. The release image should be the same we use everywhere.

And no, the enhancement is not ready, since I think the design of profiles needs a little bit more discussion. The purpose of this PR is simply to a stop-gap in 4.3 until we get to profiles.

abhinavdahiya · 2019-11-13T19:19:30Z

Updated to use the single getOverrides method. Is that what you meant @abhinavdahiya ?

So the sync_work already has a list of overrides,

cluster-version-operator/pkg/cvo/sync_worker.go

Line 68 in 9cb26a3

Overrides []configv1.ComponentOverride

and this change adds another style of override/skip at the syncworker level.

cluster-version-operator/pkg/cvo/sync_worker.go

Lines 152 to 155 in 9cb26a3

    
           // exclude is an identifier used to determine which 
        
           // manifests should be excluded based on an annotation 
        
           // of the form exclude.release.openshift.io/<identifier>=true 
        
           exclude string

And the sync work can change at each sync, but the sync worker is only configured once at startup.
is that acceptable?

secondly, i think we lost the reporting excludes are set with the current change, until we have supported profiles I would like to make sure we know when users are setting this using env.

abhinavdahiya · 2019-11-13T19:19:57Z

cc @smarterclayton

does this look more in-line to what you were hoping?

csrwng · 2019-11-13T20:33:03Z

And the sync work can change at each sync, but the sync worker is only configured once at startup.
is that acceptable?

Yes I think that is acceptable because the exclude identifier will be statically configured for the cvo deployment, unlike overrides which can be modified via the api on the clusterversion CR.

secondly, i think we lost the reporting excludes are set with the current change, until we have supported profiles I would like to make sure we know when users are setting this using env.

I didn't add the condition back because of this comment from @smarterclayton

smarterclayton · 2019-11-13T21:27:36Z

Looks fine to me.

smarterclayton · 2019-11-13T21:28:46Z

 		PayloadOverride: os.Getenv("PAYLOAD_OVERRIDE"),
 		ResyncInterval:  minResyncPeriod,
 		EnableMetrics:   true,
+		Exclude:         os.Getenv("EXCLUDE_MANIFESTS"),


When exclude is set we should log this.

Excluding manifests for %q

in the main logs so we can see it if we have bugs.

csrwng · 2019-11-13T21:55:22Z

listing hosted zones: Throttling: Rate exceeded\n\tstatus code: 400

/retest

wking · 2019-11-13T23:04:48Z

4.3 (we have exception approval)

Is there a bug we can link from this PR for this exception?

wking · 2019-11-13T23:07:26Z

+	// exclude is an identifier used to determine which
+	// manifests should be excluded based on an annotation
+	// of the form exclude.release.openshift.io/<identifier>=true
+	exclude string


Do we want a single string, here or a []string of identifiers to exclude? Related to this thread.

@wking the idea is that the identifier would be the equivalent of a single profile. If you have a different exclusion profile, you should use a different identifier. Manifests in the individual operators can contain exclusion annotations for multiple profiles.

abhinavdahiya · 2019-11-14T03:51:42Z

/lgtm

openshift-ci-robot · 2019-11-14T03:51:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, csrwng

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [abhinavdahiya]

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

Configures the CVO to exclude manifests in deployments with external control planes See openshift/cluster-version-operator#252

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 17, 2019

openshift-ci-robot requested review from crawford and smarterclayton October 17, 2019 17:02

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 17, 2019

csrwng force-pushed the hosted branch from a4d3f76 to 7ca0364 Compare November 4, 2019 12:46

csrwng changed the title ~~WIP: Add flags to specify which manifests to exclude or include~~ Add flags to specify which manifests to exclude or include Nov 4, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2019

abhinavdahiya reviewed Nov 4, 2019

View reviewed changes

csrwng force-pushed the hosted branch from 7ca0364 to 8403f9c Compare November 7, 2019 12:48

csrwng changed the title ~~Add flags to specify which manifests to exclude or include~~ Allow excluding manifests from CVO payload Nov 7, 2019

openshift-ci-robot requested a review from derekwaynecarr November 7, 2019 13:26

abhinavdahiya reviewed Nov 8, 2019

View reviewed changes

csrwng force-pushed the hosted branch from 8403f9c to c67a9a1 Compare November 8, 2019 15:48

csrwng force-pushed the hosted branch from c67a9a1 to d855dbb Compare November 8, 2019 22:47

abhinavdahiya reviewed Nov 11, 2019

View reviewed changes

smarterclayton reviewed Nov 11, 2019

View reviewed changes

smarterclayton reviewed Nov 13, 2019

View reviewed changes

Use exclude identifier and annotation to skip certain manifests

e890ab6

csrwng force-pushed the hosted branch from 9cb26a3 to e890ab6 Compare November 13, 2019 21:40

wking reviewed Nov 13, 2019

View reviewed changes

openshift-ci-robot assigned abhinavdahiya Nov 14, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 14, 2019

openshift-merge-robot merged commit ac394f1 into openshift:master Nov 14, 2019

This was referenced Nov 25, 2019

Add manifest annotations for hosted deployment exclusions openshift/cluster-authentication-operator#220

Merged

Bug 1776421: Add manifest annotations for hosted deployment exclusions (4.3) openshift/cluster-authentication-operator#221

Merged

LalatenduMohanty mentioned this pull request May 22, 2020

Cluster Profiles openshift/enhancements#200

Merged

jonesbr17 mentioned this pull request Nov 1, 2021

Add exclusion annotation for hosted deployments openshift/cluster-cloud-controller-manager-operator#141

Closed

Conversation

csrwng commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csrwng commented Nov 4, 2019

Uh oh!

abhinavdahiya commented Nov 4, 2019

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!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Nov 4, 2019

Uh oh!

csrwng commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

csrwng commented Nov 5, 2019

Uh oh!

csrwng commented Nov 5, 2019

Uh oh!

csrwng commented Nov 7, 2019

Uh oh!

abhinavdahiya commented Nov 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented Nov 8, 2019

Uh oh!

csrwng commented Nov 8, 2019

Uh oh!

csrwng commented Nov 8, 2019

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!

abhinavdahiya commented Nov 11, 2019

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!

abhinavdahiya commented Nov 13, 2019

Uh oh!

abhinavdahiya commented Nov 13, 2019

Uh oh!

csrwng commented Nov 13, 2019

Uh oh!

smarterclayton commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csrwng commented Nov 13, 2019

Uh oh!

wking commented Nov 13, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Nov 14, 2019

Uh oh!

openshift-ci-robot commented Nov 14, 2019

csrwng commented Oct 17, 2019 •

edited

Loading

csrwng commented Nov 5, 2019 •

edited

Loading