DM-37759: Rework container build workflows. #51

ktlim · 2023-02-10T00:14:03Z

No description provided.

kfindeisen

I think I follow the changes to the base container (though atomic commits would have made it easier), but I'm confused about how the service container is supposed to work. If we want to build the service container with a branch build of the base container, do we have to modify build-service.yaml?

kfindeisen · 2023-02-10T22:03:32Z

.github/workflows/build-base.yml

+        type: string
+      makeLatest:
+        description: 'Push container with "latest" tag'
+        required: false


Why isn't this input required? With a boolean I would have thought default=false would be safer.

I was concerned that marking it "required" would put a red asterisk in the input box which would somehow suggest to users that they always needed to check the box.

.github/workflows/build-base.yml

kfindeisen · 2023-02-10T22:21:56Z

.github/workflows/build-base.yml

-          docker push $IMAGE_ID:$VERSION
+          [[ -n "$STACK_TAG" ]] || STACK_TAG="d_latest"
+          MAKE_LATEST="${{ inputs.makeLatest }}"
+          [[ -n "$MAKE_LATEST" ]] || MAKE_LATEST="true"


It looks like this defaults MAKE_LATEST to true; is this intended? If I try manually running the build action, the box starts unchecked, which I would expect to mean default false.

I think it's always true or false, but you're right that I should make it false if empty. I think I was trying to emulate the previous behavior.

kfindeisen · 2023-02-10T22:37:41Z

.github/workflows/build-base.yml

+
+          for IMAGE_ID in "ghcr.io/${{ github.repository_owner }}/$IMAGE_NAME" "us-central1-docker.pkg.dev/prompt-proto/prompt/$IMAGE_NAME"; do
+            if [ "$BRANCH" == "main" ]; then
+              VERSION="$STACK_TAG"


This applies "d_latest" or "w_latest". I take it that's the best we can do, and we should still look at the container manifest to get the exact stack version?

I will add code to also tag with the actual eups tag.

.github/workflows/build-service.yml

kfindeisen · 2023-02-10T23:21:53Z

.github/workflows/build-service.yml

+        stackTag: ${{ fromJSON(needs.matrix-gen.outputs.matrix) }}
    env:
      IMAGE_NAME: prompt-proto-service
+      STACK_TAG: ${{ matrix.stackTag }}


Maybe call it BASE_TAG, to disambiguate the container label (e.g., DM-37759-w_2023_06 or latest) from the actual Stack designation (w_2023_06 or d_latest)?

.github/workflows/build-service.yml

ktlim · 2023-02-11T01:09:46Z

As to building against a branch of the base, the ticket calls out this case as unavailable. Since the base container is just a few third-party additions to the stack container, not including any of the code in this repo, I thought it was acceptable to only allow versions of it based on main.

kfindeisen · 2023-02-13T16:54:46Z

As to building against a branch of the base, the ticket calls out this case as unavailable. Since the base container is just a few third-party additions to the stack container, not including any of the code in this repo, I thought it was acceptable to only allow versions of it based on main.

Then I didn't understand the original description. I can think of two cases off the top of my head where we wouldn't want to use the latest base:

Testing, or working around, a regression in the Stack itself, as happened recently on DM-37072: Expand HSC calibs and templates in central repo #47. I thought the discussion of that issue was what motivated this work in the first place.
Testing changes made to the base container build -- I thought you wanted to support this case as well, given your comment on Jira about service container builds not waiting for the base.

Right now, it looks like supporting either of these cases requires temporarily editing STACK_TAG_LIST, then dropping that commit before merge -- i.e., the same workaround we're using now.

kfindeisen

Thanks for adding the exact stack version! Only outstanding issue is which base container(s) can be used in the service build.

kfindeisen · 2023-02-13T16:58:13Z

.github/workflows/build-base.yml

+            echo "d_latest" > lsst.docker.tag
+            echo "d_latest" > stack.tag
+          fi
+          docker run lsstsqre/centos:"$(< lsst.docker.tag)" bash -c "cat stack/miniconda*/ups_db/global.tags" > eups.tag


