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

Document Gitlab upgrade instructions (#3608) #3627

Merged
merged 5 commits into from
Apr 28, 2022

Conversation

amarjandu
Copy link
Contributor

@amarjandu amarjandu commented Nov 9, 2021

#3608

Author

  • PR title references issue
  • PR title matches issue title (preceded by Fix: for bugs) or there is a good reason why they're different
  • Title of main commit references issue
  • PR is connected to Zenhub issue and description links to issue

Author (reindex)

  • Added r tag to commit title or this PR does not require reindexing
  • Added reindex label to PR or this PR does not require reindexing

Author (freebies & chains)

  • Freebies are blocked on this PR or there are no freebies in this PR
  • Freebies are referenced in commit titles or there are no freebies in this PR
  • This PR is blocked by previous PR in the chain or this PR is not chained to another PR
  • Added chain label to the blocking PR or this PR is not chained to another PR

Author (upgrading)

  • Documented upgrading of deployments in UPGRADING.rst or this PR does not require upgrading
  • Added u tag to commit title or this PR does not require upgrading
  • Added upgrade label to PR or this PR does not require upgrading
  • Added announcement to PR description or this PR does not require announcement
  • Added checklist items for additional operator tasks or this PR does not require additional tasks

Author (requirements, before every review)

  • Ran make requirements_update or this PR does not touch requirements*.txt, common.mk, Makefile and Dockerfile
  • Added R tag to commit title or this PR does not touch requirements*.txt
  • Added reqs label to PR or this PR does not touch requirements*.txt

Author (before every review)

  • make integration_test passes in personal deployment or this PR does not touch functionality that could break the IT
  • Rebased branch on develop, squashed old fixups

Primary reviewer (after approval)

  • Commented in issue about demo expectations or labelled issue as no demo
  • Decided if PR can be labeled no sandbox
  • PR title is appropriate as title of merge commit
  • Moved ticket to Approved column
  • Assigned PR to an operator

Operator (before pushing merge the commit)

  • Checked reindex label and r commit title tag
  • Checked that demo expectations are clear or issue is labeled as no demo
  • Rebased and squashed branch
  • Sanity-checked history
  • Pushed PR branch to Github
  • Branch pushed to Gitlab and added sandbox label or PR is labeled no sandbox
  • Build passed in sandbox or PR is labeled no sandbox
  • Started reindex in sandbox or this PR does not require reindexing sandbox
  • Checked for failures in sandbox or this PR does not require reindexing sandbox
  • Added PR reference to merge commit title
  • Collected commit title tags in merge commit title
  • Moved linked issue to Merged column
  • Pushed merge commit to Github

Operator (after pushing the merge commit)

  • Made announcement requested by author or PR description does not contain an announcement
  • Moved freebies to Merged column or there are no freebies in this PR
  • Shortened the PR chain or this PR is not the base of another PR
  • Verified that N reviews labelling is accurate or this PR is authored by lead
  • Pushed merge commit to Gitlab or merge commit can be pushed later, with another PR
  • Deleted PR branch from Github and Gitlab
  • Build passes on Gitlab

Operator (reindex)

  • Started reindex in target deployment or this PR does not require reindexing
  • Checked for and triaged indexing failures or this PR does not require reindexing
  • Emptied fail queues in target deployment or this PR does not require reindexing

Operator

  • Ask Mel to change the Slack alert to point to this section of the Operator's Manual
  • Unassigned PR

@github-actions github-actions bot added the orange [process] Done by the Azul team label Nov 9, 2021
@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch from b1e97e3 to a818400 Compare November 9, 2021 19:04
@codecov
Copy link

codecov bot commented Nov 9, 2021

Codecov Report

Merging #3627 (6bd168b) into develop (0cd7255) will decrease coverage by 0.00%.
The diff coverage is 66.66%.

❗ Current head 6bd168b differs from pull request most recent head 54851a5. Consider uploading reports for the commit 54851a5 to get more accurate results

@@             Coverage Diff             @@
##           develop    #3627      +/-   ##
===========================================
- Coverage    82.67%   82.67%   -0.01%     
===========================================
  Files          120      120              
  Lines        15036    15039       +3     
===========================================
+ Hits         12431    12433       +2     
- Misses        2605     2606       +1     
Impacted Files Coverage Δ
src/azul/deployment.py 70.95% <66.66%> (-0.07%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cd7255...54851a5. Read the comment docs.

@coveralls
Copy link

coveralls commented Nov 9, 2021

Coverage Status

Coverage decreased (-0.003%) to 82.871% when pulling 54851a5 on issues/amar/3608-gl-upgrade-docs into 0cd7255 on develop.

@amarjandu amarjandu changed the title Document Gitlab upgrade instructions Document Gitlab upgrade instructions (#3608) Nov 9, 2021
@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch 2 times, most recently from 4a9c4c9 to a01eef6 Compare November 10, 2021 00:16
Copy link
Contributor

@dsotirho-ucsc dsotirho-ucsc left a comment

Choose a reason for hiding this comment

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

Approved.

OPERATOR.rst Outdated
a snapshot of its EBS volume. Edit ``terraform/gitlab/gitlab.tf.json.template.py``,
updating the versions of the docker images for ``gitlab-ce`` and
``gitlab-runner``. Then run::
Operators must check for updates to the `GitLab Docker image`_ and the
Copy link
Member

Choose a reason for hiding this comment

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

A quicker way to check for this https://gitlab.dev.singlecell.gi.ucsc.edu/help

Copy link
Contributor Author

@amarjandu amarjandu Dec 1, 2021

Choose a reason for hiding this comment

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

Wanted to reference the docker image release page that way it's explicit where to find the new image/what the new image is. IIRC the /help endpoint just states that there is an update, but does not offer more insight.

Copy link
Member

Choose a reason for hiding this comment

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

Nevertheless, please incorporate this in as a way of short-circuiting. There is no point in comparing the docker page against the TF source if the help page indicates that Gitlab is up-to-date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So to confirm, you want the operators to check that the GitLab Instance can be updated using the /help endpoint, and inspect if there are updates to the GitLab Runners by comparing the TF file to the docker release page?

Copy link
Member

Choose a reason for hiding this comment

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

I want them to check daily (since it's so easy) for updates using /help and if that indicates that updates are available, install the latest version of Gitlab and the latest compatible version of the runner.

@hannes-ucsc hannes-ucsc added the 0 reviews [process] Lead didn't request any changes label Nov 30, 2021
@hannes-ucsc hannes-ucsc removed their assignment Nov 30, 2021
@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch from a01eef6 to 8fc709e Compare November 30, 2021 19:23
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

@hannes-ucsc hannes-ucsc removed their assignment Dec 1, 2021
@hannes-ucsc hannes-ucsc added 1 review [process] Lead requested changes once and removed 0 reviews [process] Lead didn't request any changes labels Dec 1, 2021
@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch from 8fc709e to cc86a03 Compare December 2, 2021 18:47
OPERATOR.rst Outdated
a snapshot of its EBS volume. Edit ``terraform/gitlab/gitlab.tf.json.template.py``,
updating the versions of the docker images for ``gitlab-ce`` and
``gitlab-runner``. Then run::
Operators must check for updates to the GitLab instances and the GitLab Runners
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Operators must check for updates to the GitLab instances and the GitLab Runners
Operators must check for updates to GitLab

OPERATOR.rst Outdated
updating the versions of the docker images for ``gitlab-ce`` and
``gitlab-runner``. Then run::
Operators must check for updates to the GitLab instances and the GitLab Runners
on a daily basis. Updates to the Gitlab instance can be quickly checked at the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
on a daily basis. Updates to the Gitlab instance can be quickly checked at the
on a daily basis. Updates to Gitlab can be quickly checked at the

Using the word "instance" here is confusing. Primarily, we're looking for updates to the webapp and runner images, not the instance. We have to update the instance if either them are updated.

OPERATOR.rst Outdated
Operators must check for updates to the GitLab instances and the GitLab Runners
on a daily basis. Updates to the Gitlab instance can be quickly checked at the
``/help`` endpoint for `GitLab dev`_ and `GitLab prod`_. If there are updates
available, find the new release for the `GitLab Docker Image`_ and update the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
available, find the new release for the `GitLab Docker Image`_ and update the
available, find the new release for the `GitLab Docker image`_ and update the

Google "proper noun"

OPERATOR.rst Outdated
on a daily basis. Updates to the Gitlab instance can be quickly checked at the
``/help`` endpoint for `GitLab dev`_ and `GitLab prod`_. If there are updates
available, find the new release for the `GitLab Docker Image`_ and update the
`Gitlab terraform`_ file. When updating the GitLab instance check if there are
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
`Gitlab terraform`_ file. When updating the GitLab instance check if there are
`Gitlab Terraform`_ file. When updating the GitLab instance check if there are

OPERATOR.rst Outdated
``/help`` endpoint for `GitLab dev`_ and `GitLab prod`_. If there are updates
available, find the new release for the `GitLab Docker Image`_ and update the
`Gitlab terraform`_ file. When updating the GitLab instance check if there are
applicable updates to the `GitLab Runner image`_. Before starting the update
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
applicable updates to the `GitLab Runner image`_. Before starting the update
applicable updates to the `GitLab runner image`_. Before starting the update

OPERATOR.rst Outdated
available, find the new release for the `GitLab Docker Image`_ and update the
`Gitlab terraform`_ file. When updating the GitLab instance check if there are
applicable updates to the `GitLab Runner image`_. Before starting the update
process check applicable `GitLab release notes`_ for any upgrading instructions.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
process check applicable `GitLab release notes`_ for any upgrading instructions.
process, check the applicable `GitLab release notes`_ for any upgrading instructions.

OPERATOR.rst Outdated
`Gitlab terraform`_ file. When updating the GitLab instance check if there are
applicable updates to the `GitLab Runner image`_. Before starting the update
process check applicable `GitLab release notes`_ for any upgrading instructions.
When upgrading across multiple Gitlab versions follow a supported GitLab
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
When upgrading across multiple Gitlab versions follow a supported GitLab
When upgrading across multiple Gitlab versions, follow the prescribed GitLab

OPERATOR.rst Outdated
Comment on lines 169 to 175
Before any changes are applied, have the lead check for any running
``background migrations`` on the GitLab instances, if there are no migrations
in progress, stop the instance (do not terminate) and create a snapshot of its
EBS volume.
Copy link
Member

Choose a reason for hiding this comment

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

Confusing. This sound like the only the snapshotting is conditional upon the existence of background migrations. In fact, the entire upgrade is. Structure this so that checking the conditions are the first steps in the a enumerated list and describe each branch in the decision tree. Think of this as a flow chart described in plain English.

@hannes-ucsc hannes-ucsc added 2 reviews [process] Lead requested changes twice and removed 1 review [process] Lead requested changes once labels Dec 8, 2021
@hannes-ucsc hannes-ucsc removed their assignment Dec 9, 2021
@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch from cc86a03 to 0ec5c0a Compare December 14, 2021 02:02
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

Some of my comments are thumbed-up while there is no actual change addressing them. This keeps happening on your PRs. Why?

image

@hannes-ucsc hannes-ucsc removed their assignment Apr 15, 2022
@amarjandu
Copy link
Contributor Author

I can locate the commits that address the comments, there must have been an issue with how the commits were squashed down.

Screen Shot 2022-04-18 at 9 50 39 AM

@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch 3 times, most recently from 5ff62d8 to c76606c Compare April 20, 2022 17:41
Copy link
Member

@hannes-ucsc hannes-ucsc left a comment

Choose a reason for hiding this comment

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

I can locate the commits that address the comments, there must have been an issue with how the commits were squashed down.

Since the screenshot shows a reflog, it must have been you who squashed them down, right? Have you tried to figure out what that issue is you are alluding to? Since you still have the reflog, you can always go back to the starting point and try to redo what you did.

I am aware that this might come across as pedantic, but I think it's worth getting to the bottom of this, as it is not the first time we've had this problem, and I would like to prevent it from happening again. I might not always notice this problem.

@hannes-ucsc hannes-ucsc removed their assignment Apr 21, 2022
@amarjandu
Copy link
Contributor Author

It looks like the commit that carried the changes 606f9491 with commit message f template was dropped (d) rather than being fixup! (f). The image above shows that the commit was found at HEAD@{98}.

During the next rebase I attempted to move 606f9491 to be below 01803794 then change the command to f to squash the changes to the gitlab.md template into the fixup! commit (knowing that all fixups would be squashed their targets).

pick e5a8a3df Add FIXME (#3890)
pick 74d44035 Fix various typos in OPERATOR.rst
pick 864429b9 Add `scripts/create_gitlab_snapshot.py`
pick babc4723 Document GitLab update instructions (#3608)
pick 01803794 fixup! Document GitLab update instructions (#3608)
pick 4d766cb6 Add FIXME (#3942)
pick 606f9491 f template

This fixup! was met with conflicts and was aborted.

(.venv) CBSEspecter:azul amar$ git rebase -i HEAD~7
Auto-merging OPERATOR.rst
CONFLICT (content): Merge conflict in OPERATOR.rst
error: could not apply 606f9491... f template
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 606f9491... f template
(.venv) CBSEspecter:azul amar$

At this point the reflog will match what was at HEAD@{98} in the image posted above.

606f9491 (HEAD) HEAD@{0}: rebase -i (abort): updating HEAD
01803794 HEAD@{1}: rebase -i (start): checkout HEAD~7

The interactive rebase was started again, but rather than f, a d was mistakenly entered as the command, which caused the commit to be dropped.

This places the history at HEAD~{96} in the image above.

4d766cb6 (HEAD) HEAD@{0}: rebase -i (start): checkout HEAD~7
606f9491 HEAD@{1}: rebase -i (abort): updating HEAD
01803794 HEAD@{2}: rebase -i (start): checkout HEAD~7
606f9491 HEAD@{3}: reset: moving to 606f9491

Had the commit 606f9491 been squashed in correctly the reflog would show a different hash for the commits after the squash was complete.
Screen Shot 2022-04-26 at 3 54 28 PM

If you have other tools that can use my working copies reflog to provide more insights to exactly what happened I'd like to take a look, I tried using smartgit's log viewer but it did not give any insight to older commits in the reflog, from posts on the web this might have been a feature that it had at some point. I'm not sure if there there is a way that I can pull up the old rebase TODO commands that were saved when the interactive rebase was done, I think with those old files we can confirm if my findings are accurate.

@amarjandu amarjandu force-pushed the issues/amar/3608-gl-upgrade-docs branch from c76606c to 1de4e06 Compare April 27, 2022 01:16
- [ ] Target branch is `develop`
- [ ] Source branch matches `gitlab/yyyy-mm-dd`
- [ ] PR title is `Update GitLab to X.Y.Z (#4014)`
- [ ] Disconnected any other PRs currently connected to issue #4014 via ZenHub
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- [ ] Disconnected any other PRs currently connected to issue #4014 via ZenHub
- [ ] Disconnected any other PRs currently connected to #4014 via ZenHub


### Operator (after pushing the merge commit)

- [ ] Shortened the PR chain <sub>or this PR is not labeled `chain`</sub>
Copy link
Member

Choose a reason for hiding this comment

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

@achave11 FYI another mention of this label to rename to base

@achave11-ucsc achave11-ucsc force-pushed the issues/amar/3608-gl-upgrade-docs branch from 6bd168b to 54851a5 Compare April 28, 2022 00:54
@achave11-ucsc achave11-ucsc added the sandbox [process] Resolution is being verified in sandbox deployment label Apr 28, 2022
@achave11-ucsc achave11-ucsc merged commit acc7a0a into develop Apr 28, 2022
@achave11-ucsc achave11-ucsc deleted the issues/amar/3608-gl-upgrade-docs branch April 28, 2022 02:12
@achave11-ucsc achave11-ucsc removed their assignment Apr 28, 2022
@hannes-ucsc hannes-ucsc added the partial [process] PR is does not completely resolve associated ticket label Sep 12, 2022
@hannes-ucsc
Copy link
Member

#3942 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4+ reviews [process] Lead requested changes four times or more orange [process] Done by the Azul team partial [process] PR is does not completely resolve associated ticket sandbox [process] Resolution is being verified in sandbox deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants