Skip to content

Conversation

@Fedosin
Copy link
Contributor

@Fedosin Fedosin commented Jan 22, 2020

Now we store bootstrap ignition configs in Swift storage, but unfortunately Swift is not available on all OpenStack clouds. It leads to the fact that we can't deploy OpenShift on such swiftless clouds.

This PR allows to store configs in Glance, which is available on 100% of OpenStack clouds.

The newest version includes important features, like HTTP headers support,
that are required for the successful installation of OpenShift on OpenStack
without Swift.
@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jan 22, 2020
@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 22, 2020

openstack tests will fail until we upgrade the rhcos image in the installer

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 22, 2020

/retest

2 similar comments
@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 22, 2020

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 23, 2020

/retest

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 23, 2020

/test e2e-aws

@pierreprinetti
Copy link
Member

/retest

1 similar comment
@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 23, 2020

/retest

@wking
Copy link
Member

wking commented Jan 23, 2020

Looks like the OpenStack job failed to come up? AWS came up fine, so the bump to Ignition spec 2.4 doesn't seem to have caused a problem. Still would be nice to see those green though.

/retest

@abhinavdahiya
Copy link
Contributor

Dropping the Swift from destroy means that if I created a cluster using 4.2 and tried to destroy the closer with this new installer, we leak the Swift objects

@iamemilio
Copy link

That's a good point. I think we have to support swift deletion indefinitely.

@iamemilio
Copy link

Once you address Abhinav's comment, LGTM

@wking
Copy link
Member

wking commented Jan 24, 2020

Hmm, fresh OpenStack run died the same way?

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

@wking yeah,to make OpenStack test pass we need to bump Ignition in the pre-release image first: https://mirror.openshift.com/pub/openshift-v4/dependencies/rhcos/pre-release/
Because the minimal required Ignition version for this change is 0.35.
For instance, the latest 4.3 release already has it: https://mirror.openshift.com/pub/openshift-v4/dependencies/rhcos/4.3/latest/

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

@abhinavdahiya I didn't drop Swift from the destroy module. I just added a check that if there is no Swift service we are fine with it: https://github.com/openshift/installer/pull/2960/files#diff-3420abf882f61be73bc30ef12460687dR547-R550 deleteContainers function in pkg/destroy/openstack/openstack.go
So, cluster destruction will work on all OCP versions after this change.

I removed Swift code from bootstrap destruction (pkg/destroy/openstack/swift.go), and I think this is right. Now we create a Swift container, upload the config there, give the link to the bootstrap machine and then immediately destroy the container when bootstrapping is over using DeleteSwiftContainer function from pkg/destroy/openstack/swift.go.
With this patch instead of creating a container, we do the same things with Glance image.

So, this patch only affects the bootstrapping process and all other parts stay the same.

)

// DeleteSwiftContainer deletes a container and all of its objects.
func DeleteSwiftContainer(name string, cloud string) error {
Copy link
Member

Choose a reason for hiding this comment

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

It is strange that you can now safely remove this. What if folks want to delete a cluster created by a previous installer (and so using Swift) which got a Swift bootstrap Ignition store, but then failed to install. Wouldn't that leak the Swift container with destroy cluster not using this function? Of course, the fact that nothing else in pkg/destroy/openstack calls this function means that leak is not new in this PR. But it still seems like a leak to me.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, @Fedosin points out that this function was pretty similar to deleteContainers, which is left alone by this PR.

@pierreprinetti
Copy link
Member

/retest

@mandre
Copy link
Member

mandre commented Jan 24, 2020

/lgtm
/hold until we bump the rhcos image #2971

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

openstack tests are green with the RHCOS bump #2971: #2973

@sdodson
Copy link
Member

sdodson commented Jan 24, 2020

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2020
@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

/test e2e-openstack

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

openstack tests are green

@Fedosin
Copy link
Contributor Author

Fedosin commented Jan 24, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020
@openshift-bot
Copy link
Contributor

/retest

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

3 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 75ad924 into openshift:master Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-libvirt 52d52b9 link /test e2e-libvirt
ci/prow/e2e-aws-scaleup-rhel7 52d52b9 link /test e2e-aws-scaleup-rhel7

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.

Details

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.

eramoto added a commit to eramoto/installer that referenced this pull request Sep 29, 2022
Some openstack environments do not allow users to upload or download image data.
This is because the cloud providers prevent them from using illegal images on
the openstack or getting proprietary images on a local.

This PR is based on the Swift functions removed in PR openshift#2960 and adds the feature
to store a ignition file in Swift when an upload in Glance fails, as follows:

   Glance | Swift  | used
   enable | enable | storage
  --------+--------+---------
      v   |   v    | Glance
      v   |        | Glance
          |   v    | Swift   <- this PR
          |        | none

Signed-off-by: ERAMOTO Masaya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants