pkg/rhcos/builds: allow use of env var to override stream URL. #1168

darkmuggle · 2019-01-31T18:29:16Z

Use of 'OPENSHIFT_INSTALL_COREOS_IMAGE_URL' environment variable will allow RHCOS developers to select different sources for RHCOS images. Also, added 'RHCOS_URL' to set the default at binary build.

Use of 'OPENSHIFT_INSTALL_COREOS_IMAGE_URL' will allow RHCOS developers to select different sources for RHCOS images. Also, added 'RCHOS_URL' to set the default at build-time.

darkmuggle · 2019-01-31T18:33:41Z

/cc @cgwalters @ashcrow

This change allows RHCOS developers to use a different source for images. We intend on using this for developer image testing, and streams.

crawford · 2019-01-31T18:48:11Z

You have a typo in your commit message (RCHOS_URL).
/approve

Longer term, we'll want to remove this and defer to the release image when determining the the OS image. This seems fine for now though.

darkmuggle · 2019-01-31T18:50:19Z

You have a typo in your commit message (RCHOS_URL).
/approve

Fixed the message :) Thanks for catching.

abhinavdahiya · 2019-01-31T19:37:20Z

pkg/rhcos/builds.go

@@ -22,6 +23,13 @@ var (
 	baseURL = "https://releases-rhcos.svc.ci.openshift.org/storage/releases"
 )

+func init() {
+	if or, ok := os.LookupEnv("OPENSHIFT_INSTALL_COREOS_IMAGE_URL"); ok && or != "" {


why COREOS and not RHCOS ?

I guess I'd vaguely thought that we want to enable e.g. pointing the installer at a custom Fedora CoreOS build with kubelet for testing.

wking · 2019-01-31T20:01:06Z

Why can't we use the existing OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE (from #1052) for this use case?

cgwalters · 2019-01-31T20:16:16Z

Why can't we use the existing OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE (from #1052) for this use case?

That's more work to use; the developer/tester or CI system trying this out would have to scrape the API endpoint to find the build they want. (right?)

Edit, to rephrase I think we want both; the ability to override the URL and grab the latest from that endpoint, and the ability to force a particular build from either the default URL or a custom one.

darkmuggle · 2019-01-31T20:46:31Z

Why can't we use the existing OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE (from #1052) for this use case?

That's more work to use; the developer/tester or CI system trying this out would have to scrape the API endpoint to find the build they want. (right?)

Edit, to rephrase I think we want both; the ability to override the URL and grab the latest from that endpoint, and the ability to force a particular build from either the default URL or a custom one.

That's the exact reason why I proposed this enhancement.

wking · 2019-01-31T20:51:49Z

That's more work to use; the developer/tester or CI system trying this out would have to scrape the API endpoint to find the build they want. (right?)

Right, but that seems easy to script. How about:

#!/bin/sh

BASE_URL="${BASE_URL:-https://releases-rhcos.svc.ci.openshift.org/storage/releases}"
CHANNEL="${CHANNEL:=maipo}"
BUILD=
REGION="${REGION:${AWS_DEFAULT_REGION}}"

if test -z "${REGION}"
then
	printf "REGION unset\n" >&2
	exit 1
fi

if test -z "${BUILD}"
then
	BUILD="$(curl -s "${BASE_URL}/${CHANNEL}/builds.json" | jq -r '.builds[0]')"
fi

AMIS="$(curl -s "${BASE_URL}/${CHANNEL}/${BUILD}/meta.json" | jq -r ".amis")"
AMI="$(echo "${AMIS}" | jq -r ".[] | select(.name == \"${REGION}\").hvm")"
if test -z "${AMI}"
then
	printf "no AMI for %s.  Available regions:\n%s\n" "${REGION}" "$(echo "${AMIS}" | jq -r '.[].name')" >&2
	exit 1
fi
echo "${AMI}"

I don't think we need to bake the different levels of choices into the installer via environment variables.

cgwalters · 2019-01-31T21:39:17Z

Right, but that seems easy to script.

I'm not sure I'd agree with your characterization of "easy" 😉

While there are multiple consumers of the existing "build JSON" I'd like not to encourage more people to parse it than necessary.

cgwalters · 2019-01-31T21:39:45Z

Bigger picture here we forsee splitting the "RHCOS release API endpoint" between OKD and OCP, and this would help enable that.

darkmuggle · 2019-01-31T21:55:00Z

Another interesting use case this enables is the ability to have a "pre-release" endpoint that can be validated against the installer. I really appreciate the concern, I do. But enabling developers to use the tooling against other endpoints exercises a different code path than short-circuiting it with a particular AMI.

wking · 2019-01-31T21:57:51Z

Right, but that seems easy to script.

I'm not sure I'd agree with your characterization of "easy" 😉

Well, it's done, and it's only a few lines ;). How many folks do you expect to need RHCOS-forcing anyway? I'm fine carrying this script in the installer repository in the short term if you'd be more comfortable with that approach.

Bigger picture here we forsee splitting the "RHCOS release API endpoint" between OKD and OCP, and this would help enable that.

Released installer binaries will pin OCP RHCOS and update payloads, right? I don't see a reason to keep pinning OKD RHCOS builds now that we've transitioned to pinning OCP update payloads (as we have since installer v0.10.0). Master installers are floating against upstream OKD for both update-payloads and RHCOS, and I don't see that changing either. And once we get #1149 and related addressing #987, we can drop RHCOS knobs entirely and use OPENSHIFT_INSTALL_RELEASE_IMAGE_OVERRIDE for everything. So this PR feels like an additional installer knob (even if it's undocumented) that we only expect a few users to need for a short time. Am I missing something?

cgwalters · 2019-01-31T22:05:55Z

I'm fine carrying this script in the installer repository in the short term if you'd be more comfortable with that approach.

I think I personally would find it easier to change the installer source and rebuild.

Released installer binaries will pin OCP RHCOS and update payloads, right?

Right but...in order to actually have separate RHCOS builds for OKD/OCP the people setting that up are going to need the ability to easily test their new endpoint.

(Also see above re making it easy to test w/custom FCOS)

I dunno. On the other hand...we don't really have an "OCP master" concept; so perhaps at the same time as the release is cut the URL would be changed?

wking · 2019-01-31T22:33:48Z

Right but...in order to actually have separate RHCOS builds for OKD/OCP the people setting that up are going to need the ability to easily test their new endpoint.

So have whatever tooling is creating their update payload push in lookup information for the appropriate RHCOS? This doesn't seem different from any of the other components (mostly operators) feeding into update payloads. And once we get RHCOS in the update payload, even the current OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE will be as superfluous as a OPENSHIFT_INSTALL_MACHINE_CONFIG_OPERATOR_OVERRIDE, etc.

I dunno. On the other hand...we don't really have an "OCP master" concept; so perhaps at the same time as the release is cut the URL would be changed?

Maybe this is "update-payload author includes RHCOS-lookup information as well", in which case I'm completely on board ;).

crawford · 2019-02-01T00:18:12Z

I forgot about OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE (I thought we had removed it and this was putting it back) and after reading about the true motivation for this change, I have some concern.

/approve cancel

openshift-ci-robot · 2019-02-01T00:18:21Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: darkmuggle
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: steveej

If they are not already assigned, you can assign the PR to them by writing /assign @steveej in a comment when ready.

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

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

darkmuggle · 2019-02-01T00:21:31Z

I forgot about OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE (I thought we had removed it and this was putting it back) and after reading about the true motivation for this change, I have some concern.

fwiw, I modeled the PR after OEPNSHFIT_INSTALL_OS_IMAGE_OVERRIDE.

I really appreciate the thoughtful review and the robust discussion 😄

smarterclayton · 2019-02-01T14:20:09Z

Bigger picture here we forsee splitting the "RHCOS release API endpoint" between OKD and OCP, and this would help enable that.

Why? I don't think we do.

pkg/rhcos/builds: allow use of env var to override stream URL.

9611d75

Use of 'OPENSHIFT_INSTALL_COREOS_IMAGE_URL' will allow RHCOS developers to select different sources for RHCOS images. Also, added 'RCHOS_URL' to set the default at build-time.

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 31, 2019

openshift-ci-robot requested review from ashcrow, cgwalters, hardys and russellb January 31, 2019 18:33

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

abhinavdahiya reviewed Jan 31, 2019

View reviewed changes

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019

darkmuggle closed this Feb 1, 2019

darkmuggle deleted the url-override branch February 1, 2019 00:25

wking mentioned this pull request Mar 11, 2019

rhcos: Support channels #1402

Closed

wking mentioned this pull request Mar 20, 2019

rhcos: Embed full build metadata in binary #1423

Merged

wking mentioned this pull request Apr 3, 2019

AWS UPI draft openshift/openshift-docs#14241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/rhcos/builds: allow use of env var to override stream URL. #1168

pkg/rhcos/builds: allow use of env var to override stream URL. #1168

darkmuggle commented Jan 31, 2019 •

edited

Loading

darkmuggle commented Jan 31, 2019

crawford commented Jan 31, 2019

darkmuggle commented Jan 31, 2019

abhinavdahiya Jan 31, 2019

cgwalters Jan 31, 2019

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019 •

edited

Loading

darkmuggle commented Jan 31, 2019

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019

cgwalters commented Jan 31, 2019

darkmuggle commented Jan 31, 2019

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019

wking commented Jan 31, 2019

crawford commented Feb 1, 2019

openshift-ci-robot commented Feb 1, 2019

darkmuggle commented Feb 1, 2019

smarterclayton commented Feb 1, 2019

pkg/rhcos/builds: allow use of env var to override stream URL. #1168

pkg/rhcos/builds: allow use of env var to override stream URL. #1168

Conversation

darkmuggle commented Jan 31, 2019 • edited Loading

darkmuggle commented Jan 31, 2019

crawford commented Jan 31, 2019

darkmuggle commented Jan 31, 2019

abhinavdahiya Jan 31, 2019

Choose a reason for hiding this comment

cgwalters Jan 31, 2019

Choose a reason for hiding this comment

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019 • edited Loading

darkmuggle commented Jan 31, 2019

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019

cgwalters commented Jan 31, 2019

darkmuggle commented Jan 31, 2019

wking commented Jan 31, 2019

cgwalters commented Jan 31, 2019

wking commented Jan 31, 2019

crawford commented Feb 1, 2019

openshift-ci-robot commented Feb 1, 2019

darkmuggle commented Feb 1, 2019

smarterclayton commented Feb 1, 2019

darkmuggle commented Jan 31, 2019 •

edited

Loading

cgwalters commented Jan 31, 2019 •

edited

Loading