Bug 1907329: Add cluster profile support #404

guillaumerose · 2020-07-09T12:31:24Z

Implementation of openshift/enhancements#200

This adds a CLUSTER_PROFILE env variable that is used to determine which manifests to apply based on the include.release.openshift.io/ annotation.

It is exactly what is written in the enhancement, not more.

This env. variable is not used by any components and, as it is empty, it defaults to the value of the default profile self-managed-high-availability.

vrutkovs · 2020-07-22T14:51:40Z

AWS failed to bootstrap

/retest

guillaumerose · 2020-07-22T15:20:40Z

@vrutkovs yes it is because I didn't solve the issue of "where we should store the profile". Currently, the CVO expects to find it in a ConfigMap and it is still created by hand. Do you think it's a good idea to read it from a ConfigMap ? There is a chicken and egg issue with this: if the ConfigMap is not present, the CVO cannot run but the ns openshift-config is created by the CVO.

vrutkovs · 2020-07-22T15:22:57Z

CVO should fall back to default profile (so that upgrade would succeed) if the configmap is not present

guillaumerose · 2020-07-22T15:27:22Z

OK, so I will change this PR to:

load the ConfigMap as soon as the CVO starts.
if the ConfigMap doesn't exist, use the default profile.

It will be the responsibility of the installer to have the ConfigMap in place before the CVO runs for the first time. Does it sound good ?

guillaumerose · 2020-07-22T19:58:10Z

I experimented a bit more and found an interesting problem by making optional the ConfigMap.

During the boostrap, the CVO is created via a bootstrap-manifest and it runs before manifests are created (located in /manifests - even before the first one 0000_00_cluster-version-operator_00_namespace.yaml). It could not find the ConfigMap and therefore load the default profile.
Couple of minutes later, bootstrap ends, the ConfigMap is in place, the CVO reboot in the master and it's fine. But there was a period of time where the profile was not the good one.

Any thoughts on this ? How can we make the CVO waiting for the ConfigMap but still having the choice to not have it ?
Isn't it a problem to have the CVO in the bootstrap node not running the right profile ?

Perhaps if we use the ClusterVersion type to hold the profile, we will have the ConfigMap always defined.

vrutkovs · 2020-07-23T09:12:06Z

But there was a period of time where the profile was not the good one.

I'm not sure I'm following - why installed profile is incorrect? I assumed that no profile specified == "default" profile. If its not set then all operators (unless explicitly excluded) are enabled

guillaumerose · 2020-07-23T11:51:21Z

I wanted to change the behavior to load once the profile at startup but it doesn't work. CVO is launched before manifests are created. At start the ClusterVersion object doesn't exist and locks the CVO.

To work correctly with a profile, the user have to ensure the ConfigMap is created before the ClusterVersion. For snc, I did something like:

openshift-install create manifests

cp myns.yaml ${INSTALL_DIR}/openshift/0000_00_cluster-version-operator_00_myns.yaml
cp myconfigmap.yaml ${INSTALL_DIR}/openshift/0000_00_cluster-version-operator_00_myconfigmap.yaml

openshift-install create cluster

guillaumerose · 2020-07-23T17:52:38Z

/retest

guillaumerose · 2020-07-24T07:06:27Z

/retest

guillaumerose · 2020-07-24T09:03:16Z

/retest

jottofar · 2020-07-24T12:35:26Z

/test e2e-gcp-upgrade

jottofar · 2020-07-24T12:36:40Z

/retest

openshift-ci-robot · 2020-07-24T12:38:18Z

@jottofar: The specified target(s) for /test were not found.
The following commands are available to trigger jobs:

/test e2e
/test e2e-upgrade
/test images
/test integration
/test unit

Use /test all to run all jobs.

Details

In response to this:

/test e2e-gcp-upgrade

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.

guillaumerose · 2020-07-27T09:19:53Z

/retest

csrwng

Looks fine to me, just a couple of comments

pkg/cvo/sync_worker.go

csrwng · 2020-07-27T13:33:58Z

pkg/cvo/sync_worker.go

 }

+type ClusterProfileRetriever interface {
+	RetrieveClusterProfile(ctx context.Context) string


You should return an error in this function. It's probably safer to fail in the case you can't read the profile than to assume "default"

Just to clarify, you can check the error for IsNotFound and return default... however other errors probably indicate an api server error and you should probably report.

yes you're right, it's better now.
We can see in the log now: I0727 15:08:48.205731 1 updatepayload.go:336] Cannot get ConfigMap openshift-config/cluster-profile, using default profile: configmap "cluster-profile" not found

csrwng · 2020-07-27T13:35:05Z

pkg/cvo/updatepayload.go

+func (r *clusterProfileRetriever) RetrieveClusterProfile(_ context.Context) string {
+	configMap, err := r.cmConfigLister.Get(internal.ClusterProfileConfigMap)
+	if err != nil {
+		klog.Infof("cannot get configmap openshift-config/cluster-profile, using default profile")


see comment for the interface

pkg/cvo/sync_worker.go

pkg/cvo/updatepayload.go

LalatenduMohanty · 2020-12-16T14:33:38Z

/test unit

wking · 2020-12-16T17:15:42Z

/hold

Blocked by operator-framework/operator-lifecycle-manager#1887 to label the InstallPlans CRD manifest (among other things) to avoid cannot list resource "installplans" in API group "operators.coreos.com" in the namespace "openshift".

LalatenduMohanty

/lgtm

guillaumerose · 2021-01-04T16:51:39Z

/retest

rkukura · 2021-01-04T16:52:23Z

/retest

guillaumerose · 2021-01-04T19:29:57Z

/retest

guillaumerose · 2021-01-05T07:36:35Z

/retest

guillaumerose · 2021-01-05T13:23:35Z

/retest

guillaumerose · 2021-01-05T19:18:10Z

/retest

guillaumerose · 2021-01-05T21:08:54Z

/retest

guillaumerose · 2021-01-06T07:41:19Z

/retest

guillaumerose · 2021-01-06T19:35:52Z

/retest

This is matches openshift/enhancements#510 and doesn't change existing behavior

wking · 2021-01-11T18:05:36Z

/hold cancel

I'd held this for operator-framework/operator-lifecycle-manager#1887, which has since landed.

LalatenduMohanty

/lgtm

openshift-ci-robot · 2021-01-11T20:03:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: guillaumerose, LalatenduMohanty

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

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

openshift-ci-robot · 2021-01-11T20:20:35Z

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

Bugzilla bug 1907329 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1907329: Add cluster profile support

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.

openshift-ci-robot requested review from LalatenduMohanty and jottofar July 9, 2020 12:31

This was referenced Jul 10, 2020

WIP: Add a ConfigMap with cluster profile openshift/installer#3814

Closed

Cluster Profiles openshift/enhancements#200

Merged

guillaumerose changed the title ~~PoC: add cluster profile support~~ Add cluster profile support Jul 21, 2020

guillaumerose force-pushed the profilepoc branch from bd95493 to 947807a Compare July 23, 2020 11:17

guillaumerose force-pushed the profilepoc branch from 947807a to 4f8a2b8 Compare July 23, 2020 13:01

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 25, 2020

guillaumerose force-pushed the profilepoc branch from 4f8a2b8 to 0679960 Compare July 27, 2020 07:17

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 27, 2020

csrwng reviewed Jul 27, 2020

View reviewed changes

guillaumerose force-pushed the profilepoc branch 2 times, most recently from 494461c to a84ec7a Compare July 27, 2020 14:27

wking reviewed Jul 27, 2020

View reviewed changes

pkg/cvo/sync_worker.go Outdated Show resolved Hide resolved

wking reviewed Jul 27, 2020

View reviewed changes

pkg/cvo/sync_worker.go Outdated Show resolved Hide resolved

wking reviewed Jul 27, 2020

View reviewed changes

pkg/cvo/updatepayload.go Outdated Show resolved Hide resolved

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2020

LalatenduMohanty approved these changes Dec 16, 2020

View reviewed changes

openshift-ci-robot assigned LalatenduMohanty Dec 16, 2020

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 16, 2020

openshift-ci-robot mentioned this pull request Jan 4, 2021

Bug 1907329: Update /manifests with default cluster profile annotation operator-framework/operator-lifecycle-manager#1887

Merged

guillaumerose added 2 commits January 11, 2021 10:17

add current profile annotations to CVO manifests

d4700fb

This is matches openshift/enhancements#510 and doesn't change existing behavior

Use profile from environment variable CLUSTER_PROFILE

8f4d36c

guillaumerose force-pushed the profilepoc branch from 48fa4e9 to 8f4d36c Compare January 11, 2021 09:21

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 11, 2021

LalatenduMohanty approved these changes Jan 11, 2021

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 11, 2021

openshift-merge-robot merged commit 9f50a07 into openshift:master Jan 11, 2021

csrwng mentioned this pull request Apr 6, 2021

Bug 1948714: IBM Cloud manifest profile patch openshift/cluster-image-registry-operator#643

Merged

csrwng mentioned this pull request Jul 20, 2021

IBM Cloud manifest profile patch for operator deployment openshift/cluster-node-tuning-operator#252

Merged

Bug 1907329: Add cluster profile support #404

Bug 1907329: Add cluster profile support #404

Uh oh!

Conversation

guillaumerose commented Jul 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrutkovs commented Jul 22, 2020

Uh oh!

guillaumerose commented Jul 22, 2020

Uh oh!

vrutkovs commented Jul 22, 2020

Uh oh!

guillaumerose commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guillaumerose commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vrutkovs commented Jul 23, 2020

Uh oh!

guillaumerose commented Jul 23, 2020

Uh oh!

guillaumerose commented Jul 23, 2020

Uh oh!

guillaumerose commented Jul 24, 2020

Uh oh!

guillaumerose commented Jul 24, 2020

Uh oh!

jottofar commented Jul 24, 2020

Uh oh!

jottofar commented Jul 24, 2020

Uh oh!

openshift-ci-robot commented Jul 24, 2020

Uh oh!

guillaumerose commented Jul 27, 2020

Uh oh!

csrwng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

csrwng Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

csrwng Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

guillaumerose Jul 27, 2020 • 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!

csrwng Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

guillaumerose Jul 27, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LalatenduMohanty commented Dec 16, 2020

Uh oh!

wking commented Dec 16, 2020

Uh oh!

LalatenduMohanty left a comment

Choose a reason for hiding this comment

Uh oh!

guillaumerose commented Jan 4, 2021

Uh oh!

rkukura commented Jan 4, 2021

Uh oh!

guillaumerose commented Jan 4, 2021

Uh oh!

guillaumerose commented Jan 5, 2021

Uh oh!

guillaumerose commented Jan 5, 2021

Uh oh!

guillaumerose commented Jan 5, 2021

Uh oh!

guillaumerose commented Jan 5, 2021

guillaumerose commented Jul 9, 2020 •

edited

Loading

guillaumerose commented Jul 22, 2020 •

edited

Loading

guillaumerose commented Jul 22, 2020 •

edited

Loading

guillaumerose Jul 27, 2020 •

edited

Loading