Skip to content

Conversation

@hardys
Copy link

@hardys hardys commented Oct 2, 2018

Adds initial destroy support for OpenStack, currently this only considers Server resources, network resources will be considered either via an update to this PR or follow-up PR's, whichever is preferable.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2018
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Oct 2, 2018
@hardys hardys force-pushed the openstack_destroy branch 2 times, most recently from d263585 to 7af4bfc Compare October 2, 2018 16:50
@hardys hardys mentioned this pull request Oct 2, 2018
13 tasks
@hardys hardys force-pushed the openstack_destroy branch from 7af4bfc to ba2569c Compare October 3, 2018 10:34
@openshift-ci-robot 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 Oct 3, 2018
@hardys hardys changed the title WIP OpenStack Destroy OpenStack Destroy Oct 3, 2018
@openshift-ci-robot 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 Oct 3, 2018
@hardys hardys changed the title OpenStack Destroy OpenStack Destroy Initial support Oct 3, 2018
@hardys
Copy link
Author

hardys commented Oct 3, 2018

Ok I added the vendored dependencies, and rebased on the latest logger changes - this is now working well for me to delete the servers.

Do we want to land this and incrementally add the remaining resources (network/subnet/port/router), or should I keep pushing to this one PR?

@hardys
Copy link
Author

hardys commented Oct 3, 2018

Also I removed WIP as this now works, albeit only for the server resources.

Copy link
Member

Choose a reason for hiding this comment

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

nit: This is missing trailing period.

Some of your other godocs comments are as well. Obviously not the end of the world, but should be easy to fix ;).

And we probably have a number of godocs in master with the same issue; PRs welcome :).

Copy link
Author

Choose a reason for hiding this comment

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

Ah thanks, I'll go through and check - is there a way to build the godocs locally so I can check the format before pushing?

Copy link
Member

Choose a reason for hiding this comment

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

Ah thanks, I'll go through and check - is there a way to build the godocs locally so I can check the format before pushing?

$ godoc ./pkg/destroy
PACKAGE DOCUMENTATION

package destroy
    import "./pkg/destroy"

    Package destroy contains tools for destroying clusters based on their
    metadata.

VARIABLES

var Registry = make(map[string]NewFunc)
    Registry maps ClusterMetadata.Platform() to per-platform Destroyer
    creators.

TYPES

type Destroyer interface {
    Run() error
}
    Destroyer allows multiple implementations of destroy for different
    platforms.

func New(logger logrus.FieldLogger, rootDir string) (Destroyer, error)
    New returns a Destroyer based on `metadata.json` in `rootDir`.

func NewAWS(logger logrus.FieldLogger, metadata *types.ClusterMetadata) (Destroyer, error)
    NewAWS returns an AWS destroyer from ClusterMetadata.

type NewFunc func(logger logrus.FieldLogger, metadata *types.ClusterMetadata) (Destroyer, error)
    NewFunc is an interface for creating platform-specific destroyers.

SUBDIRECTORIES

	libvirt

@hardys
Copy link
Author

hardys commented Oct 3, 2018

Note I spent some time today looking at adding tags support for the network resources - unfortunately neither gophercloud or terraform-provider-openstack yet support tags for neutron resources, so looks like I'll have to add those before I can make progress, unless anyone has other suggestions on how we might identify the resources for removal?

@russellb
Copy link
Contributor

russellb commented Oct 3, 2018

Can we abuse the resource names as a way to tag them for removal?

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2018
@hardys
Copy link
Author

hardys commented Oct 4, 2018

Can we abuse the resource names as a way to tag them for removal?

Yeah I guess so, and the libvirt destroy code does that (looks for a prefix), the names would end up pretty ugly though (particularly since the installer isn't idempotent so it could easily reuse a cluster name on each run creating duplicate resources, sigh).

I did look at gophercloud today and writing the tags looks fairly simple ref gophercloud/gophercloud#1259 (WIP)

@hardys hardys force-pushed the openstack_destroy branch from ba2569c to 86ae539 Compare October 26, 2018 14:29
@flaper87
Copy link
Contributor

(it won't succeed but FYI)

/test e2e-openstack

@hardys hardys force-pushed the openstack_destroy branch from 86ae539 to 237edcc Compare October 26, 2018 15:50
@hardys
Copy link
Author

hardys commented Oct 26, 2018

Ok I'll mark this WIP although the top server commit could potentially land now - the network patches will all depend on terraform-provider-openstack changes that are under review ref https://github.com/terraform-providers/terraform-provider-openstack/issues/453

If anyone wants to help test you can pull the staged tree with all commits at https://github.com/hardys/terraform-provider-openstack/commits/neutron_tags_wip

@hardys hardys changed the title OpenStack Destroy Initial support WIP: OpenStack Destroy Initial support Oct 26, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2018
@hardys hardys changed the title WIP: OpenStack Destroy Initial support WIP: OpenStack Destroy Cluster Support Oct 26, 2018
@hardys
Copy link
Author

hardys commented Oct 26, 2018

Note this was blocked on gophercloud patches (all now landed and included in the vendored files in the commits I just pushed), and terraform-provider-openstack patches (under review as mentioned).

I've rebased this and the server/port patches work for me, so I'll go ahead and push support for the other resources so hopefully this will be in good shape when the terraform-provider-openstack patches merge (hopefully next week but we'll see how it goes).

@hardys hardys force-pushed the openstack_destroy branch from 237edcc to 91173b6 Compare October 26, 2018 16:18
@hardys hardys force-pushed the openstack_destroy branch from 18abe06 to 35a9e15 Compare November 6, 2018 15:40
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2018
@hardys
Copy link
Author

hardys commented Nov 6, 2018

@flaper87 @tomassedovic as discussed I rebased this on the latest #588 and squashed to two commits, the vendor additions for gophercloud and the destroy support itself.

You can test it by deploying as normal, then doing something like:

./bin/openshift-install --log-level=debug --dir rhcos destroy cluster

1 similar comment
@hardys
Copy link
Author

hardys commented Nov 6, 2018

@flaper87 @tomassedovic as discussed I rebased this on the latest #588 and squashed to two commits, the vendor additions for gophercloud and the destroy support itself.

You can test it by deploying as normal, then doing something like:

./bin/openshift-install --log-level=debug --dir rhcos destroy cluster

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this package anywhere (and my build fails because of that). Is it possible you've left it out in the squash/rebase?

Copy link
Author

Choose a reason for hiding this comment

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

@tomassedovic oops, yes you're right, the new package got lost due to the most recent rebase, I'll re-add it and rebase again now #588 landed, thanks for spotting it!

@hardys hardys changed the title OpenStack Destroy Cluster Support WIP: OpenStack Destroy Cluster Support Nov 13, 2018
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 13, 2018
@hardys
Copy link
Author

hardys commented Nov 13, 2018

Marking WIP again since I need to address the rebase issue and we're still blocked on a new release of terraform-provider-openstack, hopefully that will happen this week

This adds compute and networking gophercloud APIs, required for
OpenStack destroy support.
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 16, 2018
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 16, 2018

@hardys: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-openstack 18abe065cca88801b277965208ec328e9905a715 link /test e2e-openstack

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.

@hardys
Copy link
Author

hardys commented Nov 16, 2018

https://github.com/terraform-providers/terraform-provider-openstack/releases/tag/v1.12.0 has been released so I'll adjust the commit message and remove the WIP

This adds initial support for destroy for OpenStack

Note that because gophercloud doesn't currently support nova tags[1]
I've used Metadata for servers (which is already populated with the
tectonicClusterId via the server properties).

Also note this requires changes to the terraform-provider-openstack which
are in the latest 1.12 release:

https://github.com/terraform-providers/terraform-provider-openstack/releases/tag/v1.12.0
https://github.com/terraform-providers/terraform-provider-openstack/issues/453
@hardys hardys changed the title WIP: OpenStack Destroy Cluster Support OpenStack Destroy Cluster Support Nov 16, 2018
@openshift-ci-robot 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 Nov 16, 2018
@flaper87
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2018
@flaper87
Copy link
Contributor

/hold cancel
/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flaper87, hardys

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

@flaper87
Copy link
Contributor

/ok-to-test

@openshift-ci-robot openshift-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 16, 2018
@openshift-merge-robot openshift-merge-robot merged commit 5a74060 into openshift:master Nov 16, 2018
hardys pushed a commit to hardys/installer that referenced this pull request Nov 19, 2018
This was missed in openshift#391 so this adds support for cleaning up the
created container/object (used for the bootstrap ignition config)
hardys pushed a commit to hardys/ocp-doit that referenced this pull request Nov 22, 2018
Now that openshift/installer#391 landed
we can replace the bash script with the cluster destroy command.

Note you will need to pull the latest openshift/installer then
re-run 05_build_ocp_installer.sh for this to work.
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.

8 participants