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.

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

CLI: add support for deployments in oc status #18439

Merged
merged 2 commits into from
Feb 7, 2018

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Feb 5, 2018

@smarterclayton this is a long overdue...

current state:

$ oc status
In project My Project (myproject) on server https://127.0.0.1:8443

svc/ruby-deploy - 172.30.174.234:8080
  deployment/ruby-deploy deploys istag/ruby-deploy:latest <-
    bc/ruby-deploy source builds https://github.com/openshift/ruby-ex.git on istag/ruby-22-centos7:latest
      build #1 failed 5 hours ago - bbb6701: Merge pull request #18 from durandom/master (Ben Parees <[email protected]>)
    deployment #2 running for 4 hours - 0/1 pods (warning: 53 restarts)
    deployment #1 deployed 5 hours ago

TODO:

  • Add rollouts similar to deployment configs
  • Fix unit tests / Add unit tests
  • Deal with HPA

@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 Feb 5, 2018
@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2018
@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2018
@@ -578,7 +578,7 @@ func (p *pruner) addDeploymentsToGraph(dmnts *kapisext.DeploymentList) []error {
d := &dmnts.Items[i]
ref := getRef(d)
glog.V(4).Infof("Examining %s", getKindName(ref))
dNode := appsgraph.EnsureDeploymentNode(p.g, d)
dNode := kubegraph.EnsureDeploymentNode(p.g, d)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if you wonder why, I moved 'Deployments' from appsgraph which seems to hold only apps.openshift.io resources to kubegraph as Deployments are upstream.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind moving DaemonSets as well, it's the function before this one?

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for ReplicaSets a few lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting, I thought I moved replica sets already... seems like they are not used/loaded anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. moved both to kube nodes

func describeDeploymentStatus(rs *kapisext.ReplicaSet, revision int64, first bool, restartCount int32) string {
timeAt := strings.ToLower(formatRelativeTime(rs.CreationTimestamp.Time))
replicaSetRevision, _ := deployutil.Revision(rs)
if replicaSetRevision == revision {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Kargakis @tnozicka i really hope there is something better to report the state of RS created/managed by deployment... The only thing that RS contain is revision so I know this RS is "old", but it does not say anything about if it failed/etc.

Copy link
Member

Choose a reason for hiding this comment

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

@spadgett since you did the console pages for deployments

@@ -33,6 +38,12 @@ const (
ReferencedServiceAccountEdgeKind = "ReferencedServiceAccount"
// ScalingEdgeKind goes from HorizontalPodAutoscaler to scaled objects indicating that the HPA scales the object
ScalingEdgeKind = "Scaling"
// TriggersDeploymentEdgeKind points from DeploymentConfigs to ImageStreamTags that trigger the deployment
TriggersDeploymentEdgeKind = "TriggersDeployment"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was duplicated from appsgraph because of import cycles... we should probably move this into some common package

Copy link
Contributor Author

Choose a reason for hiding this comment

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

follow up, don't want to pollute this PR

default:
continue
}

g.AddEdge(hpaNode, syntheticNode, ScalingEdgeKind)
}
}

// AddTriggerEdges creates edges that point to named Docker image repositories for each image used in the deployment.
func AddTriggerEdges(g osgraph.MutableUniqueGraph, node *kubegraph.DeploymentNode) *kubegraph.DeploymentNode {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should rework this to make it usable for StatefulSets and DaemonSets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, this now works for statefulsets as well

@smarterclayton
Copy link
Contributor

smarterclayton commented Feb 5, 2018 via email

@mfojtik
Copy link
Contributor Author

mfojtik commented Feb 5, 2018

@smarterclayton ready for review :-)

@mfojtik mfojtik changed the title WIP: add support for deployments in oc status CLI: add support for deployments in oc status Feb 5, 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 Feb 5, 2018
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

A few suggestions, but this lgtm

@@ -160,7 +160,7 @@ func (o *StatusOptions) Complete(f *clientcmd.Factory, cmd *cobra.Command, baseC
canRequestProjects, _ := loginutil.CanRequestProjects(config, o.namespace)

o.describer = &describe.ProjectStatusDescriber{
K: kclientset,
KubeClient: kclientset,
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

@@ -578,7 +578,7 @@ func (p *pruner) addDeploymentsToGraph(dmnts *kapisext.DeploymentList) []error {
d := &dmnts.Items[i]
ref := getRef(d)
glog.V(4).Infof("Examining %s", getKindName(ref))
dNode := appsgraph.EnsureDeploymentNode(p.g, d)
dNode := kubegraph.EnsureDeploymentNode(p.g, d)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind moving DaemonSets as well, it's the function before this one?

@@ -578,7 +578,7 @@ func (p *pruner) addDeploymentsToGraph(dmnts *kapisext.DeploymentList) []error {
d := &dmnts.Items[i]
ref := getRef(d)
glog.V(4).Infof("Examining %s", getKindName(ref))
dNode := appsgraph.EnsureDeploymentNode(p.g, d)
dNode := kubegraph.EnsureDeploymentNode(p.g, d)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same for ReplicaSets a few lines below.

@mfojtik mfojtik force-pushed the oc-status branch 2 times, most recently from b035d04 to 86a8bc9 Compare February 5, 2018 19:39
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 5, 2018
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

lgtm, but would you mind removing the wip from the 2nd commit title ;) ?

func describeStatefulSetInServiceGroup(f formatter, node *kubegraph.StatefulSetNode) []string {
func describeDeploymentInServiceGroup(f formatter, deploy graphview.Deployment, restartFn func(node *kubegraph.ReplicaSetNode) int32) []string {
local := namespacedFormatter{currentNamespace: deploy.Deployment.Deployment.Namespace}
// TODO: Figure out what this is
Copy link
Contributor

Choose a reason for hiding this comment

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

lol

@mfojtik
Copy link
Contributor Author

mfojtik commented Feb 7, 2018

/retest

@mfojtik mfojtik force-pushed the oc-status branch 2 times, most recently from c587158 to 02412d4 Compare February 7, 2018 09:58
Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mfojtik, soltysh

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue.

@openshift-merge-robot openshift-merge-robot merged commit 0427a13 into openshift:master Feb 7, 2018
@mfojtik mfojtik deleted the oc-status branch September 5, 2018 21:07
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.

6 participants