drop capability to specify image in install-config #1052

abhinavdahiya · 2019-01-11T18:47:51Z

includes commit from #812

RHCOS image that needs to be used for installation must be sources from RHCOS build pipeline and RHCOS build.
Keeping the image related fields in install-config allows users to change these values as part of valid configuration, but we do not want users to configure this option as
the RHCOS image controls the runtime and kubelet versions we depend on.

added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.

/cc @crawford @wking

None of the options, other than Image, that were in the Libvirt MachinePool were being used. They have been removed. The Image has been pulled up to the Libvirt Platform, as there was no way to use a different image for different machines pools. For consistency with the AWS and OpenStack platforms, the Libvirt MachinePool has been retained, even though it is empty. The DefaultMachinePlatform has been retained in the Libvirt Platform as well. The code in the Master Machines and Worker Machines assets that determines the configuration to use for the machines has been adjusted for Libvirt to rectify the machine-pool-specific configuration against the default machine-pool configuration. This is not strictly necessary as, again, the Libvirt configuration is empty. It keeps the logic consistent with the other platforms, though. https://jira.coreos.com/browse/CORS-911

staebler

Changes look good.

Can we remove this function?

installer/pkg/types/openstack/validation/realvalidvaluesfetcher.go

Line 66 in bf0c0de

func (f realValidValuesFetcher) GetImageNames(cloud string) ([]string, error) {

abhinavdahiya · 2019-01-11T21:03:42Z

Changes look good.

Can we remove this function?

installer/pkg/types/openstack/validation/realvalidvaluesfetcher.go

Line 66 in bf0c0de
func (f realValidValuesFetcher) GetImageNames(cloud string) ([]string, error) {

done with 83d9bf8

wking · 2019-01-11T21:40:47Z

pkg/asset/cluster/tfvars.go

@@ -34,6 +35,7 @@ func (t *TerraformVariables) Name() string {
 func (t *TerraformVariables) Dependencies() []asset.Asset {
 	return []asset.Asset{
 		&installconfig.InstallConfig{},
+		new(rhcos.Image),


Why is this not &rhcos.Image{} like its neighbors? And the same elsewhere in this commit:

$ git show d65693719 | grep 'new(' + new(rhcos.Image), + rhcosImage := new(rhcos.Image) + new(rhcos.Image), + rhcosImage := new(rhcos.Image) + new(rhcos.Image), + rhcosImage := new(rhcos.Image)

https://play.golang.org/p/NUhwmraqAtM

prog.go:10:7: invalid pointer type *Image for composite literal

https://golang.org/ref/spec#Address_operators

wking · 2019-01-12T00:02:32Z

pkg/asset/rhcos/image.go

+	case libvirt.Name:
+		osimage, err = rhcos.QEMU(ctx, rhcos.DefaultChannel)
+	case openstack.Name:
+		osimage = "rhcos"


This is much-simplified from the previous GetImageNames you're removing in 83d9bf8d523. @hardys added that image-name select widget in #766, and sometimes OpenStack doesn't have reliable defaults (e.g. #788). Are we comfortable with a hard-coded value here, or do we need to retain some lookup logic or user input?

abhinavdahiya · 2019-01-12T00:09:41Z

Drop:

added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.

from your commit message (and maybe add it to 8957997, since that's the commit adding the variable)?

Yeah, my original thought was somebody seeing this commit of dropping image fields from InstallConfig might benefit from it... but i can move it if you like?

wking

I have one OpenStack concern and one Git nit, but I'm still fine with this PR landing as it stands before we address those. We can always circle back to OpenStack post-freeze.

wking · 2019-01-12T00:11:40Z

Yeah, my original thought was somebody seeing this commit of dropping image fields from InstallConfig might benefit from it...

Maybe mention "in follow-up work" in the commit message? Or squash this PR down a bit, currently there are commits removing properties from structs before commits removing the property consumers and things like that. On the other hand, it's nice to see smaller pivots too. I don't really care either way at this stage ;).

added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.

With baseimage user query removed from TUI, this function is no longer required. This also - drops the vendored files - updates the mocks using `hack/go-genmock.sh`

RHCOS image that needs to be used for installation must be sources from RHCOS build pipeline and RHCOS build. Keeping the image related fields in install-config allows users to change these values as part of valid configuration, but we do not want users to configure this option as the RHCOS image controls the runtime and kubelet versions we depend on.

abhinavdahiya · 2019-01-12T00:24:22Z

commit 38e8cad784a118df38ac4f0e10cffce70455a711 (HEAD -> drop_ami)
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 13:02:55 2019 -0800

    docs: update the resource dep graph

commit b07f23e0a2dafb889ac97087c78bcf4a303ddd68
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 10:33:15 2019 -0800

    types: drop image options from install config for all platforms

    RHCOS image that needs to be used for installation must be sources from RHCOS build pipeline and RHCOS build.
    Keeping the image related fields in install-config allows users to change these values as part of valid configuration, but we do not want users to configure this option as
    the RHCOS image controls the runtime and kubelet versions we depend on.

commit 255566fa916bd1e40e8b9e5b3ec688105afc32d0
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 12:51:22 2019 -0800

    types/openstack: drop GetImageNames method

    With baseimage user query removed from TUI, this function is no longer required.

    This also
    - drops the vendored files
    - updates the mocks using `hack/go-genmock.sh`

commit 94d99583dd7d65e6a6812485c82eadab1ffd6b97
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 11:17:33 2019 -0800

    asset/installconfig: drop image related code paths

commit 3eb958b278b67acc616c56d76b1ec85e17fcb5dd
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 10:42:49 2019 -0800

    pkg: use rhcos Image to fetch ami for AWS

commit 2bd6c9d1293bab2d7b50046d61f7d385c0588054
Author: Abhinav Dahiya <[email protected]>
Date:   Fri Jan 11 10:41:46 2019 -0800

    asset: add rhcos Image asset to generate image location

    added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.

@wking does this look more appropriate? i'm changing all consumers first then dropping the property..?

wking · 2019-01-12T00:31:43Z

/lgtm

openshift-ci-robot · 2019-01-12T00:31:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, wking

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~~ [abhinavdahiya,wking]

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

wking · 2019-01-12T05:01:09Z

e2e-aws had lots of:

fail [github.com/openshift/origin/test/extended/builds/build_pruning.go:52]: Expected error:
    <*errors.errorString | 0xc4217d9ac0>: {
        s: "Failed to import expected imagestreams",
    }
    Failed to import expected imagestreams
not to have occurred

ending up with:

Failing tests:

[Conformance][Area:Networking][Feature:Router] The HAProxy router should expose prometheus metrics for a route [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds] forcePull should affect pulling builder images  ForcePull test case execution custom [Suite:openshift/conformance/parallel]
[Feature:Builds] forcePull should affect pulling builder images  ForcePull test case execution docker [Suite:openshift/conformance/parallel]
[Feature:Builds] forcePull should affect pulling builder images  ForcePull test case execution s2i [Suite:openshift/conformance/parallel]
[Feature:Builds][Conformance] oc new-app  should fail with a --name longer than 58 characters [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][Conformance] oc new-app  should succeed with a --name of 58 characters [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][Conformance][valueFrom] process valueFrom in build strategy environment variables  should fail resolving unresolvable valueFrom in docker build environment variable references [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][Conformance][valueFrom] process valueFrom in build strategy environment variables  should fail resolving unresolvable valueFrom in sti build environment variable references [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][Conformance][valueFrom] process valueFrom in build strategy environment variables  should successfully resolve valueFrom in docker build environment variables [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][Conformance][valueFrom] process valueFrom in build strategy environment variables  should successfully resolve valueFrom in s2i build environment variables [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  [Conformance] buildconfigs should have a default history limit set when created via the group api [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  [Conformance] buildconfigs should not have a default history limit set when created via the legacy api [Suite:openshift/conformance/parallel/minimal]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  should prune builds after a buildConfig change [Suite:openshift/conformance/parallel]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  should prune canceled builds based on the failedBuildsHistoryLimit setting [Suite:openshift/conformance/parallel]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  should prune completed builds based on the successfulBuildsHistoryLimit setting [Suite:openshift/conformance/parallel]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  should prune errored builds based on the failedBuildsHistoryLimit setting [Suite:openshift/conformance/parallel]
[Feature:Builds][pruning] prune builds based on settings in the buildconfig  should prune failed builds based on the failedBuildsHistoryLimit setting [Suite:openshift/conformance/parallel]
[Feature:DeploymentConfig] deploymentconfigs when run iteratively [Conformance] should immediately start a new deployment [Suite:openshift/conformance/parallel/minimal]
[Feature:DeploymentConfig] deploymentconfigs with multiple image change triggers [Conformance] should run a successful deployment with a trigger used by different containers [Suite:openshift/conformance/parallel/minimal]
[Feature:DeploymentConfig] deploymentconfigs with multiple image change triggers [Conformance] should run a successful deployment with multiple triggers [Suite:openshift/conformance/parallel/minimal]
[image_ecosystem][mongodb] openshift mongodb image  creating from a template should instantiate the template [Suite:openshift/conformance/parallel]

/retest

openshift-bot · 2019-01-12T05:03:34Z

/retest

Please review the full test history for this PR and help us cut down flakes.

The `baseImage` option is no longer doing anything: openshift/installer#1052 The image must be called `rhcos`.

dgoodwin · 2019-01-23T13:08:35Z

Breaking change for Hive. :( But the env var should cover us for now. Will raise in arch call about how we should handle this image ID long term, across upgrades, in an external system like Hive.

openshift-ci-robot requested review from crawford and wking January 11, 2019 18:47

abhinavdahiya force-pushed the drop_ami branch from 8477a8b to d656937 Compare January 11, 2019 19:22

abhinavdahiya changed the title ~~WIP: drop capability to specify image in install-config~~ drop capability to specify image in install-config Jan 11, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 11, 2019

staebler reviewed Jan 11, 2019

View reviewed changes

wking reviewed Jan 11, 2019

View reviewed changes

abhinavdahiya added this to the Freeze milestone Jan 11, 2019

abhinavdahiya force-pushed the drop_ami branch from 0795a7c to 2b24684 Compare January 11, 2019 23:52

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 11, 2019

wking reviewed Jan 12, 2019

View reviewed changes

abhinavdahiya added 6 commits January 11, 2019 16:16

asset: add rhcos Image asset to generate image location

2bd6c9d

added OPENSHIFT_INSTALL_OS_IMAGE_OVERRIDE env with warning to override the image that will be used.

pkg: use rhcos Image to fetch ami for AWS

3eb958b

asset/installconfig: drop image related code paths

94d9958

types/openstack: drop GetImageNames method

255566f

With baseimage user query removed from TUI, this function is no longer required. This also - drops the vendored files - updates the mocks using `hack/go-genmock.sh`

docs: update the resource dep graph

38e8cad

abhinavdahiya force-pushed the drop_ami branch from 2b24684 to 38e8cad Compare January 12, 2019 00:22

openshift-ci-robot assigned wking Jan 12, 2019

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

staebler mentioned this pull request Jan 12, 2019

Remove clusterID from installconfig and move it to cluster #1057

Merged

wking mentioned this pull request Jan 12, 2019

types/libvirt: remove per-machine-pool configuration #812

Closed

openshift-merge-robot merged commit f583689 into openshift:master Jan 12, 2019

This was referenced Jan 12, 2019

assets/installconfig: apply defaults to install config #902

Closed

unable to override AMI using amiID #991

Closed

tomassedovic added a commit to imain/ocp-doit that referenced this pull request Jan 18, 2019

Remove baseImage from install-config.yaml

02ba7e0

The `baseImage` option is no longer doing anything: openshift/installer#1052 The image must be called `rhcos`.

wking mentioned this pull request Jan 31, 2019

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop capability to specify image in install-config #1052

drop capability to specify image in install-config #1052

abhinavdahiya commented Jan 11, 2019 •

edited

Loading

staebler left a comment

abhinavdahiya commented Jan 11, 2019

wking Jan 11, 2019 •

edited

Loading

abhinavdahiya Jan 11, 2019

abhinavdahiya Jan 11, 2019

wking Jan 12, 2019

abhinavdahiya commented Jan 12, 2019

wking left a comment

wking commented Jan 12, 2019

abhinavdahiya commented Jan 12, 2019

wking commented Jan 12, 2019

openshift-ci-robot commented Jan 12, 2019

wking commented Jan 12, 2019

openshift-bot commented Jan 12, 2019

dgoodwin commented Jan 23, 2019

drop capability to specify image in install-config #1052

drop capability to specify image in install-config #1052

Conversation

abhinavdahiya commented Jan 11, 2019 • edited Loading

staebler left a comment

Choose a reason for hiding this comment

abhinavdahiya commented Jan 11, 2019

wking Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya Jan 11, 2019

Choose a reason for hiding this comment

abhinavdahiya Jan 11, 2019

Choose a reason for hiding this comment

wking Jan 12, 2019

Choose a reason for hiding this comment

abhinavdahiya commented Jan 12, 2019

wking left a comment

Choose a reason for hiding this comment

wking commented Jan 12, 2019

abhinavdahiya commented Jan 12, 2019

wking commented Jan 12, 2019

openshift-ci-robot commented Jan 12, 2019

wking commented Jan 12, 2019

openshift-bot commented Jan 12, 2019

dgoodwin commented Jan 23, 2019

abhinavdahiya commented Jan 11, 2019 •

edited

Loading

wking Jan 11, 2019 •

edited

Loading