ci: image list for offline installation #8596

subhamkrai · 2021-08-26T11:25:14Z

Description of your changes:
create file tests/scripts/rook-ceph-image.txt which
will have list of images required for offline installation.

Closes: #6406
Signed-off-by: subhamkrai srai@redhat.com

Which issue is resolved by this Pull Request:
Resolves #6406

Checklist:

travisn

How about the following approach?

Generate the image list as part of make build
If the list changes, the dev will need to submit it with their PR, for example when the CSI images change or when we have the release version update.
Commit an images.txt to the example manifest folder. Those who need the image list can find it in a predictable place in the repo for every release tag going forward.
The official build should fail if there are any required changes to the image list. (We already have a check for changed files in the official build, right?)

This will avoid any extra steps in the release process.

BlaineEXE · 2021-08-26T15:22:10Z

.github/workflows/create-tag.yaml

@@ -40,3 +40,6 @@ jobs:
          FROM_BRANCH: ${{ env.FROM_BRANCH }}
          TO_TAG: ${{ env.TO_TAG }}
        run: tests/scripts/gen_release_notes.sh
+
+      - name: Images for offline installation
+        run: awk '/image:/ {print $2}' cluster/examples/kubernetes/ceph/operator.yaml cluster/examples/kubernetes/ceph/cluster.yaml | tee -a tests/scripts/rook-ceph-image.txt && awk '/quay.io/ || /k8s.gcr.io/ {print $3}' cluster/examples/kubernetes/ceph/operator.yaml | tr -d '"' | tee -a tests/scripts/rook-ceph-image.txt


Can this be broken up into multiple lines for easier reading like below?

command | \ tee blah next command | \ tr etc | tee etc

Is there a better way to write the below portion so that we are not going to face problems if we add reference dockerhub image in the future?

awk '/quay.io/ || /k8s.gcr.io/ {print $3}' cluster/examples/kubernetes/ceph/operator.yaml

Instead of a file somewhere, can we add this as an asset to the release? See the "Assets" for instance here https://github.com/rook/rook/releases/tag/v1.7.2.
If we manage to add this automatically it's a win for each release/branch.

we can do it automatically but the request was having a text file comment but the asset should work too.

Thoughts on this approach? It's fine if we can automatically attach an asset to the release, it just can't be a manual process to add to the release artifacts or else it will be easily missed.

going with this approach

.github/workflows/create-tag.yaml

BlaineEXE · 2021-08-26T15:29:53Z

tests/scripts/rook-ceph-image.txt

@@ -0,0 +1,16 @@
+quay.io/cephcsi/cephcsi:v3.4.0


Why is this going into tests/scripts? Is this not intended (or useful) for users?

If we are doing this, I would put it in a place where users should be empowered to find it (cluster/examples/kubernetes/ceph?) and add to the docs that it exists.

putting file in cluster/examples/kubernetes/ceph now.

BlaineEXE · 2021-08-26T15:30:25Z

tests/scripts/rook-ceph-image.txt

+k8s.gcr.io/sig-storage/csi-snapshotter:v4.1.1
+k8s.gcr.io/sig-storage/csi-attacher:v3.2.1
+quay.io/csiaddons/volumereplication-operator:v0.1.0
+rook/ceph:master


This image will change for releases right?

Agreed, each branch should have its own version. Because this was generated on the master branch we get the master tag.

subhamkrai · 2021-09-03T06:47:21Z

images/ceph/Makefile

+	# remove the file if already exists
+	[ -f ../../cluster/examples/kubernetes/ceph/rook-ceph-image.txt ] && rm ../../cluster/examples/kubernetes/ceph/rook-ceph-image.txt;\
+	awk '/image:/ {print $2}' ../../cluster/examples/kubernetes/ceph/operator.yaml ../../cluster/examples/kubernetes/ceph/cluster.yaml | \
+	cut -d: -f2- |\


I have to add this cut -d: -f2- because when running older command(cut -d: -f2-) from running from cli has no issue but when running from make build it generates in this format

image: rook/ceph:master image: quay.io/ceph/ceph:v16.2.5 # ROOK_CSI_CEPH_IMAGE: quay.io/cephcsi/cephcsi:v3.4.0 # ROOK_CSI_REGISTRAR_IMAGE: k8s.gcr.io/sig-storage/csi-node-driver-registrar:v2.2.0 # ROOK_CSI_RESIZER_IMAGE: k8s.gcr.io/sig-storage/csi-resizer:v1.2.0 # ROOK_CSI_PROVISIONER_IMAGE: k8s.gcr.io/sig-storage/csi-provisioner:v2.2.2 # ROOK_CSI_SNAPSHOTTER_IMAGE: k8s.gcr.io/sig-storage/csi-snapshotter:v4.1.1 # ROOK_CSI_ATTACHER_IMAGE: k8s.gcr.io/sig-storage/csi-attacher:v3.2.1 # CSI_VOLUME_REPLICATION_IMAGE: quay.io/csiaddons/volumereplication-operator:v0.1.0

subhamkrai · 2021-09-03T07:39:53Z

How about the following approach?

Generate the image list as part of make build

If the list changes, the dev will need to submit it with their PR, for example when the CSI images change or when we have the release version update.

Commit an images.txt to the example manifest folder. Those who need the image list can find it in a predictable place in the repo for every release tag going forward.

The official build should fail if there are any required changes to the image list. (We already have a check for changed files in the official build, right?)

just to verify above, I didn't add the filerook-ceph-image.txt to see if it is failing. I found it is not failing with exit 1.

## Build images for all platforms.'
+ tests/scripts/validate_modified_files.sh build
+ CODEGEN_ERR='found codegen files! please run '\''make codegen'\'' and update your PR'
+ MOD_ERR='changes found by mod.check. You may need to run make clean'
+ CRD_ERR='changes found by '\''make crds'\''. please run '\''make crds'\'' locally and update your PR'
+ BUILD_ERR='changes found by make build'\'', please commit your go.sum or other changed files'
+ case "$1" in
+ validate 'changes found by make build'\'', please commit your go.sum or other changed files'
++ git status --porcelain
+ git='?? cluster/examples/kubernetes/ceph/rook-ceph-image.txt'
changes found by make build', please commit your go.sum or other changed files
?? cluster/examples/kubernetes/ceph/rook-ceph-image.txt
+ for file in $git
+ '[' -n '??' ']'
+ echo 'changes found by make build'\'', please commit your go.sum or other changed files'
+ echo '?? cluster/examples/kubernetes/ceph/rook-ceph-image.txt'
+ git diff
+ exit 1

with exit 1 also tests are passing https://github.com/rook/rook/pull/8596/checks?check_run_id=3503686956

I found this https://github.community/t/no-output-on-process-completed-with-exit-code-1/123821/7

This will avoid any extra steps in the release process.

travisn

The plan is to commit the txt file, right? Looks like the build is failing because it is missing.

images/ceph/Makefile

subhamkrai · 2021-09-03T14:02:21Z

The plan is to commit the txt file, right? Looks like the build is failing because it is missing.

Yes I did it intentionally to confirm if tests are failing, and if you notice build on Linux is passing

images/ceph/Makefile

BlaineEXE · 2021-09-07T16:00:39Z

images/ceph/Makefile

+	awk '/image:/ {print $2}' ../../cluster/examples/kubernetes/ceph/operator.yaml ../../cluster/examples/kubernetes/ceph/cluster.yaml | \
+	cut -d: -f2- |\
+	tee -a ../../cluster/examples/kubernetes/ceph/images.txt && \
+	awk '/quay.io/ || /k8s.gcr.io/ {print $3}' ../../cluster/examples/kubernetes/ceph/operator.yaml | \


What if we read these from the code instead? It is rare but sometimes happens that the manifests don't get updated when the default images in the code are updated. The sed to find these can then look for Default.*Image[[:whitespace:]]+=[[:whitespace:]]+"<image name>"$ (attempt at regex, but might have bugs).

From pkg/operator/ceph/csi/spec.go:

DefaultCSIPluginImage = "quay.io/cephcsi/cephcsi:v3.4.0" DefaultRegistrarImage = "k8s.gcr.io/sig-storage/csi-node-driver-registrar:v2.2.0" DefaultProvisionerImage = "k8s.gcr.io/sig-storage/csi-provisioner:v2.2.2" DefaultAttacherImage = "k8s.gcr.io/sig-storage/csi-attacher:v3.2.1" DefaultSnapshotterImage = "k8s.gcr.io/sig-storage/csi-snapshotter:v4.1.1" DefaultResizerImage = "k8s.gcr.io/sig-storage/csi-resizer:v1.2.0" DefaultVolumeReplicationImage = "quay.io/csiaddons/volumereplication-operator:v0.1.0"

we can read images from pkg/operator/ceph/csi/spec.go for some images but in the end, we have to fetch from manifest for complete images list. IMO manifests is the right place .

For the rook/ceph:<tag> image and ceph/ceph:<tag> image, the only place they are defined is in the manifests.

But it has happened more than once that the CSI image defined in spec.go is different from what is in the manifests. When (not if) that happens in the future, Rook will fail to start up for air-gapped users because they will have mirrored images that Rook isn't using by default. As far as we know, most users do use the internal default images, so it is paramount that these images are the ones we list in this file.

There are a couple other routes we could consider, probably for a future PR.

We could add images from both the manifests and the Go code and remove duplicate tags. We could also run an analysis on the file to see if there are images listed with multiple version, and if that's the case, we know that the manifests and Go code haven't been updated both at the same time.

(probably better) We could use the Go code to generate/modify the CSI_..._IMAGE vars that are commented-out in the manifests so they don't get out of sync.

I see your points. so can we go ahead with this PR and create a follow-up PR?

images/ceph/Makefile

create file tests/scripts/rook-ceph-image.txt which will have list of images required for offline installation. Closes: rook#6406 Signed-off-by: subhamkrai <srai@redhat.com>

BlaineEXE · 2021-09-09T19:53:46Z

Merging this so it can go into v1.7.3, and I'll create a new bug to take the images from the go code as a future update.

