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

ui: add service health viz to table #14369

Merged
merged 10 commits into from
Aug 30, 2022
Merged

Conversation

ChaiWithJai
Copy link
Contributor

@ChaiWithJai ChaiWithJai commented Aug 29, 2022

This PR handles visualizing Service Health statuses in the allocations.allocation.index view within the Services table.

image

This PR raises a concern about the response from clients/allocations/:allocId/checks endpoint. Highlighting an issue where all services in the allocation are not returning a status and sometimes the API returns null.

@github-actions
Copy link

github-actions bot commented Aug 29, 2022

Ember Asset Size action

As of 30b0dfe

Files that got Bigger 🚨:

File raw gzip
nomad-ui.js +3.24 kB +766 B

Files that stayed the same size 🤷‍:

File raw gzip
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

Copy link
Contributor Author

@ChaiWithJai ChaiWithJai left a comment

Choose a reason for hiding this comment

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

@philrenaud left some questions for you in here.

ui/app/controllers/allocations/allocation/index.js Outdated Show resolved Hide resolved
@@ -13,6 +13,7 @@ export default class AllocationRoute extends Route.extend(WithWatchers) {
startWatchers(controller, model) {
if (model) {
controller.set('watcher', this.watch.perform(model));
// TODO: Add conditional logic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@philrenaud on idea around conditionally polling services would be checking if model.get('job.service') which may be the more technically sound option because we specify services on the job specification -- not at the allocation level. And in our model hook, we make an async request to get the job already.

Copy link
Contributor

Choose a reason for hiding this comment

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

model.get('job.services') will, I think, make a request to the API at /jobs/:jobid/services, which is perhaps not what we want.

If we want to look at those services defined in the job spec, that's exactly what the @union('taskServices', 'groupServices') services; references in allocation/index are doing. Those service definitions read from the responded job file, and don't make further requests. I'd consider porting those to the alloc controller or similar (anyplace higher level than the alloc index) and using that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I ported over the union logic you have. Do you have any idea how I can test this? We disable watchers in our test suite.

So for this to be correct we need to test:

  1. When both tasks and task groups have services
  2. When only tasks have them
  3. When only groups have them
  4. When neither have them

I can only test 1 and 4 right now. @philrenaud can you assist, please?

@github-actions
Copy link

github-actions bot commented Aug 29, 2022

Ember Test Audit comparison

f-ui/service-discovery 30b0dfe change
passes 1419 1408 -11
failures 0 17 +17
flaky 0 0 0
duration 9m 51s 415ms 000ms -9m 51s 415ms

Copy link
Contributor

@philrenaud philrenaud left a comment

Choose a reason for hiding this comment

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

Looks good; left a few comments.

<td data-test-service-health>
{{#if (not row.model.connect)}}
<div class="inline-chart">
<ServiceStatusBar @services={{this.serviceHealthStatuses}} @name={{row.model.name}} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop an @isNarrow={{true}} on this to more closely match designs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I should've read the component API better next time.

Comment on lines 18 to 20
const doesAllocHaveServices =
!!model.taskGroup.services ||
!!model.states.mapBy('task').map((t) => t && t.get('services'));
Copy link
Contributor

Choose a reason for hiding this comment

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

Considering filtering these by provider === "nomad". We won't need to make health checks if we only have consul services and could save some network bandwidth for users.

@@ -301,6 +304,13 @@
{{upstream.destinationName}}:{{upstream.localBindPort}}
{{/each}}
</td>
<td data-test-service-health>
{{#if (not row.model.connect)}}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think row.model.connect is correct here; there can be provider = "consul" services (service fragments) for which consult-connect is falsy. We don't want to show our chart for those, either.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, you probably want to do an eq test for provider: nomad

@ChaiWithJai ChaiWithJai merged commit b9369e1 into f-ui/service-discovery Aug 30, 2022
@ChaiWithJai ChaiWithJai deleted the temp/bork branch August 30, 2022 15:44
philrenaud added a commit that referenced this pull request Aug 31, 2022
* Added to subnav and basic table implemented

* Existing services become service fragments, and services tab aggregated beneath job route

* Index page within jobs/job/services

* Watchable services

* Lintfixes

* Links to clients and individual services set up

* Child service route

* Keyboard shortcuts on service page

* Model that shows consul services as well, plus level and provider cols

* lintfix

* Level as query param

* Watch job for service name changes too

* Group level service fixtures established

* Progress at task level and job-linked services

* Task and group services on update

* Fixture side-effect cleanup

* Basic acceptance tests for job services

* Testmodel cleanup

* Disabled mirage logging

* New cluster type specifically for services

* Without explicit job-model binding

* Trying to isolate a tostring error

* Account for new tab in keyboardnav

* More test isolation attempts

* Remove skipped tests and link task to parent group by id

ui: add service health viz to table (#14369)

* ui: add service-status-bar

* test: service-status-bar

* refact: update component api for new data struct

* ui: format service health struct

* ui:  add service health viz to table

* temp: add placeholder to remind conditional watcher

* test: write tests for transformation algorithm

* refact: update transformation algo

* ui: conditionally long poll checks endpoint

* refact: add conditional logic for nomad provider

refact: update service-fragment model to include owner info

ui: differentiate between task and group-level in derived state comp

test: add test to document behavior

refact: update tests for api change

refact: update integration test for API change

chore: remove unsused vars

chore: elvis operator to protect mirage

refact: create refId instead of internalModel

refact: update algo

refact: update conditional template logic

refact: update test for api change:

chore: cant use if and not in hbs conditional
philrenaud added a commit that referenced this pull request Sep 7, 2022
* Added to subnav and basic table implemented

* Existing services become service fragments, and services tab aggregated beneath job route

* Index page within jobs/job/services

* Watchable services

* Lintfixes

* Links to clients and individual services set up

* Child service route

* Keyboard shortcuts on service page

* Model that shows consul services as well, plus level and provider cols

* lintfix

* Level as query param

* Watch job for service name changes too

* Group level service fixtures established

* Progress at task level and job-linked services

* Task and group services on update

* Fixture side-effect cleanup

* Basic acceptance tests for job services

* Testmodel cleanup

* Disabled mirage logging

* New cluster type specifically for services

* Without explicit job-model binding

* Trying to isolate a tostring error

* Account for new tab in keyboardnav

* More test isolation attempts

* Remove skipped tests and link task to parent group by id

ui: add service health viz to table (#14369)

* ui: add service-status-bar

* test: service-status-bar

* refact: update component api for new data struct

* ui: format service health struct

* ui:  add service health viz to table

* temp: add placeholder to remind conditional watcher

* test: write tests for transformation algorithm

* refact: update transformation algo

* ui: conditionally long poll checks endpoint

* refact: add conditional logic for nomad provider

refact: update service-fragment model to include owner info

ui: differentiate between task and group-level in derived state comp

test: add test to document behavior

refact: update tests for api change

refact: update integration test for API change

chore: remove unsused vars

chore: elvis operator to protect mirage

refact: create refId instead of internalModel

refact: update algo

refact: update conditional template logic

refact: update test for api change:

chore: cant use if and not in hbs conditional
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants