UI: Proper task group breadcrumb on the allocation pages

[DingoEatingFuzz]

The breadcrumb was missing the namespace query param which led to errors being thrown when using the breadcrumb on a non-default namespace.

Only impacts enterprise, since namespaces are an enterprise feature.

[johncowen]

Hey!

I'm still getting an error here, just a different one.

This was proxying to the demo site and with ember-cli-mirage.enabled=false

[DingoEatingFuzz]

Thanks for looking at this @johncowen, can you provide steps to reproduce?

[johncowen]

It's actually really difficult to figure out the steps. It's always when I click on a breadcrumb, I think I've seen it happen on both the second to last and the third to last (I just got it to break now and that one was clicking on the third to last)

I just tried to reproduce again, and at first I couldn't get it to break. So I figured it might be something to do with the fact that I'd run it using Mirage first and then switched to use the demo site (maybe something do with the localStorage values or something). I tried doing the same, and this time I got this error:

... again its really difficult to give exact steps as it seems to be sporadic. I refreshed and clicked around a bit more, and then got it to break exactly the same as the screengrab from last week. I then cleared cleared my localStorage completely and clicked around a little more and I managed to get it to happen again, unfortunately it doesn't look like it happens following specific steps.

All of the above was experienced using whilst proxying to the demo site with ember-cli-mirage.enabled=false

Just before finishing up on this comment I just tried a few more times and I think I got it to break consistently with:

1. Navigate to http://localhost:4200/ui/
2. Click [Clients] in the left hand bar.
3. Click the second client in the table (22b31b3b)
4. Click the first allocation in the Allocations table (0fccf9f4)
5. Click the first task in the Task table (executor)
6. Click the third to last breadcrumb (executor)

It might happen in other places also.

John

[DingoEatingFuzz]

So I looked into the reload error and learned something new about Nomad. Apparently job IDs don't have to match job Names. This is rare, since it requires submitting a job via the api using JSON, but it's possible and in place in the Nomad demo cluster (https://demo.nomadproject.io/v1/job/SparkPi?namespace=analytics).

I'm going to fix this bug in a separate PR. Changed my mind. Turns out it is part of this PR.

As for the querySelector of null from mountElements error, that smells like a race condition (component isDestroying) bug, which is also separate from this PR.

Thank you for finding these bugs though!

[johncowen]

Ah ha, one of those curve balls! 😄

Cool, does that last commit (4e36c52) include the fix then? If so I can give it another once over.

On the 'querySelector of null from mountElements error', ping me when you get that one up and I can look at that then.

[DingoEatingFuzz]

Yep! All changes are pushed up. I'll comment where the bug was fixed.

[DingoEatingFuzz]

This was job.name, which meant the job route was attempting to fetch a job with the wrong ID in the event that name and plainId aren't equal (which is rare but possible).

[johncowen]

Hey,

I'm still getting errors on here, I'm not sure if they are all related or entirely different things. So apologies up front if this is a bit non-specific.

First thing that happened, that I couldn't reproduce, was that on clicking on the third to last breadcrumb, the loader animation came up and just hung, I left it for about a minute and it just stayed there. Unfortunately I hadn't opened up console so I have no error/network info for that to see if anything showed up, sorry.

After that I kept trying to see if I could reproduce ^, but then hit another thing that I do have better information for:

Overall I did notice that the 'in page sub menu' (the bit with the [Overview] button in) does this weird 'show you 0 or 1 button, then show you 4 buttons, then show you 1 button again' when you click on the third to last breadcrumb. Not a showstopper, but a bit strange, is that something you can avoid?

Ah lastly, the way I'm reproducing this now is slightly different to how I explained in my last review as the specific Allocations/Tasks are no longer in the demo cluster.

Now what I'm doing is:

1. Enter /ui/allocations/b3a21438-04a2-c33e-dd6a-23ac021b45ef/executor in your browser location bar.
2. Click on the third to last breadcrumb (executor)

The bug only seems to happen sometimes.

Ping me once you've looked at it and I'll give it another go.

Thanks,

[DingoEatingFuzz]

1. I couldn't get the page to hang.
2. The varying breadcrumbs is a side effect of the jobs.job.loading page which includes the job subnav. This was done to avoid having the subnav disappear when clicking on tabs, but since the task-group route is in the same level of the hierarchy, it too gets the (wrong) subnav. I think the current situation is better than the alternative (subnav disappearing for a moment when loading a different tab), so I'm not going to change it. It does highlight another issue though, which is that since task groups are on the same level of the hierarchy, there is potential for name collisions, (e.g., group named definition colliding with the existing /:job/definition route). This should be addressed, but it's definitely out of scope of this PR.
3. I tried your steps to reproduce multiple times and I couldn't get it to error. I have a suspicion as to what it could be though. The API for the demo site can be flaky at times, so I suspect what is happening is when going to the task group, it loads the job, the job errors for flakiness reasons, and then this results in a reload error (since the job is undefined and can't be reloaded). The correct thing to do here is treat it as a 404, since if the job is nonexistent (as best as the UI can tell), then so is the task group. Much like the routing issues, this should be addressed but it's out of scope.

