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

Sidebranch: address transition issues on replication engine and actions #9010

Merged
merged 26 commits into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
e06d8e5
small formatting changes
Monkeychip May 15, 2020
a9e20c6
change findRecord to peekRecord so it keeps track of the changing data.
Monkeychip May 15, 2020
9b9837d
add styling such that when page is loading it does not spread across …
Monkeychip May 15, 2020
3de1634
help with reload and styling on replication route
Monkeychip May 15, 2020
35f910c
initial setup for new flow that handles adding a perf secondary, and …
Monkeychip May 15, 2020
c07e81a
clean up
Monkeychip May 15, 2020
4b6e19a
add loader on rep page for situations when data is still loading, and…
Monkeychip May 18, 2020
60d9fec
fix transitionTo when coming from different replication.mode vs repli…
Monkeychip May 18, 2020
b4fce6d
set default of mode for radio checkboxes after removing from DEFAULTS…
Monkeychip May 18, 2020
fc38bba
reset and cont using onEnable because TransitionTo is not working ins…
Monkeychip May 18, 2020
99be007
remove console
Monkeychip May 18, 2020
96cd6e1
the reason we were getting transition errors :(
Monkeychip May 18, 2020
1c53f95
remove modeObjecT
Monkeychip May 18, 2020
6cecbbf
fix error by removing peek record from application and moving it lowe…
Monkeychip May 19, 2020
ee3ea14
Readd back space
Monkeychip May 19, 2020
1c7a824
this one really does fix the issue
Monkeychip May 19, 2020
ab4365a
add back peek record and add conditional to isLoadingData
Monkeychip May 19, 2020
4c126bb
figure out cluster id from service instead of hardcoded
Monkeychip May 19, 2020
921118c
fix capabilities-self error by adding a 1 sceond delay for when trans…
Monkeychip May 20, 2020
8ce3d2c
remove attempt to circumvent the peekRecord in application
Monkeychip May 20, 2020
ef8d19e
add to replication page tests and clarify replicationMode to formatte…
Monkeychip May 20, 2020
0b10c7b
fix repetive conditional
Monkeychip May 20, 2020
988f54a
capture the state when either dr.mode or performance.mode are undefin…
Monkeychip May 21, 2020
280b8b5
address some pr comments
Monkeychip May 21, 2020
a3636c5
small change
Monkeychip May 21, 2020
03eed54
add bootstrapping mode to test
Monkeychip May 21, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions ui/app/models/cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ export default DS.Model.extend({
// replicationAttrs will then return the relevant `replication-attributes` fragment
rm: service('replication-mode'),
replicationMode: alias('rm.mode'),
replicationIsInitializing: computed('dr.mode', 'performance.mode', function() {
// a mode of null only happens when a cluster is being initialized
// otherwise the mode will be 'disabled', 'primary', 'secondary'
return !this.dr.mode || !this.performance.mode;
}),
replicationAttrs: computed('dr.mode', 'performance.mode', 'replicationMode', function() {
const replicationMode = this.get('replicationMode');
return replicationMode ? get(this, replicationMode) : null;
Expand Down
8 changes: 8 additions & 0 deletions ui/app/models/replication-attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@ export default Fragment.extend({
? 'bootstrapping'
: (this.get('isSecondary') && 'secondary') || (this.get('isPrimary') && 'primary');
}),
modeForHeader: computed('mode', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This separates out a specific property for the mode displayed in the Header. We needed one that wasn't also moonlighting for a property from the URL (e.g. modeForUrl).
image

const mode = this.mode;
if (!mode) {
// mode will be false or undefined if it calls the status endpoint while still setting up the cluster
return 'loading';
}
return mode;
}),
secondaryId: attr('string'),
primaryClusterAddr: attr('string'),
knownPrimaryClusterAddrs: attr('array'),
Expand Down
26 changes: 19 additions & 7 deletions ui/lib/core/addon/components/replication-page.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export default Component.extend({
layout,
store: service(),
reindexingDetails: null,

didReceiveAttrs() {
this._super(arguments);
this.getReplicationModeStatus.perform();
Expand All @@ -26,20 +25,33 @@ export default Component.extend({
.adapterFor('replication-mode')
.fetchStatus(replicationMode);
} catch (e) {
console.log(e);
// do not handle error
}
this.set('reindexingDetails', resp);
}),
replicationMode: computed('model.{replicationMode}', function() {
formattedReplicationMode: computed('model.{replicationMode}', function() {
// dr or performance 🤯
let mode = this.model.replicationMode;
const mode = this.model.replicationMode;
return MODE[mode];
}),
clusterMode: computed('model.{replicationAttrs}', function() {
// primary or secondary
const { model } = this;
return model.replicationAttrs.mode;
}),
isLoadingData: computed('clusterMode', 'model.{replicationAttrs}', function() {
const { clusterMode } = this;
const { model } = this;
const clusterId = model.replicationAttrs.clusterId;
const replicationDisabled = model.replicationAttrs.replicationDisabled;

if (clusterMode === 'bootstrapping' || (!clusterId && !replicationDisabled)) {
// if clusterMode is bootstrapping
// if no clusterId, the data hasn't loaded yet, wait for another status endpoint to be called
return true;
}
return false;
}),
isSecondary: computed('clusterMode', function() {
const { clusterMode } = this;
return clusterMode === 'secondary';
Expand All @@ -55,10 +67,10 @@ export default Component.extend({
}
return false;
}),
message: computed('model.{anyReplicationEnabled}', 'replicationMode', function() {
message: computed('model.{anyReplicationEnabled}', 'formattedReplicationMode', function() {
if (this.model.anyReplicationEnabled) {
return `This ${this.replicationMode} secondary has not been enabled. You can do so from the ${this.replicationMode} Primary.`;
return `This ${this.formattedReplicationMode} secondary has not been enabled. You can do so from the ${this.formattedReplicationMode} Primary.`;
}
return `This cluster has not been enabled as a ${this.replicationMode} Secondary. You can do so by enabling replication and adding a secondary from the ${this.replicationMode} Primary.`;
return `This cluster has not been enabled as a ${this.formattedReplicationMode} Secondary. You can do so by enabling replication and adding a secondary from the ${this.formattedReplicationMode} Primary.`;
}),
});
12 changes: 3 additions & 9 deletions ui/lib/core/addon/mixins/replication-actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,16 +88,10 @@ export default Mixin.create({
if (action === 'disable') {
yield this.onDisable();
}
if (mode === 'secondary' && replicationMode === 'dr') {
Monkeychip marked this conversation as resolved.
Show resolved Hide resolved
// return mode so you can properly handle the transition
return mode;
}
if (action === 'enable') {
try {
yield this.onEnable(replicationMode); // should not be called for dr secondary
} catch (e) {
// TODO handle error
}
/// onEnable is a method available only to route vault.cluster.replication.index
// if action 'enable' is called from vault.cluster.replication.mode.index this method is not called
Copy link
Contributor

Choose a reason for hiding this comment

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

when you say 'this method is not called', do you mean that we never hit this conditional and execute line 94 if we're on the vault.cluster.replication.mode.index?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We hit the conditional, but because it can't find the method 'onEnable' it just stops. There is some refactoring here that could help, but onEnable is used by a lot of the actions I wasn't dealing with so I just added a comment as it's a gotchya when trying to problem solve.

yield this.onEnable(replicationMode, mode);
}
}).drop(),

Expand Down
2 changes: 1 addition & 1 deletion ui/lib/core/addon/templates/components/layout-loading.hbs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<div class="is-flex-v-centered is-flex-1 loader-inner-page" >
<div class="is-flex-v-centered is-flex-1 loader-inner-page" data-test-layout-loading>
Copy link
Contributor

Choose a reason for hiding this comment

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

this def doesn't need to change, but as an FYI since this component already has a unique class name like loader-inner-page, it doesn't necessarily need a data-test-* attribute. comes down to personal preference, i suppose :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good to know. Any chance you can add this to your Ember learn on testing 🙂📖

<div class="columns is-centered">
<div class="column is-narrow has-text-centered has-text-grey-dark has-current-color-fill">
<div class="level is-mobile">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@
</div>
<div class="level-right">
{{#if replicationDisabled}}
{{#link-to "vault.cluster.replication.mode.index" cluster.name mode class="button is-primary"}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we are using peekRecord, this was causing a console error. I can't exactly explain why the peekRecord now adds a vault.cluster.replication to the link to helper here, but it does. Here was the error, you can see the link-to is trying to hit vault.cluster.replication.vault.cluster.replication.mode.index
image

Copy link
Contributor

Choose a reason for hiding this comment

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

👏

{{#link-to "mode.index" cluster.name mode class="button is-primary"}}
Enable
{{/link-to}}
{{else}}
Expand Down
42 changes: 23 additions & 19 deletions ui/lib/core/addon/templates/components/replication-page.hbs
Original file line number Diff line number Diff line change
@@ -1,22 +1,26 @@
<div class="replication-page" data-test-replication-page>
{{yield
(hash
header=(component 'replication-header'
data=model
title=replicationMode
isSecondary=isSecondary
secondaryId=replicationDetails.secondaryId
{{#if isLoadingData }}
<LayoutLoading />
{{else}}
{{yield
(hash
header=(component 'replication-header'
data=model
title=formattedReplicationMode
isSecondary=isSecondary
secondaryId=replicationDetails.secondaryId
)
dashboard=(component
'replication-dashboard'
data=model
isSecondary=isSecondary
replicationDetails=replicationDetails
clusterMode=clusterMode
reindexingDetails=reindexingDetails
)
isDisabled=isDisabled
message=message
)
dashboard=(component
'replication-dashboard'
data=model
isSecondary=isSecondary
replicationDetails=replicationDetails
clusterMode=clusterMode
reindexingDetails=reindexingDetails
)
isDisabled=isDisabled
message=message
)
}}
}}
{{/if}}
</div>
17 changes: 3 additions & 14 deletions ui/lib/replication/addon/components/replication-summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import ReplicationActions from 'core/mixins/replication-actions';
import { task } from 'ember-concurrency';

const DEFAULTS = {
mode: 'primary',
token: null,
id: null,
loading: false,
Expand All @@ -16,10 +15,11 @@ const DEFAULTS = {
primary_cluster_addr: null,
ca_file: null,
ca_path: null,
replicationMode: 'dr',
};

export default Component.extend(ReplicationActions, DEFAULTS, {
replicationMode: 'dr',
mode: 'primary',
wizard: service(),
version: service(),
didReceiveAttrs() {
Expand Down Expand Up @@ -66,24 +66,13 @@ export default Component.extend(ReplicationActions, DEFAULTS, {
this.setProperties(DEFAULTS);
},

transitionTo: computed('mode', 'replicationMode', function() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had originally created this to handle DR secondary transition issues, but this is being reverted to how it is in master, and the transition conditions are handled in the controller/replication-mode.js

// Take transitionTo outside of a yield because it unmounts the cluster and yield cannot return anything
return () => this.router.transitionTo('vault.cluster');
}),

submit: task(function*() {
let mode;
try {
mode = yield this.submitHandler.perform(...arguments);
yield this.submitHandler.perform(...arguments);
} catch (e) {
// TODO handle error
}
// if Secondary, handle transition here, if not, handle transition in mixin Enable
if (mode === 'secondary') {
this.transitionTo();
}
}),

actions: {
onSubmit(/*action, mode, data, event*/) {
this.get('submit').perform(...arguments);
Expand Down
17 changes: 15 additions & 2 deletions ui/lib/replication/addon/controllers/replication-mode.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,26 @@
import { alias } from '@ember/object/computed';
import { inject as service } from '@ember/service';
import Controller from '@ember/controller';
import { task, timeout } from 'ember-concurrency';

export default Controller.extend({
rm: service('replication-mode'),
replicationMode: alias('rm.mode'),
waitForNewClusterToInit: task(function*(replicationMode) {
// waiting for the newly enabled cluster to init
// this ensures we don't hit a capabilities-self error, called in the model of the mode/index route
yield timeout(1000);
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 love arbitrary timeouts, but I trust you that this was necessary. For diligence: is there a way to kick this off after the other thing has finished from the index route? Looks like it does the transition fine when the mode is dr ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has everything to do with hitting the model on the route it's transitioning to. For DR it's not transitioning to the same route, only in this specific case (Enabling a Performance Secondary) does it follow this flow: index ->transitionTo-> mode/index. Because this is where that transitionTo occurs, this is the only place I was able to interrupt it before hitting the routes' model that calls the capabilities-self endpoint. Essentially, right here is the last step before we transition. There's nothing - at least that I could tell - that we could wait for to complete. We don't appear to know what's happening with the cluster's initialization at this point. 🤷‍♀️

Copy link
Contributor

Choose a reason for hiding this comment

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

i was thinking about this too -- an arbitrary timeout feels like it could have the potential to be flaky. i know you have tried many ideas, but have you considered moving the fetch for canAddSecondary and such into the controller? i took a cursory glance at the ember guides for controllers and noticed:

Controllers are used as an extension of the model loaded from the Route. Any attributes or actions that we want to share with components used on that Route could be defined on the Controller and passed down through the Route’s template.

i'm not super confident it this, but technically we are trying to compute the capabilities after the model hook returns the vault cluster with replication enabled, so it might fit. 🤷‍♀️

https://guides.emberjs.com/release/routing/controllers/#toc_where-and-when-to-use-controllers

i'm not sure, but some guidance from others would be helpful here too -- @meirish, @DingoEatingFuzz, or @johncowen, do any of you have advice for us on how to ensure a network request in the model doesn't happen until the model is already "ready"?

Choose a reason for hiding this comment

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

Not having looked massively at this code yet, my question would be:

What's the 'signal' so you know when this is 'ready'?

It sounds like there isn't one?

There's nothing - at least that I could tell - that we could wait for to complete.

So a follow up question would be, whilst potentially horrible, is there something you could poll so you know when this is 'ready'?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yeah that's tricky - I'd expect replication status or cluster status to tell you if it's bootstrapping (or some other status as it's changing). Alternatively does every request return a 40x error? If so you could poll until you don't get 40x's (not great, but more of signal like John mentioned). Or even poll the capabilities-self endpoint and catch the errors until it doesn't return one.

return this.transitionToRoute('mode', replicationMode);
}),
actions: {
onEnable(mode) {
return this.transitionToRoute('mode', mode);
onEnable(replicationMode, mode) {
if (replicationMode == 'dr' && mode === 'secondary') {
return this.transitionToRoute('vault.cluster');
} else if (replicationMode === 'dr') {
return this.transitionToRoute('mode', replicationMode);
} else {
this.waitForNewClusterToInit.perform(replicationMode);
}
},
onDisable() {
return this.transitionToRoute('index');
Expand Down
4 changes: 3 additions & 1 deletion ui/lib/replication/addon/routes/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import ClusterRoute from 'vault/mixins/cluster-route';
export default Route.extend(ClusterRoute, {
version: service(),
store: service(),
auth: service(),

beforeModel() {
return this.get('version')
Expand All @@ -17,7 +18,8 @@ export default Route.extend(ClusterRoute, {
},

model() {
return this.store.findRecord('cluster', 'vault');
const activeClusterId = this.get('auth.activeCluster');
return this.store.peekRecord('cluster', activeClusterId);
},

afterModel(model) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@
<p>{{cluster.replicationModeStatus.cluster_id}}</p>
<div class="replication">
<ReplicationPage @model={{cluster}} as |Page|>
<Page.dashboard
<Page.dashboard
@data={{cluster}}
@componentToRender={{if (eq replicationAttrs.mode 'secondary') 'replication-secondary-card' 'replication-primary-card' }}
as |Dashboard|>
Expand All @@ -352,7 +352,7 @@
<Dashboard.card
@title='Last WAL entry'
@description='Index of last Write Ahead Logs entry written on local storage.'
@metric={{replicationAttrs.lastWAL}} />
@metric={{format-number replicationAttrs.lastWAL}} />
<Dashboard.secondaryCard
@cluster={{cluster}}
@replicationAttrs={{replicationAttrs}} />
Expand Down
46 changes: 27 additions & 19 deletions ui/lib/replication/addon/templates/index.hbs
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
{{#if (eq model.mode 'unsupported')}}
<PageHeader as |p|>
<p.levelLeft>
<h1 class="title is-3 has-text-grey">
Replication unsupported
</h1>
</p.levelLeft>
</PageHeader>
<EmptyState
@title="The current cluster configuration does not support replication"
/>
{{else}}
<ReplicationSummary
@cluster={{model}}
@showModeSummary={{true}}
@onEnable={{action "onEnable"}}
@onDisable={{action "onDisable"}}
/>
{{/if}}
<section class="section">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just added section and div to file so it wouldn't span the whole width while waiting to load.

<div class="container is-widescreen">
{{#if model.replicationIsInitializing }}
<LayoutLoading />
{{else}}
{{#if (eq model.mode 'unsupported')}}
<PageHeader as |p|>
<p.levelLeft>
<h1 class="title is-3 has-text-grey">
Replication unsupported
</h1>
</p.levelLeft>
</PageHeader>
<EmptyState
@title="The current cluster configuration does not support replication"
/>
{{else}}
<ReplicationSummary
@cluster={{model}}
@showModeSummary={{true}}
@onEnable={{action "onEnable"}}
@onDisable={{action "onDisable"}}
/>
{{/if}}
{{/if}}
</div>
</section>