-
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
View the logs for the latest deployment of a config #3943
Conversation
Currently
What am I missing? |
Don't run the command using You need to add an entry in https://github.com/openshift/origin/blob/master/pkg/cmd/server/bootstrappolicy/policy.go#L78 for the |
Also, how close are your changes to defaulting resources? I think I'd rather carry reasonable patches on |
|
||
// NoWait if true causes the call to return immediately even if the deployment | ||
// is not available yet. Otherwise the server will wait until the deployment has started. | ||
NoWait bool |
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.
How about inverting this to something like Immediate
or Wait
which defaults to false
?
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.
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 had the same thought re: upstream... it seems unfortunate that we'd need to make an API revision for such generic options.
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.
If this doesn't go upstream we won't be able to specify it via oc logs
(when oc logs will support builds and deployments).
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 don't think in the short term there is a requirement that we use oc logs
for all types of logs. It's a nice to have.
On Wed, Jul 29, 2015 at 10:43 AM, Michail Kargakis <notifications@github.com
wrote:
In pkg/deploy/api/types.go
#3943 (comment):+type DeploymentLogs struct {
- kapi.TypeMeta
- kapi.ListMeta
+}
+// DeploymentLogOptions is the REST options for a deployment log
+type DeploymentLogOptions struct {
- kapi.TypeMeta
- // Follow if true indicates that the deployment log should be streamed until
- // the deployment terminates.
- Follow bool
- // NoWait if true causes the call to return immediately even if the deployment
- // is not available yet. Otherwise the server will wait until the deployment has started.
- NoWait bool
If this doesn't go upstream we won't be able to specify it via oc logs
(when oc logs will support builds and deployments).—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3943/files#r35767333.
Clayton Coleman | Lead Engineer, OpenShift
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.
Note I'm not against trying to solve these, just that it's not required to
ship 3.1
On Wed, Jul 29, 2015 at 4:03 PM, Clayton Coleman ccoleman@redhat.com
wrote:
I don't think in the short term there is a requirement that we use oc logs
for all types of logs. It's a nice to have.On Wed, Jul 29, 2015 at 10:43 AM, Michail Kargakis <
notifications@github.com> wrote:In pkg/deploy/api/types.go
#3943 (comment):+type DeploymentLogs struct {
- kapi.TypeMeta
- kapi.ListMeta
+}
+// DeploymentLogOptions is the REST options for a deployment log
+type DeploymentLogOptions struct {
- kapi.TypeMeta
- // Follow if true indicates that the deployment log should be streamed until
- // the deployment terminates.
- Follow bool
- // NoWait if true causes the call to return immediately even if the deployment
- // is not available yet. Otherwise the server will wait until the deployment has started.
- NoWait bool
If this doesn't go upstream we won't be able to specify it via oc logs
(when oc logs will support builds and deployments).—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3943/files#r35767333.Clayton Coleman | Lead Engineer, OpenShift
Clayton Coleman | Lead Engineer, OpenShift
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 can you make a call on "NoWait" vs. something like "Immediate"? Super confusing name for a boolean.
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.
Make this consistent with BuildLogOptions for now, we'll fix it in v2 api.
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.
Make this consistent with BuildLogOptions
it already is
Those changes are already functional, we just need to reach consensus on Tbh I would prefer just |
|
||
deployment, err := r.Deploy.GetDeployment(ctx, deployutil.LatestDeploymentNameForConfig(config)) | ||
if err != nil { | ||
// TODO: Fallback to latest-1? |
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.
Less efficient, but you could solve this by using the same strategy as https://github.com/openshift/origin/blob/master/pkg/cmd/cli/cmd/rollback.go#L311
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.
done
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.
It would be simpler to always just use deployment.Items[0]
... is there a case where that's insufficient?
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.
Yes, we may have a list of deployments not containing the latest.
But using Items[0]
and just checking versions is enough. Updated.
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.
Based on a comment in IRC, I was totally wrong about this... I think what you originally had was right. If you fall back at all, then you might get logs for N-1 if N completed since deployer pods are deleted upon successful completion.
I think @smarterclayton had some opinion on this too. I agree that having different log commands for these things is awkward. |
I see the deployer pod missing after finishing the deployment. Is this expected? @ironcladlou
|
|
||
// DeploymentClient defines a local interface to a deployment client for testability. | ||
type DeploymentClient interface { | ||
GetDeploymentConfig(ctx kapi.Context, name string) (*deployapi.DeploymentConfig, error) |
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 think we've reached agreement to use a client.DeploymentConfigNamespacer
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 think we've reached agreement to use a client.DeploymentConfigNamespacer
Agree. I would put waitForDeployment
behind an interface or typedef'd function for testing, though.
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.
Is a DeploymentConfigNamespacer
enough? I mostly need to interact with the deployments (list, watch) so I guess I need a ReplicationControllersNamespacer
as well, right?
This is ready for another review. |
Should deploymentlog have a description in |
buildlog was grandfathered in, but we want to fully document our REST API, so I doubt we'll allow any more exceptions. You just have to describe what the type is, when to use it, and how to use it. Gradle isn't required unless you're going to generate something for openshift-docs. |
@@ -28,7 +29,9 @@ var KnownValidationExceptions = []reflect.Type{ | |||
var MissingValidationExceptions = []reflect.Type{ | |||
reflect.TypeOf(&buildapi.BuildLogOptions{}), // TODO, looks like this one should have validation | |||
reflect.TypeOf(&buildapi.BuildLog{}), // TODO, I have no idea what this is doing | |||
reflect.TypeOf(&imageapi.DockerImage{}), // TODO, I think this type is ok to skip validation (internal), but needs review | |||
reflect.TypeOf(&deployapi.DeploymentLogOptions{}), | |||
reflect.TypeOf(&deployapi.DeploymentLog{}), |
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 doesn't this object need validation?
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 suspect this one is only returned, never accepted and could be considered for a KnownValidationExeption
(the block above)
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 you explain the "only returned, never accepted"? This is a virtual subresource, essentially unused, its client interface is masking a call to "deploymentConfigs/log".
@smarterclayton @fabianofranz I really don't like the madness of three very different ways to get logs. The difference between builds and deployments on the cli are particularly jarring. What do we want them to look like in the end? I would suggest |
@@ -175,7 +200,9 @@ func (o DeployOptions) RunDeploy() error { | |||
} | |||
return list, nil | |||
}, | |||
|
|||
LogsForConfigFn: func(config *deployapi.DeploymentConfig, opts deployapi.DeploymentLogOptions) (io.ReadCloser, error) { |
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.
This looks suspiciously like a DeploymentLogsNamespacer
or DeploymentLogInterface
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.
Everything here looks suspiciously like an already existing client interface, I am just consistent with the existing design.
@Kargakis I think you should factor out a |
@@ -33,6 +34,7 @@ var MissingPrinterCoverageExceptions = []reflect.Type{ | |||
reflect.TypeOf(&authorizationapi.SubjectAccessReview{}), | |||
reflect.TypeOf(&authorizationapi.ResourceAccessReview{}), | |||
reflect.TypeOf(&deployapi.DeploymentConfigRollback{}), | |||
reflect.TypeOf(&deployapi.DeploymentLogOptions{}), |
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't add this here. If there's not going to be a printer, add it above with a justification. Since we don't take a -f
for oc get
, it's probably good enough to say that we don't ever return this object from the API.
@deads2k is there any other way except a generator to have our own flags without maintaining our own version of |
Until we come up with a sane solution for older deployment logs can we merge this? Is it worth being blocked? @ironcladlou @deads2k @smarterclayton |
deployRollback := &deployrollback.RollbackGenerator{} | ||
deployRollbackClient := deployrollback.Client{ | ||
DCFn: deployConfigRegistry.GetDeploymentConfig, | ||
RCFn: clientDeploymentInterface{kclient}.GetDeployment, | ||
GRFn: deployRollback.GenerateRollback, | ||
} | ||
configClient, deploymentClient := c.DeploymentConfigClients() |
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 call DeploymentConfigClients twice?
For older deployments, you have three options, add generic flags to logs, In the short term, older deployments isn't a blocker. But if the logs I'd like to see an issue to track that use case to completion. Some of the On Oct 16, 2015, at 9:47 AM, Michail Kargakis notifications@github.com @deads2k https://github.com/deads2k is there any other way except a — |
@@ -163,6 +168,13 @@ func (o *OpenShiftLogsOptions) RunLog() error { | |||
} | |||
return o.runLogsForBuild(build) | |||
|
|||
case "dc", "deploymentconfig", "deploymentconfigs": |
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.
this is spreading, which isn't good. normalization, lowercasing, pluralization, and alias->actual should happen in one place. https://github.com/openshift/origin/pull/4947/files#diff-8040eed03335aa79518a88a4de565decR37 looks promising. Add a TODO to replace the lowercase above with a call to a single helper function, and clear out the singular and alias checks in this function
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 have that in the RESTMapper already - why do we need a second location.
On Oct 16, 2015, at 10:24 AM, Jordan Liggitt notifications@github.com
wrote:
In pkg/cmd/cli/cmd/logs.go
#3943 (comment):
@@ -163,6 +168,13 @@ func (o *OpenShiftLogsOptions) RunLog() error {
}
return o.runLogsForBuild(build)
- case "dc", "deploymentconfig", "deploymentconfigs":
this is spreading, which isn't good. normalization, lowercasing,
pluralization, and alias->actual should happen in one place.
https://github.com/openshift/origin/pull/4947/files#diff-8040eed03335aa79518a88a4de565decR37
looks promising. Add a TODO to replace the lowercase above with a call to a
single helper function, and clear out the singular and alias checks in this
function
—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3943/files#r42247743.
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.
true. so...
resourceType := "pods"
...
_, kind, err := mapper.VersionAndKindForResource(resourceType)
if err != nil {
return err
}
resourceType, _ = meta.KindToResource(kind, false)
and then only check "pods", "buildconfigs", "builds", "deploymentconfigs", etc?
does VersionAndKindForResource handle singlar resources, or does resourcebuilder do magic to make that work?
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 have that in the RESTMapper already - why do we need a second location.
See #4947 (comment)
does VersionAndKindForResource handle singlar resources
Yes, it does.
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'm not sure what comment you're pointing to - for actually resolving names
we should always use RESTMapper full stop. You can round trip RESTMapper
back to the full type if you somehow need that.
On Oct 16, 2015, at 10:47 AM, Michail Kargakis notifications@github.com
wrote:
In pkg/cmd/cli/cmd/logs.go
#3943 (comment):
@@ -163,6 +168,13 @@ func (o *OpenShiftLogsOptions) RunLog() error {
}
return o.runLogsForBuild(build)
- case "dc", "deploymentconfig", "deploymentconfigs":
We have that in the RESTMapper already - why do we need a second location.
See #4947 (comment)
#4947 (comment)
does VersionAndKindForResource handle singlar resources
Yes, it does.
—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/3943/files#r42250620.
I would add a flag to logs w/o a second thought but we first need to make We should definitely have an issue for older deployment logs (I will open On Fri, Oct 16, 2015 at 4:21 PM, Clayton Coleman notifications@github.com
|
Opened #5163 |
Rename this issue something more generic so I don't accidentally claim this On Oct 16, 2015, at 10:36 AM, Michail Kargakis notifications@github.com — |
@Kargakis speak of the devil: |
Yes, I have changed |
@deads2k https://github.com/openshift/origin/pull/5067/files#diff-90c84153d4e68a186940fcbad3faeea2R68 I wasn't expecting the rebase to come in faster |
any more comments here or should I squash? |
Is the upstream commit still necessary? |
[test] |
Not after the rebase. |
Evaluated for origin test up to 6d732b1 |
rebased on top of latest master and removed the UPSTREAM commit oc logs dc/ continues to work fine |
LGTM [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5962/) (Image: devenv-rhel7_2503) |
Evaluated for origin merge up to 6d732b1 |
Merged by openshift-bot
This PR adds support for getting the logs for a deployment config:
will stream the logs for the latest deployment for the mysql deploymentConfig.
Most functionality for viewing logs for older deployments is here, though it won't be exposed for now.
Fixes #3544