Skip to content

Ui/primary/handle errors#8787

Closed
andaley wants to merge 20 commits into
ui/replication-status-discoverabilityfrom
ui/primary/handle-errors
Closed

Ui/primary/handle errors#8787
andaley wants to merge 20 commits into
ui/replication-status-discoverabilityfrom
ui/primary/handle-errors

Conversation

@andaley
Copy link
Copy Markdown
Contributor

@andaley andaley commented Apr 21, 2020

Handle Errors & Display cluster states consistently

Summary of changes

  • Added error states to the State and Last WAL Entry cards on the Replication Primary dashboard
  • Large refactor to the cluster model to ensure that we show the same glyphs in the status menu and all of the dashboards
  • Added a new clusterStates helper to help with aforementioned consistency**
    • Rolled it out on the Replication components
    • Replaced old usage of cluster model properties such as drStateDisplay with this new helper
  • Updated stream-wals to display an check circle icon instead of a sync icon

Examples

Error state for a DR primary

image

Status menu on a DR primary, when cluster is in a good state

image

Status menu on a DR Secondary, when cluster itself is in a good state but the connection_state is bad

image

Questions / Feedback

  • The status menu "traffic light" (i.e. the green light), icon, and icon color currently indicate the state of the current cluster only. It does not reflect the connection_state. This means that if a secondary is OK, but it's connection state is bad, we still show the status menu as "green" (see example above). The dashboard will show an error, however. I left this be for now because changing the logic of the status menu seemed like a big undertaking but I'm open to ideas.
  • I added a defaultDisplay inside cluster-states.js in case there are other states that we're not aware of. i don't love this solution because it seems like we should be aware of all possible cluster states, but it seemed better than blowing up the app if one couldn't be found.

TODO / known issues

  • add tests
  • when enabling replication, the state card and title show up before any of the data is ready
  • the add secondary link is broken when cluster is a Performance Primary

@andaley andaley changed the base branch from master to ui/replication-status-discoverability April 21, 2020 00:25
@andaley andaley added the ui label Apr 21, 2020
Comment thread ui/lib/core/addon/templates/components/replication-mode-summary.hbs Outdated
Comment thread ui/app/models/cluster.js
return this.get(`${mode}.state`);
}
const defaultDisp = 'Synced';
const displays = {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

you can see these values are now part of the CLUSTER_STATES map here: https://github.com/hashicorp/vault/pull/8787/files#diff-b8f0db91f050a7646bbb0d17521230a2R20

Comment thread ui/app/models/cluster.js
}),

stateGlyph(state) {
const glyph = 'check-circle-outline';
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

this is the default glyph to display, including when state is unknown. i let this be, but maybe we shouldn't show an icon at all if state can't be found?

Comment thread ui/app/models/cluster.js
stateGlyph(state) {
const glyph = 'check-circle-outline';

const glyphs = {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

isSyncing: false,
},
'stream-wals': {
glyph: 'check-circle-outline',
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i changed the glyph here to check-circle-outline instead of android-sync based off of conversations with bk that stream-wals is in fact the ideal syncing state.

{{else if (eq mode 'performance')}}
{{cluster.perfReplicationStateDisplay}}
{{else}}
{{get (cluster-states modeState) "display"}}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i haven't been able to figure out where this code is actually executed -- if anyone has any ideas please lemme know! otherwise, if i can get rid of this line it means i can get rid of all of the 'display' properties inside CLUSTER_STATES

Copy link
Copy Markdown
Contributor

@chelshaw chelshaw left a comment

Choose a reason for hiding this comment

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

Great work! Love the state helper -- putting all of those in one place makes a ton of sense and seems to cover edge cases well. Will pull it down and play around with it, but so far looks great!

if (title === 'States') {
const currentClusterisOk = clusterStates([state]).isOk;
const primaryIsOk = clusterStates([connection]).isOk;
return !(currentClusterisOk && primaryIsOk);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

@andaley andaley marked this pull request as ready for review April 24, 2020 16:50
@andaley
Copy link
Copy Markdown
Contributor Author

andaley commented Apr 24, 2020

I ended up in a bad git state, so moving this PR here: #8845

@andaley andaley closed this Apr 24, 2020
@andaley andaley deleted the ui/primary/handle-errors branch March 22, 2024 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants