Move default openshift version #3094

mbarnes · 2023-08-12T18:51:39Z

Which issue this PR addresses:

[ARO-3896] Read default OCP version from CosmosDB

Depends on a couple ADO pull requests:

RP-Config: Add openShiftVersions data ✔️ merged
ARO.Pipelines: Read OpenShiftVersions from YAML configuration ✔️ merged

What this PR does / why we need it:

Make CosmosDB the single source of truth for available OpenShift versions, including the default version. Currently it's a mix: CosmosDB has the available versions but only ARO RP knows the default version.

Local Development Mode

Local development mode has been a subject of discussion within my functional team (Loki). Initially I had planned to implement a small hack to keep local development mode working as is in lieu of an internal default OpenShift version and pullspec. But the team has grander plans for local development mode, so I'm going to defer that issue here and leave the internal default version in place. But it's ONLY to be used for local development mode, not in the frontend or in any production pipelines.

Test plan for issue:

Combination of local development mode and probably deployment to INT to test aro update-versions in a pipeline.

Is there any documentation that needs to be updated for this PR?

The wiki page Multi-Version OpenShift Installations should direct readers to RP-Config rather than pkg/util/version/const.go for updating install streams.

github-actions · 2023-08-17T16:27:01Z

Please rebase pull request.

mbarnes · 2023-11-20T00:48:20Z

@SudoBrendan and @petrkotas had previously approved this.

SudoBrendan

left some comments/nits. Thanks!

cmd/aro/update_ocp_versions.go

kimorris27

Looks good to me! Have the changes been tested in INT yet?

github-actions · 2023-11-29T13:53:48Z

Please rebase pull request.

The frontend's OpenShiftVersions change feed handler will record the current default version for the rest of the frontend to use.

The RP no longer has this information internally, so the metric is no longer relevant.

Isolating some light refactoring. The purpose will become more clear in the next commit.

Read OpenShift versions and pull specs from an OPENSHIFT_VERSIONS environment variable containing a JSON object. This data includes the default OpenShift version for new installs that don't specify a version. This moves us toward eliminating hard-coded OpenShift versions in pkg/util/version/const.go.

I'm not sure what to do with this test. Install stream data has moved to RP-Config, so if the test is worth keeping then I guess the oldest supported version will have to be hard-coded and kept up-to-date. But it probably won't be.

DefaultInstallStream will remain for now, but it's ONLY for use by local development mode until we can come up with a better solution.

Clarifies their purpose and helps avoid variable name collisions.

jhoreman

Was able to personally test this in INT. Updating the default version in the cosmosDB document for OpenShiftVersions changes the default install version as expected and desired.

mociarain · 2024-01-18T17:02:30Z

cmd/aro/update_ocp_versions.go

+// Corresponds to configuration.openShiftVersions in RP-Config
+type OpenShiftVersions struct {
+	DefaultStream  map[string]string
+	InstallStreams map[string]string
+}


Seems odd to me (someone who is new to the team and GO) that we don't have some large struct to validate the contents of the RP-config already. Do we always just query part of the config piecemeal like this? Wouldn't it be more typical to load the config at start time and then access some object as needed? (Assuming that we don't have any dynamic components to the config). What am I missing?

The main goal of this work was to move the hardcoded configuration of the versioning into the database. Historically this configuration has been hardcoded so there was not a need to call it other than using pkg/util/version/

What you suggest is a good idea but we haven't gotten to a something like a general pkg/util/config at a team. In time this will probably make sense to consolidate the configuration of the RP in one package. But it was out of scope for this effort. Mainly when this was talked about there was some worry about not knowing what all the configuration was, so we decided to move one by one instead. With the idea that we would consolidate later. This is what I remember anyways.

While I wasn't the main person working on this. Matt has been moved to other priorities at the moment so the rest of Loki team is trying to push this one over the finish line for him.

+1 for a follow up discussion and effort on this for sure.

FWIW, ARO.Pipelines has the full struct and parser you're looking for. What's happening in this context is the parser is extracting the OpenShift version bits we need from RP-Config and passing it to the RP as a JSON blob by way of an environment variable. All we're parsing here is the environment variable, not RP-Config directly.

mociarain · 2024-01-18T17:10:56Z

cmd/aro/update_ocp_versions.go


-	dstRepo := dstAcr + acrDomainSuffix
-	ocpVersions := []api.OpenShiftVersion{}
+	// The DefaultStream map must have exactly one entry.


Small question to help with my understanding of how we do things:

This comment tells me the "What" but not the "Why" and in the past this has been the rule I've used to determine if we need a comment. So:

Do we need to update this comment to the "Why"?

Can we just drop the comment overall?

Do I have this all wrong? (Probably some amount of this ;) )

Always good to get feedback on this. Basically we have only one default version for install - the latest Y version. Instead of having a default version per Y stream.

mociarain · 2024-01-18T17:18:12Z

pkg/frontend/frontend.go

@@ -63,6 +63,7 @@ type frontend struct {
 	dbSubscriptions               database.Subscriptions
 	dbOpenShiftVersions           database.OpenShiftVersions

+	defaultOcpVersion  string // always enabled


Can you explain this comment more?

Will answer below.

mociarain · 2024-01-18T17:18:53Z

pkg/frontend/admin_openshiftversion_put.go

@@ -53,6 +46,11 @@ func (f *frontend) putAdminOpenShiftVersion(w http.ResponseWriter, r *http.Reque
 	if docs != nil {
 		for _, doc := range docs.OpenShiftVersionDocuments {
 			if doc.OpenShiftVersion.Properties.Version == ext.Properties.Version {
+				// prevent disabling of the default installation version


Same commenting point as above.

I think this is explained by looking at the docs of az aro create. Basically if you don't supply a version to install it will install this default version.

lranjbar

LGTM

kimorris27

Reviewed this back in December but am giving my official green check mark now that it's been tested in INT 🙂

* api: Avoid referencing DefaultInstallStream in tests * frontend: Avoid referencing DefaultInstallStream The frontend's OpenShiftVersions change feed handler will record the current default version for the rest of the frontend to use. * monitor: Remove latestGaMinorVersion metric The RP no longer has this information internally, so the metric is no longer relevant. * update_ocp_versions: Read versions from an environment variable Read OpenShift versions and pull specs from an OPENSHIFT_VERSIONS environment variable containing a JSON object. This data includes the default OpenShift version for new installs that don't specify a version. This moves us toward eliminating hard-coded OpenShift versions in pkg/util/version/const.go. * cache_fallback_discovery_client_test.go: Hard-code version I'm not sure what to do with this test. Install stream data has moved to RP-Config, so if the test is worth keeping then I guess the oldest supported version will have to be hard-coded and kept up-to-date. But it probably won't be. * version: Remove DefaultInstallStreams DefaultInstallStream will remain for now, but it's ONLY for use by local development mode until we can come up with a better solution. --------- Co-authored-by: Matthew Barnes <[email protected]>

mbarnes force-pushed the move-default-openshift-version branch from ad7f1db to bae936c Compare August 12, 2023 19:51

mbarnes changed the title ~~Move default openshift version~~ [WIP] Move default openshift version Aug 13, 2023

mbarnes force-pushed the move-default-openshift-version branch 2 times, most recently from c140b25 to b555895 Compare August 17, 2023 15:11

github-actions bot added the needs-rebase branch needs a rebase label Aug 17, 2023

mbarnes force-pushed the move-default-openshift-version branch from 572fef9 to c024abb Compare August 17, 2023 16:34

github-actions bot removed the needs-rebase branch needs a rebase label Aug 17, 2023

mbarnes force-pushed the move-default-openshift-version branch from c024abb to 965781a Compare August 17, 2023 17:00

mbarnes changed the title ~~[WIP] Move default openshift version~~ Move default openshift version Aug 17, 2023

mbarnes force-pushed the move-default-openshift-version branch from 965781a to 6c79a24 Compare August 17, 2023 18:03

mbarnes marked this pull request as ready for review August 17, 2023 18:04

mbarnes requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, facchettos, cadenmarchese, UlrichSchlueter, s-amann, SudoBrendan, ellis-johnson, yjst2012 and anshulvermapatel as code owners August 17, 2023 18:04

mbarnes added ready-for-review loki hold Hold labels Aug 17, 2023

SudoBrendan previously approved these changes Nov 20, 2023

View reviewed changes

cmd/aro/update_ocp_versions.go Show resolved Hide resolved

cmd/aro/update_ocp_versions.go Outdated Show resolved Hide resolved

cmd/aro/update_ocp_versions.go Outdated Show resolved Hide resolved

mbarnes dismissed SudoBrendan’s stale review via 8db2521 November 21, 2023 10:06

mbarnes force-pushed the move-default-openshift-version branch from 0c4c212 to 8db2521 Compare November 21, 2023 10:06

kimorris27 reviewed Nov 27, 2023

View reviewed changes

SudoBrendan mentioned this pull request Nov 28, 2023

Remove version 4.11.26 due to installation issues #3293

Merged

cadenmarchese removed the next-release To be included in the next RP release rollout label Nov 28, 2023

github-actions bot added the needs-rebase branch needs a rebase label Nov 29, 2023

Matthew Barnes added 10 commits December 6, 2023 14:29

api: Avoid referencing DefaultInstallStream in tests

d058b35

frontend: Avoid referencing DefaultInstallStream

9a94353

The frontend's OpenShiftVersions change feed handler will record the current default version for the rest of the frontend to use.

monitor: Remove latestGaMinorVersion metric

cc1c8ea

The RP no longer has this information internally, so the metric is no longer relevant.

update_ocp_versions: Refactoring

b9e928e

Isolating some light refactoring. The purpose will become more clear in the next commit.

version: Remove DefaultInstallStreams

ad5abb9

DefaultInstallStream will remain for now, but it's ONLY for use by local development mode until we can come up with a better solution.

cmd/aro: Prefix environment variable constants with "env"

837f5fb

Clarifies their purpose and helps avoid variable name collisions.

cmd/aro: Add more environment variable constants

b021c32

e2e: Use internal default OpenShift version for CI

d89b290

mbarnes force-pushed the move-default-openshift-version branch from 8db2521 to d89b290 Compare December 6, 2023 19:40

github-actions bot removed the needs-rebase branch needs a rebase label Dec 6, 2023

jhoreman approved these changes Jan 4, 2024

View reviewed changes

SudoBrendan approved these changes Jan 17, 2024

View reviewed changes

mociarain reviewed Jan 18, 2024

View reviewed changes

lranjbar approved these changes Jan 18, 2024

View reviewed changes

kimorris27 approved these changes Jan 18, 2024

View reviewed changes

SudoBrendan merged commit 809041f into Azure:master Jan 18, 2024
22 checks passed

mbarnes deleted the move-default-openshift-version branch January 18, 2024 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move default openshift version #3094

Move default openshift version #3094

mbarnes commented Aug 12, 2023 •

edited

Loading

github-actions bot commented Aug 17, 2023

mbarnes commented Nov 20, 2023

SudoBrendan left a comment

kimorris27 left a comment

github-actions bot commented Nov 29, 2023

jhoreman left a comment

mociarain Jan 18, 2024

lranjbar Jan 18, 2024

mbarnes Jan 18, 2024 •

edited

Loading

mociarain Jan 18, 2024

lranjbar Jan 18, 2024

mociarain Jan 18, 2024

lranjbar Jan 18, 2024

mociarain Jan 18, 2024

lranjbar Jan 18, 2024 •

edited

Loading

lranjbar left a comment

kimorris27 left a comment

Move default openshift version #3094

Move default openshift version #3094

Conversation

mbarnes commented Aug 12, 2023 • edited Loading

Which issue this PR addresses:

What this PR does / why we need it:

Local Development Mode

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

github-actions bot commented Aug 17, 2023

mbarnes commented Nov 20, 2023

SudoBrendan left a comment

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2023

jhoreman left a comment

Choose a reason for hiding this comment

mociarain Jan 18, 2024

Choose a reason for hiding this comment

lranjbar Jan 18, 2024

Choose a reason for hiding this comment

mbarnes Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

mociarain Jan 18, 2024

Choose a reason for hiding this comment

lranjbar Jan 18, 2024

Choose a reason for hiding this comment

mociarain Jan 18, 2024

Choose a reason for hiding this comment

lranjbar Jan 18, 2024

Choose a reason for hiding this comment

mociarain Jan 18, 2024

Choose a reason for hiding this comment

lranjbar Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

lranjbar left a comment

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

mbarnes commented Aug 12, 2023 •

edited

Loading

mbarnes Jan 18, 2024 •

edited

Loading

lranjbar Jan 18, 2024 •

edited

Loading