Build urls from pipelines widget and pipelines page is sometimes wrong

[jaseemabid]

As seen in the screenshot, both builds point to OSIO, while it should have pointed to a job in Jenkins.

I'm noticing this on the new 2a cluster. This is either the UI or Proxy. cc: @joshuawilson @kishansagathiya

[kishansagathiya]

@jaseemabid I just tried it pointed me to pipelines in Openshift Online Is this what is happening to you?

[jaseemabid]

No. I'm hovering on the second build URL (the screenshot avoided mouse pointer) and the URL is shown in the left corner of the screenshot I posted. To replicate this, make sure you are on the new cluster and your Jenkins is idled.

[aslakknutsen]

This sounds like the same 'config load order' issue in the UI. Sometimes the config loads before the widget so the URLs work, other times they don't. Refresh is a workaround. Can't find the other issue atm. @joshuawilson

[joshuawilson]

There are several. Related to https://github.com/openshiftio/openshift.io/issues/925

[aslakknutsen]

@joshuawilson How is #925 related?

[kishansagathiya]

I faced this issue as well, today.

[joshuawilson]

@aslakknutsen I don't know, I must have put the wrong number in.

[jiekang]

Possible affecting race condition found:

oauth-config-store.ts:
  load() {
    [...]
          _latestOAuthConfig = new OAuthConfig(data);
          if (this.userService.currentLoggedInUser.attributes) {
            let cluster = this.userService.currentLoggedInUser.attributes.cluster;
            let console = cluster.replace('api', 'console');
            _latestOAuthConfig.openshiftConsoleUrl = console + 'console';
          }
  [...]

If userService populates currentLoggedInUser first, this works, if not, it fails to assign the ConsoleUrl used by other parts of the application. I'm not sure why the default is not an undefined value and breaking though.

A test is to completely comment the if block to simulate false. All the pipeline build links will then point to a bogus location (localhost:3000 when run locally and I assume openshift.io when deployed)

Edit: Not to mention this is within a http.get...subscribe call which is also asynchronous. Though I think the loading code in place will allow clients to synchronize if they are written correctly.

[jiekang]

API for the user service has:

    /**
     * The currently logged in user - please use currentLoggedInUser instead of subscribing
     * TODO: move this to deprecated
     */
    loggedInUser: ConnectableObservable<User>;
    /**
     * The current logged in user - should be always populated after login
     */
    currentLoggedInUser: User;

The implementation relies on http to acquire the user data. As a client, I would actually prefer subscribing to an exposed Observable that emits the user data when it's available. That way I can order reliant things on it.

@joshuawilson who works on ngx-login-client? I'd propose not deprecating the Observable, and actually dropping currentLoggedInUser. It's not very useful since there can't actually be a guarantee on it's value meaning client needs to code logic against that anyways, which an Observable helps with greatly.

[jiekang]

Hmm user service implementation has TODO

    // TODO - switch to internal observable that is populated on initialization
    // and only expose currentLoggedInUser publicly

I guess if this is actually done, it would resolve the issue as well, assuming users of UserService won't continue until initialization of ngOnInit completes. However it sounds like an unnecessary bottleneck; I'd rather code properly for the asynchronous scenario.

[joshuawilson]

@jiekang @dgutride has worked on it in the past and knows the most about the Observables. @rohitkrai03 will be working on it from the auth team.

[dgutride]

@jiekang - please do not drop the currentLoggedInUser - there are zero instances where we do not redirect back to openshift.io after logging in - asking all pages to resolve that using http is a mistake and we should instead make sure the application is bootstrapped correctly with the logged in user information.

[jiekang]

@dgutride To clarify: we want the app to load it's components only when the user info is available, right?

Edit: There are visitable areas where a user is not logged in though, right? I need to get a clearer picture of the redirect flows in these scenarios to comment further...

[dgutride]

@jiekang - that's correct - but loggedInUser will only ever update on a full window refresh - listening to it is meaningless because even though loaded via http, it can't change so the subscription only adds overhead. We should ideally add this to a resolver or a bootstrap method that prevents the app from loading without retrieving the correct information - I'm open to suggestions but the loggedInUser subscribes all over the place are a huge problem for our maintainability.

[jiekang]

@dgutride

Assuming the constructor of a class must complete before a client can call any of it's functions, making the UserService wait some time for the loggedInUser to be populated seems like a valid approach.

The ngx-login-client UserService implementation uses a http get request to populate the data though. We will need to handle the fact that this comes from a http request which could take an unknown amount of time to complete, or never complete.

If loggedInUser must be populated, this suggests some loading state which retries the request a number of times with some kind of fail state that informs the user something is wrong with the service.

If loggedInUser need not be populated, this suggests that an Observable to subscribe to for the users who do need it is actually the right path. If so, could you elaborate on maintainability issues that arise from this? Edit: re-reading your comment, I think loggedInUser should be populated via the http request 'always', no matter what. I think that makes sense so this can be disregarded, sorry;;

Regarding further guaranteeing this is done before anything else, one method is to hook into Angular's initialization process via the APP_INITIALIZER token [1]. We should be able to provide a function that takes the UserService as an argument.

[1] https://angular.io/api/core/APP_INITIALIZER

[jiekang]

Confirmed initial diagnosis via modifying local copy of ngx-login-client:user.service.ts::constructor(...) {...}

Added a delay to the observable that sets this.currentLoggedInUser = user; to simulate for example a slow http request:

    this.loggedInUser = Observable.merge(
      broadcaster.on('loggedin')
        .map(val => 'loggedIn'),
      broadcaster.on('logout')
        .map(val => 'loggedOut'),
      broadcaster.on('authenticationError')
        .map(val => 'authenticationError')
    )
      .switchMap(val => {
        // If it's a login event, then we need to retreive the user's details
        if (val === 'loggedIn') {
          return this.http
            .get(this.userUrl, { headers: this.headers })
            .map(response => cloneDeep(response.json().data as User));
        } else {
          // Otherwise, we clear the user
          return Observable.of({} as User);
        }
      })
      .delay(5000) // Simulate slow request
      .do(user => {
        this.currentLoggedInUser = user;
        // TODO remove this - ensure nobody is using userData anymore
        this.userData = user;
      })

will cause pipeline links to be broken.

[jiekang]

@dgutride

" I'm open to suggestions but the loggedInUser subscribes all over the place are a huge problem for our maintainability."

Can you elaborate further on how subscribes are a problem for maintainability?

Upon further investigation, the setting of the loggedInUser is embedded within the login/logout flow for the entire application. The login flow relies on a redirect with query parameter containing the JWT auth token, which is grabbed and placed into local storage. I have yet to find a way to do this while inside the initialization phase of the application. In it's current state, it does not seem to be movable into the APP_INITIALIZER. I don't have recommendations at this point on the login/logout system currently in place.

As far as this issue is concerned, all users of currentLoggedInUser are subject to the race condition. This is broken. I highly recommend reverting to using the subscription until this implementation is fixed. The UserService is provided by ngx-login-client, a third-party library to fabric8-ui. The implementation of currentLoggedInUser requires all users, not just fabric8-ui to workaround it. Apart from being bad, this should at least be communicated in the API documentation.

[dgutride]

@jiekang - I understand what you are saying - we need to make sure the application cannot load if the user isn't resolved. We can add this to the parent application component and prevent the internal components from rendering until it's populated, a root resolver for the routes or anything else that turns the user information into a synchronous call. Using async calls for the user has led to significant issues and performance problems for the application so we should move forward with currentLoggedInUser and ensure it's populated by whatever means possible.

Why is what we have now bad? It is required that consumers of loggedInUser understand that they need to asynchronously wait for the result - we've had dozens of bugs where people didn't realize this an attributes are null causing the page to break and prevent logging out The subscriptions also can cause multiple api requests per page because we aren't sharing the logged in user attributes Subscriptions inherently add performance cost to angular applications because of the watches rxjs sets up

[jiekang]

@dgutride

There are a number of related topics under discussion now so I'll try to organize my thoughts more.

1. A change was introduced to fabric8-ui that is broken. It would not be broken if we had a some design. We do not have the design and adding the design would involve a large amount of refactoring of fabric8-ui. In my opinion, since said change relies on said design, the change should not be introduced until said design exists. Therefore, the change should be reverted. The design should stand as it's own issue for discussion and further work.

2. ngx-login-client is a library that is used by fabric8-ui and possibly by others. It's public API should be well documented so users are aware of how to use it. The documentation surrounding currentLoggedInUser is not currently clear. Also a separate issue for discussion and further work.

3. Anything regarding performance can only truly be supported by numbers. I can't find any blogs or documentation discussing the actual performance of rxjs Observables. However, I only used Google search with terms 'rxjs observable performance' and 'observables too many subscriptions'. If we want to talk about performance we should also be doing our own tests. As the application is not yet stable and is still changing rapidly, I think performance is low priority. A 'slow' system that works all the time is much more preferable than a 'fast' system that breaks every few days. In this particular case, at random, at any time.

4. It sounds like many issues stem from developers using Observables incorrectly. This is a cost of using a technology, it must be understood. However, asynchronous behavior is something we must tackle, whether it's via Observables, Promises or some other system. It's inherent in how the internet works.

5. Moving forward, related to (4) I think we can do better with a more strict review process. Unit tests can be written to catch these scenarios. They should be required. Developers should review the code and be able to catch these issues in the review process. People who submit code will learn from the reviews of their mistakes and then be less likely to repeat them in the future. Common mistakes could be documented for new developers to learn from. As well better accountability could help. In a previous project, if a bug made it into the codebase, the developer who wrote the code would be prompted when the underlying issue was found (ie. which lines of code that were broken) Even if in the end, it was a separate person to write the fix, it's helpful to do this so the developer who wrote the problem realizes that they wrote something problematic and can learn to not make the same mistake again. If they are never notified, they may never know and not have the opportunity to learn.

[joshuawilson]

@jiekang Those are all great points. Thanks for taking the time to write them down. I'll do my best to respond to them. (@dgutride may have more insight and can respond too)

1) The original design was to follow the basic component architecture laid out in the Angular docs. We have out grown that and we have #2501 as a start. This is something we can all work out together. 2) It is public and I agree, it needs better documentation. That falls under #2501 again. 3) Fair point. More research needs to be done on Performance. 4) RxJS is the least known technology we are using. We are going to make mistakes. We need to learn from them. 5) I agree with notifying the original dev. I try to do that when I find something. Having a stricter review process without lots of paperwork requires an internal drive for excellence. I hope that all the devs have that but I do not think they do.