Skip to content

Delete project should delete all project related resources #1601

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

Jump to bottom
Merged
merged 1 commit into from
Apr 7, 2015
Merged

Delete project should delete all project related resources #1601

merged 1 commit into from
Apr 7, 2015

Conversation

derekwaynecarr
Copy link
Member

@smarterclayton @liggitt @jwforres - please take a quick look.

this builds off the cherry pick from:
#1570

@jwforres jwforres added this to the 0.5.0 milestone Apr 6, 2015
@derekwaynecarr derekwaynecarr force-pushed the ns_delete branch 2 times, most recently from 8aac026 to a6e4c38 Compare April 6, 2015 18:43
@derekwaynecarr
Copy link
Member Author

It would have helped if my pushed branch included the unit testing changes.

Summary of changes:

  1. When running embedded Kubernetes, namespace controller is started to handle life-cycle.
  2. Update origin controller to now do finalize correctly when it purges OpenShift content.
  3. Update unit tests for origin controller.
  4. Align Project REST patterns with latest upstream.
  5. Added support for editing a project (since we are now on latest patterns)
  6. Added tests to ensure our strategy does the right thing on create vs update to align with upstream.

@smarterclayton @jwforres - this should be good to review now. this fixes the inability to delete a project in OpenShift right now ;-)

@derekwaynecarr
Copy link
Member Author

@deads2k - if you have time, I just need someone to review ;-)

deads2k
deads2k reviewed Apr 6, 2015
View reviewed changes
pkg/cmd/server/kubernetes/master.go
@@ -83,6 +82,12 @@ func (c *MasterConfig) InstallAPI(container *restful.Container) []string {
}
}

func (c *MasterConfig) RunNamespaceController() {
namespaceController := namespace.NewNamespaceManager(c.KubeClient)
namespaceController.Run(1 * time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems slow. Does this mean that my namespace may not be cleaned up for a minute? During that minute, can I keep creating things?

Copy link
Member Author

Choose a reason for hiding this comment

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

This means you will see a namespace appear in "Terminating" status for ~1 min. The value is the same default as upstream. You are unable to create content in a terminating namespace. There are two controllers that need to run in order to delete content (the k8s one, and the openshift one) so it will take ~1-2 minutes for something to truly be gone.

@deads2k
Copy link
Contributor

deads2k commented Apr 6, 2015

Minor comments. Otherwise, lgtm

@derekwaynecarr
Copy link
Member Author

Assuming no other comments will look to merge in morning. Thanks for review David

Sent from my iPhone

On Apr 6, 2015, at 4:13 PM, David Eads [email protected] wrote:

Minor comments. Otherwise, lgtm

—
Reply to this email directly or view it on GitHub.

@deads2k
Copy link
Contributor

deads2k commented Apr 7, 2015

after strategies move to pkg/project/registry/project/rest.go, lgtm.

@derekwaynecarr derekwaynecarr force-pushed the ns_delete branch 2 times, most recently from 3c41ee1 to 4c62345 Compare April 7, 2015 17:55
@derekwaynecarr
Copy link
Member Author

changes made per request.

will merge this now.

@derekwaynecarr
Copy link
Member Author

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/1497/) (Image: devenv-fedora_1224)

@derekwaynecarr
Copy link
Member Author

re [merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin up to cc1b809

@deads2k
Copy link
Contributor

deads2k commented Apr 7, 2015

Which transient did you fail on?

openshift-bot pushed a commit that referenced this pull request Apr 7, 2015
Merge pull request #1601 from derekwaynecarr/ns_delete
dc9bff9 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit dc9bff9 into openshift:master Apr 7, 2015
@smarterclayton smarterclayton changed the title Delete project results in deleting all project related resources Delete project should delete all project related resources Apr 8, 2015
@smarterclayton smarterclayton modified the milestone: 0.5.0 (beta3) Apr 23, 2015
@smarterclayton smarterclayton modified the milestone: 0.5.0 (beta3) Apr 23, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.5.0 (beta3)
Development

Successfully merging this pull request may close these issues.
5 participants
@derekwaynecarr @deads2k @openshift-bot @smarterclayton @jwforres