Dashboard: Expanded view of pods will condense after the auto-refresh interval

[aperullo]

Environment

Kubernetes version: 1.17
Dashboard version: 2
Operating system: Centos

Steps to reproduce

Click the eye or show all buttons that let you expand the view of a pod.
Wait for the auto-refresh interval, the expanded view will close itself.

Observed result

The expanded view will close itself.

Expected result

The expanded view should stay open so status changes can be read.

Comments

[floreks]

Yep, this is a known issue. We are investigating it, but it's tricky to fix.

[Eddman]

@floreks Sounds like missing trackBy in ngFor so content is rerendered event thou it was not changed :wink: I'll try to have a look as it is annoying for me as well.

[floreks]

@Eddman that's the case but it's a bit more tricky. It requires changes in our generic list component that handles display of all resource lists and we need to find unique information in there that can be used as unique id for the trackBy. Feel free to take a look at this. I suggest you to start here: https://github.com/kubernetes/dashboard/blob/master/src/app/frontend/common/resources/list.ts#L365

[Eddman]

I've had a brief look and I think it's more related to <mat-table>. Assuming they look like this:

    <mat-table [dataSource]="getData()"

on data change you call getData() that return new list of rows (or get the list as next value from an Observable, does not really matter here) and the table is simply recreating the whole content.

If you provide:

    <mat-table [dataSource]="getData()"
               [trackBy]="trackByResource"

and add into list.ts/ResourceListBase:

  trackByResource(_: number, item: R): any{
    if(item.objectMeta.uid != null) {
      return item.objectMeta.uid;
    }

    return item.objectMeta.name;
  }

I'm 70% :smile: sure it solves that issue, cause the table will be able to map each resource back to the view reference, will reuse it and will not replace the whole HTML/components in the rows/cell.

Will test that once I figure out how to run a dev version. :+1:

[floreks]

Yep, but IIRC we don't have the uid info in the frontend. We'd need to update backend or use some kind of combined resource info to create unique id.

[Eddman]

As far as I know the name in a namespace is always unique. Or do I miss something? The same applies to cluster-level resources...

[floreks]

Name is unique for the resource type across a single namespace. Our lists can display objects from multiple namespaces.

[Eddman]

so key would be namespace/name :man_shrugging:

[Eddman]

But you're right that the problem is deeper. I've implemented a test change :arrow_up: for pods list and see that for instance labels are not hidden after refresh, but errors and the actions menu are collapsed every 5 seconds.

I'll dig deeper and let you know.

[floreks]

@Eddman I am working on that. It requires updating resource list class to track list items by resource UID instead of index.

[Eddman]

Sorry got dragged away by my work duties.

Digging deeper:

Found one additional problem in action columns:

@Component({
  selector: 'kd-dynamic-cell',
  templateUrl: './template.html',
})
export class ColumnComponent<T extends ActionColumn> implements OnChanges {
  @Input() component: Type<T>;
  @Input() resource: Resource;
  @ViewChild('target', {read: ViewContainerRef, static: true}) target: ViewContainerRef;
  private componentRef_: ComponentRef<T> = undefined;

  constructor(private readonly resolver_: ComponentFactoryResolver) {}

  ngOnChanges(): void {
    if (this.componentRef_) {
      this.target.remove();
      this.componentRef_ = undefined;
    }

That is conceptually wrong I think and as side effect makes the menu close whenever a refresh occur. The code does not do any proper change detection and even thou the component does not have to be changed it is thrown away and a new one is created.

I'd suggest to check for component changes only (there we definitely need a new component instance) and let the change detector check the changes in the component itself:

ngOnChanges(changes:SimpleChanges): void {
  if(this.componentRef_ && changes.component) {
      this.target.remove();
      this.componentRef_ = undefined;
  }

  if(!this.componentRef_) {
    const factory = this.resolver_.resolveComponentFactory(this.component);
    this.componentRef_ = this.target.createComponent(factory);
  }

  this.componentRef_.instance.setObjectMeta(this.resource.objectMeta);
  this.componentRef_.instance.setTypeMeta(this.resource.typeMeta);

  if ((this.resource as CRDDetail).names !== undefined) {
    this.componentRef_.instance.setDisplayName((this.resource as CRDDetail).names.kind);
  }

  // Let the change detector run for our component
  this.componentRef_.changeDetectorRef.markForCheck();
}

[Eddman]

A very similar problem for the error/status message (apart of the index usage that also BTW breaks on sorting). Or maybe it is even worse 😄 cause here you're playing with mat-row template:

if (this.expandedRow !== undefined) {
      this.containers_.toArray()[this.expandedRow].clear();
    }

    if (this.expandedRow === index) {
      this.expandedRow = undefined;
      return;
    }

    const container = this.containers_.toArray()[index];
    const factory = this.resolver_.resolveComponentFactory(RowDetailComponent);
    const component = container.createComponent(factory);

That would maybe work it the row objects after refresh were the same instances, but these are not same, equal maybe, but different instances -> causes recheck on mat-row and mat-row clears your additional message component.

[Eddman]

And last: the reason why labels are not hidden after refresh with addition of trackBy is that the ChipsComponent does not actually react to any changes.

It creates map and key from inputs on component init and never changes these. The trackBy causes that rows keep theirsChipsComponent instances and push new changes through input, but the inputs are not used at all.

[Eddman]

That's about it, all the problems ⬆️ So I'd say there are two solutions to that:

1. You try to fix all problems and it will be hard and fragile for the future, cause with any new changes you will need to make sure it's not broken
2. Change the refresh process to keep the old objects and only merge new changes into. That will help the Angular change detector to do the job for you. I'd still suggest to fix the ChipsComponent and ColumnComponent as well...

[Eddman]

1. will also have much better performance in terms of page rendering that is one of the most expensive activities.

@floreks Do you want me to try to make a PR or you want to handle?

[floreks]

@Eddman you are welcome to work on that. I don't have that much time lately. For sure the hardest part here is fixing the trackBy function logic.