Skip to content
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

status: Warn about transient deployment trigger errors #5085

Merged
merged 1 commit into from
Nov 13, 2015
Merged

status: Warn about transient deployment trigger errors #5085

merged 1 commit into from
Nov 13, 2015

Conversation

0xmichalis
Copy link
Contributor

@deads2k

I do think we should handle multiple bad triggers as markers for warnings and the separately mark a deployment that can't succeed on the same line. Something like " #1 deployment waiting on image or update (image stream doesn't exist)"

Separate marker for each warning means we traverse the graph and not follow the message set up by the dc controller. Or is it enough to break down the message into markers?

@0xmichalis
Copy link
Contributor Author

@deads2k now that the dc controller changes are in, let's get this out of the way. What's the intention here, follow the deploymentconfig message set by the controller, or traverse the graph and report separately?

@0xmichalis
Copy link
Contributor Author

Hmm, I have been working on this today and depending on the dc message (both the content and its format) is a mess. I think I'll just traverse the graph and report separately.

@0xmichalis
Copy link
Contributor Author

Also @ironcladlou

@ironcladlou
Copy link
Contributor

Should the controller be adapted to use this code?

@0xmichalis
Copy link
Contributor Author

Should the controller be adapted to use this code?

You mean create a graph in the dc controller? Not sure how I feel about it but I would prefer if we weren't duplicating logic.

@ironcladlou
Copy link
Contributor

Should the controller be adapted to use this code?

You mean create a graph in the dc controller? Not sure how I feel about it but I would prefer if we weren't duplicating logic.

Is there some reason using the graph library there would be inappropriate? It seems to provide the core analytics we're looking for in a variety of contexts (CLI, backend code trying to do analysis, etc).

@0xmichalis
Copy link
Contributor Author

Is there some reason using the graph library there would be in appropriate? It seems to provide the core analytics we're looking for in a variety of contexts (CLI, backend code trying to do analysis, etc).

@smarterclayton ?

@smarterclayton
Copy link
Contributor

Graphs should be used for graph type operations. Looking at resources is not a graph layer thing. If there is common code it should probably be in an API helper method (not util) that does a common transform.

@openshift-bot openshift-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 22, 2015
@0xmichalis
Copy link
Contributor Author

Not sure if it's possible for the graph stuff and the dc controller to share an api helper. It's either the one or the other I think.

@smarterclayton
Copy link
Contributor

No graph library for now. If you have to refactor to do something common,
the common code should be in deploy/api/helpers.go, and both places should
use it.

On Mon, Oct 26, 2015 at 2:33 PM, Michail Kargakis notifications@github.com
wrote:

Not sure if it's possible for the graph stuff and the dc controller to
share an api helper. It's either the one or the other I think.

—
Reply to this email directly or view it on GitHub
#5085 (comment).

@0xmichalis
Copy link
Contributor Author

That's what I'm saying, I cannot extract any common code.

Is it ok if oc status and the dc controller report separately? oc status is
reporting on the spot while the dc controller lags ~2 mins until it
reconciles a config.

On Mon, Oct 26, 2015 at 7:44 PM, Clayton Coleman notifications@github.com
wrote:

No graph library for now. If you have to refactor to do something common,
the common code should be in deploy/api/helpers.go, and both places should
use it.

On Mon, Oct 26, 2015 at 2:33 PM, Michail Kargakis <
notifications@github.com>
wrote:

Not sure if it's possible for the graph stuff and the dc controller to
share an api helper. It's either the one or the other I think.

—
Reply to this email directly or view it on GitHub
#5085 (comment).

—
Reply to this email directly or view it on GitHub
#5085 (comment).

@smarterclayton
Copy link
Contributor

DC controller should report into the status of the deployment config.
Status should show that info as well.

On Mon, Oct 26, 2015 at 2:50 PM, Michail Kargakis notifications@github.com
wrote:

That's what I'm saying, I cannot extract any common code.

Is it ok if oc status and the dc controller report separately? oc status is
reporting on the spot while the dc controller lags ~2 mins until it
reconciles a config.

On Mon, Oct 26, 2015 at 7:44 PM, Clayton Coleman <notifications@github.com

wrote:

No graph library for now. If you have to refactor to do something common,
the common code should be in deploy/api/helpers.go, and both places
should
use it.

On Mon, Oct 26, 2015 at 2:33 PM, Michail Kargakis <
notifications@github.com>
wrote:

Not sure if it's possible for the graph stuff and the dc controller to
share an api helper. It's either the one or the other I think.

—
Reply to this email directly or view it on GitHub
<#5085 (comment)
.

—
Reply to this email directly or view it on GitHub
#5085 (comment).

—
Reply to this email directly or view it on GitHub
#5085 (comment).

@smarterclayton
Copy link
Contributor

[test]

@openshift-bot openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 29, 2015
@0xmichalis
Copy link
Contributor Author

I'm going to add tests for this.

@0xmichalis
Copy link
Contributor Author

Tests added and passing locally.

@0xmichalis
Copy link
Contributor Author

@openshift/team-project-committers

@0xmichalis 0xmichalis closed this Nov 5, 2015
@0xmichalis 0xmichalis reopened this Nov 5, 2015
@liggitt liggitt added this to the 1.1.1 milestone Nov 5, 2015
@0xmichalis
Copy link
Contributor Author

@smarterclayton @liggitt this is low risk and useful since the reporting by the dc controller was disabled.

@jwforres
Copy link
Member

jwforres commented Nov 5, 2015

agree with @liggitt this is 1.1.1 the bar for changes is pretty high right now

@0xmichalis
Copy link
Contributor Author

@deads2k this needs reviewing if you have time.

deads2k
deads2k reviewed Nov 11, 2015
View reviewed changes
pkg/deploy/graph/analysis/dc.go
dcNode := uncastDcNode.(*deploygraph.DeploymentConfigNode)

// 1. Image stream for tag of interest does not exist
for _, uncastIsNode := range g.SuccessorNodesByEdgeKind(uncastIstNode, imageedges.ReferencedImageStreamGraphEdgeKind) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you hide this look inside of a function, so this reads more like:

if exists := doesImageStreamExist(isTag); !exists{
    // add marker
}

It would just help me reason about how many markers I might get.

@deads2k deads2k self-assigned this Nov 11, 2015
@0xmichalis
Copy link
Contributor Author

#5681 flake

re[test]

deads2k
deads2k reviewed Nov 12, 2015
View reviewed changes
pkg/deploy/graph/analysis/dc.go
RelatedNodes: []graph.Node{uncastIstNode, bcNode},

Severity: osgraph.InfoSeverity,
Key: ImageStreamTagNotAvailableWarning,
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably ought to be an info key

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@deads2k
Copy link
Contributor

deads2k commented Nov 12, 2015

nit, lgtm otherwise.

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to e4886ff

@deads2k deads2k added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2015
@deads2k
Copy link
Contributor

deads2k commented Nov 12, 2015

[merge]

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7079/) (Image: devenv-rhel7_2701)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to e4886ff

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/7079/)

openshift-bot pushed a commit that referenced this pull request Nov 13, 2015
Merge pull request #5085 from kargakis/oc-status-deployments
52cf873 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 52cf873 into openshift:master Nov 13, 2015
@0xmichalis 0xmichalis deleted the oc-status-deployments branch November 13, 2015 15:40
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@deads2k deads2k

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
1.1.1
Development

Successfully merging this pull request may close these issues.
7 participants
@0xmichalis @ironcladlou @smarterclayton @jwforres @deads2k @openshift-bot @liggitt