Bug 1741786: pkg/cvo: Drop ClusterVersion defaulting during bootstrap

[wking]

This avoids a race between:

• The cluster-version operator pushing its internally-generated default and
• The cluster-bootstrap process pushing the installer-generated ClusterVersion into the cluster (rhbz#1741786)

Because cluster-bootstrap does not override already-existing objects, when the CVO's default won that race, the cluster would come up with a bogus channel (fast, instead of the installer's default stable-4.2, etc.) and clusterID (causing the reported Telemetry to come in under a different UUID than the one the installer provided in its metadata.json output).

By removing the default, the CVO will continue applying the current version and reporting some metrics like cluster_version{type="current"}. But it will not report metrics that depend on the state stored in the ClusterVersion object (past versions, cluster_version{type="failure"}, etc.). And even the metrics it does provide may not make it up to the Telemetry API because the monitoring operator will not be able to pass the Telemeter container the expected clusterID. I'm not clear on how the Telemeter config is updated when the ClusterVersion's clusterID does change because of this guard, but the monitoring operator is willing to continue without it. The monitoring operator only updates the Telemeter Deployment to set (or update) the cluster ID; it never clears an existing cluster ID.

While we won't get Telemetry out of the cluster until the cluster-bootstrap process (or somebody else) pushes the ClusterVersion in, we can still alert on it locally. I haven't done that in this PR, because #232 is in flight touching this space.

The defaulting logic dates back to the early CVOConfig work in ea678b1 (#2). @smarterclayton expressed concern about recovering from ClusterVersion deletion without the default, but:

• Who deletes their ClusterVersion?

• Even if someone wanted to delete their ClusterVersion, we'll want to eventually make the CVO an admission gate for ClusterVersion changes (ab4d84a). So (once we set up that admission gate), someone determined to remove ClusterVersion would have to disable that admission config before they could remove the ClusterVersion. That seems like enough steps that you wouldn't blow ClusterVersion away by accident.

• If you do successfully remove your ClusterVersion, you're going to lose all the status and history it contained. If you scale down your CVO and then remove your ClusterVersion, it's going to be hard to recover that lost information. If you left the CVO running, we could presumably cache the whole thing in memory and push it back into the cluster. That would at least avoid the "compiled-in CVO defaults differ from user's choices" issue. But we'd still be fighting with the user over the presence of ClusterVersion, and I'd rather not.

• Telemetry will continue pushing reports with the last-seen clusterID, so unless you disabled Telemetry first, future "ClusterVersion is missing" alerts (not added in this commit, see earlier paragraph mentioning alerts) would make it out to let us know that something was being silly.

I'm pushing this up to get feedback on the approach (CC @abhinavdahiya). The WIP is because I haven't updated the unit test to handle the lack of defaulting.

[openshift-ci-robot]

@wking: This pull request references a valid Bugzilla bug. 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.

Details

In response to this:

WIP: Bug 1741786: pkg/cvo: Drop ClusterVersion defaulting

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]

@wking: This pull request references a valid Bugzilla bug.

Details

In response to this:

WIP: Bug 1741786: pkg/cvo: Drop ClusterVersion defaulting

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.

[wking]

Pulled the WIP, because 26e83b2 passes unit tests locally for me. We also have a few rounds of green e2e-aws(-upgrade). I dunno what's going on with the two TestIntegrationCVO_* failures yet. [edit: ah, they're guarded by TEST_INTEGRATION].

[wking]

All green :). @abhinavdahiya, @smarterclayton, please take a look.

[smarterclayton]

/hold

Why are we creating cluster version during install and why do we have to? Your description alluded to it but didn’t lay out the current justifications. Originally CV was moving to not installer created

[smarterclayton]

To explore the counter, why does channel need to be set by installer? Metadata.json having it is a bit weird.

The CVO shouldn’t be syncing anything out of the payload until it has written the CV, so the bug here seems to be with bootstrap using idempotent create for objects instead of idenpotent create or update?

[smarterclayton]

Also alternatively, perhaps the bootstrap process should be passing intiital cluster id and channel to cvo to create bootstrap deployment,

[wking]

Why are we creating cluster version during install and why do we have to?

Historically, this was used to override broken components, but we haven't needed to do that since openshift/installer#851. Although obviously we could have been managing that from this repo instead of managing it via the installer repo.

Metadata.json having it is a bit weird.

Yeah. In fact, ClusterVersion having it is a bit weird ;). If it's just for telemetry, shouldn't it just be in the Telemetry config? Is telemetry/monitoring represented by a custom resource? Nothing stands out to me in the API. Maybe configuration is via a ConfigMap?

At the moment, Hive is using the telemetry from metadata.json to hold space for "coming OSD cluster". It could pull the Telemetry ID put of the running cluster instead, but then OSD-ness would race cluster Telemetry reporting vs. Hive's extraction. Until we get OSD-ness via the cluster_installer metrics (openshift/installer#1890 #213), which may be where we want it long-term (we'd still need a way to associate Hive-reported admin creds with the appropriate Telemetry ID).

The CVO shouldn’t be syncing anything out of the payload until it has written the CV, so the bug here seems to be with bootstrap using idempotent create for objects instead of idenpotent create or update?

I don't like multiple parties having opinions about cluster config specs. Components that have opinions about their own config could represent their defaults in status, but should stay out of spec, which should be admin-managed for these top-level configs (and installer-managed as a proxy for the admin at install-time). I'd be fine with cluster-bootstrap clobbering spec (repeatedly?) until all resource specs matched its expectations, but that's a reasonably large change vs. its current behavior.

Also alternatively, perhaps the bootstrap process should be passing intiital cluster id and channel to cvo to create bootstrap deployment...

As in "but not via ClusterVersion"? I'm fine just leaving channel/upstream defaulting to the CVO. And if we adjust Telemetry ID injection, I'd rather move it to an in-cluster Telemetry custom resource.

[smarterclayton]

To the last questions, "no", it should be as "tell the bootstrap CVO command how you want to generate its initial resources via arguments". Or pass the default CV object to bootstrap CVO command. Or inject the CV immediately after the CRD is created. There are lots of options for taking the data the installer has and ensuring the CVO gets to handle the appropriate ordering. I think cluster-bootstrap is insufficiently intelligent (or the bootstrap commands it invokes are insufficiently intelligent). Passing those flags to CVO bootstrap seems the closest to actual intent.

…

On Sun, Aug 18, 2019 at 10:02 PM W. Trevor King ***@***.***> wrote: Why are we creating cluster version during install and why do we have to? Historically, this was used to override broken components, but we haven't needed to do that since openshift/installer#851 <openshift/installer#851>. Although obviously we could have been managing that from this repo instead of managing it via the installer repo. Metadata.json having it is a bit weird. Yeah. In fact, Cluster version having it is a bit weird ;). If it's just for telemetry, shouldn't it just be in the Telemetry config <https://github.com/openshift/cluster-monitoring-operator/blob/fbd0e89e8a4fc72a993b64fb80c79145c78f8b67/pkg/manifests/config.go#L125>? Is telemetry/monitoring represented by a custom resource? Nothing stands out to me in the API <https://github.com/openshift/api/tree/a94e914914f4228d0bcba6fc8a22614c5f5e2dad/config/v1>. Maybe configuration is via a ConfigMap <https://github.com/openshift/cluster-monitoring-operator/blob/e64d3f1b595959ae9cd4388d7a4fa5d41b5f3a5a/pkg/operator/operator.go#L338> ? At the moment, Hive is using the telemetry from metadata.json to hold space for "coming OSD cluster". It could pull the Telemetry ID put of the running cluster instead, but then OSD-ness would race cluster Telemetry reporting vs. Hive's extraction. Until we get OSD-ness via the cluster_installer metrics (openshift/installer#1890 <openshift/installer#1890> #213 <#213>), which may be where we want it long-term (we'd still need a way to associate Hive-reported admin creds with the appropriate Telemetry ID). The CVO shouldn’t be syncing anything out of the payload until it has written the CV, so the bug here seems to be with bootstrap using idempotent create for objects instead of idenpotent create or update? I don't like multiple parties having opinions about cluster config specs. Components that have opinions about their own config could represent their defaults in status, but should stay out of spec, which should be admin-managed for these top-level configs (and installer-managed as a proxy for the admin at install-time). I'd be fine with cluster-bootstrap clobbering spec (repeatedly?) until all resource specs matched its expectations, but that's a reasonably large change vs. its current behavior. Also alternatively, perhaps the bootstrap process should be passing intiital cluster id and channel to cvo to create bootstrap deployment... As in "but not via ClusterVersion"? I'm fine just leaving channel/upstream defaulting to the CVO. And if we adjust Telemetry ID injection, I'd rather move it to an in-cluster Telemetry custom resource. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#238?email_source=notifications&email_token=AAI37J2HTHJA6V32SRXCPLDQFGTEPA5CNFSM4IMMANDKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4RHBTA#issuecomment-522350796>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAI37J7RX5IAFDFLC5YT4FDQFGTEPANCNFSM4IMMANDA> .

[abhinavdahiya]

Yeah. In fact, ClusterVersion having it is a bit weird ;). If it's just for telemetry, shouldn't it just be in the Telemetry config? Is telemetry/monitoring represented by a custom resource? Nothing stands out to me in the API. Maybe configuration is via a ConfigMap?

The cluster version ID is the UUID for the cluster. We use it to identify the cluster in telemetry on red hat side, also the cincinatti stacks needs a way to identify the cluster.

So I think the ID definitely belongs to the clusterversion object.

[wking]

/cherrypick release-4.1

[openshift-cherrypick-robot]

@wking: failed to push cherry-picked changes in GitHub: pushing failed, output: "To https://github.com/openshift-cherrypick-robot/cluster-version-operator\n ! [rejected] cherry-pick-238-to-release-4.1 -> cherry-pick-238-to-release-4.1 (non-fast-forward)\nerror: failed to push some refs to 'https://openshift-cherrypick-robot:[email protected]/openshift-cherrypick-robot/cluster-version-operator'\nhint: Updates were rejected because the tip of your current branch is behind\nhint: its remote counterpart. Integrate the remote changes (e.g.\nhint: 'git pull ...') before pushing again.\nhint: See the 'Note about fast-forwards' in 'git push --help' for details.\n", error: exit status 1

Details

In response to this:

/cherrypick release-4.1

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.

[wking]

Oops, we already had one :p.