Is this procedure guaranteed to return just one tag? How does it compare to reading the EUPS_TAG label from the container itself? (Apologies for my lack of Docker fluency; I really don't know what the tradeoffs are!)

The Science Pipelines base containers don't have an appropriate label, as far as I can tell, and I believe retrieving that label would require downloading the whole container anyway. Since we need the container for the build, downloading it and running a command in it is not much additional expense.

For these containers, because of the way they're built, it is guaranteed that there will only be one tag.

ktlim · 2023-02-14T19:41:23Z

I can think of two cases off the top of my head where we wouldn't want to use the latest base:
* Testing, or working around, a regression in the Stack itself, as happened recently on DM-37072. I thought the discussion of that issue was what motivated this work in the first place.
* Testing changes made to the base container build -- I thought you wanted to support this case as well, given your comment on Jira about service container builds not waiting for the base.
Right now, it looks like supporting either of these cases requires temporarily editing STACK_TAG_LIST, then dropping that commit before merge -- i.e., the same workaround we're using now.

The first case was the intended target. We would add a "stable" stack version to STACK_TAG_LIST, build its corresponding base container manually (if not already available), and build the service containers on top of it in every branch and main until the Stack regression was no longer a problem.

The second case — testing changes made to the base container build by building the service on top and testing at the service level — was not intended to be supported. The base container only has small third-party dependency additions to the stack container, none of which actually involve code from this repo. So I figured it didn't need extensive testing, and in particular not testing by running the service. Building a branch version, checking (manually) that the dependency is available and functional, and then merging prior to developing service code that requires it seemed to me to be good enough.

kfindeisen · 2023-02-14T20:46:01Z

The first case was the intended target. We would add a "stable" stack version to STACK_TAG_LIST, build its corresponding base container manually (if not already available), and build the service containers on top of it in every branch and main until the Stack regression was no longer a problem.

Can you add a usage comment, then? In particular, the fact that we would keep using the "stable" base for an extended period of time is not obvious to anyone used to the old workflow.

So I figured it didn't need extensive testing, and in particular not testing by running the service. Building a branch version, checking (manually) that the dependency is available and functional, and then merging prior to developing service code that requires it seemed to me to be good enough.

I don't think such minimal tests have been sufficient in the past, but I suspect I'm not going to change your mind on this. I'll withdraw my objection to merging as-is.

ktlim · 2023-02-14T21:25:00Z

I added some documentation for BASE_TAG_LIST.

If there are dependency installations that are service-branch-specific and need to be tested via the service, they can be added to Dockerfile.activator until proven in the branch, then moved to Dockerfile.main.

ktlim force-pushed the tickets/DM-37759 branch 14 times, most recently from 5db103a to df133e7 Compare February 10, 2023 02:38

kfindeisen reviewed Feb 10, 2023

View reviewed changes

ktlim added 3 commits February 10, 2023 15:55

Rework base container build workflow.

46d48e9

Rework service container build workflow.

17fa603

Change STACK_TAG to BASE_TAG for service.

62b10f1

ktlim force-pushed the tickets/DM-37759 branch 4 times, most recently from 11904e7 to 3fca267 Compare February 11, 2023 01:05

Compute actual eups tag and label with it.

3fb8d1b

ktlim force-pushed the tickets/DM-37759 branch from 3fca267 to 3fb8d1b Compare February 11, 2023 01:24

kfindeisen reviewed Feb 13, 2023

View reviewed changes

kfindeisen approved these changes Feb 14, 2023

View reviewed changes

Add documentation for BASE_TAG_LIST.

eace7b4

ktlim merged commit ca1d53a into main Feb 14, 2023

ktlim deleted the tickets/DM-37759 branch February 14, 2023 21:51

DM-37759: Rework container build workflows. #51

DM-37759: Rework container build workflows. #51

Uh oh!

Conversation

ktlim commented Feb 10, 2023

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktlim commented Feb 11, 2023

Uh oh!

kfindeisen commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kfindeisen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktlim commented Feb 14, 2023

Uh oh!

kfindeisen commented Feb 14, 2023

Uh oh!

ktlim commented Feb 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kfindeisen commented Feb 13, 2023 •

edited

Loading