Skip to content
Snippets Groups Projects

Add ability in backend to upgrade GitLab Runnner application

Closed Thong Kuah requested to merge upgrade-runner-chart into master
8 unresolved threads

What does this MR do?

Adds BE code to upgrade the GitLab runner application

Something in a later MR (periodic worker?, migration?) will then have to call Clusters::Applications::UpdateService.

What are the relevant issue numbers?

https://gitlab.com/gitlab-org/gitlab-ce/issues/49384

Does this MR meet the acceptance criteria?

Edited by 🤖 GitLab Bot 🤖

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Thong Kuah changed milestone to %11.8

    changed milestone to %11.8

  • Author Maintainer

    logs from upgrade-runner pod :

    Client: &version.Version{SemVer:"v2.11.0", GitCommit:"2e55dbe1fdb5fdb96b75ff144a339489417b146b", GitTreeState:"clean"}
    Error: cannot connect to Tiller
    + sleep 1s
    Retrying (30)...
    + echo 'Retrying (30)...'
    + helm repo add runner https://charts.gitlab.io
    "runner" has been added to your repositories
    + helm repo update
    Hang tight while we grab the latest from your chart repositories...
    ...Skip local chart repository
    ...Successfully got an update from the "stable" chart repository
    ...Successfully got an update from the "runner" chart repository
    Update Complete. ⎈ Happy Helming!⎈ 
    + helm upgrade runner runner/gitlab-runner --version 0.1.44 --tls --tls-ca-cert /data/helm/runner/config/ca.pem --tls-cert /data/helm/runner/config/cert.pem --tls-key /data/helm/runner/config/key.pem --reset-values --install --namespace gitlab-managed-apps -f /data/helm/runner/config/values.yaml
    Release "runner" has been upgraded. Happy Helming!
    E0117 02:55:24.411673       1 portforward.go:303] error copying from remote stream to local connection: readfrom tcp4 127.0.0.1:45371->127.0.0.1:45820: write tcp4 127.0.0.1:45371->127.0.0.1:45820: write: broken pipe
    LAST DEPLOYED: Thu Jan 17 02:55:24 2019
    NAMESPACE: gitlab-managed-apps
    STATUS: DEPLOYED
    
    RESOURCES:
    ==> v1/Secret
    NAME                  AGE
    runner-gitlab-runner  25m
    
    ==> v1/ConfigMap
    runner-gitlab-runner  25m
    
    ==> v1beta1/Deployment
    runner-gitlab-runner  25m
    
    
    NOTES:
    
    Your GitLab Runner should now be registered against the GitLab instance reachable at: "https://24046.qa-tunnel.gitlab.info/"
    
  • Author Maintainer

    However, I am not getting a new gitlab-runner pod after upgrade :(

    tkgl:gitlab tkuah$ kc get deployment -n gitlab-managed-apps
    NAME                   DESIRED   CURRENT   UP-TO-DATE   AVAILABLE   AGE
    runner-gitlab-runner   1         0         0            0           26m
    tiller-deploy          1         1         1            1           33m

    Error below:

    $ kubectl get deployment runner-gitlab-runner -n gitlab-managed-apps -o yaml
    # snip
    status:
      conditions:
      - lastTransitionTime: 2019-01-17T02:30:01Z
        lastUpdateTime: 2019-01-17T02:30:01Z
        message: Deployment has minimum availability.
        reason: MinimumReplicasAvailable
        status: "True"
        type: Available
      - lastTransitionTime: 2019-01-17T02:41:04Z
        lastUpdateTime: 2019-01-17T02:41:04Z
        message: 'pods "runner-gitlab-runner-676ddb4c7-" is forbidden: error looking up
          service account gitlab-managed-apps/runner-gitlab-runner: serviceaccount "runner-gitlab-runner"
          not found'
        reason: FailedCreate
        status: "True"
        type: ReplicaFailure
      - lastTransitionTime: 2019-01-17T02:51:05Z
        lastUpdateTime: 2019-01-17T02:51:05Z
        message: ReplicaSet "runner-gitlab-runner-676ddb4c7" has timed out progressing.
        reason: ProgressDeadlineExceeded
        status: "False"
        type: Progressing
      observedGeneration: 2
      unavailableReplicas: 1
    Edited by Thong Kuah
  • Author Maintainer

    Spot the difference ! Looks like Gitlab::Kubernetes::Helm::UpgradeCommand was missing rbac.create flags

    [35] pry(main)> puts runner.upgrade_command.generate_script
    set -xeo pipefail
    helm init --upgrade
    for i in $(seq 1 30); do helm version && break; sleep 1s; echo "Retrying ($i)..."; done
    helm repo add runner https://charts.gitlab.io
    helm repo update
    helm upgrade runner runner/gitlab-runner --version 0.1.44 --tls --tls-ca-cert /data/helm/runner/config/ca.pem --tls-cert /data/helm/runner/config/cert.pem --tls-key /data/helm/runner/config/key.pem --reset-values --install --namespace gitlab-managed-apps -f /data/helm/runner/config/values.yaml
    => nil
    [36] pry(main)> puts runner.install_command.generate_script
    set -xeo pipefail
    helm init --upgrade
    for i in $(seq 1 30); do helm version && break; sleep 1s; echo "Retrying ($i)..."; done
    helm repo add runner https://charts.gitlab.io
    helm repo update
    helm install runner/gitlab-runner --name runner --tls --tls-ca-cert /data/helm/runner/config/ca.pem --tls-cert /data/helm/runner/config/cert.pem --tls-key /data/helm/runner/config/key.pem --version 0.1.44 --set rbac.create\=true,rbac.enabled\=true --namespace gitlab-managed-apps -f /data/helm/runner/config/values.yaml
  • Thong Kuah added 1 commit

    added 1 commit

    • 1d1e466f - Add missing rbac.create flags

    Compare with previous version

  • Thong Kuah added 3 commits

    added 3 commits

    • b84c7422 - Convert flags to use shelljoin
    • dae2f2f3 - Teach runner app about #upgrade_command
    • 51a6f575 - Add missing rbac.create flags

    Compare with previous version

  • Thong Kuah added 1 commit

    added 1 commit

    Compare with previous version

43 " --install" \
44 " --namespace #{::Gitlab::Kubernetes::Helm::NAMESPACE}" \
45 " -f /data/helm/#{name}/config/values.yaml"
41 def repository_update_command
42 'helm repo update' if repository
43 end
44
45 def upgrade_command
46 command = ["helm", "upgrade", name, chart] + helm_upgrade_flags
47
48 command.shelljoin
49 end
46 50
47 "helm upgrade #{name} #{chart}#{upgrade_flags}"
51 def helm_upgrade_flags
52 reset_values_flag = ['--reset-values']
  • 55 return unless files.key?(:'ca.pem')
    81 return [] unless files.key?(:'ca.pem')
    56 82
    57 " --tls" \
    58 " --tls-ca-cert #{files_dir}/ca.pem" \
    59 " --tls-cert #{files_dir}/cert.pem" \
    60 " --tls-key #{files_dir}/key.pem"
    83 [
    84 '--tls',
    85 '--tls-ca-cert', "#{files_dir}/ca.pem",
    86 '--tls-cert', "#{files_dir}/cert.pem",
    87 '--tls-key', "#{files_dir}/key.pem"
    88 ]
    61 89 end
    62 90 end
    63 91 end
    • I think we have quite a bit of duplication across different commmands. Did you notice any good opportunities for refactoring this now that we're adding many more lines of similar code? Perhaps there aren't enough similarities?

    • Author Maintainer

      Did you notice any good opportunities for refactoring this now that we're adding many more lines of similar code?

      Not yet, will have a look

    • Please register or sign in to reply
  • Thong Kuah added 3 commits

    added 3 commits

    • c47432d8 - Teach runner app about #upgrade_command
    • 7f933bfb - Add missing rbac.create flags
    • 8c18bc69 - Reduce long lines in spec

    Compare with previous version

  • Thong Kuah mentioned in merge request !23924 (merged)

    mentioned in merge request !23924 (merged)

  • Thong Kuah changed title from Teach runner app about #upgrade_command to Add ability in backend to upgrade GitLab Runnner application

    changed title from Teach runner app about #upgrade_command to Add ability in backend to upgrade GitLab Runnner application

  • Thong Kuah changed the description

    changed the description

  • Thong Kuah added 1 commit

    added 1 commit

    • a3ed6712 - Add service to upgrade cluster applications

    Compare with previous version

  • Thong Kuah marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • Thong Kuah added 160 commits

    added 160 commits

    Compare with previous version

  • Thong Kuah added 1 commit

    added 1 commit

    • 2245e633 - Create kubernetes_namespace for factory

    Compare with previous version

  • Thong Kuah added 1 commit

    added 1 commit

    • 2ef6efcc - Port upgrade check worker and service from EE

    Compare with previous version

  • 3 Warnings
    This merge request is quite big (more than 768 lines changed), please consider splitting it into multiple merge requests.
    This merge request is missing the database label.
    This merge request adds files that do not enforce frozen string literal. See https://gitlab.com/gitlab-org/gitlab-ce/issues/47424 for more information.
    1 Message
    📖 This merge request adds or changes files that require a review from the Database team.

    Database Review

    The following files require a review from the Database team:

    • db/migrate/20190123004235_add_last_update_started_at_to_applications_prometheus_ce.rb
    • db/schema.rb

    To make sure these changes are reviewed, take the following steps:

    1. Edit your merge request, and add gl-database to the list of Group approvers.
    2. Mention @gl-database in a separate comment, and explain what needs to be reviewed by the team. Please don't mention the team until your changes are ready for review.

    Enable Frozen String Literal

    The following files should have # frozen_string_literal: true on the first line:

    • spec/services/clusters/applications/schedule_update_service_spec.rb
    • spec/workers/cluster_update_app_worker_spec.rb

    Generated by 🚫 Danger

    Edited by 🤖 GitLab Bot 🤖
  • Thong Kuah added 2 commits

    added 2 commits

    • 88282ac2 - Port upgrade check worker and service from EE
    • b2ddf376 - Add service to upgrade cluster applications

    Compare with previous version

  • Thong Kuah
    Thong Kuah @tkuah started a thread on the diff
  • 3 module Clusters
    4 module Applications
    5 class CheckUpgradeProgressService < BaseHelmService
    6 def execute
    7 return unless app.updating?
    8
    9 case phase
    10 when ::Gitlab::Kubernetes::Pod::SUCCEEDED
    11 on_success
    12 when ::Gitlab::Kubernetes::Pod::FAILED
    13 on_failed
    14 else
    15 check_timeout
    16 end
    17 rescue ::Kubeclient::HttpError => e
    18 app.make_update_errored!("Kubernetes error: #{e.message}") unless app.update_errored?
  • Thong Kuah
    Thong Kuah @tkuah started a thread on the diff
  • 15 check_timeout
    16 end
    17 rescue ::Kubeclient::HttpError => e
    18 app.make_update_errored!("Kubernetes error: #{e.message}") unless app.update_errored?
    19 end
    20
    21 private
    22
    23 def on_success
    24 app.make_updated!
    25 ensure
    26 remove_pod
    27 end
    28
    29 def on_failed
    30 app.make_update_errored!(errors || 'Update silently failed')
  • Thong Kuah
    Thong Kuah @tkuah started a thread on the diff
  • 1 # frozen_string_literal: true
    2
    3 module Clusters
    4 module Applications
    5 class CheckUpgradeProgressService < BaseHelmService
  • Thong Kuah added 2 commits

    added 2 commits

    • 7a6d0df9 - Port upgrade check worker and service from EE
    • cd743ab9 - Add service to upgrade cluster applications

    Compare with previous version

  • Thong Kuah
    Thong Kuah @tkuah started a thread on the diff
  • 1 # frozen_string_literal: true
    2
    3 module Clusters
    4 module Applications
    5 class UpdateService < BaseHelmService
    6 def execute
    7 return if app.updating?
    8
    9 begin
    10 app.make_updating!
    11 helm_api.update(upgrade_command)
  • Thong Kuah
    Thong Kuah @tkuah started a thread on the diff
  • 1 # frozen_string_literal: true
    2
    3 module Clusters
    4 module Applications
    5 class UpdateService < BaseHelmService
    6 def execute
    7 return if app.updating?
  • Thong Kuah added 2 commits

    added 2 commits

    • ad576faa - Port upgrade check worker and service from EE
    • f918dacf - Add service to upgrade cluster applications

    Compare with previous version

  • Thong Kuah added 1 commit

    added 1 commit

    Compare with previous version

  • Thong Kuah added 97 commits

    added 97 commits

    Compare with previous version

  • Thong Kuah changed the description

    changed the description

  • Author Maintainer
  • closed

  • 🤖 GitLab Bot 🤖 changed the description

    changed the description

  • Please register or sign in to reply
    Loading