BlaineEXE · 2021-09-09T19:55:45Z

images/ceph/Makefile

+	tee -a $(MANIFESTS_DIR)/images.txt && \
+	awk '/quay.io/ || /k8s.gcr.io/ {print $3}' $(MANIFESTS_DIR)/operator.yaml | \
+	cut -d: -f2- |\
+	tr -d '"' | tee -a $(MANIFESTS_DIR)/images.txt


For future reference, this should be replaced with lines that get the CSI-related images from pkg/operator/ceph/csi/spec.go

ci: image list for offline installation (backport #8596)

subhamkrai requested review from travisn and leseb August 26, 2021 11:25

subhamkrai force-pushed the offline-installation branch from e48522c to de2a238 Compare August 26, 2021 12:28

travisn requested changes Aug 26, 2021

View reviewed changes

BlaineEXE requested changes Aug 26, 2021

View reviewed changes

subhamkrai force-pushed the offline-installation branch from de2a238 to a0628bf Compare September 3, 2021 06:43

subhamkrai commented Sep 3, 2021

View reviewed changes

subhamkrai force-pushed the offline-installation branch from a0628bf to fc3ae28 Compare September 3, 2021 07:22

subhamkrai force-pushed the offline-installation branch from fc3ae28 to bc6cf73 Compare September 3, 2021 07:40

subhamkrai requested review from travisn and BlaineEXE September 3, 2021 08:40

travisn requested changes Sep 3, 2021

View reviewed changes

images/ceph/Makefile Outdated Show resolved Hide resolved

leseb requested changes Sep 6, 2021

View reviewed changes

images/ceph/Makefile Outdated Show resolved Hide resolved

subhamkrai force-pushed the offline-installation branch 3 times, most recently from 8737f6d to ac3ea7d Compare September 7, 2021 09:06

subhamkrai requested review from leseb and travisn September 7, 2021 09:08

travisn approved these changes Sep 7, 2021

View reviewed changes

leseb requested changes Sep 7, 2021

View reviewed changes

images/ceph/Makefile Outdated Show resolved Hide resolved

BlaineEXE reviewed Sep 7, 2021

View reviewed changes

subhamkrai force-pushed the offline-installation branch from ac3ea7d to 1cd925e Compare September 8, 2021 13:19

subhamkrai requested a review from leseb September 8, 2021 13:27

leseb requested changes Sep 8, 2021

View reviewed changes

images/ceph/Makefile Outdated Show resolved Hide resolved

subhamkrai force-pushed the offline-installation branch 2 times, most recently from d7b4816 to 326d491 Compare September 8, 2021 14:21

ci: image list for offline installation

7bc9a13

create file tests/scripts/rook-ceph-image.txt which will have list of images required for offline installation. Closes: rook#6406 Signed-off-by: subhamkrai <srai@redhat.com>

subhamkrai force-pushed the offline-installation branch from 326d491 to 7bc9a13 Compare September 8, 2021 14:22

leseb approved these changes Sep 8, 2021

View reviewed changes

BlaineEXE approved these changes Sep 9, 2021

View reviewed changes

BlaineEXE merged commit 94eb451 into rook:master Sep 9, 2021

BlaineEXE reviewed Sep 9, 2021

View reviewed changes

BlaineEXE mentioned this pull request Sep 9, 2021

Build offline image list from default images in Go code #8687

Closed

travisn added the backport-release-1.7 label Sep 9, 2021

mergify bot mentioned this pull request Sep 9, 2021

ci: image list for offline installation (backport #8596) #8688

Merged

travisn added a commit that referenced this pull request Sep 9, 2021

Merge pull request #8688 from rook/mergify/bp/release-1.7/pr-8596

2b05cfe

ci: image list for offline installation (backport #8596)

subhamkrai deleted the offline-installation branch September 13, 2021 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: image list for offline installation #8596

ci: image list for offline installation #8596

subhamkrai commented Aug 26, 2021

travisn left a comment

BlaineEXE Aug 26, 2021

BlaineEXE Aug 26, 2021

leseb Aug 27, 2021

subhamkrai Aug 27, 2021

travisn Aug 27, 2021

subhamkrai Sep 3, 2021

BlaineEXE Aug 26, 2021

subhamkrai Sep 3, 2021

BlaineEXE Aug 26, 2021

leseb Aug 27, 2021

subhamkrai Sep 3, 2021

subhamkrai commented Sep 3, 2021

travisn left a comment

subhamkrai commented Sep 3, 2021

BlaineEXE Sep 7, 2021

subhamkrai Sep 8, 2021

BlaineEXE Sep 8, 2021

subhamkrai Sep 9, 2021

BlaineEXE commented Sep 9, 2021

BlaineEXE Sep 9, 2021

ci: image list for offline installation #8596

ci: image list for offline installation #8596

Conversation

subhamkrai commented Aug 26, 2021

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

subhamkrai commented Sep 3, 2021

travisn left a comment

Choose a reason for hiding this comment

subhamkrai commented Sep 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Sep 9, 2021

Choose a reason for hiding this comment