Skip to content

Add RHCOSBaseURI to templating for bootstrap files #2035

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hardys wants to merge 1 commit into from
Closed

Add RHCOSBaseURI to templating for bootstrap files #2035

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions pkg/asset/ignition/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ type bootstrapTemplateData struct {
ReleaseImage string
Proxy *configv1.ProxyStatus
Registries []sysregistriesv2.Registry
RHCOSBaseURI string
}

// Bootstrap is an asset that generates the ignition config for bootstrap nodes.
Expand Down Expand Up @@ -216,12 +217,26 @@ func (a *Bootstrap) getTemplateData(installConfig *types.InstallConfig, releaseI
}
}

var rhcosImageBaseURI string
if ri, ok := os.LookupEnv("OPENSHIFT_INSTALL_IMAGE_BASE_URI_OVERRIDE"); ok && ri != "" {
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry but this new env variable cannot be added. These env variables bypass installer input models and we would like to keep them to moving towards zero number.

Can you provide how you expect this variable to be used?

Copy link
Author

@hardys hardys Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I can remove it, I was following the existing conventions, and I was thinking it would be a better alternative to the "hidden" CACHEURL in openshift-metal3/kni-installer@c3e0e8e

The reason this is needed/useful is that for both repeated local testing and CI downloading the large RHCOS image is slow and a waste of bandwidth - also it may be required to avoid hitting installer hard-coded timeouts when using a slow connection that cannot download the images fast enough.

If we remove this variable, I'll need to add a new install-config variable for the baremetal platform that can provide the IP for the CACHEURL (since it's hard-coded in the commit referenced above), would that be a more acceptable approach?

logrus.Warn("Found override for rhcosImageBaseURI. Please be warned, this is not advised")
rhcosImageBaseURI = ri
} else {
var err error
rhcosImageBaseURI, err = rhcos.BaseURI()
if err != nil {
return nil, err
}
logrus.Debugf("Using internal constant for release image base URI %s", rhcosImageBaseURI)
}

return &bootstrapTemplateData{
PullSecret: installConfig.PullSecret,
ReleaseImage: releaseImage,
EtcdCluster: strings.Join(etcdEndpoints, ","),
Proxy: &proxy.Status,
Registries: registries,
RHCOSBaseURI: rhcosImageBaseURI,
}, nil
}

Expand Down
23 changes: 23 additions & 0 deletions pkg/rhcos/baseuri.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package rhcos

import (
"context"
"net/url"

"github.com/pkg/errors"
)

// BaseURI fetches the BaseURI where images can be downloaded from
func BaseURI() (string, error) {
Copy link
Contributor

@abhinavdahiya abhinavdahiya Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This information is rather kept private to the package... Please use or add other image functions that provide th boot-image directly.

Copy link
Author

@hardys hardys Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the downloader container we're using requires the base URI

https://github.com/openshift/ironic-rhcos-downloader/blob/master/get-resource.sh

We can rework that, or pass the full URL to the openstack image perhaps. cc @derekhiggins

Copy link
Contributor

@derekhiggins derekhiggins Jul 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rework that, or pass the full URL to the openstack image perhaps. cc @derekhiggins

Sounds fine to me, let me know if you want to update the parameters the downloader container expects,

meta, err := fetchRHCOSBuild(context.TODO())
if err != nil {
return "", errors.Wrap(err, "failed to fetch RHCOS metadata")
}

base, err := url.Parse(meta.BaseURI)
if err != nil {
return "", err
}

return base.String(), nil
}