[johncowen]

Hey,

I'm still getting different errors around the breadcrumbs, although I'm not sure if its breadcrumbs or something else, here's another:

This could be a similar loading error to what I got on Friday, I've just noticed whilst writing this that this time that the page did eventually load once I got a 524 error. Here's what the console looks like now:

This does improve the situation from the original error, which I think was happening all the time, and I get the impression it would be better to address these other things separately, so I'm gonna go with your preference here and approve this - at the end of the day it does improve on the original bug.

I have had a look over the code, and I'm going to dive back into it a bit now and I might have a few non-blocking questions from that.

Thanks for bearing with it.

John

[johncowen]

Ok first things first these are completely non-blocking and just a result of a bit of a bigger dive into some of the files changed here, not even necessarily things that are part of this change:

You seem to be doing things here that I would have thought you'd be putting in an Ember Data Adapter:

nomad/ui/app/services/system.js

Lines 21 to 24 in 4e36c52

	.authorizedRequest(`/${namespace}/status/leader`)
	.then(res => res.json())
	.then(rpcAddr => ({ rpcAddr }))
	.then(leader => {

From working with Ember Data it seems to go to great lengths to abstract away the inner workings of an API, so much so that you don't interact with things using URLs, but instead using javascript functions. I'm not a great fan of the way it does this, but I do think it's the right thing to do. Is there a plan to move this into an Adapter in the future? And what is the reason behind not putting it within an Adapter?

Another thing I noticed in this file is all the window.localStorage setting you are doing in here, didn't I see somewhere that you had a centralized place to do this with stringify-ing and default-ing features all in one place?

No rush for these at all, just queries.

Thanks,

John

[DingoEatingFuzz]

For the leader request as well as the regions request, I am not using Ember Data because they are both single, stateless queries. Given how they are currently used, I decided moving them into adapters/serializers/models would be over-engineering. It's totally possible to make a counter-argument here and say that it's better to have everything use the same data pipeline, but I still think my reasoning is valid. If there comes a day we do more with the leader or regions, I'll move them into ED proper.

As for localStorage, I do have a localStorage computed property macro, but it doesn't work quite the same way.

[johncowen]

Cool, I was wondering if you could put the url's at least in an Adapter and then use adapter.urlForAuthorizedRequest (or similar) so at least your url creation is where ember peeps expect it to be. If you like that idea, just something for possible future refinement. There might be the odd other adapter method you could use here also (maybe handleRequest).

I think what I'm coming to here which is more the TLDR; is, what about just using an adapter here, but not the serializer/store?

[DingoEatingFuzz]

I get the urge to put it in an adapter, since dealing with string concatenation in components and such is not ideal, but I think making an adapter for these use cases is the worser of two evils. Mostly because of the way Ember Data treats adapters.

I don't see adapters as free-standing primitives. They are always paired with serializers and models. And this is mostly because adapters aren't called directly. There are exemptions, like store.adapterFor, but typically if you see an adapter with a name like region, you would expect to be able to do store.findAll('region'), and without at the very least a model, that won't work.

I could see potentially having some other abstraction for these URLs, but the time being I don't mind having the URLs directly in the service and elsewhere. It's a pretty small blast radius considering how isolated the usages are.

[johncowen]

but typically if you see an adapter with a name like region, you would expect to be able to do store.findAll('region'), and without at the very least a model, that won't work.

Good enough reason for me 👍

I suppose half of this system service is kind of a 'repository' for regions (and possibly namespaces also?), but not a 'repository' backed by ember data which is a-ok (a repository would hide that fact).

You have some view related bits in here also. Don't mean to sound whatever here considering that other convo, but what do you think of the idea of splitting this up so you have data retrieving stuff in a repository-like service and the view related stuff in a view helper? (obviously very 'me' this, but give it a think - I don't mind either way)

[github-actions]

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.