Merge branch 'master' into issue_60515issue_60515

8 files changed, 125 insertions, 33 deletions

diff --git a/doc/development/README.md b/doc/development/README.md
index d2f09fc01de..b7322ab731f 100644
--- a/doc/development/README.md
+++ b/doc/development/README.md
@@ -35,7 +35,7 @@ description: 'Learn how to contribute to GitLab.'
 - [API styleguide](api_styleguide.md) Use this styleguide if you are
   contributing to the API.
 - [GraphQL API styleguide](api_graphql_styleguide.md) Use this
-  styleguide if you are contribution to the [GraphQL API](../api/graphql/index.md)
+  styleguide if you are contributing to the [GraphQL API](../api/graphql/index.md)
 - [Sidekiq guidelines](sidekiq_style_guide.md) for working with Sidekiq workers
 - [Working with Gitaly](gitaly.md)
 - [Manage feature flags](feature_flags.md)
diff --git a/doc/development/contributing/issue_workflow.md b/doc/development/contributing/issue_workflow.md
index e3a1dc711fd..0396f7ebc45 100644
--- a/doc/development/contributing/issue_workflow.md
+++ b/doc/development/contributing/issue_workflow.md
@@ -163,9 +163,9 @@ or ~"Stretch". Any open issue for a previous milestone should be labeled
 
 Priority labels help us define the time a ~bug fix should be completed. Priority determines how quickly the defect turnaround time must be.
 If there are multiple defects, the priority decides which defect has to be fixed immediately versus later.
-This label documents the planned timeline & urgency which is used to measure against our actual SLA on delivering ~bug fixes.
+This label documents the planned timeline & urgency which is used to measure against our target SLO on delivering ~bug fixes.
 
-| Label | Meaning         | Defect SLA (applies only to ~bug and ~security defects)                                                    |
+| Label | Meaning         | Target SLO (applies only to ~bug and ~security defects)                                                    |
 |-------|-----------------|----------------------------------------------------------------------------|
 | ~P1   | Urgent Priority | The current release + potentially immediate hotfix to GitLab.com (30 days) |
 | ~P2   | High Priority   | The next release (60 days)                                                 |
diff --git a/doc/development/fe_guide/graphql.md b/doc/development/fe_guide/graphql.md
index 8e06aa5d173..9fcd32fddfa 100644
--- a/doc/development/fe_guide/graphql.md
+++ b/doc/development/fe_guide/graphql.md
@@ -18,6 +18,12 @@ To save query compilation at runtime, webpack can directly import `.graphql`
 files. This allows webpack to preprocess the query at compile time instead
 of the client doing compilation of queries.
 
+To distinguish queries from mutations and fragments, the following naming convention is recommended:
+
+- `allUsers.query.graphql` for queries;
+- `addUser.mutation.graphql` for mutations;
+- `basicUser.fragment.graphql` for fragments.
+
 ## Usage in Vue
 
 To use Vue Apollo, import the [Vue Apollo][vue-apollo] plugin as well
diff --git a/doc/development/gitaly.md b/doc/development/gitaly.md
index 4ef20d5fd74..c8beb808a54 100644
--- a/doc/development/gitaly.md
+++ b/doc/development/gitaly.md
@@ -245,7 +245,7 @@ Here are the steps to gate a new feature in Gitaly behind a feature flag.
      // go implementation
    } else {
    	findAllTagsRequests.WithLabelValues("ruby").Inc()
-     // ruby impelmentation
+     // ruby implementation
    }
    ```
 
diff --git a/doc/development/import_export.md b/doc/development/import_export.md
index fd067b80c16..64c91f151c5 100644
--- a/doc/development/import_export.md
+++ b/doc/development/import_export.md
@@ -147,7 +147,6 @@ The `ModelConfigurationSpec` checks and confirms the addition of new models:
   If you think this model should be included in the export, please add it to `#{Gitlab::ImportExport.config_file}`.
 
   Definitely add it to `#{File.expand_path(ce_models_yml)}`
-  #{"or `#{File.expand_path(ee_models_yml)}` if the model/associations are EE-specific\n" if ee_models_hash.any?}
   to signal that you've handled this error and to prevent it from showing up in the future.
 MSG
 ```
@@ -253,7 +252,7 @@ Model relationships to be included in the project import/export:
 ```yaml
 project_tree:
   - labels:
-      :priorities
+    - :priorities
   - milestones:
     - events:
       - :push_event_payload
diff --git a/doc/development/testing_guide/best_practices.md b/doc/development/testing_guide/best_practices.md
index 82439c94c5a..71e3b7740cb 100644
--- a/doc/development/testing_guide/best_practices.md
+++ b/doc/development/testing_guide/best_practices.md
@@ -17,7 +17,7 @@ our test design. We can find some helpful heuristics documented in the Handbook
 
 ## Run tests against MySQL
 
-By default, tests are only run againts PostgreSQL, but you can run them on
+By default, tests are only run against PostgreSQL, but you can run them on
 demand against MySQL by following one of the following conventions:
 
 | Convention           | Valid example                |
diff --git a/doc/development/testing_guide/end_to_end/quick_start_guide.md b/doc/development/testing_guide/end_to_end/quick_start_guide.md
index afe76acf9c9..d33ef0fc229 100644
--- a/doc/development/testing_guide/end_to_end/quick_start_guide.md
+++ b/doc/development/testing_guide/end_to_end/quick_start_guide.md
@@ -220,7 +220,7 @@ As the pre-conditions for our test suite, the things that needs to happen before
 - The user logging in;
 - A premium license already being set;
 - A project being created with an issue and labels already set;
-- The issue page being opened with only one scoped label applied to the it.
+- The issue page being opened with only one scoped label applied to it.
 
 > When running end-to-end tests as part of the GitLab's continuous integration process [a license is already set as an environment variable](https://gitlab.com/gitlab-org/gitlab-ee/blob/1a60d926740db10e3b5724713285780a4f470531/qa/qa/ee/strategy.rb#L20). For running tests locally you can set up such license by following the document [what tests can be run?](https://gitlab.com/gitlab-org/gitlab-qa/blob/master/docs/what_tests_can_be_run.md#supported-remote-grid-environment-variables), based on the [supported GitLab environment variables](https://gitlab.com/gitlab-org/gitlab-qa/blob/master/docs/what_tests_can_be_run.md#supported-gitlab-environment-variables).
 
@@ -357,11 +357,13 @@ In the following we describe the changes needed in each of the resource files me
 
 Now, let's make it possible to create an issue resource through the API.
 
-First, in the [issue resource](https://gitlab.com/gitlab-org/gitlab-ee/blob/d3584e80b4236acdf393d815d604801573af72cc/qa/qa/resource/issue.rb), let's expose its labels attribute.
+First, in the [issue resource](https://gitlab.com/gitlab-org/gitlab-ee/blob/d3584e80b4236acdf393d815d604801573af72cc/qa/qa/resource/issue.rb), let's expose its id and labels attributes.
 
-Add the following `attribute :labels` right below the [`attribute :title`](https://gitlab.com/gitlab-org/gitlab-ee/blob/d3584e80b4236acdf393d815d604801573af72cc/qa/qa/resource/issue.rb#L15).
+Add the following `attribute :id` and `attribute :labels` right above the [`attribute :title`](https://gitlab.com/gitlab-org/gitlab-ee/blob/d3584e80b4236acdf393d815d604801573af72cc/qa/qa/resource/issue.rb#L15).
 
-> This line is needed to allow for labels to be automatically added to an issue when fabricating it via API.
+> This line is needed to allow for the issue fabrication, and for labels to be automatically added to the issue when fabricating it via API.
+
+> We add the attributes above the existing attribute to keep them alphabetically organized.
 
 Next, add the following code right below the [`fabricate!`](https://gitlab.com/gitlab-org/gitlab-ee/blob/d3584e80b4236acdf393d815d604801573af72cc/qa/qa/resource/issue.rb#L27) method.
 
@@ -376,8 +378,8 @@ end
 
 def api_post_body
   {
-    title: title,
-    labels: [labels]
+    labels: [labels],
+    title: title
   }
 end
 ```
@@ -392,7 +394,7 @@ By defining the `api_post_path` method, we allow the [`ApiFabricator`](https://g
 
 By defining the `api_post_body` method, we allow the [`ApiFabricator.api_post`](https://gitlab.com/gitlab-org/gitlab-ee/blob/a9177ca1812bac57e2b2fa4560e1d5dd8ffac38b/qa/qa/resource/api_fabricator.rb#L68) method to know which data to send when making the `POST` request.
 
-> Notice that we pass both `title` and `labels` attributes in the `api_post_body`, where `labels` receives an array of labels, and [`title` is required](https://docs.gitlab.com/ee/api/issues.html#new-issue).
+> Notice that we pass both `labels` and `title` attributes in the `api_post_body`, where `labels` receives an array of labels, and [`title` is required](https://docs.gitlab.com/ee/api/issues.html#new-issue). Also, notice that we keep them alphabetically organized.
 
 **Label resource**
 
@@ -417,8 +419,8 @@ end
 
 def api_post_body
   {
-    name: @title,
-    color: @color
+    color: @color,
+    name: @title
   }
 end
 ```
@@ -431,13 +433,13 @@ By defining the `api_post_path` method, we allow for the [`ApiFabricator `](http
 
 By defining the `api_post_body` method, we we allow for the [`ApiFabricator.api_post`](https://gitlab.com/gitlab-org/gitlab-ee/blob/a9177ca1812bac57e2b2fa4560e1d5dd8ffac38b/qa/qa/resource/api_fabricator.rb#L68) method to know which data to send when making the `POST` request.
 
-> Notice that we pass both `name` and `color` attributes in the `api_post_body` since [those are required](https://docs.gitlab.com/ee/api/labels.html#create-a-new-label).
+> Notice that we pass both `color` and `name` attributes in the `api_post_body` since [those are required](https://docs.gitlab.com/ee/api/labels.html#create-a-new-label). Also, notice that we keep them alphabetically organized.
 
 ### 8. Page Objects
 
 Page Objects are used in end-to-end tests for maintenance reasons, where a page's elements and methods are defined to be reused in any test.
 
-> Page Objects are auto-loaded in the `qa/qa.rb` file and available in all the test files (`*_spec.rb`).
+> Page Objects are auto-loaded in the [`qa/qa.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/qa/qa.rb) file and available in all the test files (`*_spec.rb`).
 
 Take a look at the [Page Objects] documentation.
 
@@ -487,25 +489,25 @@ Let's now update the Issue Page Object.
 
 The file we will have to change is the [Issue Page Object](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/qa/qa/page/project/issue/show.rb).
 
-First, add the following code right below the definition of an already implemented view:
+First, add the following code right below the definition of an already implemented view (keep in mind that view's definitions and their elements should be alphabetically ordered):
 
 ```ruby
-view 'app/views/shared/issuable/_sidebar.html.haml' do
-  element :labels_block
-  element :edit_link_labels
-  element :dropdown_menu_labels
-end
-
 view 'app/helpers/dropdowns_helper.rb' do
   element :dropdown_input_field
 end
+
+view 'app/views/shared/issuable/_sidebar.html.haml' do
+  element :dropdown_menu_labels
+  element :edit_link_labels
+  element :labels_block
+end
 ```
 
 Similarly to what we did before, let's first change the Page Object even without the elements being defined in the view (`_sidebar.html.haml`) and the `dropdowns_helper.rb` files, and later we will update them by adding the appropriate CSS selectors.
 
 Now, let's implement the methods `select_labels_and_refresh` and `text_of_labels_block`.
 
-Somewhere between the definition of the views and the private methods, add the following snippet of code:
+Somewhere between the definition of the views and the private methods, add the following snippet of code (these should also be alphabetically ordered for organization reasons):
 
 ```ruby
 def select_labels_and_refresh(labels)
@@ -530,6 +532,7 @@ end
 ##### Details of `select_labels_and_refresh`
 
 Notice that we have not only moved the `select_labels_and_refresh` method, but we have also changed its implementation to:
+
 1. Click the `:edit_link_labels` element previously defined, instead of using `find('.block.labels .edit-link').click`
 2. Use `within_element(:dropdown_menu_labels, text: label)`, and inside of it, we call `send_keys_to_element(:dropdown_input_field, [label, :enter])`, which is a method that we will implement in the `QA::Page::Base` class to replace `find('.dropdown-menu-labels .dropdown-input-field').send_keys [label, :enter]`
 3. Use `click_body` after iterating on each label, instead of using `find('#content-body').click`
@@ -545,19 +548,31 @@ Now let's change the view and the `dropdowns_helper` files to add the selectors
 
 In the [app/views/shared/issuable/_sidebar.html.haml](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/views/shared/issuable/_sidebar.html.haml) file, on [line 105 ](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L105), add an extra class `qa-edit-link-labels`.
 
-The code should look like this: `= link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link qa-edit-link-labels float-right'`.
+The code should look like this:
+
+```haml
+= link_to _('Edit'), '#', class: 'js-sidebar-dropdown-toggle edit-link float-right qa-edit-link-labels'
+```
 
 In the same file, on [line 121](https://gitlab.com/gitlab-org/gitlab-ee/blob/84043fa72ca7f83ae9cde48ad670e6d5d16501a3/app/views/shared/issuable/_sidebar.html.haml#L121), add an extra class `.qa-dropdown-menu-labels`.
 
-The code should look like this: `.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.qa-dropdown-menu-labels.dropdown-menu-selectable`.
+The code should look like this:
+
+```haml
+.dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable.qa-dropdown-menu-labels
+```
 
 In the [`dropdowns_helper.rb`](https://gitlab.com/gitlab-org/gitlab-ee/blob/master/app/helpers/dropdowns_helper.rb) file, on [line 94](https://gitlab.com/gitlab-org/gitlab-ee/blob/99e51a374f2c20bee0989cac802e4b5621f72714/app/helpers/dropdowns_helper.rb#L94), add an extra class `qa-dropdown-input-field`.
 
-The code should look like this: `filter_output = search_field_tag search_id, nil, class: "dropdown-input-field qa-dropdown-input-field", placeholder: placeholder, autocomplete: 'off'`.
+The code should look like this:
+
+```ruby
+filter_output = search_field_tag search_id, nil, class: "dropdown-input-field qa-dropdown-input-field", placeholder: placeholder, autocomplete: 'off'
+```
 
 > Classes starting with `qa-` are used for testing purposes only, and by defining such classes in the elements we add **testability** in the application.
 
-> When defining a class like `qa-labels-block`, it is transformed into `:labels_block` for usage in the Page Objects. So, `qa-edit-link-labels` is tranformed into `:edit_link_labels`, `qa-dropdown-menu-labels` is transformed into `:dropdown_menu_labels`, and `qa-dropdown-input-field` is transformed into `:dropdown_input_field`. Also, we use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective `qa-` selectors in the specified views.
+> When defining a class like `qa-labels-block`, it is transformed into `:labels_block` for usage in the Page Objects. So, `qa-edit-link-labels` is transformed into `:edit_link_labels`, `qa-dropdown-menu-labels` is transformed into `:dropdown_menu_labels`, and `qa-dropdown-input-field` is transformed into `:dropdown_input_field`. Also, we use a [sanity test](https://gitlab.com/gitlab-org/gitlab-ce/tree/master/qa/qa/page#how-did-we-solve-fragile-tests-problem) to check that defined elements have their respective `qa-` selectors in the specified views.
 
 > We did not define the `qa-labels-block` class in the `app/views/shared/issuable/_sidebar.html.haml` file because it was already there to be used.
 
@@ -565,7 +580,7 @@ The code should look like this: `filter_output = search_field_tag search_id, nil
 
 The last thing that we have to do is to update `QA::Page::Base` class to add the `send_keys_to_element` method on it.
 
-Add the following snippet of code somewhere where class methods are defined:
+Add the following snippet of code somewhere where class methods are defined (remember to organize methods alphabetically, and if you see a place where this standard is not being followed, it would be helpful if you could rearrange it):
 
 ```ruby
 def send_keys_to_element(name, keys)
diff --git a/doc/development/testing_guide/review_apps.md b/doc/development/testing_guide/review_apps.md
index b5fde92a23d..63b7b97c32f 100644
--- a/doc/development/testing_guide/review_apps.md
+++ b/doc/development/testing_guide/review_apps.md
@@ -160,6 +160,78 @@ secure note named **gitlab-{ce,ee} Review App's root password**.
    `review-qa-raise-e-12chm0-migrations.1-nqwtx`.
 1. Click on the `Container logs` link.
 
+### Troubleshoot a pending `dns-gitlab-review-app-external-dns` Deployment
+
+#### Finding the problem
+
+[In the past](https://gitlab.com/gitlab-org/gitlab-ce/issues/62834), it happened
+that the `dns-gitlab-review-app-external-dns` Deployment was in a pending state,
+effectively preventing all the Review Apps from getting a DNS record assigned,
+making them unreachable via domain name.
+
+This in turn prevented other components of the Review App to properly start
+(e.g. `gitlab-runner`).
+
+After some digging, we found that new mounts were failing, when being performed
+with transient scopes (e.g. pods) of `systemd-mount`:
+
+```
+MountVolume.SetUp failed for volume "dns-gitlab-review-app-external-dns-token-sj5jm" : mount failed: exit status 1
+Mounting command: systemd-run
+Mounting arguments: --description=Kubernetes transient mount for /var/lib/kubelet/pods/06add1c3-87b4-11e9-80a9-42010a800107/volumes/kubernetes.io~secret/dns-gitlab-review-app-external-dns-token-sj5jm --scope -- mount -t tmpfs tmpfs /var/lib/kubelet/pods/06add1c3-87b4-11e9-80a9-42010a800107/volumes/kubernetes.io~secret/dns-gitlab-review-app-external-dns-token-sj5jm
+Output: Failed to start transient scope unit: Connection timed out
+```
+
+This probably happened because the GitLab chart creates 67 resources, leading to
+a lot of mount points being created on the underlying GCP node.
+
+The [underlying issue seems to be a `systemd` bug](https://github.com/kubernetes/kubernetes/issues/57345#issuecomment-359068048)
+that was fixed in `systemd` `v237`. Unfortunately, our GCP nodes are currently
+using `v232`.
+
+For the record, the debugging steps to find out this issue were:
+
+1. Switch kubectl context to review-apps-ce (we recommend using [kubectx](https://kubectx.dev/))
+1. `kubectl get pods | grep dns`
+1. `kubectl describe pod <pod name>` & confirm exact error message
+1. Web search for exact error message, following rabbit hole to [a relevant kubernetes bug report](https://github.com/kubernetes/kubernetes/issues/57345)
+1. Access the node over SSH via the GCP console (**Computer Engine > VM
+  instances** then click the "SSH" button for the node where the `dns-gitlab-review-app-external-dns` pod runs)
+1. In the node: `systemctl --version` => systemd 232
+1. Gather some more information:
+   - `mount | grep kube | wc -l` => e.g. 290
+   - `systemctl list-units --all | grep -i var-lib-kube | wc -l` => e.g. 142
+1. Check how many pods are in a bad state:
+   - Get all pods running a given node: `kubectl get pods --field-selector=spec.nodeName=NODE_NAME`
+   - Get all the `Running` pods on a given node: `kubectl get pods --field-selector=spec.nodeName=NODE_NAME | grep Running`
+   - Get all the pods in a bad state on a given node: `kubectl get pods --field-selector=spec.nodeName=NODE_NAME | grep -v 'Running' | grep -v 'Completed'`
+
+#### Solving the problem
+
+To resolve the problem, we needed to (forcibly) drain some nodes:
+
+1. Try a normal drain on the node where the `dns-gitlab-review-app-external-dns`
+  pod runs so that Kubernetes automatically move it to another node: `kubectl drain NODE_NAME`
+1. If that doesn't work, you can also perform a forcible "drain" the node by removing all pods: `kubectl delete pods --field-selector=spec.nodeName=NODE_NAME`
+1. In the node:
+   - Perform `systemctl daemon-reload` to remove the dead/inactive units
+   - If that doesn't solve the problem, perform a hard reboot: `sudo systemctl reboot`
+1. Uncordon any cordoned nodes: `kubectl uncordon NODE_NAME`
+
+In parallel, since most Review Apps were in a broken state, we deleted them to
+clean up the list of non-`Running` pods.
+Following is a command to delete Review Apps based on their last deployment date
+(current date was June 6th at the time) with
+
+```
+helm ls -d | grep "Jun  4" | cut -f1 | xargs helm delete --purge
+```
+
+#### Mitigation steps taken to avoid this problem in the future
+
+We've created a new node pool with smaller machines so that it's less likely
+that a machine will hit the "too many mount points" problem in the future.
+
 ## Frequently Asked Questions
 
 **Isn't it too much to trigger CNG image builds on every test run? This creates
@@ -172,11 +244,11 @@ thousands of unused Docker images.**
 **How big are the Kubernetes clusters (`review-apps-ce` and `review-apps-ee`)?**
 
   > The clusters are currently set up with a single pool of preemptible nodes,
-  with a minimum of 1 node and a maximum of 50 nodes.
+  with a minimum of 1 node and a maximum of 500 nodes.
 
 **What are the machine running on the cluster?**
 
-  > We're currently using `n1-standard-16` (16 vCPUs, 60 GB memory) machines.
+  > We're currently using `n1-standard-1` (1 vCPU, 3.75 GB memory) machines.
 
 **How do we secure this from abuse? Apps are open to the world so we need to
 find a way to limit it to only us.**