Add latest changes from gitlab-org/gitlab@master

app/assets/javascripts/monitoring/stores/actions.js

@@ -4,28 +4,22 @@ import createFlash from '~/flash';
 import trackDashboardLoad from '../monitoring_tracking_helper';
 import statusCodes from '../../lib/utils/http_status';
 import { backOff } from '../../lib/utils/common_utils';
-import { s__, __ } from '../../locale';
+import { s__ } from '../../locale';
 
-const MAX_REQUESTS = 3;
+const TWO_MINUTES = 120000;
 
-export function backOffRequest(makeRequestCallback) {
-  let requestCounter = 0;
+function backOffRequest(makeRequestCallback) {
   return backOff((next, stop) => {
     makeRequestCallback()
       .then(resp => {
         if (resp.status === statusCodes.NO_CONTENT) {
-          requestCounter += 1;
-          if (requestCounter < MAX_REQUESTS) {
           next();
-          } else {
-            stop(new Error(__('Failed to connect to the prometheus server')));
-          }
         } else {
           stop(resp);
         }
       })
       .catch(stop);
-  });
+  }, TWO_MINUTES);
 }
 
 export const setGettingStartedEmptyState = ({ commit }) => {
@@ -52,11 +46,6 @@ export const receiveMetricsDashboardFailure = ({ commit }, error) => {
   commit(types.RECEIVE_METRICS_DATA_FAILURE, error);
 };
 
-export const requestMetricsData = ({ commit }) => commit(types.REQUEST_METRICS_DATA);
-export const receiveMetricsDataSuccess = ({ commit }, data) =>
-  commit(types.RECEIVE_METRICS_DATA_SUCCESS, data);
-export const receiveMetricsDataFailure = ({ commit }, error) =>
-  commit(types.RECEIVE_METRICS_DATA_FAILURE, error);
 export const receiveDeploymentsDataSuccess = ({ commit }, data) =>
   commit(types.RECEIVE_DEPLOYMENTS_DATA_SUCCESS, data);
 export const receiveDeploymentsDataFailure = ({ commit }) =>
@@ -149,10 +138,14 @@ export const fetchPrometheusMetrics = ({ state, commit, dispatch }, params) => {
     });
   });
 
-  return Promise.all(promises).then(() => {
+  return Promise.all(promises)
+    .then(() => {
       if (state.metricsWithData.length === 0) {
         commit(types.SET_NO_DATA_EMPTY_STATE);
       }
+    })
+    .catch(() => {
+      createFlash(s__(`Metrics|There was an error while retrieving metrics`), 'warning');
     });
 };
 

changelogs/unreleased/32778-change-retries-approach-promql-queries.yml

+---
+title: Improve the way the metrics dashboard waits for data
+merge_request: 20687
+author:
+type: fixed

doc/administration/high_availability/README.md

@@ -82,12 +82,13 @@ Complete the following installation steps in order. A link at the end of each
 section will bring you back to the Scalable Architecture Examples section so
 you can continue with the next step.
 
+1. [Load Balancer(s)](load_balancer.md)[^2]
+1. [Consul](consul.md)
 1. [PostgreSQL](database.md#postgresql-in-a-scaled-environment) with [PGBouncer](https://docs.gitlab.com/ee/administration/high_availability/pgbouncer.html)
 1. [Redis](redis.md#redis-in-a-scaled-environment)
 1. [Gitaly](gitaly.md) (recommended) and / or [NFS](nfs.md)[^4]
 1. [GitLab application nodes](gitlab.md)
     - With [Object Storage service enabled](../gitaly/index.md#eliminating-nfs-altogether)[^3]
-1. [Load Balancer(s)](load_balancer.md)[^2]
 1. [Monitoring node (Prometheus and Grafana)](monitoring_node.md)
 
 ### Full Scaling

doc/development/sql.md

@@ -123,6 +123,103 @@ class AddUsersLowerUsernameEmailIndexes < ActiveRecord::Migration[4.2]
 end
 ```
 
+## Reliably referencing database columns
+
+ActiveRecord by default returns all columns from the queried database table. In some cases the returned rows might need to be customized, for example:
+
+- Specify only a few columns to reduce the amount of data returned from the database.
+- Include columns from `JOIN` relations.
+- Perform calculations (`SUM`, `COUNT`).
+
+In this example we specify the columns, but not their tables:
+
+- `path` from the `projects` table
+- `user_id` from the `merge_requests` table
+
+The query:
+
+```ruby
+# bad, avoid
+Project.select("path, user_id").joins(:merge_requests) # SELECT path, user_id FROM "projects" ...
+```
+
+Later on, a new feature adds an extra column to the `projects` table: `user_id`. During deployment there might be a short time window where the database migration is already executed, but the new version of the application code is not deployed yet. When the query mentioned above executes during this period, the query will fail with the following error message: `PG::AmbiguousColumn: ERROR: column reference "user_id" is ambiguous`
+
+The problem is caused by the way the attributes are selected from the database. The `user_id` column is present in both the `users` and `merge_requests` tables. The query planner cannot decide which table to use when looking up the `user_id` column.
+
+When writing a customized `SELECT` statement, it's better to **explicitly specify the columns with the table name**.
+
+### Good (prefer)
+
+```ruby
+Project.select(:path, 'merge_requests.user_id').joins(:merge_requests)
+
+# SELECT "projects"."path", merge_requests.user_id as user_id FROM "projects" ...
+```
+
+```ruby
+Project.select(:path, :'merge_requests.user_id').joins(:merge_requests)
+
+# SELECT "projects"."path", "merge_requests"."id" as user_id FROM "projects" ...
+```
+
+Example using Arel (`arel_table`):
+
+```ruby
+Project.select(:path, MergeRequest.arel_table[:user_id]).joins(:merge_requests)
+
+# SELECT "projects"."path", "merge_requests"."user_id" FROM "projects" ...
+```
+
+When writing raw SQL query:
+
+```sql
+SELECT projects.path, merge_requests.user_id FROM "projects"...
+```
+
+When the raw SQL query is parameterized (needs escaping):
+
+```ruby
+include ActiveRecord::ConnectionAdapters::Quoting
+
+"""
+SELECT
+  #{quote_table_name('projects')}.#{quote_column_name('path')},
+  #{quote_table_name('merge_requests')}.#{quote_column_name('user_id')}
+FROM ...
+"""
+```
+
+### Bad (avoid)
+
+```ruby
+Project.select('id, path, user_id').joins(:merge_requests).to_sql
+
+# SELECT id, path, user_id FROM "projects" ...
+```
+
+```ruby
+Project.select("path", "user_id").joins(:merge_requests)
+# SELECT "projects"."path", "user_id" FROM "projects" ...
+
+# or
+
+Project.select(:path, :user_id).joins(:merge_requests)
+# SELECT "projects"."path", "user_id" FROM "projects" ...
+```
+
+When a column list is given, ActiveRecord tries to match the arguments against the columns defined in the `projects` table and prepend the table name automatically. In this case, the `id` column is not going to be a problem, but the `user_id` column could return unexpected data:
+
+```ruby
+Project.select(:id, :user_id).joins(:merge_requests)
+
+# Before deployment (user_id is taken from the merge_requests table):
+# SELECT "projects"."id", "user_id" FROM "projects" ...
+
+# After deployment (user_id is taken from the projects table):
+# SELECT "projects"."id", "projects"."user_id" FROM "projects" ...
+```
+
 ## Plucking IDs
 
 This can't be stressed enough: **never** use ActiveRecord's `pluck` to pluck a

locale/gitlab.pot

@@ -7181,9 +7181,6 @@ msgstr ""
 msgid "Failed to check related branches."
 msgstr ""
 
-msgid "Failed to connect to the prometheus server"
-msgstr ""
-
 msgid "Failed to create Merge Request. Please try again."
 msgstr ""
 

scripts/review_apps/review-apps.sh

@@ -169,10 +169,12 @@ function install_external_dns() {
     helm repo update --tiller-namespace "${namespace}"
 
     # Default requested: CPU => 0, memory => 0
+    # Chart > 2.6.1 has a problem with AWS so we're pinning it for now.
+    # See https://gitlab.com/gitlab-org/gitlab/issues/37269 and https://github.com/kubernetes-sigs/external-dns/issues/1262
     helm install stable/external-dns \
       --tiller-namespace "${namespace}" \
       --namespace "${namespace}" \
-      --version '^2.2.1' \
+      --version '2.6.1' \
       --name "${release}" \
       --set provider="aws" \
       --set aws.credentials.secretKey="${REVIEW_APPS_AWS_SECRET_KEY}" \

spec/frontend/cycle_analytics/limit_warning_component_spec.js

 import Vue from 'vue';
+import { shallowMount } from '@vue/test-utils';
 import Translate from '~/vue_shared/translate';
-import limitWarningComp from '~/cycle_analytics/components/limit_warning_component.vue';
+import LimitWarningComponent from '~/cycle_analytics/components/limit_warning_component.vue';
 
 Vue.use(Translate);
 
+const createComponent = props =>
+  shallowMount(LimitWarningComponent, {
+    propsData: {
+      ...props,
+    },
+    sync: false,
+    attachToDocument: true,
+  });
+
 describe('Limit warning component', () => {
   let component;
-  let LimitWarningComponent;
 
   beforeEach(() => {
-    LimitWarningComponent = Vue.extend(limitWarningComp);
+    component = null;
+  });
+
+  afterEach(() => {
+    component.destroy();
   });
 
   it('should not render if count is not exactly than 50', () => {
-    component = new LimitWarningComponent({
-      propsData: {
-        count: 5,
-      },
-    }).$mount();
+    component = createComponent({ count: 5 });
 
-    expect(component.$el.textContent.trim()).toBe('');
+    expect(component.text().trim()).toBe('');
 
-    component = new LimitWarningComponent({
-      propsData: {
-        count: 55,
-      },
-    }).$mount();
+    component = createComponent({ count: 55 });
 
-    expect(component.$el.textContent.trim()).toBe('');
+    expect(component.text().trim()).toBe('');
   });
 
   it('should render if count is exactly 50', () => {
-    component = new LimitWarningComponent({
-      propsData: {
-        count: 50,
-      },
-    }).$mount();
+    component = createComponent({ count: 50 });
 
-    expect(component.$el.textContent.trim()).toBe('Showing 50 events');
+    expect(component.text().trim()).toBe('Showing 50 events');
   });
 });

spec/frontend/monitoring/store/actions_spec.js

 import MockAdapter from 'axios-mock-adapter';
 import Tracking from '~/tracking';
-import { TEST_HOST } from 'helpers/test_constants';
 import testAction from 'helpers/vuex_action_helper';
 import axios from '~/lib/utils/axios_utils';
 import statusCodes from '~/lib/utils/http_status';
 import { backOff } from '~/lib/utils/common_utils';
+import createFlash from '~/flash';
 
 import store from '~/monitoring/stores';
 import * as types from '~/monitoring/stores/mutation_types';
 import {
-  backOffRequest,
   fetchDashboard,
   receiveMetricsDashboardSuccess,
   receiveMetricsDashboardFailure,
@@ -17,7 +16,6 @@ import {
   fetchEnvironmentsData,
   fetchPrometheusMetrics,
   fetchPrometheusMetric,
-  requestMetricsData,
   setEndpoints,
   setGettingStartedEmptyState,
 } from '~/monitoring/stores/actions';
@@ -31,6 +29,7 @@ import {
 } from '../mock_data';
 
 jest.mock('~/lib/utils/common_utils');
+jest.mock('~/flash');
 
 const resetStore = str => {
   str.replaceState({
@@ -40,71 +39,36 @@ const resetStore = str => {
   });
 };
 
-const MAX_REQUESTS = 3;
-
-describe('Monitoring store helpers', () => {
+describe('Monitoring store actions', () => {
   let mock;
-
-  // Mock underlying `backOff` function to remove in-built delay.
-  backOff.mockImplementation(
-    callback =>
-      new Promise((resolve, reject) => {
-        const stop = arg => (arg instanceof Error ? reject(arg) : resolve(arg));
-        const next = () => callback(next, stop);
-        callback(next, stop);
-      }),
-  );
-
   beforeEach(() => {
     mock = new MockAdapter(axios);
-  });
-
-  afterEach(() => {
-    mock.restore();
-  });
 
-  describe('backOffRequest', () => {
-    it('returns immediately when recieving a 200 status code', () => {
-      mock.onGet(TEST_HOST).reply(200);
+    // Mock `backOff` function to remove exponential algorithm delay.
+    jest.useFakeTimers();
 
-      return backOffRequest(() => axios.get(TEST_HOST)).then(() => {
-        expect(mock.history.get.length).toBe(1);
-      });
+    backOff.mockImplementation(callback => {
+      const q = new Promise((resolve, reject) => {
+        const stop = arg => (arg instanceof Error ? reject(arg) : resolve(arg));
+        const next = () => callback(next, stop);
+        // Define a timeout based on a mock timer
+        setTimeout(() => {
+          callback(next, stop);
         });
-
-    it(`repeats the network call ${MAX_REQUESTS} times when receiving a 204 response`, done => {
-      mock.onGet(TEST_HOST).reply(statusCodes.NO_CONTENT, {});
-
-      backOffRequest(() => axios.get(TEST_HOST))
-        .then(done.fail)
-        .catch(() => {
-          expect(mock.history.get.length).toBe(MAX_REQUESTS);
-          done();
       });
+      // Run all resolved promises in chain
+      jest.runOnlyPendingTimers();
+      return q;
     });
   });
-});
-
-describe('Monitoring store actions', () => {
-  let mock;
-  beforeEach(() => {
-    mock = new MockAdapter(axios);
-  });
   afterEach(() => {
     resetStore(store);
-    mock.restore();
-  });
-  describe('requestMetricsData', () => {
-    it('sets emptyState to loading', () => {
-      const commit = jest.fn();
-      const { state } = store;
-      requestMetricsData({
-        state,
-        commit,
-      });
-      expect(commit).toHaveBeenCalledWith(types.REQUEST_METRICS_DATA);
-    });
+    mock.reset();
+
+    backOff.mockReset();
+    createFlash.mockReset();
   });
+
   describe('fetchDeploymentsData', () => {
     it('commits RECEIVE_DEPLOYMENTS_DATA_SUCCESS on error', done => {
       const dispatch = jest.fn();
@@ -362,17 +326,11 @@ describe('Monitoring store actions', () => {
     it('commits empty state when state.groups is empty', done => {
       const state = storeState();
       const params = {};
-      fetchPrometheusMetrics(
-        {
-          state,
-          commit,
-          dispatch,
-        },
-        params,
-      )
+      fetchPrometheusMetrics({ state, commit, dispatch }, params)
         .then(() => {
           expect(commit).toHaveBeenCalledWith(types.SET_NO_DATA_EMPTY_STATE);
           expect(dispatch).not.toHaveBeenCalled();
+          expect(createFlash).not.toHaveBeenCalled();
           done();
         })
         .catch(done.fail);
@@ -382,20 +340,42 @@ describe('Monitoring store actions', () => {
       const state = storeState();
       state.dashboard.panel_groups = metricsDashboardResponse.dashboard.panel_groups;
       const metric = state.dashboard.panel_groups[0].panels[0].metrics[0];
-      fetchPrometheusMetrics(
-        {
-          state,
-          commit,
-          dispatch,
-        },
+      fetchPrometheusMetrics({ state, commit, dispatch }, params)
+        .then(() => {
+          expect(dispatch).toHaveBeenCalledTimes(3);
+          expect(dispatch).toHaveBeenCalledWith('fetchPrometheusMetric', {
+            metric,
             params,
-      )
+          });
+
+          expect(createFlash).not.toHaveBeenCalled();
+
+          done();
+        })
+        .catch(done.fail);
+      done();
+    });
+
+    it('dispatches fetchPrometheusMetric for each panel query, handles an error', done => {
+      const params = {};
+      const state = storeState();
+      state.dashboard.panel_groups = metricsDashboardResponse.dashboard.panel_groups;
+      const metric = state.dashboard.panel_groups[0].panels[0].metrics[0];
+
+      // Mock having one out of three metrics failing
+      dispatch.mockRejectedValueOnce(new Error('Error fetching this metric'));
+      dispatch.mockResolvedValue();
+
+      fetchPrometheusMetrics({ state, commit, dispatch }, params)
         .then(() => {
           expect(dispatch).toHaveBeenCalledTimes(3);
           expect(dispatch).toHaveBeenCalledWith('fetchPrometheusMetric', {
             metric,
             params,
           });
+
+          expect(createFlash).toHaveBeenCalledTimes(1);
+
           done();
         })
         .catch(done.fail);
@@ -403,28 +383,75 @@ describe('Monitoring store actions', () => {
     });
   });
   describe('fetchPrometheusMetric', () => {
-    it('commits prometheus query result', done => {
-      const commit = jest.fn();
     const params = {
       start: '2019-08-06T12:40:02.184Z',
       end: '2019-08-06T20:40:02.184Z',
     };
-      const metric = metricsDashboardResponse.dashboard.panel_groups[0].panels[0].metrics[0];
-      const state = storeState();
-      const data = metricsGroupsAPIResponse[0].panels[0].metrics[0];
-      const response = {
-        data,
-      };
-      mock.onGet('http://test').reply(200, response);
+    let commit;
+    let metric;
+    let state;
+    let data;
+
+    beforeEach(() => {
+      commit = jest.fn();
+      state = storeState();
+      [metric] = metricsDashboardResponse.dashboard.panel_groups[0].panels[0].metrics;
+      [data] = metricsGroupsAPIResponse[0].panels[0].metrics;
+    });
+
+    it('commits result', done => {
+      mock.onGet('http://test').reply(200, { data }); // One attempt
+
       fetchPrometheusMetric({ state, commit }, { metric, params })
         .then(() => {
           expect(commit).toHaveBeenCalledWith(types.SET_QUERY_RESULT, {
             metricId: metric.metric_id,
             result: data.result,
           });
+
+          expect(mock.history.get).toHaveLength(1);
           done();
         })
         .catch(done.fail);
     });
+
+    it('commits result, when waiting for results', done => {
+      // Mock multiple attempts while the cache is filling up
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').reply(200, { data }); // 4th attempt
+
+      const fetch = fetchPrometheusMetric({ state, commit }, { metric, params });
+
+      fetch
+        .then(() => {
+          expect(commit).toHaveBeenCalledWith(types.SET_QUERY_RESULT, {
+            metricId: metric.metric_id,
+            result: data.result,
+          });
+          expect(mock.history.get).toHaveLength(4);
+          done();
+        })
+        .catch(done.fail);
+    });
+
+    it('commits failure, when waiting for results and getting a server error', done => {
+      // Mock multiple attempts while the cache is filling up and fails
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').replyOnce(statusCodes.NO_CONTENT);
+      mock.onGet('http://test').reply(500); // 4th attempt
+
+      fetchPrometheusMetric({ state, commit }, { metric, params })
+        .then(() => {
+          done.fail();
+        })
+        .catch(() => {
+          expect(commit).not.toHaveBeenCalled();
+          expect(mock.history.get).toHaveLength(4);
+          done();
+        });
+    });
   });
 });

spec/frontend/vue_shared/components/markdown/header_spec.js

-import Vue from 'vue';
+import { shallowMount } from '@vue/test-utils';
 import $ from 'jquery';
-import headerComponent from '~/vue_shared/components/markdown/header.vue';
+import HeaderComponent from '~/vue_shared/components/markdown/header.vue';
+import ToolbarButton from '~/vue_shared/components/markdown/toolbar_button.vue';
 
 describe('Markdown field header component', () => {
-  let vm;
+  let wrapper;
 
-  beforeEach(() => {
-    const Component = Vue.extend(headerComponent);
-
-    vm = new Component({
+  const createWrapper = props => {
+    wrapper = shallowMount(HeaderComponent, {
       propsData: {
         previewMarkdown: false,
+        ...props,
       },
-    }).$mount();
+      sync: false,
+      attachToDocument: true,
+    });
+  };
+
+  const findToolbarButtons = () => wrapper.findAll(ToolbarButton);
+  const findToolbarButtonByProp = (prop, value) =>
+    findToolbarButtons()
+      .filter(button => button.props(prop) === value)
+      .at(0);
+
+  beforeEach(() => {
+    createWrapper();
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+    wrapper = null;
   });
 
   it('renders markdown header buttons', () => {
@@ -29,75 +46,72 @@ describe('Markdown field header component', () => {
       'Add a table',
       'Go full screen',
     ];
-    const elements = vm.$el.querySelectorAll('.toolbar-btn');
+    const elements = findToolbarButtons();
 
-    elements.forEach((buttonEl, index) => {
-      expect(buttonEl.getAttribute('data-original-title')).toBe(buttons[index]);
+    elements.wrappers.forEach((buttonEl, index) => {
+      expect(buttonEl.props('buttonTitle')).toBe(buttons[index]);
     });
   });
 
   it('renders `write` link as active when previewMarkdown is false', () => {
-    expect(vm.$el.querySelector('li:nth-child(1)').classList.contains('active')).toBeTruthy();
+    expect(wrapper.find('li:nth-child(1)').classes()).toContain('active');
   });
 
   it('renders `preview` link as active when previewMarkdown is true', () => {
-    vm.previewMarkdown = true;
+    createWrapper({ previewMarkdown: true });
 
-    Vue.nextTick(() => {
-      expect(vm.$el.querySelector('li:nth-child(2)').classList.contains('active')).toBeTruthy();
-    });
+    expect(wrapper.find('li:nth-child(2)').classes()).toContain('active');
   });
 
   it('emits toggle markdown event when clicking preview', () => {
-    jest.spyOn(vm, '$emit').mockImplementation();
-
-    vm.$el.querySelector('.js-preview-link').click();
+    wrapper.find('.js-preview-link').trigger('click');
 
-    expect(vm.$emit).toHaveBeenCalledWith('preview-markdown');
+    expect(wrapper.emitted('preview-markdown').length).toEqual(1);
 
-    vm.$el.querySelector('.js-write-link').click();
+    wrapper.find('.js-write-link').trigger('click');
 
-    expect(vm.$emit).toHaveBeenCalledWith('write-markdown');
+    expect(wrapper.emitted('write-markdown').length).toEqual(1);
   });
 
   it('does not emit toggle markdown event when triggered from another form', () => {
-    jest.spyOn(vm, '$emit').mockImplementation();
-
     $(document).triggerHandler('markdown-preview:show', [
       $(
         '<form><div class="js-vue-markdown-field"><textarea class="markdown-area"></textarea></div></form>',
       ),
     ]);
 
-    expect(vm.$emit).not.toHaveBeenCalled();
+    expect(wrapper.emitted('preview-markdown')).toBeFalsy();
+    expect(wrapper.emitted('write-markdown')).toBeFalsy();
   });
 
   it('blurs preview link after click', () => {
-    const link = vm.$el.querySelector('li:nth-child(2) button');
+    const link = wrapper.find('li:nth-child(2) button');
     jest.spyOn(HTMLElement.prototype, 'blur').mockImplementation();
 
-    link.click();
+    link.trigger('click');
 
-    expect(link.blur).toHaveBeenCalled();
+    expect(link.element.blur).toHaveBeenCalled();
   });
 
   it('renders markdown table template', () => {
-    expect(vm.mdTable).toEqual(
+    const tableButton = findToolbarButtonByProp('icon', 'table');
+
+    expect(tableButton.props('tag')).toEqual(
       '| header | header |\n| ------ | ------ |\n| cell | cell |\n| cell | cell |',
     );
   });
 
   it('renders suggestion template', () => {
-    vm.lineContent = 'Some content';
-
-    expect(vm.mdSuggestion).toEqual('```suggestion:-0+0\n{text}\n```');
+    expect(findToolbarButtonByProp('buttonTitle', 'Insert suggestion').props('tag')).toEqual(
+      '```suggestion:-0+0\n{text}\n```',
+    );
   });
 
   it('does not render suggestion button if `canSuggest` is set to false', () => {
-    vm.canSuggest = false;
-
-    Vue.nextTick(() => {
-      expect(vm.$el.querySelector('.js-suggestion-btn')).toBe(null);
+    createWrapper({
+      canSuggest: false,
     });
+
+    expect(wrapper.find('.js-suggestion-btn').exists()).toBe(false);
   });
 });