Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

release: never use build number in image family#63157

Merged
burmudar merged 1 commit into
mainfrom
wb/release+executors/always-use-build-num
Jun 7, 2024
Merged

release: never use build number in image family#63157
burmudar merged 1 commit into
mainfrom
wb/release+executors/always-use-build-num

Conversation

@burmudar

@burmudar burmudar commented Jun 7, 2024

Copy link
Copy Markdown
Contributor

the executor image and docker mirror image should now follow the following naming convention:

Image family: sourcegraph-executors-[nightly|internal|'']-<MAJOR>-<MINOR>
Image name: sourcegraph-executor-[nightly|internal|'']-<MAJOR>-<MINOR>-<BUILD_NUMBER>

example:
Image family: sourcegraph-executors-5-4
Image name: sourcegraph-executor-5-4-277666

What happens during releases and not releases?

Nightly

nightly suffix
Image family: sourcegraph-executors-nightly-<MAJOR>-<MINOR>
Image name: sourcegraph-executor-nightly-<MAJOR>-<MINOR>-<BUILD_NUMBER>

Internal

internal suffix
Image family: sourcegraph-executors-internal-<MAJOR>-<MINOR>
Image name: sourcegraph-executor-internal-<MAJOR>-<MINOR>-<BUILD_NUMBER>

Public / Promote to public

** No suffix **

Image family: sourcegraph-executors-<MAJOR>-<MINOR>
Image name: sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>

Important

Should we keep the imagine name stable at sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>
and only change the family name?

Why?

The Image family dictates the collection of images and that changes each major minor and or release phase so there is really no use in changing the image name too, except at a glance you can see from the name what image family it belongs to?

Test plan

Changelog

the executor image and docker mirror image should now follow the
following naming convention:

Image family: sourcegraph-executors-<MAJOR>-<MINOR>
Image name: sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>

example:
Image family: sourcegraph-executors-5-4
Image name: sourcegraph-executor-5-4-277666
@burmudar burmudar requested review from a team, eseliger and jhchabran June 7, 2024 14:49
@cla-bot cla-bot Bot added the cla-signed label Jun 7, 2024
@burmudar burmudar requested a review from BolajiOlajide June 7, 2024 14:50

@eseliger eseliger left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the PR description makes sense to me

@craigfurman craigfurman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

1 question, because I'm not 100% sure this works 🤔

I think we'll need to backport this too once we merge it, but I'm not 100% sure! I'd imagine ami.push.sh is run from the release branch, hence why I'd guess yes.

panic("cannot parse version")
}
imageFamily = fmt.Sprintf("sourcegraph-executors-internal-%d-%d-%d", ver.Major(), ver.Minor(), ver.Patch())
imageFamily = fmt.Sprintf("sourcegraph-executors-internal-%d-%d", ver.Major(), ver.Minor())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we ever actually want these internal names?

Suggested change
imageFamily = fmt.Sprintf("sourcegraph-executors-internal-%d-%d", ver.Major(), ver.Minor())
imageFamily = fmt.Sprintf("sourcegraph-executors-%d-%d", ver.Major(), ver.Minor())

And similar for the docker-mirror. If I understood correctly.

@craigfurman craigfurman Jun 7, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I did read the PR description btw, I'm just not getting why 😅
Does anything actually pull the internal varieties?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes we do - without internal in the image family name an executor meant for internal releases only will be published with the image family name sourcegraph-executors which means the terraform in terraform-aws and terraform-google who look for latest images in the image family sourcegraph-executors-<MAJOR>-<MINOR> will pick the image up after every MAJOR.MINOR bump.

So ultimately, internal releases are kept to the sourcegraph-executors-internal and once you promote to public the image is rebuilt and put into sourcegraph-executors which means the released terraform will now pick it up.

This is also has a nice quality which I think the release team can investigate. This technically allows us to test internal releases _with the terraform. If you want to opt into an internal release you just have to update the image family in the terraform - almost like alpha, beta changes.

Sorry if I am over explaining it! Not my intention at all.

The major caveat not mentioned here are at all, is that you kinda just have to know is that you'll only see the sourcegraph-executor-MAJOR-MINOR release once the tagged release build finishes which flies completely under the radar - which I think we need to fix somehow.

One idea I just got on how this can be done is part of the promote to public release steps in the release manifest is to run:

# promote to public 
gcloud compute images create --source-image=sourcegraph-executors-internal-MAJOR-MINOR-BUILD sourcegraph-executors-MAJOR-MINOR-4-BUILD --family sourcegraph-executors-MAJOR-MINOR

This stays more true to the spirit of promotion, in that a new image is not built by CI, instead the image used for internal releases just gets copied to a new name.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

💡 right, I think I get it! No worries about over-explaining, it's good.

I think this fixes a bug in which there is executor image pull downtime between promotion and the tag pipeline running. We'd clobbered the executors image and stripped its family (for some reason). Since this was the only VM in its family, family queries would return nothing. By "forking" the image names every build, we fix this.

As you say, the new image won't actually be available for a little while after the release looks done, until the tag pipeline runs. That is something we should probably fix at some point. Maybe by moving most of the promote/finalize scripting into the tag pipeline, so that release artifacts are all published by one pipeline (the tag one).

That can be future work of course, this is an improvement.

Am I right about this needing a backport btw?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Awesome 👏🏼

Yup - this should be backported ♻️

@craigfurman craigfurman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🚀

@burmudar burmudar merged commit 8bb0ab5 into main Jun 7, 2024
@burmudar burmudar deleted the wb/release+executors/always-use-build-num branch June 7, 2024 15:23
@craigfurman craigfurman added the backport 5.4.5099 label used to backport PRs to the 5.4.5099 release branch label Jun 9, 2024
sourcegraph-release-bot pushed a commit that referenced this pull request Jun 9, 2024
the executor image and docker mirror image should now follow the
following naming convention:

Image family:
`sourcegraph-executors-[nightly|internal|'']-<MAJOR>-<MINOR>`
Image name:
`sourcegraph-executor-[nightly|internal|'']-<MAJOR>-<MINOR>-<BUILD_NUMBER>`

example:
Image family: `sourcegraph-executors-5-4`
Image name: `sourcegraph-executor-5-4-277666`

## What happens during releases and _not_ releases?
#### Nightly
**`nightly` suffix**
Image family: `sourcegraph-executors-nightly-<MAJOR>-<MINOR>`
Image name:
`sourcegraph-executor-nightly-<MAJOR>-<MINOR>-<BUILD_NUMBER>`
#### Internal
**`internal` suffix**
Image family: `sourcegraph-executors-internal-<MAJOR>-<MINOR>`
Image name:
`sourcegraph-executor-internal-<MAJOR>-<MINOR>-<BUILD_NUMBER>`
#### Public / Promote to public

** No suffix **

Image family: `sourcegraph-executors-<MAJOR>-<MINOR>`
Image name: `sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>`

>  [!IMPORTANT]
> Should we keep the imagine name stable at
`sourcegraph-executor-<MAJOR>-<MINOR>-<BUILD_NUMBER>`
> and only change the family name?
>
> **Why?**
>
> The Image family dictates the collection of images and that changes
each major minor and or release phase so there is really no use in
changing the image name too, except at a glance you can see from the
name what image family it belongs to?
## Test plan

<!-- All pull requests REQUIRE a test plan:
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles
-->

## Changelog

<!--
1. Ensure your pull request title is formatted as: $type($domain): $what
2. Add bullet list items for each additional detail you want to cover
(see example below)
3. You can edit this after the pull request was merged, as long as
release shipping it hasn't been promoted to the public.
4. For more information, please see this how-to
https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c?

Audience: TS/CSE > Customers > Teammates (in that order).

Cheat sheet: $type = chore|fix|feat $domain:
source|search|ci|release|plg|cody|local|...
-->

<!--
Example:

Title: fix(search): parse quotes with the appropriate context
Changelog section:

## Changelog

- When a quote is used with regexp pattern type, then ...
- Refactored underlying code.
-->

(cherry picked from commit 8bb0ab5)
craigfurman pushed a commit that referenced this pull request Jun 10, 2024
…63178)

the executor image and docker mirror image should now follow the
following naming convention:

Image family:
`sourcegraph-executors-[nightly|internal|&#39;&#39;]-&lt;MAJOR&gt;-&lt;MINOR&gt;`
Image name:
`sourcegraph-executor-[nightly|internal|&#39;&#39;]-&lt;MAJOR&gt;-&lt;MINOR&gt;-&lt;BUILD_NUMBER&gt;`

example:
Image family: `sourcegraph-executors-5-4`
Image name: `sourcegraph-executor-5-4-277666`

## What happens during releases and _not_ releases?
#### Nightly
**`nightly` suffix**
Image family:
`sourcegraph-executors-nightly-&lt;MAJOR&gt;-&lt;MINOR&gt;`
Image name:
`sourcegraph-executor-nightly-&lt;MAJOR&gt;-&lt;MINOR&gt;-&lt;BUILD_NUMBER&gt;`
#### Internal
**`internal` suffix**
Image family:
`sourcegraph-executors-internal-&lt;MAJOR&gt;-&lt;MINOR&gt;`
Image name:
`sourcegraph-executor-internal-&lt;MAJOR&gt;-&lt;MINOR&gt;-&lt;BUILD_NUMBER&gt;`
#### Public / Promote to public

** No suffix **

Image family: `sourcegraph-executors-&lt;MAJOR&gt;-&lt;MINOR&gt;`
Image name:
`sourcegraph-executor-&lt;MAJOR&gt;-&lt;MINOR&gt;-&lt;BUILD_NUMBER&gt;`

&gt;  [!IMPORTANT]
&gt; Should we keep the imagine name stable at
`sourcegraph-executor-&lt;MAJOR&gt;-&lt;MINOR&gt;-&lt;BUILD_NUMBER&gt;`
&gt; and only change the family name? 
&gt;
&gt; **Why?**
&gt;
&gt; The Image family dictates the collection of images and that changes
each major minor and or release phase so there is really no use in
changing the image name too, except at a glance you can see from the
name what image family it belongs to?
## Test plan




## Changelog




 <br> Backport 8bb0ab5 from #63157

Co-authored-by: William Bezuidenhout <william.bezuidenhout@sourcegraph.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

backport 5.4.5099 label used to backport PRs to the 5.4.5099 release branch cla-signed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants