Refactor RHCOS fetch API, support fetching qemu from partial config #856

cgwalters · 2018-12-10T18:21:57Z

The main thing I'm really fixing here is that I have an installer
wrapper script: https://github.com/cgwalters/homegit/blob/master/bin/okdinst

And I noticed recently that I kept fetching old RHCOS versions; that's
of course because the image URL was hardcoded in my install config. What
I really want is to fetch that dynamically.

So this PR changes the TF rendering code to do an on-demand fetch
if the image isn't filled out in the libvirt config.

Now further, as I was arguing in this PR:
#710 (comment)

The result of an install run from installer git master is currently a function
of the 3-tuple:
(installer, RHCOS, release payload)

I'd actually like to print out all 3 clearly in the logs; this is
a first step.

Today as far as I can see we do separate fetches in AWS for master,
bootstrap and worker, which is a bit broken. Need to move that
to a higher level, and this will help with that.

openshift-ci-robot · 2018-12-10T18:22:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @russellb 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

pkg/types/libvirt/machinepool.go

This is a trival subset of openshift#856 As I was arguing in this PR: The result of an install run is currently a function of the 3-tuple: (installer, RHCOS, release payload) I'd actually like to print out all 3 clearly in the logs; this is a first step to show the RHCOS version.

cgwalters · 2018-12-11T15:01:18Z

OK split out #866

However, some of the work in this PR to retain the structured metadata I think is still useful.

cgwalters · 2018-12-11T15:04:26Z

Though of course if we go to an always-pinned model this PR would be obsolete.

cgwalters · 2018-12-12T23:06:07Z

/hold

cgwalters · 2018-12-19T20:49:43Z

/hold cancel

OK, this is rebased 🏄‍♂️ and while a lot of the code is the same, we landed the much more trivial #866 in the meantime.

This PR now has a different primary purpose; see the updated commit message for details.

cgwalters · 2018-12-20T13:40:21Z

/hold

Will rebase this after #902 lands.

crawford · 2018-12-21T00:21:19Z

@cgwalters after #902 lands, what does this PR get us?

cgwalters · 2018-12-21T13:25:38Z

It makes the API more closely model how the coreos-assembler metadata looks. It's a useful step towards e.g. only fetching the AMI values once - it looks like to me today we fetch twice for AWS.

For example, I want to change things to log/print the RHCOS version number for AWS too - one can always derive the AMI from that.

wking · 2018-12-21T15:16:56Z

pkg/tfvars/libvirt/cache.go

@@ -28,7 +28,7 @@ func (libvirt *Libvirt) UseCachedImage() (err error) {
 		return nil
 	}

-	logrus.Infof("Fetching OS image: %s", filepath.Base(libvirt.Image))
+	logrus.Infof("Fetching qemu URL: %s", filepath.Base(libvirt.Image))


nit: The log message should describe the resource being fetched, but "URL" is just how we're fetching it. If you don't like "OS image", how about "QEMU image"? Or "QCOW2" (although I don't know if we actually require this to be QCOW2 or if libvirt can handle other formats too)?

wking · 2018-12-21T15:19:43Z

pkg/types/libvirt/machinepool.go

@@ -13,6 +13,8 @@ type MachinePool struct {
 	// Image is the URL to the OS image.
 	// E.g. "http://aos-ostree.rhev-ci-vms.eng.rdu2.redhat.com/rhcos/images/cloud/latest/rhcos-qemu.qcow2.gz"
 	Image string `json:"image"`
+	// Metadata string describing the image
+	ImageDescription string `json:"imageDescription,omitempty"`


MachinePool is for user-supplied input (although we supply defaults when the user doesn't care). This string doesn't seem like an actionable input.

wking · 2018-12-21T15:26:11Z

pkg/tfvars/tfvars.go

+		// is reusing an install config)
+		qemuImage := cfg.Platform.Libvirt.DefaultMachinePlatform.Image
+		if qemuImage == "" {
+			rhcosBuild, err := rhcos.FetchBuild(context.TODO(), rhcos.DefaultChannel)


This is too late (and the in-master AMI fetch above is also too late). We need these resolved by the time we generate the machine(-set) values we set up for the cluster API. #792 tries to straighten out dependencies to make this work.

The *main* thing I'm really fixing here is that I have an installer wrapper script: https://github.com/cgwalters/homegit/blob/master/bin/okdinst And I noticed recently that I kept fetching old RHCOS versions; that's of course because the image URL was hardcoded in my install config. What I really want is to fetch that dynamically. So this PR changes the TF rendering code to do an on-demand fetch if the image isn't filled out in the libvirt config. Now further, as I was arguing in this PR: openshift#710 (comment) The result of an install run from installer git master is currently a function of the 3-tuple: (installer, RHCOS, release payload) I'd actually like to print out all 3 clearly in the logs; this is a first step. Today as far as I can see we do separate fetches in AWS for master, bootstrap and worker, which is a bit broken. Need to move that to a higher level, and this will help with that.

This is primarily targeted for local development/testing.

openshift-ci-robot · 2019-01-14T10:48:14Z

@cgwalters: PR needs rebase.

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 · 2019-05-15T19:29:07Z

@cgwalters: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/golint	`b547e64`	link	`/test golint`
ci/prow/gofmt	`b547e64`	link	`/test gofmt`
ci/prow/e2e-aws-upgrade	`b547e64`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-aws	`b547e64`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

abhinavdahiya · 2019-06-19T22:53:38Z

This looks abandoned. and installer has the images hardcoded for boot.

/close

openshift-ci-robot · 2019-06-19T22:53:40Z

@abhinavdahiya: Closed this PR.

In response to this:

This looks abandoned. and installer has the images hardcoded for boot.

/close

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 added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 10, 2018

openshift-ci-robot requested review from aaronlevy and crawford December 10, 2018 18:22

cgwalters force-pushed the refactor-rhcos branch from 0170392 to 3132f2a Compare December 10, 2018 18:47

abhinavdahiya suggested changes Dec 10, 2018

View reviewed changes

pkg/types/libvirt/machinepool.go Show resolved Hide resolved

cgwalters mentioned this pull request Dec 11, 2018

libvirt/cache: Display target OS version #866

Merged

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 12, 2018

cgwalters force-pushed the refactor-rhcos branch from 3132f2a to 1e9b39e Compare December 19, 2018 20:47

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

cgwalters changed the title ~~Refactor RHCOS fetch to print fetched OS version~~ Refactor RHCOS fetch API, support fetching qemu from partial config Dec 19, 2018

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

cgwalters mentioned this pull request Dec 20, 2018

assets/installconfig: apply defaults to install config #902

Closed

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 20, 2018

wking reviewed Dec 21, 2018

View reviewed changes

cgwalters force-pushed the refactor-rhcos branch from 1e9b39e to 862c20f Compare January 3, 2019 18:02

cgwalters added 2 commits January 7, 2019 14:48

rhcos/qemu: Readd environment var OPENSHIFT_INSTALL_LIBVIRT_IMAGE

b547e64

This is primarily targeted for local development/testing.

cgwalters force-pushed the refactor-rhcos branch from 862c20f to b547e64 Compare January 7, 2019 14:48

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

openshift-ci-robot closed this Jun 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor RHCOS fetch API, support fetching qemu from partial config #856

Refactor RHCOS fetch API, support fetching qemu from partial config #856

cgwalters commented Dec 10, 2018 •

edited

Loading

openshift-ci-robot commented Dec 10, 2018

cgwalters commented Dec 11, 2018

cgwalters commented Dec 11, 2018 •

edited

Loading

cgwalters commented Dec 12, 2018

cgwalters commented Dec 19, 2018

cgwalters commented Dec 20, 2018

crawford commented Dec 21, 2018

cgwalters commented Dec 21, 2018

wking Dec 21, 2018 •

edited

Loading

wking Dec 21, 2018

wking Dec 21, 2018

openshift-ci-robot commented Jan 14, 2019

openshift-ci-robot commented May 15, 2019

abhinavdahiya commented Jun 19, 2019

openshift-ci-robot commented Jun 19, 2019

Refactor RHCOS fetch API, support fetching qemu from partial config #856

Refactor RHCOS fetch API, support fetching qemu from partial config #856

Conversation

cgwalters commented Dec 10, 2018 • edited Loading

openshift-ci-robot commented Dec 10, 2018

cgwalters commented Dec 11, 2018

cgwalters commented Dec 11, 2018 • edited Loading

cgwalters commented Dec 12, 2018

cgwalters commented Dec 19, 2018

cgwalters commented Dec 20, 2018

crawford commented Dec 21, 2018

cgwalters commented Dec 21, 2018

wking Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

wking Dec 21, 2018

Choose a reason for hiding this comment

wking Dec 21, 2018

Choose a reason for hiding this comment

openshift-ci-robot commented Jan 14, 2019

openshift-ci-robot commented May 15, 2019

abhinavdahiya commented Jun 19, 2019

openshift-ci-robot commented Jun 19, 2019

cgwalters commented Dec 10, 2018 •

edited

Loading

cgwalters commented Dec 11, 2018 •

edited

Loading

wking Dec 21, 2018 •

edited

Loading