-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Add support for 'osc stop|label' from upstream #1944
Add support for 'osc stop|label' from upstream #1944
Conversation
# Shut down all resources in the path/to/resources directory | ||
$ %[1]s stop -f path/to/resources | ||
` | ||
cmd.Long = fmt.Sprintf(longDesc, fullName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reaper needs to be implemented for DeploymentConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do that in a follow on pull, but I don't want to expose stop for osc until that works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I'll work on it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@smarterclayton fyi after the latest change in stop code we can use osc stop
as is since if the passed resource doesn't implement the Reaper, stop will fallback to delete (https://github.com/GoogleCloudPlatform/kubernetes/blob/e1256c08027748c4a0dc8078f1f8db5afb673923/pkg/kubectl/cmd/delete.go#L112).
Regarding the implementation, DC first has to support the Resizer and then the Reaper. How are we going to implement those since DC is OpenShift-specific resource and we need to make that change in Kubernetes?
https://github.com/GoogleCloudPlatform/kubernetes/blob/e1256c08027748c4a0dc8078f1f8db5afb673923/pkg/kubectl/stop.go#L51
https://github.com/GoogleCloudPlatform/kubernetes/blob/e1256c08027748c4a0dc8078f1f8db5afb673923/pkg/kubectl/resize.go#L92
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe I'm wrong about the resizer but still...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
----- Original Message -----
+Examples:
+
Shut down foo.
- $ %[1]s stop replicationcontroller foo
Stop pods and services with label name=myLabel.
- $ %[1]s stop pods,services -l name=myLabel
Shut down the service defined in service.json
- $ %[1]s stop -f service.json
Shut down all resources in the path/to/resources directory
- $ %[1]s stop -f path/to/resources
+`- cmd.Long = fmt.Sprintf(longDesc, fullName)
Well, maybe I'm wrong about the resizer but still...
Resizer I believe has an abstraction as well that goes into the Factory. There are other issues with Resize though, which is that currently deployments aren't preserving resize and it's not clear whether the deployment configs value for replicas should be used. For now, Resizer resizing the current dcs/deployment would be most correct.
Will hold on to this until the next Kube rebase. Then it can be merged w/o implementing the Reaper for dc. |
This still needs to implement the Reaper but for doing that dcs first have to implement the Resizer and then resizing down to zero is all this will take. |
For a DC, the resizer should find the most recent (highest deployment number) rc associated with that deployment, and apply resize to that deployment. Can you make that change?
|
I will give it a go. |
I've updated this PR to work with the resizing PR. Merged both of them in a test branch and:
|
@@ -13,6 +13,64 @@ import ( | |||
) | |||
|
|||
const ( | |||
stop_long = `Gracefully shut down a resource by id or filename. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stopLong and stopExample
I didn't actually delete the dc. Fixed. |
[ $(osc describe bc/ruby-helloworld-sample | grep acustom=label) ] | ||
echo "label: ok" | ||
osc stop svc/ruby-helloworld-sample | ||
osc delete all -l app=dockerbuild |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify the dc was deleted in your test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was about to do that
Travis is failing |
[test] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2248/) |
re[test] |
Failing due to
might be a race introduced by your changes |
The race should be fixed by #2294 |
The deployment pod isn't deleted when stopping a dc... Looking at it. |
That's the responsibility of the deployment controller.
|
It seems like the deployment controller will delete the deployer pod only when the deployment is successful. Does the replication controller have a status at all when it's deleted by a cli command? Should we change its status to complete before deleting it (can we even do that?) so that the deployer controller can delete the pod or should we just go on by manually deleting it? |
----- Original Message -----
The deployment controller will try to start it again, which we don't want.
We shouldn't be changing anything on the RC - that's the responsibility of the deployment controller to manage, and it's likely we'd end up having code in two places. I'd prefer that we not be deleting it from the CLI.
|
@smarterclayton this is ready for merging. The error on Travis is fixed here: #2308 |
Re deployer pod, opened a separate issue to track it down: #2316 |
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) | ||
cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out)) | ||
cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you reorder these? The order is significant, and is intentional. Please revert the ordering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure of the previous order but now the commands are alphabetically sorted in two groups: OpenShift-specific and Kubernetes wrappers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want that. The order is intentional, most used commands followed by lesser used commands.
----- Original Message -----
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
I am not sure of the previous order but now the commands are
alphabetically sorted in two groups: OpenShift-specific and Kubernetes
wrappers.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30515535
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, for what it's worth (not much i'm sure) i think alphabetic makes more sense. if i'm trying to scan for the usage of a command i know the name of, it's easier to find if the list is alphabetical. and if i don't know the name of the command i'm looking for, i still may have guesses which are going to lead me to navigate the list in an alphabetic search.
i don't really see the value in having the most common ones at the top, given that the whole list is going to be displayed anyway (in fact you could argue the most common ones should be at the bottom since that's what's going to be front and center after someone runs osc)
----- Original Message -----
From: "Clayton Coleman" notifications@github.com
To: "openshift/origin" origin@noreply.github.com
Cc: "Ben Parees" bparees@redhat.com
Sent: Monday, May 18, 2015 1:34:59 PM
Subject: Re: [origin] Add support for 'osc stop|label' from upstream (#1944)cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
We don't want that. The order is intentional, most used commands followed by
lesser used commands.----- Original Message -----
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out))
cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout",
fullName+"
login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
I am not sure of the previous order but now the commands are
alphabetically sorted in two groups: OpenShift-specific and Kubernetes
wrappers.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30515535
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30529266
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The organization of the main help is going to be like "git help". Ordered by function and utility. The group boundaries will be based on the order.
----- Original Message -----
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
hm, for what it's worth (not much i'm sure) i think alphabetic makes more
sense. if i'm trying to scan for the usage of a command i know the name of,
it's easier to find if the list is alphabetical. and if i don't know the
name of the command i'm looking for, i still may have guesses which are
going to lead me to navigate the list in an alphabetic search.i don't really see the value in having the most common ones at the top, given
that the whole list is going to be displayed anyway (in fact you could argue
the most common ones should be at the bottom since that's what's going to be
front and center after someone runs osc)----- Original Message -----
From: "Clayton Coleman" notifications@github.com
To: "openshift/origin" origin@noreply.github.com
Cc: "Ben Parees" bparees@redhat.com
Sent: Monday, May 18, 2015 1:34:59 PM
Subject: Re: [origin] Add support for 'osc stop|label' from upstream
(#1944)cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out))
cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout",
fullName+"
login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
We don't want that. The order is intentional, most used commands followed
by
lesser used commands.----- Original Message -----
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
I am not sure of the previous order but now the commands are
alphabetically sorted in two groups: OpenShift-specific and
Kubernetes
wrappers.
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30515535
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30529266
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30530444
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we sort within group boundaries? please? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No :) See "rhc help" for what you're going to get :)
----- Original Message -----
cmds.AddCommand(cmd.NewCmdLogin(fullName, f, in, out)) cmds.AddCommand(cmd.NewCmdLogout("logout", fullName+" logout", fullName+" login", f, in, out))
- cmds.AddCommand(cmd.NewCmdNewApplication(fullName, f, out))
can we sort within group boundaries? please? :)
Reply to this email directly or view it on GitHub:
https://github.com/openshift/origin/pull/1944/files#r30530947
This commit adds two commands from Kubernetes: stop and label. While label was rather trivial to import in OpenShift, for stop to be fully functional deploymentConfigs had first to implement the kubectl.Resizer interface (work on the Resizer can be found in #2158) which is a prerequisite for another needed interface, kubectl.Reaper (work on the Reaper can be found here).
LGTM [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/2248/) (Image: devenv-fedora_1564) |
Evaluated for origin up to 633b1b9 |
Merged by openshift-bot
cc: @smarterclayton @bparees (https://trello.com/c/XxiMNEHv/553-add-support-for-osc-stop-label-from-upstream-codefreeze)