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

Handle k8s edge cases in the web console overview. #1028

Merged
merged 1 commit into from
Feb 18, 2015
Merged

Handle k8s edge cases in the web console overview. #1028

merged 1 commit into from
Feb 18, 2015

Conversation

jwforres
Copy link
Member

No description provided.

@jwforres
Copy link
Member Author

handle_edge_cases

edge_cases_with_dep_configs

@jwforres
Copy link
Member Author

@liggitt @smarterclayton

Couple things to note:

  • I am hiding RCs/deployments that have no pods, as we discussed already this is because old deployments are not being reaped right now.
  • I am hiding singleton pods that are in the Succeeded and Terminated states
  • I am hiding singleton pods in any state if they are either build or deploy pods
  • Only visual different between an RC and a deployment, is the extra "what triggered me" metadata next to the timestamp.
  • Currently no visual difference between a single pod with no RC and a single pod that is part of an RC. @liggitt and I discussed that this will change once we have scaling details for RCs.

@liggitt
Copy link
Contributor

liggitt commented Feb 16, 2015

is there any way to capture the API responses to LIST * you used to build this, and save them as fixtures to trigger somehow?

@jwforres
Copy link
Member Author

I already opened a separate PR with just the fixtures #1031

@liggitt
Copy link
Contributor

liggitt commented Feb 16, 2015

ah, great

@smarterclayton
Copy link
Contributor

Hrm... "there is no service for these pods" (beyond being bad english) seems misplaced. We may want to tell the user why we grouped them, but not imply that the grouping is a service.

liggitt
liggitt reviewed Feb 16, 2015
View reviewed changes
assets/app/views/project.html
to hide our build pods from these view. Or we need to actually reap old build pods.
Also hide deployer pods.
-->
<section ng-repeat="(podName, pod) in solitaryPods" ng-if="pod.status.phase != 'Succeeded' && pod.status.phase != 'Terminated' && !buildPods[podName] && !(pod | annotation : 'deployment')">
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this logic into the controller method? If you want to keep track of all solitaryPods, as well as the filtered ones we're going to display, you could have unfilteredSolitaryPods and solitaryPods in the controller, then have the view just render solitaryPods.

@jwforres jwforres force-pushed the overview_edge_cases branch from fb3901b to 33354c6 Compare February 16, 2015 19:20
liggitt
liggitt reviewed Feb 16, 2015
View reviewed changes
assets/app/scripts/controllers/overview.js
$scope.deploymentPods[name] = ls.select($scope.pods);
});

console.log("podsByServiceByLabel", $scope.podsByServiceByLabel);
Copy link
Contributor

Choose a reason for hiding this comment

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

copy/paste

@liggitt
Copy link
Contributor

liggitt commented Feb 17, 2015

Output of rename brainstorm:

on service update
    services
    deploymentsByServiceByDeploymentConfig
    deploymentConfigsByService
    podsByService
    monopodsByService
on deployment update
    deploymentsByServiceByDeploymentConfig
    podsByDeployment
    monopodsByService
on deployment config update
    deploymentConfigsByService
on pods update
    podsByDeployment
    podsByService
    monopodsByService

// All pods under existing services
// no "" service key
podsByService
// All pods under existing deployments
// no "" deployment key
podsByDeployment
// Pods not in a deployment
// "" service key for pods not under any service
// Excludes utility pods like build pods and completed deployment pods (for now)
monopodsByService
// All deployments
// "" service key for deployments not under any service
// "" deployment config for deployments not created from a deployment config
// Note that deployments under a service may be grouped by a deployment config which itself is no longer under the service (deploymentConfigsByService[serviceId][deploymentConfigId] would be undefined)
deploymentsByServiceByDeploymentConfig
// All deployment configs
// "" service key for deployment configs not under any service
deploymentConfigsByService

liggitt
liggitt reviewed Feb 17, 2015
View reviewed changes
assets/app/scripts/controllers/overview.js
@@ -58,6 +71,99 @@ angular.module('openshiftConsole')
console.log("podsByServiceByLabel", $scope.podsByServiceByLabel);
};

var podRelationships = function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

building all the pod maps in one function (podsByDeployment, podsByService, monopodsByService) would let us avoid building and evaluating the same label selectors on the same pods repeatedly

@jwforres jwforres force-pushed the overview_edge_cases branch 3 times, most recently from 9f66d55 to 641d1c2 Compare February 18, 2015 18:21
@jwforres jwforres changed the title [WIP] Handle k8s edge cases in the web console overview. Handle k8s edge cases in the web console overview. Feb 18, 2015
liggitt
liggitt reviewed Feb 18, 2015
View reviewed changes
assets/app/scripts/controllers/overview.js
// Not using angular.forEach so that we can break out early
for (var i = 0; !isInDepInSvc && i < deployments.length; i++) {
var depName = deployments[i];
if ($scope.deploymentsByService[svcName][depName]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

any chance $scope.deploymentsByService[svcName] could be undefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes it can actually

@liggitt
Copy link
Contributor

liggitt commented Feb 18, 2015

LGTM, squash, update screenshots, update bindata

@jwforres jwforres force-pushed the overview_edge_cases branch from 76966ed to 4912608 Compare February 18, 2015 22:18
@jwforres
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/961/) (Image: devenv-fedora_831)

@openshift-bot
Copy link
Contributor

Evaluated for origin up to 4912608

openshift-bot pushed a commit that referenced this pull request Feb 18, 2015
Merge pull request #1028 from jwforres/overview_edge_cases
35cc671 
Merged by openshift-bot
@openshift-bot openshift-bot merged commit 35cc671 into openshift:master Feb 18, 2015
@jwforres jwforres deleted the overview_edge_cases branch March 19, 2015 20:03
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
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jwforres @liggitt @smarterclayton @openshift-bot